Bug 83348 - add a webkit-patch print-baselines command
Summary: add a webkit-patch print-baselines command
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-05 20:45 PDT by Dirk Pranke
Modified: 2012-04-09 16:28 PDT (History)
4 users (show)

See Also:


Attachments
Patch (10.92 KB, patch)
2012-04-06 15:16 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
Patch (11.18 KB, patch)
2012-04-06 18:33 PDT, Dirk Pranke
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2012-04-05 20:45:56 PDT
See bug 64426 and bug 83347 for backstory ...
Comment 1 Dirk Pranke 2012-04-06 15:16:06 PDT
Created attachment 136071 [details]
Patch
Comment 2 Dirk Pranke 2012-04-06 15:18:27 PDT
Note in this case I didn't create a separate 'view' for the output since the usage is highly specialized and only a couple of lines.
Comment 3 Adam Barth 2012-04-06 15:23:22 PDT
Comment on attachment 136071 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136071&action=review

> Tools/Scripts/webkitpy/layout_tests/port/base.py:320
> +        for extension in ('wav', 'webarchive', 'txt', 'png'):

Can we centralize this list of extensions?  I think we have another copy of in rebaseline.py.
Comment 4 Adam Barth 2012-04-06 15:24:17 PDT
Makes sense.
Comment 5 Dirk Pranke 2012-04-06 15:27:32 PDT
(In reply to comment #3)
> (From update of attachment 136071 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136071&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/port/base.py:320
> > +        for extension in ('wav', 'webarchive', 'txt', 'png'):
> 
> Can we centralize this list of extensions?  I think we have another copy of in rebaseline.py.

Sure, I can publish this as a public method ...
Comment 6 Dirk Pranke 2012-04-06 18:33:38 PDT
Created attachment 136117 [details]
Patch
Comment 7 Adam Barth 2012-04-08 12:56:06 PDT
Comment on attachment 136117 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136117&action=review

> Tools/Scripts/webkitpy/layout_tests/port/base.py:324
> +    def expected_baseline_dict(self, test_name):
> +        """Returns a dict mapping baseline suffix to relative path for each baseline in
> +        a test. For reftests, it returns ".==" or ".!=" instead of the suffix."""
> +        # FIXME: The name similarity between this and expected_baselines() below, is unfortunate.
> +        # We should probably rename them both.
> +        baseline_dict = {}
> +        reference_files = self.reference_files(test_name)
> +        if reference_files:
> +            # FIXME: How should this handle more than one type of reftest?
> +            baseline_dict['.' + reference_files[0][0]] = self.relative_test_filename(reference_files[0][1])
> +
> +        for extension in self.baseline_extensions():
> +            path = self.expected_filename(test_name, extension, return_default=False)
> +            baseline_dict[extension] = self.relative_test_filename(path) if path else path
> +
> +        return baseline_dict

So, if png is missing, then this will map PNG to none?  Interesting.  The reftest part of this seems like an odd special case that might bite us in the future.

If you're looking for another name for this function, you might consider expected_baselines_by_extension.  That's a naming convention we used a bit in garden-o-matic to keep track of which dictionaries mapped what to what.

> Tools/Scripts/webkitpy/tool/commands/queries.py:509
> +            make_option('-p', '--platform', action='store',
> +                        help='platform/port(s) to display expectations for. Use glob-style wildcards for multiple ports (note that that will imply --csv)'),
> +            make_option('--all', action='store_true', default=False,
> +                        help='display the baselines for *all* tests'),
> +            make_option('--csv', action='store_true', default=False,
> +                        help='Print a CSV-style report that includes the port name, test_name, test platform, baseline type, baseline location, and baseline platform'),
> +            make_option('--include-virtual-tests', action='store_true',
> +                        help='Include virtual tests'),

You mention above that you're planning to write a followup patch that refactors the port-factory make_option calls to be shared rather than copy/pasted.

> Tools/Scripts/webkitpy/tool/commands/queries.py:555
> +        m = self._platform_regexp.match(relpath)

Can we use a more descriptive variable name than m ?
Comment 8 Dirk Pranke 2012-04-09 13:37:04 PDT
(In reply to comment #7)
> (From update of attachment 136117 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136117&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/port/base.py:324
> > +    def expected_baseline_dict(self, test_name):
> > +        """Returns a dict mapping baseline suffix to relative path for each baseline in
> > +        a test. For reftests, it returns ".==" or ".!=" instead of the suffix."""
> > +        # FIXME: The name similarity between this and expected_baselines() below, is unfortunate.
> > +        # We should probably rename them both.
> > +        baseline_dict = {}
> > +        reference_files = self.reference_files(test_name)
> > +        if reference_files:
> > +            # FIXME: How should this handle more than one type of reftest?
> > +            baseline_dict['.' + reference_files[0][0]] = self.relative_test_filename(reference_files[0][1])
> > +
> > +        for extension in self.baseline_extensions():
> > +            path = self.expected_filename(test_name, extension, return_default=False)
> > +            baseline_dict[extension] = self.relative_test_filename(path) if path else path
> > +
> > +        return baseline_dict
> 
> So, if png is missing, then this will map PNG to none?  Interesting.

Yes, unfortunately, the tool (like the rest of webkitpy) doesn't know whether a test *should* emit PNG/WAV/etc. We could do a bit more work to see if the baselines exist on any port, but that didn't seem worth it at this point.

>  The reftest part of this seems like an odd special case that might bite us in the future.

Yup, I expect I'll have to do something about this at some point if this command ends up being useful.

> If you're looking for another name for this function, you might consider expected_baselines_by_extension.  That's a naming convention we used a bit in garden-o-matic to keep track of which dictionaries mapped what to what.

Good idea.

> 
> > Tools/Scripts/webkitpy/tool/commands/queries.py:509
> > +            make_option('-p', '--platform', action='store',
> > +                        help='platform/port(s) to display expectations for. Use glob-style wildcards for multiple ports (note that that will imply --csv)'),
> > +            make_option('--all', action='store_true', default=False,
> > +                        help='display the baselines for *all* tests'),
> > +            make_option('--csv', action='store_true', default=False,
> > +                        help='Print a CSV-style report that includes the port name, test_name, test platform, baseline type, baseline location, and baseline platform'),
> > +            make_option('--include-virtual-tests', action='store_true',
> > +                        help='Include virtual tests'),
> 
> You mention above that you're planning to write a followup patch that refactors the port-factory make_option calls to be shared rather than copy/pasted.
> 

Right, will do.

> > Tools/Scripts/webkitpy/tool/commands/queries.py:555
> > +        m = self._platform_regexp.match(relpath)
> 
> Can we use a more descriptive variable name than m ?

Sure, will fix. Thanks for the review!
Comment 9 Dirk Pranke 2012-04-09 16:28:39 PDT
Committed r113639: <http://trac.webkit.org/changeset/113639>