Diff
Modified: trunk/Tools/ChangeLog (87625 => 87626)
--- trunk/Tools/ChangeLog 2011-05-28 21:45:43 UTC (rev 87625)
+++ trunk/Tools/ChangeLog 2011-05-28 22:26:15 UTC (rev 87626)
@@ -1,3 +1,22 @@
+2011-05-28 Adam Barth <[email protected]>
+
+ Reviewed by Eric Seidel.
+
+ EWS builds patches that fail to build twice, which seems useless and slows down the bots
+ https://bugs.webkit.org/show_bug.cgi?id=55585
+
+ This patch switches all the early warning system bots over to the new
+ PatchAnalysisTask-based infrastructure. This patch makes these bots
+ more efficient (in the case where patches fail to build) and paves the
+ way for running tests on these bots!
+
+ * Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py:
+ * Scripts/webkitpy/tool/bot/earlywarningsystemtask.py:
+ * Scripts/webkitpy/tool/bot/patchanalysistask.py:
+ * Scripts/webkitpy/tool/commands/earlywarningsystem.py:
+ * Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:
+ * Scripts/webkitpy/tool/commands/queues.py:
+
2011-05-28 Kenichi Ishibashi <[email protected]>
Reviewed by Kent Tamura.
Modified: trunk/Tools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py (87625 => 87626)
--- trunk/Tools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py 2011-05-28 21:45:43 UTC (rev 87625)
+++ trunk/Tools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py 2011-05-28 22:26:15 UTC (rev 87626)
@@ -81,7 +81,10 @@
archive.filename = "mock-archive-%s.zip" % patch.id()
return archive
+ def build_style(self):
+ return "both"
+
class FailingTestCommitQueue(MockCommitQueue):
def __init__(self, error_plan, test_failure_plan):
MockCommitQueue.__init__(self, error_plan)
Modified: trunk/Tools/Scripts/webkitpy/tool/bot/earlywarningsystemtask.py (87625 => 87626)
--- trunk/Tools/Scripts/webkitpy/tool/bot/earlywarningsystemtask.py 2011-05-28 21:45:43 UTC (rev 87625)
+++ trunk/Tools/Scripts/webkitpy/tool/bot/earlywarningsystemtask.py 2011-05-28 22:26:15 UTC (rev 87626)
@@ -40,6 +40,10 @@
class EarlyWarningSystemTask(PatchAnalysisTask):
+ def __init__(self, delegate, patch, run_tests=True):
+ PatchAnalysisTask.__init__(self, delegate, patch)
+ self._run_tests = run_tests
+
def validate(self):
self._patch = self._delegate.refetch_patch(self._patch)
if self._patch.is_obsolete():
@@ -63,4 +67,6 @@
if not self._build_without_patch():
return False
return self.report_failure()
+ if not self._run_tests:
+ return True
return self._test_patch()
Modified: trunk/Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py (87625 => 87626)
--- trunk/Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py 2011-05-28 21:45:43 UTC (rev 87625)
+++ trunk/Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py 2011-05-28 22:26:15 UTC (rev 87626)
@@ -55,6 +55,9 @@
def archive_last_layout_test_results(self, patch):
raise NotImplementedError("subclasses must implement")
+ def build_style(self):
+ raise NotImplementedError("subclasses must implement")
+
# We could make results_archive optional, but for now it's required.
def report_flaky_tests(self, patch, flaky_tests, results_archive):
raise NotImplementedError("subclasses must implement")
@@ -110,7 +113,7 @@
"build",
"--no-clean",
"--no-update",
- "--build-style=both",
+ "--build-style=%s" % self._delegate.build_style(),
],
"Built patch",
"Patch does not build")
@@ -120,7 +123,7 @@
"build",
"--force-clean",
"--no-update",
- "--build-style=both",
+ "--build-style=%s" % self._delegate.build_style(),
],
"Able to build without patch",
"Unable to build without patch")
Modified: trunk/Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py (87625 => 87626)
--- trunk/Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py 2011-05-28 21:45:43 UTC (rev 87625)
+++ trunk/Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py 2011-05-28 22:26:15 UTC (rev 87626)
@@ -37,8 +37,10 @@
from webkitpy.tool.commands.queues import AbstractReviewQueue
-class AbstractEarlyWarningSystem(AbstractReviewQueue):
+class AbstractEarlyWarningSystem(AbstractReviewQueue, EarlyWarningSystemTaskDelegate):
_build_style = "release"
+ # FIXME: Switch _run_tests from opt-in to opt-out once more bots are ready to run tests.
+ _run_tests = False
def __init__(self):
AbstractReviewQueue.__init__(self)
@@ -47,84 +49,10 @@
def should_proceed_with_work_item(self, patch):
return True
- def _can_build(self):
- try:
- self.run_webkit_patch([
- "build",
- self.port.flag(),
- "--build-style=%s" % self._build_style,
- "--force-clean",
- "--no-update"])
- return True
- except ScriptError, e:
- failure_log = self._log_from_script_error_for_upload(e)
- self._update_status("Unable to perform a build", results_file=failure_log)
- return False
-
- def _build(self, patch, first_run=False):
- try:
- args = [
- "build-attachment",
- self.port.flag(),
- "--build",
- "--build-style=%s" % self._build_style,
- "--force-clean",
- "--quiet",
- "--non-interactive",
- patch.id()]
- if not first_run:
- # See commit-queue for an explanation of what we're doing here.
- args.append("--no-update")
- args.append("--parent-command=%s" % self.name)
- self.run_webkit_patch(args)
- return True
- except ScriptError, e:
- if first_run:
- return False
- raise
-
- def review_patch(self, patch):
- if patch.is_obsolete():
- self._did_error(patch, "%s does not process obsolete patches." % self.name)
- return False
-
- if patch.bug().is_closed():
- self._did_error(patch, "%s does not process patches on closed bugs." % self.name)
- return False
-
- if not self._build(patch, first_run=True):
- if not self._can_build():
- return False
- self._build(patch)
- return True
-
- @classmethod
- def _post_reject_message_on_bug(cls, tool, patch, status_id, extra_message_text=None):
- results_link = tool.status_server.results_url_for_status(status_id)
- message = "Attachment %s did not pass %s (%s):\nOutput: %s" % (patch.id(), cls.name, cls.port_name, results_link)
- # FIXME: We might want to add some text about rejecting from the commit-queue in
- # the case where patch.commit_queue() isn't already set to '-'.
- if cls.watchers:
- tool.bugs.add_cc_to_bug(patch.bug_id(), cls.watchers)
- tool.bugs.set_flag_on_attachment(patch.id(), "commit-queue", "-", message, extra_message_text)
-
- @classmethod
- def handle_script_error(cls, tool, state, script_error):
- is_svn_apply = script_error.command_name() == "svn-apply"
- status_id = cls._update_status_for_script_error(tool, state, script_error, is_error=is_svn_apply)
- if is_svn_apply:
- QueueEngine.exit_after_handled_error(script_error)
- cls._post_reject_message_on_bug(tool, state['patch'], status_id)
- exit(1)
-
-
-# FIXME: This should merge with AbstractEarlyWarningSystem once all the EWS
-# bots run tests.
-class AbstractTestingEWS(AbstractEarlyWarningSystem, EarlyWarningSystemTaskDelegate):
def begin_work_queue(self):
# FIXME: This violates abstraction
self._tool._port = self.port
- AbstractEarlyWarningSystem.begin_work_queue(self)
+ AbstractReviewQueue.begin_work_queue(self)
self._expected_failures = ExpectedFailures()
self._layout_test_results_reader = LayoutTestResultsReader(self._tool, self._log_directory())
@@ -135,8 +63,17 @@
return None
return "New failing tests:\n%s" % "\n".join(unexpected_failures)
+ def _post_reject_message_on_bug(self, tool, patch, status_id, extra_message_text=None):
+ results_link = tool.status_server.results_url_for_status(status_id)
+ message = "Attachment %s did not pass %s (%s):\nOutput: %s" % (patch.id(), self.name, self.port_name, results_link)
+ # FIXME: We might want to add some text about rejecting from the commit-queue in
+ # the case where patch.commit_queue() isn't already set to '-'.
+ if self.watchers:
+ tool.bugs.add_cc_to_bug(patch.bug_id(), self.watchers)
+ tool.bugs.set_flag_on_attachment(patch.id(), "commit-queue", "-", message, extra_message_text)
+
def review_patch(self, patch):
- task = EarlyWarningSystemTask(self, patch)
+ task = EarlyWarningSystemTask(self, patch, self._run_tests)
if not task.validate():
self._did_error(patch, "%s did not process patch." % self.name)
return False
@@ -180,6 +117,9 @@
def archive_last_layout_test_results(self, patch):
return self._layout_test_results_reader.archive(patch)
+ def build_style(self):
+ return self._build_style
+
def refetch_patch(self, patch):
return self._tool.bugs.fetch_attachment(patch.id())
@@ -234,17 +174,14 @@
]
-class ChromiumLinuxEWS(AbstractTestingEWS):
+class ChromiumLinuxEWS(AbstractChromiumEWS):
# FIXME: We should rename this command to cr-linux-ews, but that requires
# a database migration. :(
name = "chromium-ews"
port_name = "chromium-xvfb"
+ run_tests = True
- # FIXME: ChromiumLinuxEWS should inherit from AbstractChromiumEWS once
- # all the Chromium EWS bots run tests
- watchers = AbstractChromiumEWS.watchers
-
class ChromiumWindowsEWS(AbstractChromiumEWS):
name = "cr-win-ews"
Modified: trunk/Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py (87625 => 87626)
--- trunk/Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py 2011-05-28 21:45:43 UTC (rev 87625)
+++ trunk/Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py 2011-05-28 22:26:15 UTC (rev 87626)
@@ -37,54 +37,9 @@
class AbstractEarlyWarningSystemTest(QueuesTest):
- # Needed to define port_name, used in AbstractEarlyWarningSystem.__init__
- class TestEWS(AbstractEarlyWarningSystem):
- name = "mock-ews"
- port_name = "win" # Needs to be a port which port/factory understands.
-
- def test_can_build(self):
- queue = self.TestEWS()
- queue.bind_to_tool(MockTool(log_executive=True))
- queue._options = MockOptions(port=None)
- expected_stderr = "MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'build', '--port=win', '--build-style=release', '--force-clean', '--no-update']\n"
- OutputCapture().assert_outputs(self, queue._can_build, [], expected_stderr=expected_stderr)
-
- def mock_run_webkit_patch(args):
- raise ScriptError("MOCK script error")
-
- queue.run_webkit_patch = mock_run_webkit_patch
- expected_stderr = "MOCK: update_status: mock-ews Unable to perform a build\n"
- OutputCapture().assert_outputs(self, queue._can_build, [], expected_stderr=expected_stderr)
-
- # FIXME: This belongs on an AbstractReviewQueueTest object in queues_unittest.py
- def test_subprocess_handled_error(self):
- queue = AbstractReviewQueue()
- queue.bind_to_tool(MockTool())
-
- def mock_review_patch(patch):
- raise ScriptError('MOCK script error', exit_code=QueueEngine.handled_error_code)
-
- queue.review_patch = mock_review_patch
- mock_patch = queue._tool.bugs.fetch_attachment(197)
- expected_stderr = "MOCK: release_work_item: None 197\n"
- OutputCapture().assert_outputs(self, queue.process_work_item, [mock_patch], expected_stderr=expected_stderr, expected_exception=ScriptError)
-
- def test_post_reject_message_on_bug(self):
- tool = MockTool()
- patch = tool.bugs.fetch_attachment(197)
- expected_stderr = """MOCK setting flag 'commit-queue' to '-' on attachment '197' with comment 'Attachment 197 did not pass mock-ews (win):
-Output: http://dummy_url' and additional comment 'EXTRA'
-"""
- OutputCapture().assert_outputs(self,
- self.TestEWS._post_reject_message_on_bug,
- kwargs={'tool': tool, 'patch': patch, 'status_id': 1, 'extra_message_text': "EXTRA"},
- expected_stderr=expected_stderr)
-
-
-class AbstractTestingEWSTest(QueuesTest):
def test_failing_tests_message(self):
# Needed to define port_name, used in AbstractEarlyWarningSystem.__init__
- class TestEWS(AbstractTestingEWS):
+ class TestEWS(AbstractEarlyWarningSystem):
port_name = "win" # Needs to be a port which port/factory understands.
ews = TestEWS()
@@ -98,46 +53,30 @@
class EarlyWarningSytemTest(QueuesTest):
- def test_failed_builds(self):
- ews = ChromiumWindowsEWS()
- ews.bind_to_tool(MockTool())
- ews._build = lambda patch, first_run=False: False
- ews._can_build = lambda: True
- mock_patch = ews._tool.bugs.fetch_attachment(197)
- ews.review_patch(mock_patch)
-
def _default_expected_stderr(self, ews):
string_replacemnts = {
"name": ews.name,
"port": ews.port_name,
- "watchers": ews.watchers,
}
expected_stderr = {
"begin_work_queue": self._default_begin_work_queue_stderr(ews.name, ews._tool.scm().checkout_root),
"handle_unexpected_error": "Mock error message\n",
"next_work_item": "",
"process_work_item": "MOCK: update_status: %(name)s Pass\nMOCK: release_work_item: %(name)s 197\n" % string_replacemnts,
- "handle_script_error": """MOCK: update_status: %(name)s ScriptError error message
-MOCK setting flag 'commit-queue' to '-' on attachment '197' with comment 'Attachment 197 did not pass %(name)s (%(port)s):
-Output: http://dummy_url' and additional comment 'None'
-""" % string_replacemnts,
+ "handle_script_error": "ScriptError error message\n",
}
return expected_stderr
def _test_builder_ews(self, ews):
ews.bind_to_tool(MockTool())
- expected_exceptions = {
- "handle_script_error": SystemExit,
- }
- self.assert_queue_outputs(ews, expected_stderr=self._default_expected_stderr(ews), expected_exceptions=expected_exceptions)
+ self.assert_queue_outputs(ews, expected_stderr=self._default_expected_stderr(ews))
def _test_committer_only_ews(self, ews):
ews.bind_to_tool(MockTool())
expected_stderr = self._default_expected_stderr(ews)
string_replacemnts = {"name": ews.name}
expected_stderr["process_work_item"] = "MOCK: update_status: %(name)s Error: %(name)s cannot process patches from non-committers :(\nMOCK: release_work_item: %(name)s 197\n" % string_replacemnts
- expected_exceptions = {"handle_script_error": SystemExit}
- self.assert_queue_outputs(ews, expected_stderr=expected_stderr, expected_exceptions=expected_exceptions)
+ self.assert_queue_outputs(ews, expected_stderr=expected_stderr)
def _test_testing_ews(self, ews):
ews.layout_test_results = lambda: None
Modified: trunk/Tools/Scripts/webkitpy/tool/commands/queues.py (87625 => 87626)
--- trunk/Tools/Scripts/webkitpy/tool/commands/queues.py 2011-05-28 21:45:43 UTC (rev 87625)
+++ trunk/Tools/Scripts/webkitpy/tool/commands/queues.py 2011-05-28 22:26:15 UTC (rev 87626)
@@ -325,6 +325,9 @@
def archive_last_layout_test_results(self, patch):
return self._layout_test_results_reader.archive(patch)
+ def build_style(self):
+ return "both"
+
def refetch_patch(self, patch):
return self._tool.bugs.fetch_attachment(patch.id())
