Bug 76976 - Only store the SVN revision in the summarized results if we're on a builder
Summary: Only store the SVN revision in the summarized results if we're on a builder
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ojan Vafai
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-24 19:16 PST by Ojan Vafai
Modified: 2012-01-26 13:26 PST (History)
6 users (show)

See Also:


Attachments
Patch (6.98 KB, patch)
2012-01-24 19:17 PST, Ojan Vafai
no flags Details | Formatted Diff | Diff
Patch (7.70 KB, patch)
2012-01-25 13:33 PST, Ojan Vafai
no flags Details | Formatted Diff | Diff
Patch (7.43 KB, patch)
2012-01-25 17:01 PST, Ojan Vafai
dpranke: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2012-01-24 19:16:54 PST
Only store the SVN revision in the summarized results if we're on a builder
Comment 1 Ojan Vafai 2012-01-24 19:17:40 PST
Created attachment 123874 [details]
Patch
Comment 2 Dirk Pranke 2012-01-24 19:23:23 PST
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?
Comment 3 WebKit Review Bot 2012-01-25 04:42:34 PST
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
Comment 4 Adam Roben (:aroben) 2012-01-25 08:55:35 PST
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.
Comment 5 Ojan Vafai 2012-01-25 11:17:02 PST
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")?
Comment 6 Ojan Vafai 2012-01-25 12:42:03 PST
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.
Comment 7 Adam Roben (:aroben) 2012-01-25 12:47:17 PST
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".
Comment 8 Dirk Pranke 2012-01-25 13:05:32 PST
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 :).
Comment 9 Ojan Vafai 2012-01-25 13:33:39 PST
Created attachment 124000 [details]
Patch
Comment 10 Ojan Vafai 2012-01-25 13:33:53 PST
Did you mean something like this?
Comment 11 Adam Roben (:aroben) 2012-01-25 14:12:54 PST
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.
Comment 12 Dirk Pranke 2012-01-25 14:25:41 PST
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.
Comment 13 Dirk Pranke 2012-01-25 14:26:04 PST
Oh, I meant to say that it looks fine otherwise, though.
Comment 14 Ojan Vafai 2012-01-25 17:01:28 PST
Created attachment 124036 [details]
Patch
Comment 15 Dirk Pranke 2012-01-25 17:05:31 PST
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!
Comment 16 Ojan Vafai 2012-01-26 13:26:18 PST
Committed r106035: <http://trac.webkit.org/changeset/106035>