Bug 58211 – Add method to make a Port able to retrieve TestOutputSets from its builders.

• Component: New Bugs
• Status: RESOLVED FIXED

[James Kozianski]

Add method to make a Port able to retrieve TestOutputSets from its builders.

[James Kozianski]

Created attachment 88963 [details]
Patch

[Ojan Vafai]

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.

[Ojan Vafai]

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.

[James Kozianski]

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

[James Kozianski]

Created attachment 92119 [details]
Patch

[Eric Seidel (no email)]

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

[James Kozianski]

*** Bug 60058 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

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.

[James Kozianski]

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

[Eric Seidel (no email)]

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

[James Kozianski]

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

[James Kozianski]

Created attachment 92165 [details]
Patch

[Eric Seidel (no email)]

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?

[James Kozianski]

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

[James Kozianski]

Created attachment 92262 [details]
Patch

[Eric Seidel (no email)]

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.

[James Kozianski]

Created attachment 92266 [details]
Patch

[James Kozianski]

(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

[Eric Seidel (no email)]

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.

[James Kozianski]

Created attachment 92268 [details]
Patch

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Comment on attachment 92268 [details]
Patch

Thanks.

[James Kozianski]

Committed r85797: <http://trac.webkit.org/changeset/85797>