Only store the SVN revision in the summarized results if we're on a builder
Created attachment 123874 [details] Patch
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 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 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 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 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 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 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 :).
Created attachment 124000 [details] Patch
Did you mean something like this?
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 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.
Oh, I meant to say that it looks fine otherwise, though.
Created attachment 124036 [details] Patch
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!
Committed r106035: <http://trac.webkit.org/changeset/106035>