Bug 47647 – commit-queue should not fail patches due to flaky tests

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

commit-queue should not fail patches due to flaky tests

[Eric Seidel (no email)]

Created attachment 70704 [details]
Patch

[Adam Barth]

Comment on attachment 70704 [details]
Patch

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.

[Adam Barth]

Created attachment 70713 [details]
Patch

[Adam Barth]

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.

[WebKit Review Bot]

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.

[Eric Seidel (no email)]

I made additional changes to the patch before reading my bugmail.  I'll upload my copy soon and we can rectify the differences.

[Eric Seidel (no email)]

Created attachment 70743 [details]
an alternative from this morning

[Eric Seidel (no email)]

Created attachment 70745 [details]
combined patch

[Eric Seidel (no email)]

Created attachment 70747 [details]
Patch

[Eric Seidel (no email)]

Created attachment 70752 [details]
Now should work on python 2.5

[Eric Seidel (no email)]

I'm running this on my local commit-queue node and it seems to work fine.

[Eric Seidel (no email)]

Created attachment 70759 [details]
Fix typo in CommitQueue.layout_test_results (untestable)

[Adam Barth]

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

Nice.

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

[Eric Seidel (no email)]

Committed r69829: <http://trac.webkit.org/changeset/69829>

[Csaba Osztrogonác]

(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: ''

[Eric Seidel (no email)]

Was fixed in bug 47713.