Bug 47647 - commit-queue should not fail patches due to flaky tests
Summary: commit-queue should not fail patches due to flaky tests
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Eric Seidel (no email)
Depends on:
Reported: 2010-10-13 20:30 PDT by Eric Seidel (no email)
Modified: 2010-10-15 06:32 PDT (History)
5 users (show)

See Also:

Patch (11.91 KB, patch)
2010-10-13 20:32 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch (27.16 KB, patch)
2010-10-13 23:36 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
an alternative from this morning (14.63 KB, patch)
2010-10-14 08:51 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
combined patch (30.12 KB, patch)
2010-10-14 09:32 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch (33.06 KB, patch)
2010-10-14 10:08 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Now should work on python 2.5 (33.30 KB, patch)
2010-10-14 11:04 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Fix typo in CommitQueue.layout_test_results (untestable) (33.30 KB, patch)
2010-10-14 11:32 PDT, Eric Seidel (no email)
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2010-10-13 20:30:17 PDT
commit-queue should not fail patches due to flaky tests
Comment 1 Eric Seidel (no email) 2010-10-13 20:32:10 PDT
Created attachment 70704 [details]
Comment 2 Adam Barth 2010-10-13 20:40:24 PDT
Comment on attachment 70704 [details]

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

I like the idea, but the static information about where to find results.html isn't right.

> WebKitTools/Scripts/webkitpy/common/net/layouttestresults.py:87
> +        if not results_path:
> +            results_path = "/tmp/layout-test-results/results.html"

This doesn't seem right.

> WebKitTools/Scripts/webkitpy/tool/bot/commitqueuetask.py:124
> +        results = LayoutTestResults.results_from_local_run()

so static.  in this beautiful non-static class

I think this actually reads the disk during the commitqueuetask unit tests.  Sad face.

> WebKitTools/Scripts/webkitpy/tool/bot/commitqueuetask.py:144
> +        # The CommitQueue Command object knows its name and how to
> +        # cc the right watchers.

This comment seems unnecessary.
Comment 3 Adam Barth 2010-10-13 23:36:26 PDT
Created attachment 70713 [details]
Comment 4 Adam Barth 2010-10-13 23:38:16 PDT
Making the path a property of the port kind of led to a yak shaving expedition, but that stuff seemed worth doing.  We could probably separate the two patches, if you think that would be better.
Comment 5 WebKit Review Bot 2010-10-13 23:39:21 PDT
Attachment 70713 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKitTools/Scripts/webkitpy/tool/bot/commitqueuetask.py:34:  deprecated form of raising exception  [pep8/W602] [5]
WebKitTools/Scripts/webkitpy/tool/bot/commitqueuetask.py:37:  deprecated form of raising exception  [pep8/W602] [5]
WebKitTools/Scripts/webkitpy/tool/bot/commitqueuetask.py:40:  deprecated form of raising exception  [pep8/W602] [5]
WebKitTools/Scripts/webkitpy/tool/bot/commitqueuetask.py:43:  deprecated form of raising exception  [pep8/W602] [5]
WebKitTools/Scripts/webkitpy/tool/bot/commitqueuetask.py:46:  deprecated form of raising exception  [pep8/W602] [5]
Total errors found: 5 in 17 files

If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Eric Seidel (no email) 2010-10-14 08:21:53 PDT
I made additional changes to the patch before reading my bugmail.  I'll upload my copy soon and we can rectify the differences.
Comment 7 Eric Seidel (no email) 2010-10-14 08:51:34 PDT
Created attachment 70743 [details]
an alternative from this morning
Comment 8 Eric Seidel (no email) 2010-10-14 09:32:53 PDT
Created attachment 70745 [details]
combined patch
Comment 9 Eric Seidel (no email) 2010-10-14 10:08:41 PDT
Created attachment 70747 [details]
Comment 10 Eric Seidel (no email) 2010-10-14 11:04:11 PDT
Created attachment 70752 [details]
Now should work on python 2.5
Comment 11 Eric Seidel (no email) 2010-10-14 11:07:10 PDT
I'm running this on my local commit-queue node and it seems to work fine.
Comment 12 Eric Seidel (no email) 2010-10-14 11:32:53 PDT
Created attachment 70759 [details]
Fix typo in CommitQueue.layout_test_results (untestable)
Comment 13 Adam Barth 2010-10-14 14:02:15 PDT
Comment on attachment 70759 [details]
Fix typo in CommitQueue.layout_test_results (untestable)

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

> WebKitTools/Scripts/webkitpy/common/config/ports.py:108
> +    @classmethod
> +    def layout_tests_results_path(cls):
> +        return "/tmp/layout-test-results/results.html"

We should really change ports to be instances.  I think things should be well enough isolated to make that work now.

> WebKitTools/Scripts/webkitpy/common/net/buildbot.py:220
> +        # FIXME: We need to move this sort of 404 logic into NetworkTransaction or similar.

Or into a network abstraction.

> WebKitTools/Scripts/webkitpy/tool/bot/commitqueuetask.py:30
> +from webkitpy.common.net.layouttestresults import LayoutTestResults

This import looks to be unused.

> WebKitTools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py:60
> +    def refetch_patch(self, patch):
> +        return patch


> WebKitTools/Scripts/webkitpy/tool/commands/queues.py:278
> +        try:
> +            # FIXME: We need a nice open() abstraction for better mocking here.
> +            with codecs.open(results_path, "r", "utf-8") as results_file:
> +                return LayoutTestResults.results_from_string(results_file)
> +        except OSError, e:  # File does not exist or can't be read.
> +            return None

This whole idiom should be on tool.filesystem().read(path)

> WebKitTools/Scripts/webkitpy/tool/steps/steps_unittest.py:60
> -    def test_runtests_leopard_commit_queue_hack(self):
> +    def test_runtests_leopard_commit_queue_hack_step(self):

We have this problem of two tests with the same name a lot.  I wonder if we can scan the codebase for them somehow.
Comment 14 Eric Seidel (no email) 2010-10-14 17:51:28 PDT
Committed r69829: <http://trac.webkit.org/changeset/69829>
Comment 15 Csaba Osztrogonác 2010-10-14 23:14:24 PDT
(In reply to comment #14)
> Committed r69829: <http://trac.webkit.org/changeset/69829>

It broke Qt and GTK python unit tests with the following error message:

ERROR: test_runtests_leopard_commit_queue_hack_step (webkitpy.tool.steps.steps_unittest.StepsTest)
Traceback (most recent call last):
  File "/home/webkitbuildbot/slaves/release32bit/buildslave/qt-linux-release/build/WebKitTools/Scripts/webkitpy/tool/steps/steps_unittest.py", line 62, in test_runtests_leopard_commit_queue_hack_step
    OutputCapture().assert_outputs(self, self._run_step, [RunTests], expected_stderr=expected_stderr)
  File "/home/webkitbuildbot/slaves/release32bit/buildslave/qt-linux-release/build/WebKitTools/Scripts/webkitpy/common/system/outputcapture.py", line 62, in assert_outputs
    return_value = function(*args, **kwargs)
  File "/home/webkitbuildbot/slaves/release32bit/buildslave/qt-linux-release/build/WebKitTools/Scripts/webkitpy/tool/steps/steps_unittest.py", line 47, in _run_step
    step(tool, options).run(state)
  File "/home/webkitbuildbot/slaves/release32bit/buildslave/qt-linux-release/build/WebKitTools/Scripts/webkitpy/tool/steps/runtests.py", line 66, in run
    if self._tool.port().name() == "Mac" and self._tool.port().is_leopard():
  File "/home/webkitbuildbot/slaves/release32bit/buildslave/qt-linux-release/build/WebKitTools/Scripts/webkitpy/common/config/ports.py", line 129, in is_leopard
    return tuple(cls._system_version()[:2]) == (10, 5)
  File "/home/webkitbuildbot/slaves/release32bit/buildslave/qt-linux-release/build/WebKitTools/Scripts/webkitpy/common/config/ports.py", line 125, in _system_version
    return map(int, version_tuple)
ValueError: invalid literal for int() with base 10: ''
Comment 16 Eric Seidel (no email) 2010-10-15 06:31:54 PDT
Was fixed in bug 47713.