Only store the SVN revision in the summarized results if we're on a builder

• Component: New Bugs
• Status: RESOLVED FIXED

[Ojan Vafai]

Only store the SVN revision in the summarized results if we're on a builder

[Ojan Vafai]

Created attachment 123874 [details]
Patch

[Dirk Pranke]

Comment on attachment 123874 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py:413
> +        return unexpected_results

Do lines 377-413 have anything to do with this patch?

[WebKit Review Bot]

Comment on attachment 123874 [details]
Patch

Attachment 123874 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11159496

New failing tests:
media/audio-garbage-collect.html

[Adam Roben (:aroben)]

Comment on attachment 123874 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:237
> -        results['revision'] = port_obj.host.scm().head_svn_revision()
> +        # We only use the svn revision if we're uploading the results to a server.
> +        # Don't do this by default since it takes >100ms.
> +        # Use builder_name as a check that we're on a builder and so will upload the
> +        # results. This is only used for making trac links point to the test at
> +        # the revision the test was run at.
> +        if port_obj.get_option("builder_name"):
> +            results['revision'] = port_obj.host.scm().head_svn_revision()

It would be nice if we had some higher-level logic that decided when to use Trac links or not. Then this code could ask that logic rather than having to have special knowledge of what other parts of the code do.

[Ojan Vafai]

Comment on attachment 123874 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:237
>> +        if port_obj.get_option("builder_name"):
>> +            results['revision'] = port_obj.host.scm().head_svn_revision()
> 
> It would be nice if we had some higher-level logic that decided when to use Trac links or not. Then this code could ask that logic rather than having to have special knowledge of what other parts of the code do.

Yeah, I'm torn. We already have --use-remote-links-to-tests that is currently a noop and all the non-Chromium bots pass. I was hoping to get rid of that though. We have too many command-line flags as it is.

Or was your point that this code should be cleaner? e.g. have something like and is_on_builder method on the port object that happens to be implemented as port_obj.get_option("builder_name")?

[Ojan Vafai]

Comment on attachment 123874 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py:413
>> +        return unexpected_results
> 
> Do lines 377-413 have anything to do with this patch?

They are needed to test summarize_results. Currently the only tests for summarize_results are in printing_unittest. I mostly just copy-pasted from there to here.

[Adam Roben (:aroben)]

Comment on attachment 123874 [details]
Patch

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

>>> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:237
>>> +            results['revision'] = port_obj.host.scm().head_svn_revision()
>> 
>> It would be nice if we had some higher-level logic that decided when to use Trac links or not. Then this code could ask that logic rather than having to have special knowledge of what other parts of the code do.
> 
> Yeah, I'm torn. We already have --use-remote-links-to-tests that is currently a noop and all the non-Chromium bots pass. I was hoping to get rid of that though. We have too many command-line flags as it is.
> 
> Or was your point that this code should be cleaner? e.g. have something like and is_on_builder method on the port object that happens to be implemented as port_obj.get_option("builder_name")?

I meant that we should have some way of directly asking "will we be using Trac links to tests?". That's a lot less confusing than having this code know "having a builder_name means we're using Trac links to tests".

[Dirk Pranke]

Comment on attachment 123874 [details]
Patch

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

>>> Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py:413
>>> +        return unexpected_results
>> 
>> Do lines 377-413 have anything to do with this patch?
> 
> They are needed to test summarize_results. Currently the only tests for summarize_results are in printing_unittest. I mostly just copy-pasted from there to here.

Oh, duh, right :).

[Ojan Vafai]

Created attachment 124000 [details]
Patch

[Ojan Vafai]

Did you mean something like this?

[Adam Roben (:aroben)]

Comment on attachment 124000 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:234
> +        # We only use the svn revision for using trac links in the results.html file,
> +        # which we only need on the buildbots. Don't do this by default since it takes >100ms.
> +        if port_obj.use_trac_links_in_results_html():
> +            results['revision'] = port_obj.host.scm().head_svn_revision()

So much nicer! Though the "which we only need on the buildbots" part of the comment would probably be more appropriate inside the use_trac_links_in_results_html() method.

[Dirk Pranke]

Comment on attachment 124000 [details]
Patch

I don't think this method belongs on the Port object - different ports aren't likely to override it. Can you just put it on the Manager?

I also wonder if it would be better to have a more generic concept like "running_on_a_bot()" rather than the specific concept like "use_trac_links_in_results_html", but I'm not sure what the best name would be here.

[Dirk Pranke]

Oh, I meant to say that it looks fine otherwise, though.

[Ojan Vafai]

Created attachment 124036 [details]
Patch

[Dirk Pranke]

Comment on attachment 124036 [details]
Patch

close enough. summarize_results() should probably be a method on the Manager as well, but that function is way too big and has other problems anyway, so this is fine for now. Thanks!

[Ojan Vafai]

Committed r106035: <http://trac.webkit.org/changeset/106035>