Bug 83348

Created attachment 136071 [details] Patch

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 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.

Makes sense.

(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 ...

Created attachment 136117 [details] Patch

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 ?

(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!

Committed r113639: <http://trac.webkit.org/changeset/113639>