commit-queue should not fail patches due to flaky tests
Created attachment 70704 [details] Patch
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.
Created attachment 70713 [details] Patch
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.
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.
I made additional changes to the patch before reading my bugmail. I'll upload my copy soon and we can rectify the differences.
Created attachment 70743 [details] an alternative from this morning
Created attachment 70745 [details] combined patch
Created attachment 70747 [details] Patch
Created attachment 70752 [details] Now should work on python 2.5
I'm running this on my local commit-queue node and it seems to work fine.
Created attachment 70759 [details] Fix typo in CommitQueue.layout_test_results (untestable)
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.
Committed r69829: <http://trac.webkit.org/changeset/69829>
(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: ''
Was fixed in bug 47713.