diff options
Diffstat (limited to 'WebKitTools/Scripts/webkitpy/tool')
33 files changed, 663 insertions, 474 deletions
diff --git a/WebKitTools/Scripts/webkitpy/tool/bot/commitqueuetask.py b/WebKitTools/Scripts/webkitpy/tool/bot/commitqueuetask.py index a347972..02e203c 100644 --- a/WebKitTools/Scripts/webkitpy/tool/bot/commitqueuetask.py +++ b/WebKitTools/Scripts/webkitpy/tool/bot/commitqueuetask.py @@ -27,19 +27,39 @@ # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. from webkitpy.common.system.executive import ScriptError +from webkitpy.common.net.layouttestresults import LayoutTestResults + + +class CommitQueueTaskDelegate(object): + def run_command(self, command): + raise NotImplementedError("subclasses must implement") + + def command_passed(self, message, patch): + raise NotImplementedError("subclasses must implement") + + def command_failed(self, message, script_error, patch): + raise NotImplementedError("subclasses must implement") + + def refetch_patch(self, patch): + raise NotImplementedError("subclasses must implement") + + def layout_test_results(self): + raise NotImplementedError("subclasses must implement") + + def report_flaky_tests(self, patch, flaky_tests): + raise NotImplementedError("subclasses must implement") class CommitQueueTask(object): - def __init__(self, tool, commit_queue, patch): - self._tool = tool - self._commit_queue = commit_queue + def __init__(self, delegate, patch): + self._delegate = delegate self._patch = patch self._script_error = None def _validate(self): # Bugs might get closed, or patches might be obsoleted or r-'d while the # commit-queue is processing. - self._patch = self._tool.bugs.fetch_attachment(self._patch.id()) + self._patch = self._delegate.refetch_patch(self._patch) if self._patch.is_obsolete(): return False if self._patch.bug().is_closed(): @@ -52,12 +72,12 @@ class CommitQueueTask(object): def _run_command(self, command, success_message, failure_message): try: - self._commit_queue.run_webkit_patch(command) - self._commit_queue.command_passed(success_message, patch=self._patch) + self._delegate.run_command(command) + self._delegate.command_passed(success_message, patch=self._patch) return True except ScriptError, e: self._script_error = e - self.failure_status_id = self._commit_queue.command_failed(failure_message, script_error=self._script_error, patch=self._patch) + self.failure_status_id = self._delegate.command_failed(failure_message, script_error=self._script_error, patch=self._patch) return False def _apply(self): @@ -76,7 +96,6 @@ class CommitQueueTask(object): "build", "--no-clean", "--no-update", - "--build", "--build-style=both", "--quiet", ], @@ -88,7 +107,6 @@ class CommitQueueTask(object): "build", "--force-clean", "--no-update", - "--build", "--build-style=both", "--quiet", ], @@ -121,6 +139,12 @@ class CommitQueueTask(object): "Able to pass tests without patch", "Unable to pass tests without patch (tree is red?)") + def _failing_tests_from_last_run(self): + results = self._delegate.layout_test_results() + if not results: + return None + return results.failing_tests() + def _land(self): return self._run_command([ "land-attachment", @@ -134,6 +158,29 @@ class CommitQueueTask(object): "Landed patch", "Unable to land patch") + def _report_flaky_tests(self, flaky_tests): + self._delegate.report_flaky_tests(self._patch, flaky_tests) + + def _test_patch(self): + if self._patch.is_rollout(): + return True + if self._test(): + return True + + first_failing_tests = self._failing_tests_from_last_run() + if self._test(): + self._report_flaky_tests(first_failing_tests) + return True + + second_failing_tests = self._failing_tests_from_last_run() + if first_failing_tests != second_failing_tests: + self._report_flaky_tests(first_failing_tests + second_failing_tests) + return False + + if self._build_and_test_without_patch(): + raise self._script_error # The error from the previous ._test() run is real, report it. + return False # Tree must be red, just retry later. + def run(self): if not self._validate(): return False @@ -143,12 +190,8 @@ class CommitQueueTask(object): if not self._build_without_patch(): return False raise self._script_error - if not self._patch.is_rollout(): - if not self._test(): - if not self._test(): - if not self._build_and_test_without_patch(): - return False - raise self._script_error + if not self._test_patch(): + return False # Make sure the patch is still valid before landing (e.g., make sure # no one has set commit-queue- since we started working on the patch.) if not self._validate(): diff --git a/WebKitTools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py b/WebKitTools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py index 8b46146..6fa0667 100644 --- a/WebKitTools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py +++ b/WebKitTools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py @@ -36,11 +36,11 @@ from webkitpy.tool.bot.commitqueuetask import * from webkitpy.tool.mocktool import MockTool -class MockCommitQueue: +class MockCommitQueue(CommitQueueTaskDelegate): def __init__(self, error_plan): self._error_plan = error_plan - def run_webkit_patch(self, command): + def run_command(self, command): log("run_webkit_patch: %s" % command) if self._error_plan: error = self._error_plan.pop(0) @@ -56,19 +56,28 @@ class MockCommitQueue: failure_message, script_error, patch.id())) return 3947 + def refetch_patch(self, patch): + return patch + + def layout_test_results(self): + return None + + def report_flaky_tests(self, patch, flaky_tests): + log("report_flaky_tests: patch='%s' flaky_tests='%s'" % (patch.id(), flaky_tests)) + class CommitQueueTaskTest(unittest.TestCase): def _run_through_task(self, commit_queue, expected_stderr, expected_exception=None): tool = MockTool(log_executive=True) patch = tool.bugs.fetch_attachment(197) - task = CommitQueueTask(tool, commit_queue, patch) + task = CommitQueueTask(commit_queue, patch) OutputCapture().assert_outputs(self, task.run, expected_stderr=expected_stderr, expected_exception=expected_exception) def test_success_case(self): commit_queue = MockCommitQueue([]) expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', '--quiet', 197] command_passed: success_message='Applied patch' patch='197' -run_webkit_patch: ['build', '--no-clean', '--no-update', '--build', '--build-style=both', '--quiet'] +run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both', '--quiet'] command_passed: success_message='Built patch' patch='197' run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive'] command_passed: success_message='Passed tests' patch='197' @@ -93,9 +102,9 @@ command_failed: failure_message='Patch does not apply' script_error='MOCK apply ]) expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', '--quiet', 197] command_passed: success_message='Applied patch' patch='197' -run_webkit_patch: ['build', '--no-clean', '--no-update', '--build', '--build-style=both', '--quiet'] +run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both', '--quiet'] command_failed: failure_message='Patch does not build' script_error='MOCK build failure' patch='197' -run_webkit_patch: ['build', '--force-clean', '--no-update', '--build', '--build-style=both', '--quiet'] +run_webkit_patch: ['build', '--force-clean', '--no-update', '--build-style=both', '--quiet'] command_passed: success_message='Able to build without patch' patch='197' """ self._run_through_task(commit_queue, expected_stderr, ScriptError) @@ -108,9 +117,9 @@ command_passed: success_message='Able to build without patch' patch='197' ]) expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', '--quiet', 197] command_passed: success_message='Applied patch' patch='197' -run_webkit_patch: ['build', '--no-clean', '--no-update', '--build', '--build-style=both', '--quiet'] +run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both', '--quiet'] command_failed: failure_message='Patch does not build' script_error='MOCK build failure' patch='197' -run_webkit_patch: ['build', '--force-clean', '--no-update', '--build', '--build-style=both', '--quiet'] +run_webkit_patch: ['build', '--force-clean', '--no-update', '--build-style=both', '--quiet'] command_failed: failure_message='Unable to build without patch' script_error='MOCK clean build failure' patch='197' """ self._run_through_task(commit_queue, expected_stderr) @@ -123,12 +132,13 @@ command_failed: failure_message='Unable to build without patch' script_error='MO ]) expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', '--quiet', 197] command_passed: success_message='Applied patch' patch='197' -run_webkit_patch: ['build', '--no-clean', '--no-update', '--build', '--build-style=both', '--quiet'] +run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both', '--quiet'] command_passed: success_message='Built patch' patch='197' run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive'] command_failed: failure_message='Patch does not pass tests' script_error='MOCK tests failure' patch='197' run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive'] command_passed: success_message='Passed tests' patch='197' +report_flaky_tests: patch='197' flaky_tests='None' run_webkit_patch: ['land-attachment', '--force-clean', '--ignore-builders', '--quiet', '--non-interactive', '--parent-command=commit-queue', 197] command_passed: success_message='Landed patch' patch='197' """ @@ -143,7 +153,7 @@ command_passed: success_message='Landed patch' patch='197' ]) expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', '--quiet', 197] command_passed: success_message='Applied patch' patch='197' -run_webkit_patch: ['build', '--no-clean', '--no-update', '--build', '--build-style=both', '--quiet'] +run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both', '--quiet'] command_passed: success_message='Built patch' patch='197' run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive'] command_failed: failure_message='Patch does not pass tests' script_error='MOCK test failure' patch='197' @@ -164,7 +174,7 @@ command_passed: success_message='Able to pass tests without patch' patch='197' ]) expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', '--quiet', 197] command_passed: success_message='Applied patch' patch='197' -run_webkit_patch: ['build', '--no-clean', '--no-update', '--build', '--build-style=both', '--quiet'] +run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both', '--quiet'] command_passed: success_message='Built patch' patch='197' run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive'] command_failed: failure_message='Patch does not pass tests' script_error='MOCK test failure' patch='197' @@ -184,7 +194,7 @@ command_failed: failure_message='Unable to pass tests without patch (tree is red ]) expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', '--quiet', 197] command_passed: success_message='Applied patch' patch='197' -run_webkit_patch: ['build', '--no-clean', '--no-update', '--build', '--build-style=both', '--quiet'] +run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both', '--quiet'] command_passed: success_message='Built patch' patch='197' run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive'] command_passed: success_message='Passed tests' patch='197' diff --git a/WebKitTools/Scripts/webkitpy/tool/bot/feeders.py b/WebKitTools/Scripts/webkitpy/tool/bot/feeders.py index 15eaaf3..dc892a4 100644 --- a/WebKitTools/Scripts/webkitpy/tool/bot/feeders.py +++ b/WebKitTools/Scripts/webkitpy/tool/bot/feeders.py @@ -28,18 +28,15 @@ from webkitpy.common.system.deprecated_logging import log from webkitpy.common.net.bugzilla import CommitterValidator +from webkitpy.tool.grammar import pluralize class AbstractFeeder(object): def __init__(self, tool): self._tool = tool - def feed(tool): - raise NotImplementedError, "subclasses must implement" - - def update_work_items(self, item_ids): - self._tool.status_server.update_work_items(self.queue_name, item_ids) - log("Feeding %s items %s" % (self.queue_name, item_ids)) + def feed(self): + raise NotImplementedError("subclasses must implement") class CommitQueueFeeder(AbstractFeeder): @@ -49,11 +46,17 @@ class CommitQueueFeeder(AbstractFeeder): AbstractFeeder.__init__(self, tool) self.committer_validator = CommitterValidator(self._tool.bugs) + def _update_work_items(self, item_ids): + # FIXME: This is the last use of update_work_items, the commit-queue + # should move to feeding patches one at a time like the EWS does. + self._tool.status_server.update_work_items(self.queue_name, item_ids) + log("Feeding %s items %s" % (self.queue_name, item_ids)) + def feed(self): patches = self._validate_patches() patches = sorted(patches, self._patch_cmp) patch_ids = [patch.id() for patch in patches] - self.update_work_items(patch_ids) + self._update_work_items(patch_ids) def _patches_for_bug(self, bug_id): return self._tool.bugs.fetch_bug(bug_id).commit_queued_patches(include_invalid=True) @@ -71,3 +74,17 @@ class CommitQueueFeeder(AbstractFeeder): if rollout_cmp != 0: return rollout_cmp return cmp(a.attach_date(), b.attach_date()) + + +class EWSFeeder(AbstractFeeder): + def __init__(self, tool): + self._ids_sent_to_server = set() + AbstractFeeder.__init__(self, tool) + + def feed(self): + ids_needing_review = set(self._tool.bugs.queries.fetch_attachment_ids_from_review_queue()) + new_ids = ids_needing_review.difference(self._ids_sent_to_server) + log("Feeding EWS (%s, %s new)" % (pluralize("r? patch", len(ids_needing_review)), len(new_ids))) + for attachment_id in new_ids: # Order doesn't really matter for the EWS. + self._tool.status_server.submit_to_ews(attachment_id) + self._ids_sent_to_server.add(attachment_id) diff --git a/WebKitTools/Scripts/webkitpy/tool/bot/patchcollection.py b/WebKitTools/Scripts/webkitpy/tool/bot/patchcollection.py deleted file mode 100644 index 6100cf8..0000000 --- a/WebKitTools/Scripts/webkitpy/tool/bot/patchcollection.py +++ /dev/null @@ -1,78 +0,0 @@ -# Copyright (c) 2010 Google Inc. All rights reserved. -# -# Redistribution and use in source and binary forms, with or without -# modification, are permitted provided that the following conditions are -# met: -# -# * Redistributions of source code must retain the above copyright -# notice, this list of conditions and the following disclaimer. -# * Redistributions in binary form must reproduce the above -# copyright notice, this list of conditions and the following disclaimer -# in the documentation and/or other materials provided with the -# distribution. -# * Neither the name of Google Inc. nor the names of its -# contributors may be used to endorse or promote products derived from -# this software without specific prior written permission. -# -# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS -# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT -# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR -# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT -# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, -# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT -# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, -# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY -# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT -# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE -# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. - - -class PersistentPatchCollectionDelegate: - def collection_name(self): - raise NotImplementedError, "subclasses must implement" - - def fetch_potential_patch_ids(self): - raise NotImplementedError, "subclasses must implement" - - def status_server(self): - raise NotImplementedError, "subclasses must implement" - - def is_terminal_status(self, status): - raise NotImplementedError, "subclasses must implement" - - -class PersistentPatchCollection: - def __init__(self, delegate): - self._delegate = delegate - self._name = self._delegate.collection_name() - self._status = self._delegate.status_server() - self._status_cache = {} - - def _cached_status(self, patch_id): - cached = self._status_cache.get(patch_id) - if cached: - return cached - status = self._status.patch_status(self._name, patch_id) - if status and self._delegate.is_terminal_status(status): - self._status_cache[patch_id] = status - return status - - def _is_active_patch_id(self, patch_id): - """Active patches are patches waiting to be processed from this collection.""" - status = self._cached_status(patch_id) - return not status or not self._delegate.is_terminal_status(status) - - def _fetch_active_patch_ids(self): - patch_ids = self._delegate.fetch_potential_patch_ids() - return filter(lambda patch_id: self._is_active_patch_id(patch_id), patch_ids) - - def next(self): - # Note: We only fetch all the ids so we can post them back to the server. - # This will go away once we have a feeder queue and all other queues are - # just pulling their next work item from the server. - patch_ids = self._fetch_active_patch_ids() - # FIXME: We're assuming self._name is a valid queue-name. - self._status.update_work_items(self._name, patch_ids) - if not patch_ids: - return None - return patch_ids[0] diff --git a/WebKitTools/Scripts/webkitpy/tool/bot/queueengine.py b/WebKitTools/Scripts/webkitpy/tool/bot/queueengine.py index 8118653..8b016e8 100644 --- a/WebKitTools/Scripts/webkitpy/tool/bot/queueengine.py +++ b/WebKitTools/Scripts/webkitpy/tool/bot/queueengine.py @@ -125,8 +125,8 @@ class QueueEngine: traceback.print_exc() # Don't try tell the status bot, in case telling it causes an exception. self._sleep("Exception while preparing queue") - # Never reached. - self._ensure_work_log_closed() + self._stopping("Delegate terminated queue.") + return 0 def _stopping(self, message): log("\n%s" % message) diff --git a/WebKitTools/Scripts/webkitpy/tool/bot/queueengine_unittest.py b/WebKitTools/Scripts/webkitpy/tool/bot/queueengine_unittest.py index bfec401..37d8502 100644 --- a/WebKitTools/Scripts/webkitpy/tool/bot/queueengine_unittest.py +++ b/WebKitTools/Scripts/webkitpy/tool/bot/queueengine_unittest.py @@ -43,6 +43,7 @@ class LoggingDelegate(QueueEngineDelegate): self._test = test self._callbacks = [] self._run_before = False + self.stop_message = None expected_callbacks = [ 'queue_log_path', @@ -52,7 +53,8 @@ class LoggingDelegate(QueueEngineDelegate): 'should_proceed_with_work_item', 'work_item_log_path', 'process_work_item', - 'should_continue_work_queue' + 'should_continue_work_queue', + 'stop_work_queue', ] def record(self, method_name): @@ -95,21 +97,20 @@ class LoggingDelegate(QueueEngineDelegate): self.record("handle_unexpected_error") self._test.assertEquals(work_item, "work_item") + def stop_work_queue(self, message): + self.record("stop_work_queue") + self.stop_message = message + class RaisingDelegate(LoggingDelegate): def __init__(self, test, exception): LoggingDelegate.__init__(self, test) self._exception = exception - self.stop_message = None def process_work_item(self, work_item): self.record("process_work_item") raise self._exception - def stop_work_queue(self, message): - self.record("stop_work_queue") - self.stop_message = message - class NotSafeToProceedDelegate(LoggingDelegate): def should_proceed_with_work_item(self, work_item): @@ -132,16 +133,15 @@ class FastQueueEngine(QueueEngine): class QueueEngineTest(unittest.TestCase): def test_trivial(self): delegate = LoggingDelegate(self) - work_queue = QueueEngine("trivial-queue", delegate, threading.Event()) - work_queue.run() + self._run_engine(delegate) + self.assertEquals(delegate.stop_message, "Delegate terminated queue.") self.assertEquals(delegate._callbacks, LoggingDelegate.expected_callbacks) self.assertTrue(os.path.exists(os.path.join(self.temp_dir, "queue_log_path"))) self.assertTrue(os.path.exists(os.path.join(self.temp_dir, "work_log_path", "work_item.log"))) def test_unexpected_error(self): delegate = RaisingDelegate(self, ScriptError(exit_code=3)) - work_queue = QueueEngine("error-queue", delegate, threading.Event()) - work_queue.run() + self._run_engine(delegate) expected_callbacks = LoggingDelegate.expected_callbacks[:] work_item_index = expected_callbacks.index('process_work_item') # The unexpected error should be handled right after process_work_item starts @@ -151,11 +151,18 @@ class QueueEngineTest(unittest.TestCase): def test_handled_error(self): delegate = RaisingDelegate(self, ScriptError(exit_code=QueueEngine.handled_error_code)) - work_queue = QueueEngine("handled-error-queue", delegate, threading.Event()) - work_queue.run() + self._run_engine(delegate) self.assertEquals(delegate._callbacks, LoggingDelegate.expected_callbacks) - def _test_terminating_queue(self, exception, expected_message): + def _run_engine(self, delegate, engine=None, termination_message=None): + if not engine: + engine = QueueEngine("test-queue", delegate, threading.Event()) + if not termination_message: + termination_message = "Delegate terminated queue." + expected_stderr = "\n%s\n" % termination_message + OutputCapture().assert_outputs(self, engine.run, [], expected_stderr=expected_stderr) + + def _test_terminating_queue(self, exception, termination_message): work_item_index = LoggingDelegate.expected_callbacks.index('process_work_item') # The terminating error should be handled right after process_work_item. # There should be no other callbacks after stop_work_queue. @@ -163,14 +170,10 @@ class QueueEngineTest(unittest.TestCase): expected_callbacks.append("stop_work_queue") delegate = RaisingDelegate(self, exception) - work_queue = QueueEngine("terminating-queue", delegate, threading.Event()) - - output = OutputCapture() - expected_stderr = "\n%s\n" % expected_message - output.assert_outputs(self, work_queue.run, [], expected_stderr=expected_stderr) + self._run_engine(delegate, termination_message=termination_message) self.assertEquals(delegate._callbacks, expected_callbacks) - self.assertEquals(delegate.stop_message, expected_message) + self.assertEquals(delegate.stop_message, termination_message) def test_terminating_error(self): self._test_terminating_queue(KeyboardInterrupt(), "User terminated queue.") @@ -178,15 +181,10 @@ class QueueEngineTest(unittest.TestCase): def test_not_safe_to_proceed(self): delegate = NotSafeToProceedDelegate(self) - work_queue = FastQueueEngine(delegate) - work_queue.run() + self._run_engine(delegate, engine=FastQueueEngine(delegate)) expected_callbacks = LoggingDelegate.expected_callbacks[:] - next_work_item_index = expected_callbacks.index('next_work_item') - # We slice out the common part of the expected callbacks. - # We add 2 here to include should_proceed_with_work_item, which is - # a pain to search for directly because it occurs twice. - expected_callbacks = expected_callbacks[:next_work_item_index + 2] - expected_callbacks.append('should_continue_work_queue') + expected_callbacks.remove('work_item_log_path') + expected_callbacks.remove('process_work_item') self.assertEquals(delegate._callbacks, expected_callbacks) def test_now(self): diff --git a/WebKitTools/Scripts/webkitpy/tool/bot/sheriff.py b/WebKitTools/Scripts/webkitpy/tool/bot/sheriff.py index a38c3cf..da506bc 100644 --- a/WebKitTools/Scripts/webkitpy/tool/bot/sheriff.py +++ b/WebKitTools/Scripts/webkitpy/tool/bot/sheriff.py @@ -77,55 +77,15 @@ class Sheriff(object): ]) return parse_bug_id(output) - def _rollout_reason(self, builders): - # FIXME: This should explain which layout tests failed - # however, that would require Build objects here, either passed - # in through failure_info, or through Builder.latest_build. - names = [builder.name() for builder in builders] - return "Caused builders %s to fail." % join_with_separators(names) - - def post_automatic_rollout_patch(self, commit_info, builders): - # For now we're only posting rollout patches for commit-queue patches. - commit_bot_email = "eseidel@chromium.org" - if commit_bot_email == commit_info.committer_email(): - try: - self.post_rollout_patch(commit_info.revision(), - self._rollout_reason(builders)) - except ScriptError, e: - log("Failed to create-rollout.") - - def post_blame_comment_on_bug(self, commit_info, builders, blame_list): + def post_blame_comment_on_bug(self, commit_info, builders, tests): if not commit_info.bug_id(): return comment = "%s might have broken %s" % ( view_source_url(commit_info.revision()), join_with_separators([builder.name() for builder in builders])) - if len(blame_list) > 1: - comment += "\nThe following changes are on the blame list:\n" - comment += "\n".join(map(view_source_url, blame_list)) + if tests: + comment += "\nThe following tests are not passing:\n" + comment += "\n".join(tests) self._tool.bugs.post_comment_to_bug(commit_info.bug_id(), comment, cc=self._sheriffbot.watchers) - - # FIXME: Should some of this logic be on BuildBot? - def provoke_flaky_builders(self, revisions_causing_failures): - # We force_build builders that are red but have not "failed" (i.e., - # been red twice). We do this to avoid a deadlock situation where a - # flaky test blocks the commit-queue and there aren't any other - # patches being landed to re-spin the builder. - failed_builders = sum([revisions_causing_failures[key] for - key in revisions_causing_failures.keys()], []) - failed_builder_names = \ - set([builder.name() for builder in failed_builders]) - idle_red_builder_names = \ - set([builder["name"] - for builder in self._tool.buildbot.idle_red_core_builders()]) - - # We only want to provoke these builders if they are idle and have not - # yet "failed" (i.e., been red twice) to avoid overloading the bots. - flaky_builder_names = idle_red_builder_names - failed_builder_names - - for name in flaky_builder_names: - flaky_builder = self._tool.buildbot.builder_with_name(name) - flaky_builder.force_build(username=self._sheriffbot.name, - comments="Probe for flakiness.") diff --git a/WebKitTools/Scripts/webkitpy/tool/bot/sheriff_unittest.py b/WebKitTools/Scripts/webkitpy/tool/bot/sheriff_unittest.py index c375ff9..690af1f 100644 --- a/WebKitTools/Scripts/webkitpy/tool/bot/sheriff_unittest.py +++ b/WebKitTools/Scripts/webkitpy/tool/bot/sheriff_unittest.py @@ -47,15 +47,6 @@ class MockSheriffBot(object): class SheriffTest(unittest.TestCase): - def test_rollout_reason(self): - sheriff = Sheriff(MockTool(), MockSheriffBot()) - builders = [ - Builder("Foo", None), - Builder("Bar", None), - ] - reason = "Caused builders Foo and Bar to fail." - self.assertEquals(sheriff._rollout_reason(builders), reason) - def test_post_blame_comment_on_bug(self): def run(): sheriff = Sheriff(MockTool(), MockSheriffBot()) @@ -68,38 +59,32 @@ class SheriffTest(unittest.TestCase): commit_info.revision = lambda: 4321 # Should do nothing with no bug_id sheriff.post_blame_comment_on_bug(commit_info, builders, []) - sheriff.post_blame_comment_on_bug(commit_info, builders, [2468, 5646]) + sheriff.post_blame_comment_on_bug(commit_info, builders, ["mock-test-1", "mock-test-2"]) # Should try to post a comment to the bug, but MockTool.bugs does nothing. commit_info.bug_id = lambda: 1234 sheriff.post_blame_comment_on_bug(commit_info, builders, []) - sheriff.post_blame_comment_on_bug(commit_info, builders, [3432]) - sheriff.post_blame_comment_on_bug(commit_info, builders, [841, 5646]) + sheriff.post_blame_comment_on_bug(commit_info, builders, ["mock-test-1"]) + sheriff.post_blame_comment_on_bug(commit_info, builders, ["mock-test-1", "mock-test-2"]) - expected_stderr = u"MOCK bug comment: bug_id=1234, cc=['watcher@example.com']\n--- Begin comment ---\\http://trac.webkit.org/changeset/4321 might have broken Foo and Bar\n--- End comment ---\n\nMOCK bug comment: bug_id=1234, cc=['watcher@example.com']\n--- Begin comment ---\\http://trac.webkit.org/changeset/4321 might have broken Foo and Bar\n--- End comment ---\n\nMOCK bug comment: bug_id=1234, cc=['watcher@example.com']\n--- Begin comment ---\\http://trac.webkit.org/changeset/4321 might have broken Foo and Bar\nThe following changes are on the blame list:\nhttp://trac.webkit.org/changeset/841\nhttp://trac.webkit.org/changeset/5646\n--- End comment ---\n\n" - OutputCapture().assert_outputs(self, run, expected_stderr=expected_stderr) + expected_stderr = u"""MOCK bug comment: bug_id=1234, cc=['watcher@example.com'] +--- Begin comment --- +http://trac.webkit.org/changeset/4321 might have broken Foo and Bar +--- End comment --- - def test_provoke_flaky_builders(self): - def run(): - tool = MockTool() - tool.buildbot.light_tree_on_fire() - sheriff = Sheriff(tool, MockSheriffBot()) - revisions_causing_failures = {} - sheriff.provoke_flaky_builders(revisions_causing_failures) - expected_stderr = "MOCK: force_build: name=Builder2, username=mock-sheriff-bot, comments=Probe for flakiness.\n" - OutputCapture().assert_outputs(self, run, expected_stderr=expected_stderr) +MOCK bug comment: bug_id=1234, cc=['watcher@example.com'] +--- Begin comment --- +http://trac.webkit.org/changeset/4321 might have broken Foo and Bar +The following tests are not passing: +mock-test-1 +--- End comment --- - def test_post_blame_comment_on_bug(self): - sheriff = Sheriff(MockTool(), MockSheriffBot()) - builders = [ - Builder("Foo", None), - Builder("Bar", None), - ] - commit_info = Mock() - commit_info.bug_id = lambda: None - commit_info.revision = lambda: 4321 - commit_info.committer = lambda: None - commit_info.committer_email = lambda: "foo@example.com" - commit_info.reviewer = lambda: None - commit_info.author = lambda: None - sheriff.post_automatic_rollout_patch(commit_info, builders) +MOCK bug comment: bug_id=1234, cc=['watcher@example.com'] +--- Begin comment --- +http://trac.webkit.org/changeset/4321 might have broken Foo and Bar +The following tests are not passing: +mock-test-1 +mock-test-2 +--- End comment --- +""" + OutputCapture().assert_outputs(self, run, expected_stderr=expected_stderr) diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/commandtest.py b/WebKitTools/Scripts/webkitpy/tool/commands/commandtest.py index de92cd3..adc6d81 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/commandtest.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/commandtest.py @@ -33,5 +33,16 @@ from webkitpy.tool.mocktool import MockOptions, MockTool class CommandsTest(unittest.TestCase): def assert_execute_outputs(self, command, args, expected_stdout="", expected_stderr="", options=MockOptions(), tool=MockTool()): + options.blocks = True + options.cc = 'MOCK cc' + options.component = 'MOCK component' + options.confirm = True + options.email = 'MOCK email' + options.git_commit = 'MOCK git commit' + options.obsolete_patches = True + options.open_bug = True + options.port = 'MOCK port' + options.quiet = True + options.reviewer = 'MOCK reviewer' command.bind_to_tool(tool) OutputCapture().assert_outputs(self, command.execute, [options, args, tool], expected_stdout=expected_stdout, expected_stderr=expected_stderr) diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/download.py b/WebKitTools/Scripts/webkitpy/tool/commands/download.py index 9916523..ed5c604 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/download.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/download.py @@ -43,6 +43,17 @@ from webkitpy.tool.multicommandtool import AbstractDeclarativeCommand from webkitpy.common.system.deprecated_logging import error, log +class Clean(AbstractSequencedCommand): + name = "clean" + help_text = "Clean the working copy" + steps = [ + steps.CleanWorkingDirectory, + ] + + def _prepare_state(self, options, args, tool): + options.force_clean = True + + class Update(AbstractSequencedCommand): name = "update" help_text = "Update working copy (used internally)" @@ -61,6 +72,9 @@ class Build(AbstractSequencedCommand): steps.Build, ] + def _prepare_state(self, options, args, tool): + options.build = True + class BuildAndTest(AbstractSequencedCommand): name = "build-and-test" diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/download_unittest.py b/WebKitTools/Scripts/webkitpy/tool/commands/download_unittest.py index faddd50..6af1f64 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/download_unittest.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/download_unittest.py @@ -57,15 +57,19 @@ class AbstractRolloutPrepCommandTest(unittest.TestCase): class DownloadCommandsTest(CommandsTest): def _default_options(self): options = MockOptions() - options.force_clean = False - options.clean = True + options.build = True + options.build_style = True options.check_builders = True - options.quiet = False + options.check_style = True + options.clean = True + options.close_bug = True + options.force_clean = False + options.force_patch = True options.non_interactive = False - options.update = True - options.build = True + options.parent_command = 'MOCK parent command' + options.quiet = False options.test = True - options.close_bug = True + options.update = True return options def test_build(self): diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem.py b/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem.py index 86e2e15..5ec468e 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem.py @@ -48,7 +48,6 @@ class AbstractEarlyWarningSystem(AbstractReviewQueue): self.run_webkit_patch([ "build", self.port.flag(), - "--build", "--build-style=%s" % self._build_style, "--force-clean", "--no-update", @@ -149,10 +148,6 @@ class ChromiumWindowsEWS(AbstractChromiumEWS): name = "cr-win-ews" -class ChromiumMacEWS(AbstractChromiumEWS): - name = "cr-mac-ews" - - # For platforms that we can't run inside a VM (like Mac OS X), we require # patches to be uploaded by committers, who are generally trustworthy folk. :) class AbstractCommitterOnlyEWS(AbstractEarlyWarningSystem): @@ -167,6 +162,14 @@ class AbstractCommitterOnlyEWS(AbstractEarlyWarningSystem): return AbstractEarlyWarningSystem.process_work_item(self, patch) +# FIXME: Inheriting from AbstractCommitterOnlyEWS is kinda a hack, but it +# happens to work because AbstractChromiumEWS and AbstractCommitterOnlyEWS +# provide disjoint sets of functionality, and Python is otherwise smart +# enough to handle the diamond inheritance. +class ChromiumMacEWS(AbstractChromiumEWS, AbstractCommitterOnlyEWS): + name = "cr-mac-ews" + + class MacEWS(AbstractCommitterOnlyEWS): name = "mac-ews" port_name = "mac" diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py b/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py index 3b0ea47..c400f81 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py @@ -48,9 +48,9 @@ class EarlyWarningSytemTest(QueuesTest): expected_stderr = { "begin_work_queue": self._default_begin_work_queue_stderr(ews.name, os.getcwd()), # FIXME: Use of os.getcwd() is wrong, should be scm.checkout_root "handle_unexpected_error": "Mock error message\n", - "next_work_item": "MOCK: update_work_items: %(name)s [103]\n" % string_replacemnts, - "process_work_item": "MOCK: update_status: %(name)s Pass\n" % string_replacemnts, - "handle_script_error": "MOCK: update_status: %(name)s ScriptError error message\nMOCK bug comment: bug_id=142, cc=%(watchers)s\n--- Begin comment ---\\Attachment 197 did not build on %(port)s:\nBuild output: http://dummy_url\n--- End comment ---\n\n" % string_replacemnts, + "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\nMOCK bug comment: bug_id=142, cc=%(watchers)s\n--- Begin comment ---\nAttachment 197 did not build on %(port)s:\nBuild output: http://dummy_url\n--- End comment ---\n\n" % string_replacemnts, } return expected_stderr @@ -85,3 +85,12 @@ class EarlyWarningSytemTest(QueuesTest): "handle_script_error": SystemExit, } self.assert_queue_outputs(ews, expected_stderr=expected_stderr, expected_exceptions=expected_exceptions) + + def test_chromium_mac_ews(self): + ews = ChromiumMacEWS() + expected_stderr = self._default_expected_stderr(ews) + expected_stderr["process_work_item"] = "MOCK: update_status: cr-mac-ews Error: cr-mac-ews cannot process patches from non-committers :(\n" + expected_exceptions = { + "handle_script_error": SystemExit, + } + self.assert_queue_outputs(ews, expected_stderr=expected_stderr, expected_exceptions=expected_exceptions) diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/queries.py b/WebKitTools/Scripts/webkitpy/tool/commands/queries.py index c6e45aa..16ddc2c 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/queries.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/queries.py @@ -30,6 +30,8 @@ from optparse import make_option +import webkitpy.tool.steps as steps + from webkitpy.common.checkout.commitinfo import CommitInfo from webkitpy.common.config.committers import CommitterList from webkitpy.common.net.buildbot import BuildBot @@ -41,6 +43,21 @@ from webkitpy.common.system.deprecated_logging import log from webkitpy.layout_tests import port +class SuggestReviewers(AbstractDeclarativeCommand): + name = "suggest-reviewers" + help_text = "Suggest reviewers for a patch based on recent changes to the modified files." + + def __init__(self): + options = [ + steps.Options.git_commit, + ] + AbstractDeclarativeCommand.__init__(self, options=options) + + def execute(self, options, args, tool): + reviewers = tool.checkout().suggested_reviewers(options.git_commit) + print "\n".join([reviewer.full_name for reviewer in reviewers]) + + class BugsToCommit(AbstractDeclarativeCommand): name = "bugs-to-commit" help_text = "List bugs in the commit-queue" @@ -162,15 +179,6 @@ class WhatBroke(AbstractDeclarativeCommand): print "All builders are passing!" -class WhoBrokeIt(AbstractDeclarativeCommand): - name = "who-broke-it" - help_text = "Print a list of revisions causing failures on %s" % BuildBot.default_host - - def execute(self, options, args, tool): - for revision, builders in self._tool.buildbot.failure_map(False).revisions_causing_failures().items(): - print "r%s appears to have broken %s" % (revision, [builder.name() for builder in builders]) - - class ResultsFor(AbstractDeclarativeCommand): name = "results-for" help_text = "Print a list of failures for the passed revision from bots on %s" % BuildBot.default_host @@ -197,17 +205,21 @@ class FailureReason(AbstractDeclarativeCommand): name = "failure-reason" help_text = "Lists revisions where individual test failures started at %s" % BuildBot.default_host - def _print_blame_information_for_transition(self, green_build, red_build, failing_tests): - regression_window = RegressionWindow(green_build, red_build) - revisions = regression_window.revisions() + def _blame_line_for_revision(self, revision): + try: + commit_info = self._tool.checkout().commit_info_for_revision(revision) + except Exception, e: + return "FAILED to fetch CommitInfo for r%s, exception: %s" % (revision, e) + if not commit_info: + return "FAILED to fetch CommitInfo for r%s, likely missing ChangeLog" % revision + return commit_info.blame_string(self._tool.bugs) + + def _print_blame_information_for_transition(self, regression_window, failing_tests): + red_build = regression_window.failing_build() print "SUCCESS: Build %s (r%s) was the first to show failures: %s" % (red_build._number, red_build.revision(), failing_tests) print "Suspect revisions:" - for revision in revisions: - commit_info = self._tool.checkout().commit_info_for_revision(revision) - if commit_info: - print commit_info.blame_string(self._tool.bugs) - else: - print "FAILED to fetch CommitInfo for r%s, likely missing ChangeLog" % revision + for revision in regression_window.revisions(): + print self._blame_line_for_revision(revision) def _explain_failures_for_builder(self, builder, start_revision): print "Examining failures for \"%s\", starting at r%s" % (builder.name(), start_revision) @@ -244,7 +256,8 @@ class FailureReason(AbstractDeclarativeCommand): print "No change in build %s (r%s), %s unexplained failures (%s in this build)" % (build._number, build.revision(), len(results_to_explain), len(failures)) last_build_with_results = build continue - self._print_blame_information_for_transition(build, last_build_with_results, fixed_results) + regression_window = RegressionWindow(build, last_build_with_results) + self._print_blame_information_for_transition(regression_window, fixed_results) last_build_with_results = build results_to_explain -= fixed_results if results_to_explain: @@ -274,6 +287,75 @@ class FailureReason(AbstractDeclarativeCommand): return self._explain_failures_for_builder(builder, start_revision=int(start_revision)) +class FindFlakyTests(AbstractDeclarativeCommand): + name = "find-flaky-tests" + help_text = "Lists tests that often fail for a single build at %s" % BuildBot.default_host + + def _find_failures(self, builder, revision): + build = builder.build_for_revision(revision, allow_failed_lookups=True) + if not build: + print "No build for %s" % revision + return (None, None) + results = build.layout_test_results() + if not results: + print "No results build %s (r%s)" % (build._number, build.revision()) + return (None, None) + failures = set(results.failing_tests()) + if len(failures) >= 20: + # FIXME: We may need to move this logic into the LayoutTestResults class. + # The buildbot stops runs after 20 failures so we don't have full results to work with here. + print "Too many failures in build %s (r%s), ignoring." % (build._number, build.revision()) + return (None, None) + return (build, failures) + + def _increment_statistics(self, flaky_tests, flaky_test_statistics): + for test in flaky_tests: + count = flaky_test_statistics.get(test, 0) + flaky_test_statistics[test] = count + 1 + + def _print_statistics(self, statistics): + print "=== Results ===" + print "Occurances Test name" + for value, key in sorted([(value, key) for key, value in statistics.items()]): + print "%10d %s" % (value, key) + + def _walk_backwards_from(self, builder, start_revision, limit): + flaky_test_statistics = {} + all_previous_failures = set([]) + one_time_previous_failures = set([]) + previous_build = None + for i in range(limit): + revision = start_revision - i + print "Analyzing %s ... " % revision, + (build, failures) = self._find_failures(builder, revision) + if failures == None: + # Notice that we don't loop on the empty set! + continue + print "has %s failures" % len(failures) + flaky_tests = one_time_previous_failures - failures + if flaky_tests: + print "Flaky tests: %s %s" % (sorted(flaky_tests), + previous_build.results_url()) + self._increment_statistics(flaky_tests, flaky_test_statistics) + one_time_previous_failures = failures - all_previous_failures + all_previous_failures = failures + previous_build = build + self._print_statistics(flaky_test_statistics) + + def _builder_to_analyze(self): + statuses = self._tool.buildbot.builder_statuses() + choices = [status["name"] for status in statuses] + chosen_name = User.prompt_with_list("Which builder to analyze:", choices) + for status in statuses: + if status["name"] == chosen_name: + return (self._tool.buildbot.builder_with_name(chosen_name), status["built_revision"]) + + def execute(self, options, args, tool): + (builder, latest_revision) = self._builder_to_analyze() + limit = self._tool.user.prompt("How many revisions to look through? [10000] ") or 10000 + return self._walk_backwards_from(builder, latest_revision, limit=int(limit)) + + class TreeStatus(AbstractDeclarativeCommand): name = "tree-status" help_text = "Print the status of the %s buildbots" % BuildBot.default_host diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/queries_unittest.py b/WebKitTools/Scripts/webkitpy/tool/commands/queries_unittest.py index 7dddfe7..05a4a5c 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/queries_unittest.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/queries_unittest.py @@ -26,12 +26,15 @@ # (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +import unittest + from webkitpy.common.net.bugzilla import Bugzilla from webkitpy.thirdparty.mock import Mock from webkitpy.tool.commands.commandtest import CommandsTest from webkitpy.tool.commands.queries import * from webkitpy.tool.mocktool import MockTool + class QueryCommandsTest(CommandsTest): def test_bugs_to_commit(self): expected_stderr = "Warning, attachment 128 on bug 42 has invalid committer (non-committer@example.com)\n" @@ -71,3 +74,17 @@ class QueryCommandsTest(CommandsTest): expected_stdout = "Test 'media' is not skipped by any port.\n" self.assert_execute_outputs(SkippedPorts(), ("media",), expected_stdout) + + +class FailureReasonTest(unittest.TestCase): + def test_blame_line_for_revision(self): + tool = MockTool() + command = FailureReason() + command.bind_to_tool(tool) + # This is an artificial example, mostly to test the CommitInfo lookup failure case. + self.assertEquals(command._blame_line_for_revision(None), "FAILED to fetch CommitInfo for rNone, likely missing ChangeLog") + + def raising_mock(self): + raise Exception("MESSAGE") + tool.checkout().commit_info_for_revision = raising_mock + self.assertEquals(command._blame_line_for_revision(None), "FAILED to fetch CommitInfo for rNone, exception: MESSAGE") diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/queues.py b/WebKitTools/Scripts/webkitpy/tool/commands/queues.py index 80fd2ea..7b3002a 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/queues.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/queues.py @@ -27,6 +27,9 @@ # (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +from __future__ import with_statement + +import codecs import time import traceback import os @@ -36,17 +39,18 @@ from optparse import make_option from StringIO import StringIO from webkitpy.common.net.bugzilla import CommitterValidator +from webkitpy.common.net.layouttestresults import path_for_layout_test, LayoutTestResults from webkitpy.common.net.statusserver import StatusServer from webkitpy.common.system.executive import ScriptError from webkitpy.common.system.deprecated_logging import error, log from webkitpy.tool.commands.stepsequence import StepSequenceErrorHandler -from webkitpy.tool.bot.commitqueuetask import CommitQueueTask -from webkitpy.tool.bot.feeders import CommitQueueFeeder -from webkitpy.tool.bot.patchcollection import PersistentPatchCollection, PersistentPatchCollectionDelegate +from webkitpy.tool.bot.commitqueuetask import CommitQueueTask, CommitQueueTaskDelegate +from webkitpy.tool.bot.feeders import CommitQueueFeeder, EWSFeeder from webkitpy.tool.bot.queueengine import QueueEngine, QueueEngineDelegate from webkitpy.tool.grammar import pluralize from webkitpy.tool.multicommandtool import Command, TryAgain + class AbstractQueue(Command, QueueEngineDelegate): watchers = [ ] @@ -78,6 +82,10 @@ class AbstractQueue(Command, QueueEngineDelegate): # because our global option code looks for the first argument which does # not begin with "-" and assumes that is the command name. webkit_patch_args += ["--status-host=%s" % self._tool.status_server.host] + if self._tool.status_server.bot_id: + webkit_patch_args += ["--bot-id=%s" % self._tool.status_server.bot_id] + if self._options.port: + webkit_patch_args += ["--port=%s" % self._options.port] webkit_patch_args.extend(args) return self._tool.executive.run_and_throw_if_fail(webkit_patch_args) @@ -94,7 +102,7 @@ class AbstractQueue(Command, QueueEngineDelegate): def begin_work_queue(self): log("CAUTION: %s will discard all local changes in \"%s\"" % (self.name, self._tool.scm().checkout_root)) - if self.options.confirm: + if self._options.confirm: response = self._tool.user.prompt("Are you sure? Type \"yes\" to continue: ") if (response != "yes"): error("User declined.") @@ -106,7 +114,7 @@ class AbstractQueue(Command, QueueEngineDelegate): def should_continue_work_queue(self): self._iteration_count += 1 - return not self.options.iterations or self._iteration_count <= self.options.iterations + return not self._options.iterations or self._iteration_count <= self._options.iterations def next_work_item(self): raise NotImplementedError, "subclasses must implement" @@ -123,7 +131,7 @@ class AbstractQueue(Command, QueueEngineDelegate): # Command methods def execute(self, options, args, tool, engine=QueueEngine): - self.options = options # FIXME: This code is wrong. Command.options is a list, this assumes an Options element! + self._options = options # FIXME: This code is wrong. Command.options is a list, this assumes an Options element! self._tool = tool # FIXME: This code is wrong too! Command.bind_to_tool handles this! return engine(self.name, self, self._tool.wakeup_event).run() @@ -159,6 +167,7 @@ class FeederQueue(AbstractQueue): AbstractQueue.begin_work_queue(self) self.feeders = [ CommitQueueFeeder(self._tool), + EWSFeeder(self._tool), ] def next_work_item(self): @@ -190,6 +199,9 @@ class AbstractPatchQueue(AbstractQueue): def _fetch_next_work_item(self): return self._tool.status_server.next_work_item(self.name) + def _release_work_item(self, patch): + self._tool.status_server.release_work_item(self.name, patch) + def _did_pass(self, patch): self._update_status(self._pass_status, patch) @@ -207,7 +219,7 @@ class AbstractPatchQueue(AbstractQueue): return os.path.join(self._log_directory(), "%s.log" % patch.bug_id()) -class CommitQueue(AbstractPatchQueue, StepSequenceErrorHandler): +class CommitQueue(AbstractPatchQueue, StepSequenceErrorHandler, CommitQueueTaskDelegate): name = "commit-queue" # AbstractPatchQueue methods @@ -229,7 +241,7 @@ class CommitQueue(AbstractPatchQueue, StepSequenceErrorHandler): def process_work_item(self, patch): self._cc_watchers(patch.bug_id()) - task = CommitQueueTask(self._tool, self, patch) + task = CommitQueueTask(self, patch) try: if task.run(): self._did_pass(patch) @@ -239,10 +251,22 @@ class CommitQueue(AbstractPatchQueue, StepSequenceErrorHandler): validator = CommitterValidator(self._tool.bugs) validator.reject_patch_from_commit_queue(patch.id(), self._error_message_for_bug(task.failure_status_id, e)) self._did_fail(patch) + self._release_work_item(patch) + + def _error_message_for_bug(self, status_id, script_error): + if not script_error.output: + return script_error.message_with_output() + results_link = self._tool.status_server.results_url_for_status(status_id) + return "%s\nFull output: %s" % (script_error.message_with_output(), results_link) def handle_unexpected_error(self, patch, message): self.committer_validator.reject_patch_from_commit_queue(patch.id(), message) + # CommitQueueTaskDelegate methods + + def run_command(self, command): + self.run_webkit_patch(command) + def command_passed(self, message, patch): self._update_status(message, patch=patch) @@ -250,11 +274,36 @@ class CommitQueue(AbstractPatchQueue, StepSequenceErrorHandler): failure_log = self._log_from_script_error_for_upload(script_error) return self._update_status(message, patch=patch, results_file=failure_log) - def _error_message_for_bug(self, status_id, script_error): - if not script_error.output: - return script_error.message_with_output() - results_link = self._tool.status_server.results_url_for_status(status_id) - return "%s\nFull output: %s" % (script_error.message_with_output(), results_link) + # FIXME: This exists for mocking, but should instead be mocked via + # some sort of tool.filesystem() object. + def _read_file_contents(self, path): + try: + with codecs.open(path, "r", "utf-8") as open_file: + return open_file.read() + except OSError, e: # File does not exist or can't be read. + return None + + # FIXME: This may belong on the Port object. + def layout_test_results(self): + results_path = self._tool.port().layout_tests_results_path() + results_html = self._read_file_contents(results_path) + if not results_html: + return None + return LayoutTestResults.results_from_string(results_html) + + def refetch_patch(self, patch): + return self._tool.bugs.fetch_attachment(patch.id()) + + def _author_emails_for_tests(self, flaky_tests): + test_paths = map(path_for_layout_test, flaky_tests) + commit_infos = self._tool.checkout().recent_commit_infos_for_files(test_paths) + return [commit_info.author().bugzilla_email() for commit_info in commit_infos if commit_info.author()] + + def report_flaky_tests(self, patch, flaky_tests): + authors = self._author_emails_for_tests(flaky_tests) + cc_explaination = " The author(s) of the test(s) have been CCed on this bug." if authors else "" + message = "The %s encountered the following flaky tests while processing attachment %s:\n\n%s\n\nPlease file bugs against the tests.%s The commit-queue is continuing to process your patch." % (self.name, patch.id(), "\n".join(flaky_tests), cc_explaination) + self._tool.bugs.post_comment_to_bug(patch.bug_id(), message, cc=authors) # StepSequenceErrorHandler methods @@ -278,6 +327,7 @@ class CommitQueue(AbstractPatchQueue, StepSequenceErrorHandler): raise TryAgain() +# FIXME: All the Rietveld code is no longer used and should be deleted. class RietveldUploadQueue(AbstractPatchQueue, StepSequenceErrorHandler): name = "rietveld-upload-queue" @@ -323,40 +373,27 @@ class RietveldUploadQueue(AbstractPatchQueue, StepSequenceErrorHandler): cls._reject_patch(tool, state["patch"].id()) -class AbstractReviewQueue(AbstractPatchQueue, PersistentPatchCollectionDelegate, StepSequenceErrorHandler): +class AbstractReviewQueue(AbstractPatchQueue, StepSequenceErrorHandler): + """This is the base-class for the EWS queues and the style-queue.""" def __init__(self, options=None): AbstractPatchQueue.__init__(self, options) def review_patch(self, patch): - raise NotImplementedError, "subclasses must implement" - - # PersistentPatchCollectionDelegate methods - - def collection_name(self): - return self.name - - def fetch_potential_patch_ids(self): - return self._tool.bugs.queries.fetch_attachment_ids_from_review_queue() - - def status_server(self): - return self._tool.status_server - - def is_terminal_status(self, status): - return status == "Pass" or status == "Fail" or status.startswith("Error:") + raise NotImplementedError("subclasses must implement") # AbstractPatchQueue methods def begin_work_queue(self): AbstractPatchQueue.begin_work_queue(self) - self._patches = PersistentPatchCollection(self) def next_work_item(self): - patch_id = self._patches.next() - if patch_id: - return self._tool.bugs.fetch_attachment(patch_id) + patch_id = self._fetch_next_work_item() + if not patch_id: + return None + return self._tool.bugs.fetch_attachment(patch_id) def should_proceed_with_work_item(self, patch): - raise NotImplementedError, "subclasses must implement" + raise NotImplementedError("subclasses must implement") def process_work_item(self, patch): try: @@ -368,6 +405,8 @@ class AbstractReviewQueue(AbstractPatchQueue, PersistentPatchCollectionDelegate, if e.exit_code != QueueEngine.handled_error_code: self._did_fail(patch) raise e + finally: + self._release_work_item(patch) def handle_unexpected_error(self, patch, message): log(message) diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py b/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py index 029814e..b37b5dc 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py @@ -60,24 +60,31 @@ class AbstractQueueTest(CommandsTest): def test_log_directory(self): self.assertEquals(TestQueue()._log_directory(), "test-queue-logs") - def _assert_run_webkit_patch(self, run_args): + def _assert_run_webkit_patch(self, run_args, port=None): queue = TestQueue() tool = MockTool() + tool.status_server.bot_id = "gort" tool.executive = Mock() queue.bind_to_tool(tool) + queue._options = Mock() + queue._options.port = port queue.run_webkit_patch(run_args) - expected_run_args = ["echo", "--status-host=example.com"] + run_args + expected_run_args = ["echo", "--status-host=example.com", "--bot-id=gort"] + if port: + expected_run_args.append("--port=%s" % port) + expected_run_args.extend(run_args) tool.executive.run_and_throw_if_fail.assert_called_with(expected_run_args) def test_run_webkit_patch(self): self._assert_run_webkit_patch([1]) self._assert_run_webkit_patch(["one", 2]) + self._assert_run_webkit_patch([1], port="mockport") def test_iteration_count(self): queue = TestQueue() - queue.options = Mock() - queue.options.iterations = 3 + queue._options = Mock() + queue._options.iterations = 3 self.assertTrue(queue.should_continue_work_queue()) self.assertTrue(queue.should_continue_work_queue()) self.assertTrue(queue.should_continue_work_queue()) @@ -85,7 +92,7 @@ class AbstractQueueTest(CommandsTest): def test_no_iteration_count(self): queue = TestQueue() - queue.options = Mock() + queue._options = Mock() self.assertTrue(queue.should_continue_work_queue()) self.assertTrue(queue.should_continue_work_queue()) self.assertTrue(queue.should_continue_work_queue()) @@ -128,6 +135,8 @@ MOCK setting flag 'commit-queue' to '-' on attachment '128' with comment 'Reject - If you have committer rights please correct the error in WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.' MOCK: update_work_items: commit-queue [106, 197] Feeding commit-queue items [106, 197] +Feeding EWS (1 r? patch, 1 new) +MOCK: submit_to_ews: 103 """, "handle_unexpected_error": "Mock error message\n", } @@ -139,25 +148,13 @@ class AbstractPatchQueueTest(CommandsTest): queue = AbstractPatchQueue() tool = MockTool() queue.bind_to_tool(tool) + queue._options = Mock() + queue._options.port = None self.assertEquals(queue._fetch_next_work_item(), None) tool.status_server = MockStatusServer(work_items=[2, 1, 3]) self.assertEquals(queue._fetch_next_work_item(), 2) -class AbstractReviewQueueTest(CommandsTest): - def test_patch_collection_delegate_methods(self): - queue = TestReviewQueue() - tool = MockTool() - queue.bind_to_tool(tool) - self.assertEquals(queue.collection_name(), "test-review-queue") - self.assertEquals(queue.fetch_potential_patch_ids(), [103]) - queue.status_server() - self.assertTrue(queue.is_terminal_status("Pass")) - self.assertTrue(queue.is_terminal_status("Fail")) - self.assertTrue(queue.is_terminal_status("Error: Your patch exploded")) - self.assertFalse(queue.is_terminal_status("Foo")) - - class NeedsUpdateSequence(StepSequence): def _run(self, tool, options, state): raise CheckoutNeedsUpdate([], 1, "", None) @@ -172,7 +169,20 @@ class AlwaysCommitQueueTool(object): class SecondThoughtsCommitQueue(CommitQueue): - def _build_and_test_patch(self, patch, first_run=True): + def __init__(self): + self._reject_patch = False + CommitQueue.__init__(self) + + def run_command(self, command): + # We want to reject the patch after the first validation, + # so wait to reject it until after some other command has run. + self._reject_patch = True + return CommitQueue.run_command(self, command) + + def refetch_patch(self, patch): + if not self._reject_patch: + return self._tool.bugs.fetch_attachment(patch.id()) + attachment_dictionary = { "id": patch.id(), "bug_id": patch.bug_id(), @@ -185,9 +195,7 @@ class SecondThoughtsCommitQueue(CommitQueue): "committer_email": "foo@bar.com", "attacher_email": "Contributer1", } - patch = Attachment(attachment_dictionary, None) - self._tool.bugs.set_override_patch(patch) - return True + return Attachment(attachment_dictionary, None) class CommitQueueTest(QueuesTest): @@ -215,6 +223,7 @@ MOCK: update_status: commit-queue Pass "process_work_item": """MOCK: update_status: commit-queue Patch does not apply MOCK setting flag 'commit-queue' to '-' on attachment '197' with comment 'Rejecting patch 197 from commit-queue.' and additional comment 'MOCK script error' MOCK: update_status: commit-queue Fail +MOCK: release_work_item: commit-queue 197 """, "handle_unexpected_error": "MOCK setting flag 'commit-queue' to '-' on attachment '197' with comment 'Rejecting patch 197 from commit-queue.' and additional comment 'Mock error message'\n", "handle_script_error": "ScriptError error message\n", @@ -236,7 +245,7 @@ MOCK: update_status: commit-queue Fail "next_work_item": "", "process_work_item": """MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 197] MOCK: update_status: commit-queue Applied patch -MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'build', '--no-clean', '--no-update', '--build', '--build-style=both', '--quiet'] +MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'build', '--no-clean', '--no-update', '--build-style=both', '--quiet'] MOCK: update_status: commit-queue Built patch MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive'] MOCK: update_status: commit-queue Passed tests @@ -259,7 +268,7 @@ MOCK: update_status: commit-queue Pass "next_work_item": "", "process_work_item": """MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 197] MOCK: update_status: commit-queue Applied patch -MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'build', '--no-clean', '--no-update', '--build', '--build-style=both', '--quiet'] +MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'build', '--no-clean', '--no-update', '--build-style=both', '--quiet'] MOCK: update_status: commit-queue Built patch MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive'] MOCK: update_status: commit-queue Passed tests @@ -290,14 +299,39 @@ MOCK: update_status: commit-queue Pass def test_manual_reject_during_processing(self): queue = SecondThoughtsCommitQueue() queue.bind_to_tool(MockTool()) + queue._options = Mock() + queue._options.port = None expected_stderr = """MOCK: update_status: commit-queue Applied patch MOCK: update_status: commit-queue Built patch MOCK: update_status: commit-queue Passed tests -MOCK: update_status: commit-queue Landed patch -MOCK: update_status: commit-queue Pass +MOCK: update_status: commit-queue Retry +MOCK: release_work_item: commit-queue 197 """ OutputCapture().assert_outputs(self, queue.process_work_item, [MockPatch()], expected_stderr=expected_stderr) + def test_report_flaky_tests(self): + queue = CommitQueue() + queue.bind_to_tool(MockTool()) + expected_stderr = """MOCK bug comment: bug_id=142, cc=['abarth@webkit.org'] +--- Begin comment --- +The commit-queue encountered the following flaky tests while processing attachment 197: + +foo/bar.html +bar/baz.html + +Please file bugs against the tests. The author(s) of the test(s) have been CCed on this bug. The commit-queue is continuing to process your patch. +--- End comment --- + +""" + OutputCapture().assert_outputs(self, queue.report_flaky_tests, [MockPatch(), ["foo/bar.html", "bar/baz.html"]], expected_stderr=expected_stderr) + + def test_layout_test_results(self): + queue = CommitQueue() + queue.bind_to_tool(MockTool()) + queue._read_file_contents = lambda path: None + self.assertEquals(queue.layout_test_results(), None) + queue._read_file_contents = lambda path: "" + self.assertEquals(queue.layout_test_results(), None) class RietveldUploadQueueTest(QueuesTest): def test_rietveld_upload_queue(self): @@ -315,11 +349,11 @@ class StyleQueueTest(QueuesTest): def test_style_queue(self): expected_stderr = { "begin_work_queue": self._default_begin_work_queue_stderr("style-queue", MockSCM.fake_checkout_root), - "next_work_item": "MOCK: update_work_items: style-queue [103]\n", + "next_work_item": "", "should_proceed_with_work_item": "MOCK: update_status: style-queue Checking style\n", - "process_work_item": "MOCK: update_status: style-queue Pass\n", + "process_work_item": "MOCK: update_status: style-queue Pass\nMOCK: release_work_item: style-queue 197\n", "handle_unexpected_error": "Mock error message\n", - "handle_script_error": "MOCK: update_status: style-queue ScriptError error message\nMOCK bug comment: bug_id=142, cc=[]\n--- Begin comment ---\\Attachment 197 did not pass style-queue:\n\nScriptError error message\n\nIf any of these errors are false positives, please file a bug against check-webkit-style.\n--- End comment ---\n\n", + "handle_script_error": "MOCK: update_status: style-queue ScriptError error message\nMOCK bug comment: bug_id=142, cc=[]\n--- Begin comment ---\nAttachment 197 did not pass style-queue:\n\nScriptError error message\n\nIf any of these errors are false positives, please file a bug against check-webkit-style.\n--- End comment ---\n\n", } expected_exceptions = { "handle_script_error": SystemExit, diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/queuestest.py b/WebKitTools/Scripts/webkitpy/tool/commands/queuestest.py index 9f3583d..379d380 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/queuestest.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/queuestest.py @@ -80,13 +80,18 @@ class QueuesTest(unittest.TestCase): string_replacements = {"name": name, 'checkout_dir': checkout_dir} return "CAUTION: %(name)s will discard all local changes in \"%(checkout_dir)s\"\nRunning WebKit %(name)s.\nMOCK: update_status: %(name)s Starting Queue\n" % string_replacements - def assert_queue_outputs(self, queue, args=None, work_item=None, expected_stdout=None, expected_stderr=None, expected_exceptions=None, options=Mock(), tool=MockTool()): + def assert_queue_outputs(self, queue, args=None, work_item=None, expected_stdout=None, expected_stderr=None, expected_exceptions=None, options=None, tool=None): + if not tool: + tool = MockTool() if not expected_stdout: expected_stdout = {} if not expected_stderr: expected_stderr = {} if not args: args = [] + if not options: + options = Mock() + options.port = None if not work_item: work_item = self.mock_work_item tool.user.prompt = lambda message: "yes" diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/sheriffbot.py b/WebKitTools/Scripts/webkitpy/tool/commands/sheriffbot.py index 23d013d..145f485 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/sheriffbot.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/sheriffbot.py @@ -54,77 +54,47 @@ class SheriffBot(AbstractQueue, StepSequenceErrorHandler): self._irc_bot = SheriffIRCBot(self._tool, self._sheriff) self._tool.ensure_irc_connected(self._irc_bot.irc_delegate()) - def work_item_log_path(self, new_failures): - return os.path.join("%s-logs" % self.name, "%s.log" % new_failures.keys()[0]) - - def _new_failures(self, revisions_causing_failures, old_failing_svn_revisions): - # We ignore failures that might have been caused by svn_revisions that - # we've already complained about. This is conservative in the sense - # that we might be ignoring some new failures, but our experience has - # been that skipping this check causes a lot of spam for builders that - # take a long time to cycle. - old_failing_builder_names = [] - for svn_revision in old_failing_svn_revisions: - old_failing_builder_names.extend( - [builder.name() for builder in revisions_causing_failures[svn_revision]]) - - new_failures = {} - for svn_revision, builders in revisions_causing_failures.items(): - if svn_revision in old_failing_svn_revisions: - # FIXME: We should re-process the work item after some time delay. - # https://bugs.webkit.org/show_bug.cgi?id=36581 - continue - new_builders = [builder for builder in builders - if builder.name() not in old_failing_builder_names] - if new_builders: - new_failures[svn_revision] = new_builders - - return new_failures + def work_item_log_path(self, failure_map): + return None + + def _is_old_failure(self, revision): + return self._tool.status_server.svn_revision(revision) def next_work_item(self): self._irc_bot.process_pending_messages() self._update() - # We do one read from buildbot to ensure a consistent view. - revisions_causing_failures = self._tool.buildbot.failure_map().revisions_causing_failures() - - # Similarly, we read once from our the status_server. - old_failing_svn_revisions = [] - for svn_revision in revisions_causing_failures.keys(): - if self._tool.status_server.svn_revision(svn_revision): - old_failing_svn_revisions.append(svn_revision) + # FIXME: We need to figure out how to provoke_flaky_builders. - new_failures = self._new_failures(revisions_causing_failures, - old_failing_svn_revisions) + failure_map = self._tool.buildbot.failure_map() + failure_map.filter_out_old_failures(self._is_old_failure) + if failure_map.is_empty(): + return None + return failure_map - self._sheriff.provoke_flaky_builders(revisions_causing_failures) - return new_failures - - def should_proceed_with_work_item(self, new_failures): + def should_proceed_with_work_item(self, failure_map): # Currently, we don't have any reasons not to proceed with work items. return True - def process_work_item(self, new_failures): - blame_list = new_failures.keys() - for svn_revision, builders in new_failures.items(): + def process_work_item(self, failure_map): + failing_revisions = failure_map.failing_revisions() + for revision in failing_revisions: + builders = failure_map.builders_failing_for(revision) + tests = failure_map.tests_failing_for(revision) try: - commit_info = self._tool.checkout().commit_info_for_revision(svn_revision) + commit_info = self._tool.checkout().commit_info_for_revision(revision) if not commit_info: print "FAILED to fetch CommitInfo for r%s, likely missing ChangeLog" % revision continue self._sheriff.post_irc_warning(commit_info, builders) - self._sheriff.post_blame_comment_on_bug(commit_info, - builders, - blame_list) - self._sheriff.post_automatic_rollout_patch(commit_info, - builders) + self._sheriff.post_blame_comment_on_bug(commit_info, builders, tests) + finally: for builder in builders: - self._tool.status_server.update_svn_revision(svn_revision, - builder.name()) + self._tool.status_server.update_svn_revision(revision, builder.name()) return True - def handle_unexpected_error(self, new_failures, message): + def handle_unexpected_error(self, failure_map, message): log(message) # StepSequenceErrorHandler methods diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/sheriffbot_unittest.py b/WebKitTools/Scripts/webkitpy/tool/commands/sheriffbot_unittest.py index a63ec24..32eb016 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/sheriffbot_unittest.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/sheriffbot_unittest.py @@ -30,7 +30,7 @@ import os from webkitpy.tool.commands.queuestest import QueuesTest from webkitpy.tool.commands.sheriffbot import SheriffBot -from webkitpy.tool.mocktool import MockBuilder +from webkitpy.tool.mocktool import * class SheriffBotTest(QueuesTest): @@ -38,36 +38,19 @@ class SheriffBotTest(QueuesTest): builder2 = MockBuilder("Builder2") def test_sheriff_bot(self): - mock_work_item = { - 29837: [self.builder1], - } + mock_work_item = MockFailureMap(MockTool().buildbot) expected_stderr = { "begin_work_queue": self._default_begin_work_queue_stderr("sheriff-bot", os.getcwd()), "next_work_item": "", - "process_work_item": "MOCK: irc.post: abarth, darin, eseidel: http://trac.webkit.org/changeset/29837 might have broken Builder1\nMOCK bug comment: bug_id=42, cc=['abarth@webkit.org', 'eric@webkit.org']\n--- Begin comment ---\\http://trac.webkit.org/changeset/29837 might have broken Builder1\n--- End comment ---\n\n", + "process_work_item": """MOCK: irc.post: abarth, darin, eseidel: http://trac.webkit.org/changeset/29837 might have broken Builder1 +MOCK bug comment: bug_id=42, cc=['abarth@webkit.org', 'eric@webkit.org'] +--- Begin comment --- +http://trac.webkit.org/changeset/29837 might have broken Builder1 +The following tests are not passing: +mock-test-1 +--- End comment --- + +""", "handle_unexpected_error": "Mock error message\n" } self.assert_queue_outputs(SheriffBot(), work_item=mock_work_item, expected_stderr=expected_stderr) - - revisions_causing_failures = { - 1234: [builder1], - 1235: [builder1, builder2], - } - - def test_new_failures(self): - old_failing_svn_revisions = [] - self.assertEquals(SheriffBot()._new_failures(self.revisions_causing_failures, - old_failing_svn_revisions), - self.revisions_causing_failures) - - def test_new_failures_with_old_revisions(self): - old_failing_svn_revisions = [1234] - self.assertEquals(SheriffBot()._new_failures(self.revisions_causing_failures, - old_failing_svn_revisions), - {1235: [builder2]}) - - def test_new_failures_with_old_revisions(self): - old_failing_svn_revisions = [1235] - self.assertEquals(SheriffBot()._new_failures(self.revisions_causing_failures, - old_failing_svn_revisions), - {}) diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/upload_unittest.py b/WebKitTools/Scripts/webkitpy/tool/commands/upload_unittest.py index 5f3f400..0d096b6 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/upload_unittest.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/upload_unittest.py @@ -52,11 +52,12 @@ class UploadCommandsTest(CommandsTest): def test_post(self): options = MockOptions() + options.cc = None + options.check_style = True + options.comment = None options.description = "MOCK description" options.request_commit = False options.review = True - options.comment = None - options.cc = None expected_stderr = """Running check-webkit-style MOCK: user.open_url: file://... Obsoleting 2 old patches on bug 42 @@ -81,11 +82,12 @@ MOCK: user.open_url: http://example.com/42 def test_upload(self): options = MockOptions() + options.cc = None + options.check_style = True + options.comment = None options.description = "MOCK description" options.request_commit = False options.review = True - options.comment = None - options.cc = None expected_stderr = """Running check-webkit-style MOCK: user.open_url: file://... Obsoleting 2 old patches on bug 42 @@ -103,7 +105,7 @@ MOCK: user.open_url: http://example.com/42 options = Mock() options.bug_id = 42 options.comment = "MOCK comment" - expected_stderr = "Bug: <http://example.com/42> Bug with two r+'d and cq+'d patches, one of which has an invalid commit-queue setter.\nRevision: 9876\nMOCK: user.open_url: http://example.com/42\nAdding comment to Bug 42.\nMOCK bug comment: bug_id=42, cc=None\n--- Begin comment ---\\MOCK comment\n\nCommitted r9876: <http://trac.webkit.org/changeset/9876>\n--- End comment ---\n\n" + expected_stderr = "Bug: <http://example.com/42> Bug with two r+'d and cq+'d patches, one of which has an invalid commit-queue setter.\nRevision: 9876\nMOCK: user.open_url: http://example.com/42\nAdding comment to Bug 42.\nMOCK bug comment: bug_id=42, cc=None\n--- Begin comment ---\nMOCK comment\n\nCommitted r9876: <http://trac.webkit.org/changeset/9876>\n--- End comment ---\n\n" self.assert_execute_outputs(MarkBugFixed(), [], expected_stderr=expected_stderr, tool=tool, options=options) def test_edit_changelog(self): diff --git a/WebKitTools/Scripts/webkitpy/tool/main.py b/WebKitTools/Scripts/webkitpy/tool/main.py index 9531b63..ce6666e 100755 --- a/WebKitTools/Scripts/webkitpy/tool/main.py +++ b/WebKitTools/Scripts/webkitpy/tool/main.py @@ -34,6 +34,7 @@ import threading from webkitpy.common.checkout.api import Checkout from webkitpy.common.checkout.scm import default_scm +from webkitpy.common.config.ports import WebKitPort from webkitpy.common.net.bugzilla import Bugzilla from webkitpy.common.net.buildbot import BuildBot from webkitpy.common.net.rietveld import Rietveld @@ -52,15 +53,16 @@ from webkitpy.tool.commands.queues import * from webkitpy.tool.commands.sheriffbot import * from webkitpy.tool.commands.upload import * from webkitpy.tool.multicommandtool import MultiCommandTool -from webkitpy.common.system.deprecated_logging import log class WebKitPatch(MultiCommandTool): global_options = [ make_option("-v", "--verbose", action="store_true", dest="verbose", default=False, help="enable all logging"), make_option("--dry-run", action="store_true", dest="dry_run", default=False, help="do not touch remote servers"), - make_option("--status-host", action="store", dest="status_host", type="string", nargs=1, help="Hostname (e.g. localhost or commit.webkit.org) where status updates should be posted."), - make_option("--irc-password", action="store", dest="irc_password", type="string", nargs=1, help="Password to use when communicating via IRC."), + make_option("--status-host", action="store", dest="status_host", type="string", help="Hostname (e.g. localhost or commit.webkit.org) where status updates should be posted."), + make_option("--bot-id", action="store", dest="bot_id", type="string", help="Identifier for this bot (if multiple bots are running for a queue)"), + make_option("--irc-password", action="store", dest="irc_password", type="string", help="Password to use when communicating via IRC."), + make_option("--port", action="store", dest="port", default=None, help="Specify a port (e.g., mac, qt, gtk, ...)."), ] def __init__(self, path): @@ -72,6 +74,7 @@ class WebKitPatch(MultiCommandTool): self.buildbot = BuildBot() self.executive = Executive() self._irc = None + self._port = None self.user = User() self._scm = None self._checkout = None @@ -90,6 +93,9 @@ class WebKitPatch(MultiCommandTool): self._checkout = Checkout(self.scm()) return self._checkout + def port(self): + return self._port + def ensure_irc_connected(self, irc_delegate): if not self._irc: self._irc = IRCProxy(irc_delegate) @@ -123,8 +129,12 @@ class WebKitPatch(MultiCommandTool): self.codereview.dryrun = True if options.status_host: self.status_server.set_host(options.status_host) + if options.bot_id: + self.status_server.set_bot_id(options.bot_id) if options.irc_password: self.irc_password = options.irc_password + # If options.port is None, we'll get the default port for this platform. + self._port = WebKitPort.port(options.port) def should_execute_command(self, command): if command.requires_local_commits and not self.scm().supports_local_commits(): diff --git a/WebKitTools/Scripts/webkitpy/tool/mocktool.py b/WebKitTools/Scripts/webkitpy/tool/mocktool.py index 277bd08..05b30dd 100644 --- a/WebKitTools/Scripts/webkitpy/tool/mocktool.py +++ b/WebKitTools/Scripts/webkitpy/tool/mocktool.py @@ -317,7 +317,7 @@ class MockBugzilla(Mock): flag_name, flag_value, attachment_id, comment_text, additional_comment_text)) def post_comment_to_bug(self, bug_id, comment_text, cc=None): - log("MOCK bug comment: bug_id=%s, cc=%s\n--- Begin comment ---\%s\n--- End comment ---\n" % ( + log("MOCK bug comment: bug_id=%s, cc=%s\n--- Begin comment ---\n%s\n--- End comment ---\n" % ( bug_id, cc, comment_text)) def add_patch_to_bug(self, @@ -350,14 +350,24 @@ class MockBuilder(object): self._name, username, comments)) -class MockFailureMap(): +class MockFailureMap(object): def __init__(self, buildbot): self._buildbot = buildbot - def revisions_causing_failures(self): - return { - "29837": [self._buildbot.builder_with_name("Builder1")], - } + def is_empty(self): + return False + + def filter_out_old_failures(self, is_old_revision): + pass + + def failing_revisions(self): + return [29837] + + def builders_failing_for(self, revision): + return [self._buildbot.builder_with_name("Builder1")] + + def tests_failing_for(self, revision): + return ["mock-test-1"] class MockBuildBot(object): @@ -419,7 +429,7 @@ class MockSCM(Mock): # will actually be the root. Since getcwd() is wrong, use a globally fake root for now. self.checkout_root = self.fake_checkout_root - def create_patch(self, git_commit): + def create_patch(self, git_commit, changed_files=None): return "Patch1" def commit_ids_from_commitish_arguments(self, args): @@ -447,6 +457,9 @@ class MockCheckout(object): _committer_list = CommitterList() def commit_info_for_revision(self, svn_revision): + # The real Checkout would probably throw an exception, but this is the only way tests have to get None back at the moment. + if not svn_revision: + return None return CommitInfo(svn_revision, "eric@webkit.org", { "bug_id": 42, "author_name": "Adam Barth", @@ -459,7 +472,10 @@ class MockCheckout(object): def bug_id_for_revision(self, svn_revision): return 12345 - def modified_changelogs(self, git_commit): + def recent_commit_infos_for_files(self, paths): + return [self.commit_info_for_revision(32)] + + def modified_changelogs(self, git_commit, changed_files=None): # Ideally we'd return something more interesting here. The problem is # that LandDiff will try to actually read the patch from disk! return [] @@ -515,8 +531,9 @@ class MockIRC(object): class MockStatusServer(object): - def __init__(self, work_items=None): + def __init__(self, bot_id=None, work_items=None): self.host = "example.com" + self.bot_id = bot_id self._work_items = work_items or [] def patch_status(self, queue_name, patch_id): @@ -530,10 +547,16 @@ class MockStatusServer(object): return None return self._work_items[0] + def release_work_item(self, queue_name, patch): + log("MOCK: release_work_item: %s %s" % (queue_name, patch.id())) + def update_work_items(self, queue_name, work_items): self._work_items = work_items log("MOCK: update_work_items: %s %s" % (queue_name, work_items)) + def submit_to_ews(self, patch_id): + log("MOCK: submit_to_ews: %s" % (patch_id)) + def update_status(self, queue_name, status, patch=None, results_file=None): log("MOCK: update_status: %s %s" % (queue_name, status)) return 187 @@ -567,9 +590,17 @@ class MockExecute(Mock): return "MOCK output of child process" -class MockOptions(Mock): - no_squash = False - squash = False +class MockOptions(object): + """Mock implementation of optparse.Values.""" + + def __init__(self, **kwargs): + # The caller can set option values using keyword arguments. We don't + # set any values by default because we don't know how this + # object will be used. Generally speaking unit tests should + # subclass this or provider wrapper functions that set a common + # set of options. + for key, value in kwargs.items(): + self.__dict__[key] = value class MockRietveld(): @@ -630,3 +661,22 @@ class MockTool(): def path(self): return "echo" + + def port(self): + return Mock() + + +class MockBrowser(object): + params = {} + + def open(self, url): + pass + + def select_form(self, name): + pass + + def __setitem__(self, key, value): + self.params[key] = value + + def submit(self): + return Mock(file) diff --git a/WebKitTools/Scripts/webkitpy/tool/bot/patchcollection_unittest.py b/WebKitTools/Scripts/webkitpy/tool/mocktool_unittest.py index 4ec6e25..cceaa2e 100644 --- a/WebKitTools/Scripts/webkitpy/tool/bot/patchcollection_unittest.py +++ b/WebKitTools/Scripts/webkitpy/tool/mocktool_unittest.py @@ -1,19 +1,19 @@ -# Copyright (c) 2009 Google Inc. All rights reserved. +# Copyright (C) 2010 Google Inc. All rights reserved. # # Redistribution and use in source and binary forms, with or without # modification, are permitted provided that the following conditions are # met: -# -# * Redistributions of source code must retain the above copyright +# +# * Redistributions of source code must retain the above copyright # notice, this list of conditions and the following disclaimer. -# * Redistributions in binary form must reproduce the above +# * Redistributions in binary form must reproduce the above # copyright notice, this list of conditions and the following disclaimer # in the documentation and/or other materials provided with the # distribution. -# * Neither the name of Google Inc. nor the names of its +# * Neither the name of Google Inc. nor the names of its # contributors may be used to endorse or promote products derived from # this software without specific prior written permission. -# +# # THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS # "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT # LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR @@ -28,25 +28,32 @@ import unittest -from webkitpy.tool.bot.patchcollection import PersistentPatchCollection, PersistentPatchCollectionDelegate -from webkitpy.thirdparty.mock import Mock +from mocktool import MockOptions -class TestPersistentPatchCollectionDelegate(PersistentPatchCollectionDelegate): - def collection_name(self): - return "test-collection" +class MockOptionsTest(unittest.TestCase): + # MockOptions() should implement the same semantics that + # optparse.Values does. - def fetch_potential_patch_ids(self): - return [42, 192, 87] + def test_get__set(self): + # Test that we can still set options after we construct the + # object. + options = MockOptions() + options.foo = 'bar' + self.assertEqual(options.foo, 'bar') - def status_server(self): - return Mock() + def test_get__unset(self): + # Test that unset options raise an exception (regular Mock + # objects return an object and hence are different from + # optparse.Values()). + options = MockOptions() + self.assertRaises(AttributeError, lambda: options.foo) - def is_terminal_status(self, status): - return False + def test_kwarg__set(self): + # Test that keyword arguments work in the constructor. + options = MockOptions(foo='bar') + self.assertEqual(options.foo, 'bar') -class PersistentPatchCollectionTest(unittest.TestCase): - def test_next(self): - collection = PersistentPatchCollection(TestPersistentPatchCollectionDelegate()) - collection.next() +if __name__ == '__main__': + unittest.main() diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/abstractstep.py b/WebKitTools/Scripts/webkitpy/tool/steps/abstractstep.py index 9ceb2cb..5525ea0 100644 --- a/WebKitTools/Scripts/webkitpy/tool/steps/abstractstep.py +++ b/WebKitTools/Scripts/webkitpy/tool/steps/abstractstep.py @@ -36,27 +36,23 @@ class AbstractStep(object): def __init__(self, tool, options): self._tool = tool self._options = options - self._port = None + # FIXME: This should use tool.port() def _run_script(self, script_name, args=None, quiet=False, port=WebKitPort): log("Running %s" % script_name) command = [port.script_path(script_name)] if args: command.extend(args) - # FIXME: This should use self.port() self._tool.executive.run_and_throw_if_fail(command, quiet) - # FIXME: The port should live on the tool. - def port(self): - if self._port: - return self._port - self._port = WebKitPort.port(self._options.port) - return self._port + def _changed_files(self, state): + return self.cached_lookup(state, "changed_files") _well_known_keys = { - "diff": lambda self, state: self._tool.scm().create_patch(self._options.git_commit), - "changelogs": lambda self, state: self._tool.checkout().modified_changelogs(self._options.git_commit), "bug_title": lambda self, state: self._tool.bugs.fetch_bug(state["bug_id"]).title(), + "changed_files": lambda self, state: self._tool.scm().changed_files(self._options.git_commit), + "diff": lambda self, state: self._tool.scm().create_patch(self._options.git_commit, changed_files=self._changed_files(state)), + "changelogs": lambda self, state: self._tool.checkout().modified_changelogs(self._options.git_commit, changed_files=self._changed_files(state)), } def cached_lookup(self, state, key, promise=None): @@ -67,6 +63,11 @@ class AbstractStep(object): state[key] = promise(self, state) return state[key] + def did_modify_checkout(self, state): + state["diff"] = None + state["changelogs"] = None + state["changed_files"] = None + @classmethod def options(cls): return [ diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/build.py b/WebKitTools/Scripts/webkitpy/tool/steps/build.py index 456db25..0990b8b 100644 --- a/WebKitTools/Scripts/webkitpy/tool/steps/build.py +++ b/WebKitTools/Scripts/webkitpy/tool/steps/build.py @@ -41,7 +41,7 @@ class Build(AbstractStep): ] def build(self, build_style): - self._tool.executive.run_and_throw_if_fail(self.port().build_webkit_command(build_style=build_style), self._options.quiet) + self._tool.executive.run_and_throw_if_fail(self._tool.port().build_webkit_command(build_style=build_style), self._options.quiet) def run(self, state): if not self._options.build: diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/editchangelog.py b/WebKitTools/Scripts/webkitpy/tool/steps/editchangelog.py index de9b4e4..4d9646f 100644 --- a/WebKitTools/Scripts/webkitpy/tool/steps/editchangelog.py +++ b/WebKitTools/Scripts/webkitpy/tool/steps/editchangelog.py @@ -35,3 +35,4 @@ class EditChangeLog(AbstractStep): def run(self, state): os.chdir(self._tool.scm().checkout_root) self._tool.user.edit_changelog(self.cached_lookup(state, "changelogs")) + self.did_modify_checkout(state) diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/options.py b/WebKitTools/Scripts/webkitpy/tool/steps/options.py index 3dc1963..835fdba 100644 --- a/WebKitTools/Scripts/webkitpy/tool/steps/options.py +++ b/WebKitTools/Scripts/webkitpy/tool/steps/options.py @@ -50,7 +50,6 @@ class Options(object): obsolete_patches = make_option("--no-obsolete", action="store_false", dest="obsolete_patches", default=True, help="Do not obsolete old patches before posting this one.") open_bug = make_option("--open-bug", action="store_true", dest="open_bug", default=False, help="Opens the associated bug in a browser.") parent_command = make_option("--parent-command", action="store", dest="parent_command", default=None, help="(Internal) The command that spawned this instance.") - port = make_option("--port", action="store", dest="port", default=None, help="Specify a port (e.g., mac, qt, gtk, ...).") quiet = make_option("--quiet", action="store_true", dest="quiet", default=False, help="Produce less console output.") request_commit = make_option("--request-commit", action="store_true", dest="request_commit", default=False, help="Mark the patch as needing auto-commit after review.") review = make_option("--no-review", action="store_false", dest="review", default=True, help="Do not mark the patch for review.") diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/preparechangelog.py b/WebKitTools/Scripts/webkitpy/tool/steps/preparechangelog.py index ce04024..099dfe3 100644 --- a/WebKitTools/Scripts/webkitpy/tool/steps/preparechangelog.py +++ b/WebKitTools/Scripts/webkitpy/tool/steps/preparechangelog.py @@ -39,7 +39,6 @@ class PrepareChangeLog(AbstractStep): @classmethod def options(cls): return AbstractStep.options() + [ - Options.port, Options.quiet, Options.email, Options.git_commit, @@ -62,7 +61,7 @@ class PrepareChangeLog(AbstractStep): self._ensure_bug_url(state) return os.chdir(self._tool.scm().checkout_root) - args = [self.port().script_path("prepare-ChangeLog")] + args = [self._tool.port().script_path("prepare-ChangeLog")] if state.get("bug_id"): args.append("--bug=%s" % state["bug_id"]) if self._options.email: @@ -75,4 +74,4 @@ class PrepareChangeLog(AbstractStep): self._tool.executive.run_and_throw_if_fail(args, self._options.quiet) except ScriptError, e: error("Unable to prepare ChangeLogs.") - state["diff"] = None # We've changed the diff + self.did_modify_checkout(state) diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/runtests.py b/WebKitTools/Scripts/webkitpy/tool/steps/runtests.py index aff1fd9..dcbfc44 100644 --- a/WebKitTools/Scripts/webkitpy/tool/steps/runtests.py +++ b/WebKitTools/Scripts/webkitpy/tool/steps/runtests.py @@ -37,7 +37,6 @@ class RunTests(AbstractStep): Options.test, Options.non_interactive, Options.quiet, - Options.port, ] def run(self, state): @@ -46,14 +45,17 @@ class RunTests(AbstractStep): # Run the scripting unit tests first because they're quickest. log("Running Python unit tests") - self._tool.executive.run_and_throw_if_fail(self.port().run_python_unittests_command()) + self._tool.executive.run_and_throw_if_fail(self._tool.port().run_python_unittests_command()) log("Running Perl unit tests") - self._tool.executive.run_and_throw_if_fail(self.port().run_perl_unittests_command()) - log("Running JavaScriptCore tests") - self._tool.executive.run_and_throw_if_fail(self.port().run_javascriptcore_tests_command(), quiet=True) + self._tool.executive.run_and_throw_if_fail(self._tool.port().run_perl_unittests_command()) + + javascriptcore_tests_command = self._tool.port().run_javascriptcore_tests_command() + if javascriptcore_tests_command: + log("Running JavaScriptCore tests") + self._tool.executive.run_and_throw_if_fail(javascriptcore_tests_command, quiet=True) log("Running run-webkit-tests") - args = self.port().run_webkit_tests_command() + args = self._tool.port().run_webkit_tests_command() if self._options.non_interactive: args.append("--no-launch-safari") args.append("--exit-after-n-failures=1") @@ -61,7 +63,7 @@ class RunTests(AbstractStep): # FIXME: Hack to work around https://bugs.webkit.org/show_bug.cgi?id=38912 # when running the commit-queue on a mac leopard machine since compositing # does not work reliably on Leopard due to various graphics driver/system bugs. - if self.port().name() == "Mac" and self.port().is_leopard(): + if self._tool.port().name() == "Mac" and self._tool.port().is_leopard(): tests_to_ignore = [] tests_to_ignore.append("compositing") diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/steps_unittest.py b/WebKitTools/Scripts/webkitpy/tool/steps/steps_unittest.py index 15f275a..7eb8e3a 100644 --- a/WebKitTools/Scripts/webkitpy/tool/steps/steps_unittest.py +++ b/WebKitTools/Scripts/webkitpy/tool/steps/steps_unittest.py @@ -37,39 +37,49 @@ from webkitpy.tool.steps.promptforbugortitle import PromptForBugOrTitle class StepsTest(unittest.TestCase): + def _step_options(self): + options = MockOptions() + options.non_interactive = True + options.port = 'MOCK port' + options.quiet = True + options.test = True + return options + def _run_step(self, step, tool=None, options=None, state=None): if not tool: tool = MockTool() if not options: - options = MockOptions() + options = self._step_options() if not state: state = {} step(tool, options).run(state) def test_update_step(self): - options = MockOptions() + tool = MockTool() + options = self._step_options() options.update = True expected_stderr = "Updating working directory\n" - OutputCapture().assert_outputs(self, self._run_step, [Update, options], expected_stderr=expected_stderr) + OutputCapture().assert_outputs(self, self._run_step, [Update, tool, options], expected_stderr=expected_stderr) def test_prompt_for_bug_or_title_step(self): tool = MockTool() tool.user.prompt = lambda message: 42 self._run_step(PromptForBugOrTitle, tool=tool) - def test_runtests_leopard_commit_queue_hack(self): + def test_runtests_leopard_commit_queue_hack_step(self): expected_stderr = "Running Python unit tests\nRunning Perl unit tests\nRunning JavaScriptCore tests\nRunning run-webkit-tests\n" OutputCapture().assert_outputs(self, self._run_step, [RunTests], expected_stderr=expected_stderr) - def test_runtests_leopard_commit_queue_hack(self): - mock_options = MockOptions() - mock_options.non_interactive = True + def test_runtests_leopard_commit_queue_hack_command(self): + mock_options = self._step_options() step = RunTests(MockTool(log_executive=True), mock_options) # FIXME: We shouldn't use a real port-object here, but there is too much to mock at the moment. mock_port = WebKitPort() mock_port.name = lambda: "Mac" mock_port.is_leopard = lambda: True - step.port = lambda: mock_port + tool = MockTool(log_executive=True) + tool.port = lambda: mock_port + step = RunTests(tool, mock_options) expected_stderr = """Running Python unit tests MOCK run_and_throw_if_fail: ['WebKitTools/Scripts/test-webkitpy'] Running Perl unit tests diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/update.py b/WebKitTools/Scripts/webkitpy/tool/steps/update.py index 0f450f3..cd1d4d8 100644 --- a/WebKitTools/Scripts/webkitpy/tool/steps/update.py +++ b/WebKitTools/Scripts/webkitpy/tool/steps/update.py @@ -36,11 +36,10 @@ class Update(AbstractStep): def options(cls): return AbstractStep.options() + [ Options.update, - Options.port, ] def run(self, state): if not self._options.update: return log("Updating working directory") - self._tool.executive.run_and_throw_if_fail(self.port().update_webkit_command(), quiet=True) + self._tool.executive.run_and_throw_if_fail(self._tool.port().update_webkit_command(), quiet=True) diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/updatechangelogswithreview_unittest.py b/WebKitTools/Scripts/webkitpy/tool/steps/updatechangelogswithreview_unittest.py index a037422..b475378 100644 --- a/WebKitTools/Scripts/webkitpy/tool/steps/updatechangelogswithreview_unittest.py +++ b/WebKitTools/Scripts/webkitpy/tool/steps/updatechangelogswithreview_unittest.py @@ -41,5 +41,8 @@ class UpdateChangeLogsWithReviewerTest(unittest.TestCase): def test_empty_state(self): capture = OutputCapture() - step = UpdateChangeLogsWithReviewer(MockTool(), MockOptions()) + options = MockOptions() + options.reviewer = 'MOCK reviewer' + options.git_commit = 'MOCK git commit' + step = UpdateChangeLogsWithReviewer(MockTool(), options) capture.assert_outputs(self, step.run, [{}]) |