Add method to make a Port able to retrieve TestOutputSets from its builders.
Created attachment 88963 [details] Patch
Comment on attachment 88963 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88963&action=review > Tools/Scripts/webkitpy/layout_tests/port/base.py:504 > + def buildbot_testoutputset(self, platforms): This should be buildbot_test_output_set. > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:205 > + platform_builders = { > + 'chromium-mac': 'Webkit_Mac10_5', > + 'chromium-win-vista': 'Webkit_Win', > + 'chromium-linux': 'Webkit_Linux__dbg__1_', > + } This is a generally useful map. Lets move this to a map next to ALL_BASELINE_VARIANTS and generate ALL_BASELINE_VARIANTS from this map. Lets talk in person about what the best location for this data is. > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:212 > + zip_url = 'http://build.chromium.org/f/chromium/layout_test_results/' + \ \ line continuations are so old-school. use parens instead. > Tools/Scripts/webkitpy/layout_tests/port/factory.py:131 > +def get_all(options=None): This doesn't look like it's used in this patch. Did you mean to include it? > Tools/Scripts/webkitpy/layout_tests/port/factory.py:134 > + return dict([(port_name, get(port_name, options=options)) > + for port_name in all_port_names()]) This indentation is weird. Also, we don't have a line limit. I think this would read fine on one line. > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:80 > + platform_builders = { > + 'mac-snowleopard': 'SnowLeopard Intel Release (Tests)', > + 'mac-leopard': 'Leopard Intel Debug (Tests)', > + 'win': 'Windows 7 Release (WebKit2 Tests)', > + 'chromium-linux': 'GTK Linux 32-bit Release', > + } ditto above > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:86 > + bb = BuildBot() abbreviations are discouraged in webkit code. also, you don't really need this local variable, do you? it's only used on line 90.
Comment on attachment 88963 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88963&action=review >> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:205 >> + } > > This is a generally useful map. Lets move this to a map next to ALL_BASELINE_VARIANTS and generate ALL_BASELINE_VARIANTS from this map. Lets talk in person about what the best location for this data is. Thinking about this more, I think maybe this should be a map in factory.py? Then all_port_names in factory.py can generate the list based on this map, this code can use the map and ALL_BASELINE_VARIANTS doesn't need to exist since it can just be generated from the list in factory.py.
(In reply to comment #2) > (From update of attachment 88963 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=88963&action=review > > > Tools/Scripts/webkitpy/layout_tests/port/base.py:504 > > + def buildbot_testoutputset(self, platforms): > > This should be buildbot_test_output_set. Done. > > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:205 > > + platform_builders = { > > + 'chromium-mac': 'Webkit_Mac10_5', > > + 'chromium-win-vista': 'Webkit_Win', > > + 'chromium-linux': 'Webkit_Linux__dbg__1_', > > + } > > This is a generally useful map. Lets move this to a map next to ALL_BASELINE_VARIANTS and generate ALL_BASELINE_VARIANTS from this map. Lets talk in person about what the best location for this data is. Done. > > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:212 > > + zip_url = 'http://build.chromium.org/f/chromium/layout_test_results/' + \ > > \ line continuations are so old-school. use parens instead. Done. > > Tools/Scripts/webkitpy/layout_tests/port/factory.py:131 > > +def get_all(options=None): > > This doesn't look like it's used in this patch. Did you mean to include it? This is kind of irrelevant to this change - reverted. > > Tools/Scripts/webkitpy/layout_tests/port/factory.py:134 > > + return dict([(port_name, get(port_name, options=options)) > > + for port_name in all_port_names()]) > > This indentation is weird. Also, we don't have a line limit. I think this would read fine on one line. > > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:80 > > + platform_builders = { > > + 'mac-snowleopard': 'SnowLeopard Intel Release (Tests)', > > + 'mac-leopard': 'Leopard Intel Debug (Tests)', > > + 'win': 'Windows 7 Release (WebKit2 Tests)', > > + 'chromium-linux': 'GTK Linux 32-bit Release', > > + } > > ditto above Done. > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:86 > > + bb = BuildBot() > > abbreviations are discouraged in webkit code. also, you don't really need this local variable, do you? it's only used on line 90. Good point - I've removed the variable.
Created attachment 92119 [details] Patch
Comment on attachment 92119 [details] Patch What's a TestOutputSet and how does it differ from LayoutTestResults (in common.net). I feel like we've had this discussion before, but refresh my memory. :)
*** Bug 60058 has been marked as a duplicate of this bug. ***
Comment on attachment 92119 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=92119&action=review > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:209 > + if platform not in PORT_TO_BUILDER_NAME: I think I would have exposed a lookup function (or list class) instead of exposing the actual list. Then you call builder_name_for_platform(platform) and if it's None do this exeption. > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:212 > + zip_url = ('http://build.chromium.org/f/chromium/layout_test_results/' + We may want to put this in common.config.urls, not sure. > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:85 > + for builder in BuildBot().builders(): > + builders_by_name[builder.name()] = builder Isn't there already a dictionary function which does this? I guess: builders_by_name = dict([(builder.name(), builder) for builder in BuildBot.builders()]) is what I'm thinking of. > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:87 > + for platform in platforms: This could also be re-written as list comprehensions if you wanted. the set_platform call is the only thing that makes the not directly obvious. Heck, if it used a _results_for_platform helper function, the rest of this implementation could be shared between webkit.py and chromium.py, no? > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:89 > + rs = builder.latest_cached_build().results() generally we avoid short variable names. I think results or result_set would be more readable here.
(In reply to comment #6) > (From update of attachment 92119 [details]) > What's a TestOutputSet and how does it differ from LayoutTestResults (in common.net). I feel like we've had this discussion before, but refresh my memory. :) Sure :-) A TestOutputSet is a set of TestOutputs, which are what the LayoutTests generate when they run. So a TestOutput is the type of a baseline (you compare a test's output with a baseline to determine if the test is passing). LayoutTestResults I think is a container for information extracted from results.html, that is, a list of failing tests.
(In reply to comment #9) > LayoutTestResults I think is a container for information extracted from results.html, that is, a list of failing tests. That's correct. It's a container for information abstracted from a layout test run. Parsed from the *.json files or .html files for either ORWT or NRWT. Thanks.
(In reply to comment #8) > (From update of attachment 92119 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=92119&action=review > > > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:209 > > + if platform not in PORT_TO_BUILDER_NAME: > > I think I would have exposed a lookup function (or list class) instead of exposing the actual list. Then you call builder_name_for_platform(platform) and if it's None do this exeption. Done. > > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:212 > > + zip_url = ('http://build.chromium.org/f/chromium/layout_test_results/' + > > We may want to put this in common.config.urls, not sure. Makes sense - I added a chromium_results_zip_url() function. > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:85 > > + for builder in BuildBot().builders(): > > + builders_by_name[builder.name()] = builder > > Isn't there already a dictionary function which does this? > > I guess: > builders_by_name = dict([(builder.name(), builder) for builder in BuildBot.builders()]) > is what I'm thinking of. Done. > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:87 > > + for platform in platforms: > > This could also be re-written as list comprehensions if you wanted. the set_platform call is the only thing that makes the not directly obvious. With the introduction of _results_for_platform this is no longer a list comprehension. > Heck, if it used a _results_for_platform helper function, the rest of this implementation could be shared between webkit.py and chromium.py, no? Done. > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:89 > > + rs = builder.latest_cached_build().results() > > generally we avoid short variable names. I think results or result_set would be more readable here. Done.
Created attachment 92165 [details] Patch
Comment on attachment 92165 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=92165&action=review > Tools/Scripts/webkitpy/layout_tests/port/builders.py:91 > + if platform in PORT_TO_BUILDER_NAME: > + return PORT_TO_BUILDER_NAME[platform] > + return None This is equivalent to PORT_TO_BUILDER_NAME.get(platform) I still agree it should be wrapped in a function like this! It can just be a one-line function. I think PORT_TO_BUILDER_NAME will eventually end up private to this module. > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:209 > + if builder_name is None: You can probably just use if not builder_name, since an empty builder name makes as much sense as None does. :) > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:74 > + builders_by_name = dict([(builder.name(), builder) for builder in BuildBot().builders()]) Seems we don't want to compute this until we need it. (aka, after we've successfully looked up the builder_name). Doesn't really matter since in the normal case we'll succeed. We're also going to end up recomputing this every time we call this function. Which is OK. > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:81 > + result_set = builder.latest_cached_build().results() > + result_set.set_platform(platform) > + return result_set So why is the webkit one different? Because the chromium bots spit out zips?
(In reply to comment #13) > (From update of attachment 92165 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=92165&action=review > > > Tools/Scripts/webkitpy/layout_tests/port/builders.py:91 > > + if platform in PORT_TO_BUILDER_NAME: > > + return PORT_TO_BUILDER_NAME[platform] > > + return None > > This is equivalent to PORT_TO_BUILDER_NAME.get(platform) > > I still agree it should be wrapped in a function like this! It can just be a one-line function. > > I think PORT_TO_BUILDER_NAME will eventually end up private to this module. Cool! Done. > > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:209 > > + if builder_name is None: > > You can probably just use if not builder_name, since an empty builder name makes as much sense as None does. :) Done. > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:74 > > + builders_by_name = dict([(builder.name(), builder) for builder in BuildBot().builders()]) > > Seems we don't want to compute this until we need it. (aka, after we've successfully looked up the builder_name). Doesn't really matter since in the normal case we'll succeed. > > We're also going to end up recomputing this every time we call this function. Which is OK. Yeah, it looked pretty cheap. I've moved the builders_by_name to after the builder_name lookup. > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:81 > > + result_set = builder.latest_cached_build().results() > > + result_set.set_platform(platform) > > + return result_set > > So why is the webkit one different? Because the chromium bots spit out zips? Yep, that's right - Chromium bots store the latest results in a constant place, but the WebKit ones store them in a directory that contains the revision number (I think), so it needs a bit more logic to figure out which are the latest results.
Created attachment 92262 [details] Patch
Attachment 92262 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1 Tools/Scripts/webkitpy/common/config/urls.py:41: too many blank lines (3) [pep8/E303] [5] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 92266 [details] Patch
(In reply to comment #16) > Attachment 92262 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1 > > Tools/Scripts/webkitpy/common/config/urls.py:41: too many blank lines (3) [pep8/E303] [5] > Total errors found: 1 in 6 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. Hmm, it doesn't report any problems locally. I've tried removing the blank line following urls.py:41
Attachment 92266 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1 Tools/Scripts/webkitpy/common/config/urls.py:41: too many blank lines (3) [pep8/E303] [5] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 92268 [details] Patch
Attachment 92268 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1 Tools/Scripts/webkitpy/common/config/urls.py:41: too many blank lines (3) [pep8/E303] [5] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 92268 [details] Patch Thanks.
Committed r85797: <http://trac.webkit.org/changeset/85797>