diff options
Diffstat (limited to 'WebKitTools/Scripts/webkitpy/tool/commands')
9 files changed, 219 insertions, 283 deletions
diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/download.py b/WebKitTools/Scripts/webkitpy/tool/commands/download.py index ed0e3d6..9916523 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/download.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/download.py @@ -289,7 +289,7 @@ class AbstractRolloutPrepCommand(AbstractSequencedCommand): argument_names = "REVISION REASON" def _commit_info(self, revision): - commit_info = self.tool.checkout().commit_info_for_revision(revision) + commit_info = self._tool.checkout().commit_info_for_revision(revision) if commit_info and commit_info.bug_id(): # Note: Don't print a bug URL here because it will confuse the # SheriffBot because the SheriffBot just greps the output diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/openbugs.py b/WebKitTools/Scripts/webkitpy/tool/commands/openbugs.py index 5da5bbb..1b51c9f 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/openbugs.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/openbugs.py @@ -41,8 +41,8 @@ class OpenBugs(AbstractDeclarativeCommand): def _open_bugs(self, bug_ids): for bug_id in bug_ids: - bug_url = self.tool.bugs.bug_url_for_bug_id(bug_id) - self.tool.user.open_url(bug_url) + bug_url = self._tool.bugs.bug_url_for_bug_id(bug_id) + self._tool.user.open_url(bug_url) # _find_bugs_in_string mostly exists for easy unit testing. def _find_bugs_in_string(self, string): diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/queries.py b/WebKitTools/Scripts/webkitpy/tool/commands/queries.py index 9b8d162..c6e45aa 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/queries.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/queries.py @@ -33,6 +33,7 @@ from optparse import make_option from webkitpy.common.checkout.commitinfo import CommitInfo from webkitpy.common.config.committers import CommitterList from webkitpy.common.net.buildbot import BuildBot +from webkitpy.common.net.regressionwindow import RegressionWindow from webkitpy.common.system.user import User from webkitpy.tool.grammar import pluralize from webkitpy.tool.multicommandtool import AbstractDeclarativeCommand @@ -112,7 +113,7 @@ class LastGreenRevision(AbstractDeclarativeCommand): help_text = "Prints the last known good revision" def execute(self, options, args, tool): - print self.tool.buildbot.last_green_revision() + print self._tool.buildbot.last_green_revision() class WhatBroke(AbstractDeclarativeCommand): @@ -122,29 +123,26 @@ class WhatBroke(AbstractDeclarativeCommand): def _print_builder_line(self, builder_name, max_name_width, status_message): print "%s : %s" % (builder_name.ljust(max_name_width), status_message) - # FIXME: This is slightly different from Builder.suspect_revisions_for_green_to_red_transition - # due to needing to detect the "hit the limit" case an print a special message. def _print_blame_information_for_builder(self, builder_status, name_width, avoid_flakey_tests=True): - builder = self.tool.buildbot.builder_with_name(builder_status["name"]) + builder = self._tool.buildbot.builder_with_name(builder_status["name"]) red_build = builder.build(builder_status["build_number"]) - (last_green_build, first_red_build) = builder.find_failure_transition(red_build) - if not first_red_build: + regression_window = builder.find_regression_window(red_build) + if not regression_window.failing_build(): self._print_builder_line(builder.name(), name_width, "FAIL (error loading build information)") return - if not last_green_build: - self._print_builder_line(builder.name(), name_width, "FAIL (blame-list: sometime before %s?)" % first_red_build.revision()) + if not regression_window.build_before_failure(): + self._print_builder_line(builder.name(), name_width, "FAIL (blame-list: sometime before %s?)" % regression_window.failing_build().revision()) return - suspect_revisions = range(first_red_build.revision(), last_green_build.revision(), -1) - suspect_revisions.reverse() + revisions = regression_window.revisions() first_failure_message = "" - if (first_red_build == builder.build(builder_status["build_number"])): + if (regression_window.failing_build() == builder.build(builder_status["build_number"])): first_failure_message = " FIRST FAILURE, possibly a flaky test" - self._print_builder_line(builder.name(), name_width, "FAIL (blame-list: %s%s)" % (suspect_revisions, first_failure_message)) - for revision in suspect_revisions: - commit_info = self.tool.checkout().commit_info_for_revision(revision) + self._print_builder_line(builder.name(), name_width, "FAIL (blame-list: %s%s)" % (revisions, first_failure_message)) + 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) + print commit_info.blame_string(self._tool.bugs) else: print "FAILED to fetch CommitInfo for r%s, likely missing ChangeLog" % revision @@ -169,7 +167,7 @@ class WhoBrokeIt(AbstractDeclarativeCommand): 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.revisions_causing_failures(False).items(): + 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]) @@ -188,7 +186,7 @@ class ResultsFor(AbstractDeclarativeCommand): print " %s" % filename def execute(self, options, args, tool): - builders = self.tool.buildbot.builders() + builders = self._tool.buildbot.builders() for builder in builders: print "%s:" % builder.name() build = builder.build_for_revision(args[0], allow_failed_lookups=True) @@ -200,13 +198,14 @@ class FailureReason(AbstractDeclarativeCommand): 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): - suspect_revisions = green_build.builder().suspect_revisions_for_transition(green_build, red_build) + regression_window = RegressionWindow(green_build, red_build) + revisions = regression_window.revisions() 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 suspect_revisions: - commit_info = self.tool.checkout().commit_info_for_revision(revision) + 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) + print commit_info.blame_string(self._tool.bugs) else: print "FAILED to fetch CommitInfo for r%s, likely missing ChangeLog" % revision @@ -255,7 +254,7 @@ class FailureReason(AbstractDeclarativeCommand): return 0 def _builder_to_explain(self): - builder_statuses = self.tool.buildbot.builder_statuses() + builder_statuses = self._tool.buildbot.builder_statuses() red_statuses = [status for status in builder_statuses if not status["is_green"]] print "%s failing" % (pluralize("builder", len(red_statuses))) builder_choices = [status["name"] for status in red_statuses] @@ -264,11 +263,11 @@ class FailureReason(AbstractDeclarativeCommand): # FIXME: prompt_with_list should really take a set of objects and a set of names and then return the object. for status in red_statuses: if status["name"] == chosen_name: - return (self.tool.buildbot.builder_with_name(chosen_name), status["built_revision"]) + return (self._tool.buildbot.builder_with_name(chosen_name), status["built_revision"]) def execute(self, options, args, tool): (builder, latest_revision) = self._builder_to_explain() - start_revision = self.tool.user.prompt("Revision to walk backwards from? [%s] " % latest_revision) or latest_revision + start_revision = self._tool.user.prompt("Revision to walk backwards from? [%s] " % latest_revision) or latest_revision if not start_revision: print "Revision required." return 1 diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/queues.py b/WebKitTools/Scripts/webkitpy/tool/commands/queues.py index bc9ee42..80fd2ea 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/queues.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/queues.py @@ -27,6 +27,7 @@ # (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 time import traceback import os @@ -39,6 +40,8 @@ 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.queueengine import QueueEngine, QueueEngineDelegate from webkitpy.tool.grammar import pluralize @@ -50,6 +53,7 @@ class AbstractQueue(Command, QueueEngineDelegate): _pass_status = "Pass" _fail_status = "Fail" + _retry_status = "Retry" _error_status = "Error" def __init__(self, options=None): # Default values should never be collections (like []) as default values are shared between invocations @@ -62,20 +66,20 @@ class AbstractQueue(Command, QueueEngineDelegate): def _cc_watchers(self, bug_id): try: - self.tool.bugs.add_cc_to_bug(bug_id, self.watchers) + self._tool.bugs.add_cc_to_bug(bug_id, self.watchers) except Exception, e: traceback.print_exc() log("Failed to CC watchers.") def run_webkit_patch(self, args): - webkit_patch_args = [self.tool.path()] + webkit_patch_args = [self._tool.path()] # FIXME: This is a hack, we should have a more general way to pass global options. # FIXME: We must always pass global options and their value in one argument # 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] + webkit_patch_args += ["--status-host=%s" % self._tool.status_server.host] webkit_patch_args.extend(args) - return self.tool.executive.run_and_throw_if_fail(webkit_patch_args) + return self._tool.executive.run_and_throw_if_fail(webkit_patch_args) def _log_directory(self): return "%s-logs" % self.name @@ -89,16 +93,16 @@ class AbstractQueue(Command, QueueEngineDelegate): raise NotImplementedError, "subclasses must implement" def begin_work_queue(self): - log("CAUTION: %s will discard all local changes in \"%s\"" % (self.name, self.tool.scm().checkout_root)) + log("CAUTION: %s will discard all local changes in \"%s\"" % (self.name, self._tool.scm().checkout_root)) if self.options.confirm: - response = self.tool.user.prompt("Are you sure? Type \"yes\" to continue: ") + response = self._tool.user.prompt("Are you sure? Type \"yes\" to continue: ") if (response != "yes"): error("User declined.") log("Running WebKit %s." % self.name) - self.tool.status_server.update_status(self.name, "Starting Queue") + self._tool.status_server.update_status(self.name, "Starting Queue") def stop_work_queue(self, reason): - self.tool.status_server.update_status(self.name, "Stopping Queue, reason: %s" % reason) + self._tool.status_server.update_status(self.name, "Stopping Queue, reason: %s" % reason) def should_continue_work_queue(self): self._iteration_count += 1 @@ -120,8 +124,8 @@ class AbstractQueue(Command, QueueEngineDelegate): 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.tool = tool # FIXME: This code is wrong too! Command.bind_to_tool handles this! - return engine(self.name, self, self.tool.wakeup_event).run() + 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() @classmethod def _log_from_script_error_for_upload(cls, script_error, output_limit=None): @@ -144,20 +148,47 @@ class AbstractQueue(Command, QueueEngineDelegate): return tool.status_server.update_status(cls.name, message, state["patch"], failure_log) +class FeederQueue(AbstractQueue): + name = "feeder-queue" + + _sleep_duration = 30 # seconds + + # AbstractPatchQueue methods + + def begin_work_queue(self): + AbstractQueue.begin_work_queue(self) + self.feeders = [ + CommitQueueFeeder(self._tool), + ] + + def next_work_item(self): + # This really show inherit from some more basic class that doesn't + # understand work items, but the base class in the heirarchy currently + # understands work items. + return "synthetic-work-item" + + def should_proceed_with_work_item(self, work_item): + return True + + def process_work_item(self, work_item): + for feeder in self.feeders: + feeder.feed() + time.sleep(self._sleep_duration) + return True + + def work_item_log_path(self, work_item): + return None + + def handle_unexpected_error(self, work_item, message): + log(message) + + class AbstractPatchQueue(AbstractQueue): def _update_status(self, message, patch=None, results_file=None): - self.tool.status_server.update_status(self.name, message, patch, results_file) - - # Note, eventually this will be done by a separate "feeder" queue - # whose job it is to poll bugzilla and feed work items into the - # status server for other queues to munch on. - def _update_work_items(self, patch_ids): - self.tool.status_server.update_work_items(self.name, patch_ids) - if patch_ids: - self.log_progress(patch_ids) + return self._tool.status_server.update_status(self.name, message, patch, results_file) def _fetch_next_work_item(self): - return self.tool.status_server.next_work_item(self.name) + return self._tool.status_server.next_work_item(self.name) def _did_pass(self, patch): self._update_status(self._pass_status, patch) @@ -165,6 +196,9 @@ class AbstractPatchQueue(AbstractQueue): def _did_fail(self, patch): self._update_status(self._fail_status, patch) + def _did_retry(self, patch): + self._update_status(self._retry_status, patch) + def _did_error(self, patch, reason): message = "%s: %s" % (self._error_status, reason) self._update_status(message, patch) @@ -172,178 +206,63 @@ class AbstractPatchQueue(AbstractQueue): def work_item_log_path(self, patch): return os.path.join(self._log_directory(), "%s.log" % patch.bug_id()) - def log_progress(self, patch_ids): - log("%s in %s [%s]" % (pluralize("patch", len(patch_ids)), self.name, ", ".join(map(str, patch_ids)))) - class CommitQueue(AbstractPatchQueue, StepSequenceErrorHandler): name = "commit-queue" - def __init__(self): - AbstractPatchQueue.__init__(self) # AbstractPatchQueue methods def begin_work_queue(self): AbstractPatchQueue.begin_work_queue(self) - self.committer_validator = CommitterValidator(self.tool.bugs) - - def _validate_patches_in_commit_queue(self): - # Not using BugzillaQueries.fetch_patches_from_commit_queue() so we can reject patches with invalid committers/reviewers. - bug_ids = self.tool.bugs.queries.fetch_bug_ids_from_commit_queue() - all_patches = sum([self.tool.bugs.fetch_bug(bug_id).commit_queued_patches(include_invalid=True) for bug_id in bug_ids], []) - return self.committer_validator.patches_after_rejecting_invalid_commiters_and_reviewers(all_patches) - - def _patch_cmp(self, a, b): - # Sort first by is_rollout, then by attach_date. - # Reversing the order so that is_rollout is first. - rollout_cmp = cmp(b.is_rollout(), a.is_rollout()) - if (rollout_cmp != 0): - return rollout_cmp - return cmp(a.attach_date(), b.attach_date()) - - def _feed_work_items_to_server(self): - # Grab the set of patches from bugzilla, sort them, and update the status server. - # Eventually this will all be done by a separate feeder queue. - patches = self._validate_patches_in_commit_queue() - patches = sorted(patches, self._patch_cmp) - self._update_work_items([patch.id() for patch in patches]) + self.committer_validator = CommitterValidator(self._tool.bugs) def next_work_item(self): - self._feed_work_items_to_server() - # The grab the next patch to work on back from the status server. patch_id = self._fetch_next_work_item() if not patch_id: return None - return self.tool.bugs.fetch_attachment(patch_id) - - def _can_build_and_test_without_patch(self): - try: - self.run_webkit_patch([ - "build-and-test", - "--force-clean", - "--build", - "--test", - "--non-interactive", - "--no-update", - "--build-style=both", - "--quiet"]) - return True - except ScriptError, e: - failure_log = self._log_from_script_error_for_upload(e) - self._update_status("Unable to build and test without patch", results_file=failure_log) - return False + return self._tool.bugs.fetch_attachment(patch_id) def should_proceed_with_work_item(self, patch): patch_text = "rollout patch" if patch.is_rollout() else "patch" - self._update_status("Landing %s" % patch_text, patch) + self._update_status("Processing %s" % patch_text, patch) return True - def _build_and_test_patch(self, patch, first_run=False): + def process_work_item(self, patch): + self._cc_watchers(patch.bug_id()) + task = CommitQueueTask(self._tool, self, patch) try: - args = [ - "build-and-test-attachment", - "--force-clean", - "--build", - "--non-interactive", - "--build-style=both", - "--quiet", - patch.id() - ] - # We don't bother to run tests for rollouts as that makes them too slow. - if not patch.is_rollout(): - args.append("--test") - if not first_run: - # The first time through, we don't reject the patch from the - # commit queue because we want to make sure we can build and - # test ourselves. However, the second time through, we - # register ourselves as the parent-command so we can reject - # the patch on failure. - args.append("--parent-command=commit-queue") - # The second time through, we also don't want to update so we - # know we're testing the same revision that we successfully - # built and tested. - args.append("--no-update") - self.run_webkit_patch(args) - return True + if task.run(): + self._did_pass(patch) + return True + self._did_retry(patch) except ScriptError, e: - failure_log = self._log_from_script_error_for_upload(e) - self._update_status("Unable to build and test patch", patch=patch, results_file=failure_log) - if first_run: - return False + 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) - raise - - def _revalidate_patch(self, patch): - # Bugs might get closed, or patches might be obsoleted or r-'d while the - # commit-queue is processing. Do one last minute check before landing. - patch = self.tool.bugs.fetch_attachment(patch.id()) - if patch.is_obsolete(): - return None - if patch.bug().is_closed(): - return None - if not patch.committer(): - return None - # Reviewer is not required. Misisng reviewers will be caught during the ChangeLog check during landing. - return patch - - def _land(self, patch): - try: - args = [ - "land-attachment", - "--force-clean", - "--non-interactive", - "--ignore-builders", - "--quiet", - "--parent-command=commit-queue", - patch.id(), - ] - self.run_webkit_patch(args) - self._did_pass(patch) - except ScriptError, e: - failure_log = self._log_from_script_error_for_upload(e) - self._update_status("Unable to land patch", patch=patch, results_file=failure_log) - self._did_fail(patch) - raise - - def process_work_item(self, patch): - self._cc_watchers(patch.bug_id()) - if not self._build_and_test_patch(patch, first_run=True): - self._update_status("Building and testing without the patch as a sanity check", patch) - # The patch failed to build and test. It's possible that the - # tree is busted. To check that case, we try to build and test - # without the patch. - if not self._can_build_and_test_without_patch(): - return False - self._update_status("Build and test succeeded, trying again with patch", patch) - # Hum, looks like the patch is actually bad. Of course, we could - # have been bitten by a flaky test the first time around. We try - # to build and test again. If it fails a second time, we're pretty - # sure its a bad test and re can reject it outright. - self._build_and_test_patch(patch) - # Do one last check to catch any bug changes (cq-, closed, reviewer changed, etc.) - # This helps catch races between the bots if locks expire. - patch = self._revalidate_patch(patch) - if not patch: - return False - self._land(patch) - return True def handle_unexpected_error(self, patch, message): self.committer_validator.reject_patch_from_commit_queue(patch.id(), message) - # StepSequenceErrorHandler methods - @staticmethod - def _error_message_for_bug(tool, status_id, script_error): + def command_passed(self, message, patch): + self._update_status(message, patch=patch) + + def command_failed(self, message, script_error, patch): + 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 = tool.status_server.results_url_for_status(status_id) + results_link = self._tool.status_server.results_url_for_status(status_id) return "%s\nFull output: %s" % (script_error.message_with_output(), results_link) - @classmethod + # StepSequenceErrorHandler methods + def handle_script_error(cls, tool, state, script_error): - status_id = cls._update_status_for_script_error(tool, state, script_error) - validator = CommitterValidator(tool.bugs) - validator.reject_patch_from_commit_queue(state["patch"].id(), cls._error_message_for_bug(tool, status_id, script_error)) + # Hitting this error handler should be pretty rare. It does occur, + # however, when a patch no longer applies to top-of-tree in the final + # land step. + log(script_error.message_with_output()) @classmethod def handle_checkout_needs_update(cls, tool, state, options, error): @@ -368,7 +287,7 @@ class RietveldUploadQueue(AbstractPatchQueue, StepSequenceErrorHandler): # AbstractPatchQueue methods def next_work_item(self): - patch_id = self.tool.bugs.queries.fetch_first_patch_from_rietveld_queue() + patch_id = self._tool.bugs.queries.fetch_first_patch_from_rietveld_queue() if patch_id: return patch_id self._update_status("Empty queue") @@ -393,7 +312,7 @@ class RietveldUploadQueue(AbstractPatchQueue, StepSequenceErrorHandler): def handle_unexpected_error(self, patch, message): log(message) - self._reject_patch(self.tool, patch.id()) + self._reject_patch(self._tool, patch.id()) # StepSequenceErrorHandler methods @@ -417,10 +336,10 @@ class AbstractReviewQueue(AbstractPatchQueue, PersistentPatchCollectionDelegate, return self.name def fetch_potential_patch_ids(self): - return self.tool.bugs.queries.fetch_attachment_ids_from_review_queue() + return self._tool.bugs.queries.fetch_attachment_ids_from_review_queue() def status_server(self): - return self.tool.status_server + return self._tool.status_server def is_terminal_status(self, status): return status == "Pass" or status == "Fail" or status.startswith("Error:") @@ -434,7 +353,7 @@ class AbstractReviewQueue(AbstractPatchQueue, PersistentPatchCollectionDelegate, def next_work_item(self): patch_id = self._patches.next() if patch_id: - return self.tool.bugs.fetch_attachment(patch_id) + return self._tool.bugs.fetch_attachment(patch_id) def should_proceed_with_work_item(self, patch): raise NotImplementedError, "subclasses must implement" diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py b/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py index 2deee76..029814e 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py @@ -47,20 +47,16 @@ class TestReviewQueue(AbstractReviewQueue): name = "test-review-queue" +class TestFeederQueue(FeederQueue): + _sleep_duration = 0 + + class MockRolloutPatch(MockPatch): def is_rollout(self): return True class AbstractQueueTest(CommandsTest): - def _assert_log_progress_output(self, patch_ids, progress_output): - OutputCapture().assert_outputs(self, TestQueue().log_progress, [patch_ids], expected_stderr=progress_output) - - def test_log_progress(self): - self._assert_log_progress_output([1,2,3], "3 patches in test-queue [1, 2, 3]\n") - self._assert_log_progress_output(["1","2","3"], "3 patches in test-queue [1, 2, 3]\n") - self._assert_log_progress_output([1], "1 patch in test-queue [1]\n") - def test_log_directory(self): self.assertEquals(TestQueue()._log_directory(), "test-queue-logs") @@ -115,6 +111,29 @@ class AbstractQueueTest(CommandsTest): self._assert_log_message(script_error, expected_output) +class FeederQueueTest(QueuesTest): + def test_feeder_queue(self): + queue = TestFeederQueue() + tool = MockTool(log_executive=True) + expected_stderr = { + "begin_work_queue": self._default_begin_work_queue_stderr("feeder-queue", MockSCM.fake_checkout_root), + "should_proceed_with_work_item": "", + "next_work_item": "", + "process_work_item": """Warning, attachment 128 on bug 42 has invalid committer (non-committer@example.com) +Warning, attachment 128 on bug 42 has invalid committer (non-committer@example.com) +MOCK setting flag 'commit-queue' to '-' on attachment '128' with comment 'Rejecting patch 128 from commit-queue.' and additional comment 'non-committer@example.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py. + +- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. + +- 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] +""", + "handle_unexpected_error": "Mock error message\n", + } + self.assert_queue_outputs(queue, tool=tool, expected_stderr=expected_stderr) + + class AbstractPatchQueueTest(CommandsTest): def test_fetch_next_work_item(self): queue = AbstractPatchQueue() @@ -167,7 +186,7 @@ class SecondThoughtsCommitQueue(CommitQueue): "attacher_email": "Contributer1", } patch = Attachment(attachment_dictionary, None) - self.tool.bugs.set_override_patch(patch) + self._tool.bugs.set_override_patch(patch) return True @@ -175,40 +194,58 @@ class CommitQueueTest(QueuesTest): def test_commit_queue(self): expected_stderr = { "begin_work_queue": self._default_begin_work_queue_stderr("commit-queue", MockSCM.fake_checkout_root), - "should_proceed_with_work_item": "MOCK: update_status: commit-queue Landing patch\n", - # FIXME: The commit-queue warns about bad committers twice. This is due to the fact that we access Attachment.reviewer() twice and it logs each time. - "next_work_item": """Warning, attachment 128 on bug 42 has invalid committer (non-committer@example.com) -Warning, attachment 128 on bug 42 has invalid committer (non-committer@example.com) -MOCK setting flag 'commit-queue' to '-' on attachment '128' with comment 'Rejecting patch 128 from commit-queue.' and additional comment 'non-committer@example.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py.\n\n- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.\n\n- 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] -2 patches in commit-queue [106, 197] + "should_proceed_with_work_item": "MOCK: update_status: commit-queue Processing patch\n", + "next_work_item": "", + "process_work_item": """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 """, - "process_work_item": "MOCK: update_status: commit-queue Pass\n", "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": "MOCK: update_status: commit-queue ScriptError error message\nMOCK setting flag 'commit-queue' to '-' on attachment '197' with comment 'Rejecting patch 197 from commit-queue.' and additional comment 'ScriptError error message'\n", + "handle_script_error": "ScriptError error message\n", } self.assert_queue_outputs(CommitQueue(), expected_stderr=expected_stderr) + def test_commit_queue_failure(self): + expected_stderr = { + "begin_work_queue": self._default_begin_work_queue_stderr("commit-queue", MockSCM.fake_checkout_root), + "should_proceed_with_work_item": "MOCK: update_status: commit-queue Processing patch\n", + "next_work_item": "", + "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 +""", + "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", + } + queue = CommitQueue() + + def mock_run_webkit_patch(command): + raise ScriptError('MOCK script error') + + queue.run_webkit_patch = mock_run_webkit_patch + self.assert_queue_outputs(queue, expected_stderr=expected_stderr) + def test_rollout(self): tool = MockTool(log_executive=True) tool.buildbot.light_tree_on_fire() expected_stderr = { "begin_work_queue": self._default_begin_work_queue_stderr("commit-queue", MockSCM.fake_checkout_root), - "should_proceed_with_work_item": "MOCK: update_status: commit-queue Landing patch\n", - # FIXME: The commit-queue warns about bad committers twice. This is due to the fact that we access Attachment.reviewer() twice and it logs each time. - "next_work_item": """Warning, attachment 128 on bug 42 has invalid committer (non-committer@example.com) -Warning, attachment 128 on bug 42 has invalid committer (non-committer@example.com) -MOCK setting flag 'commit-queue' to '-' on attachment '128' with comment 'Rejecting patch 128 from commit-queue.' and additional comment 'non-committer@example.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py. - -- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - -- 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] -2 patches in commit-queue [106, 197] + "should_proceed_with_work_item": "MOCK: update_status: commit-queue Processing patch\n", + "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: 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 +MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'land-attachment', '--force-clean', '--ignore-builders', '--quiet', '--non-interactive', '--parent-command=commit-queue', 197] +MOCK: update_status: commit-queue Landed patch +MOCK: update_status: commit-queue Pass """, - "process_work_item": "MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'build-and-test-attachment', '--force-clean', '--build', '--non-interactive', '--build-style=both', '--quiet', 197, '--test']\nMOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'land-attachment', '--force-clean', '--non-interactive', '--ignore-builders', '--quiet', '--parent-command=commit-queue', 197]\nMOCK: update_status: commit-queue Pass\n", "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": "MOCK: update_status: commit-queue ScriptError error message\nMOCK setting flag 'commit-queue' to '-' on attachment '197' with comment 'Rejecting patch 197 from commit-queue.' and additional comment 'ScriptError error message'\n", + "handle_script_error": "ScriptError error message\n", } self.assert_queue_outputs(CommitQueue(), tool=tool, expected_stderr=expected_stderr) @@ -218,52 +255,23 @@ MOCK: update_work_items: commit-queue [106, 197] rollout_patch = MockRolloutPatch() expected_stderr = { "begin_work_queue": self._default_begin_work_queue_stderr("commit-queue", MockSCM.fake_checkout_root), - "should_proceed_with_work_item": "MOCK: update_status: commit-queue Landing rollout patch\n", - # FIXME: The commit-queue warns about bad committers twice. This is due to the fact that we access Attachment.reviewer() twice and it logs each time. - "next_work_item": """Warning, attachment 128 on bug 42 has invalid committer (non-committer@example.com) -Warning, attachment 128 on bug 42 has invalid committer (non-committer@example.com) -MOCK setting flag 'commit-queue' to '-' on attachment '128' with comment 'Rejecting patch 128 from commit-queue.' and additional comment 'non-committer@example.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py. - -- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - -- 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] -2 patches in commit-queue [106, 197] + "should_proceed_with_work_item": "MOCK: update_status: commit-queue Processing rollout patch\n", + "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: 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 +MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'land-attachment', '--force-clean', '--ignore-builders', '--quiet', '--non-interactive', '--parent-command=commit-queue', 197] +MOCK: update_status: commit-queue Landed patch +MOCK: update_status: commit-queue Pass """, - "process_work_item": "MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'build-and-test-attachment', '--force-clean', '--build', '--non-interactive', '--build-style=both', '--quiet', 197]\nMOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'land-attachment', '--force-clean', '--non-interactive', '--ignore-builders', '--quiet', '--parent-command=commit-queue', 197]\nMOCK: update_status: commit-queue Pass\n", "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": "MOCK: update_status: commit-queue ScriptError error message\nMOCK setting flag 'commit-queue' to '-' on attachment '197' with comment 'Rejecting patch 197 from commit-queue.' and additional comment 'ScriptError error message'\n", + "handle_script_error": "ScriptError error message\n", } self.assert_queue_outputs(CommitQueue(), tool=tool, work_item=rollout_patch, expected_stderr=expected_stderr) - def test_can_build_and_test(self): - queue = CommitQueue() - tool = MockTool() - tool.executive = Mock() - queue.bind_to_tool(tool) - self.assertTrue(queue._can_build_and_test_without_patch()) - expected_run_args = ["echo", "--status-host=example.com", "build-and-test", "--force-clean", "--build", "--test", "--non-interactive", "--no-update", "--build-style=both", "--quiet"] - tool.executive.run_and_throw_if_fail.assert_called_with(expected_run_args) - - def _mock_attachment(self, is_rollout, attach_date): - attachment = Mock() - attachment.is_rollout = lambda: is_rollout - attachment.attach_date = lambda: attach_date - return attachment - - def test_patch_cmp(self): - long_ago_date = datetime(1900, 1, 21) - recent_date = datetime(2010, 1, 21) - attachment1 = self._mock_attachment(is_rollout=False, attach_date=recent_date) - attachment2 = self._mock_attachment(is_rollout=False, attach_date=long_ago_date) - attachment3 = self._mock_attachment(is_rollout=True, attach_date=recent_date) - attachment4 = self._mock_attachment(is_rollout=True, attach_date=long_ago_date) - attachments = [attachment1, attachment2, attachment3, attachment4] - expected_sort = [attachment4, attachment3, attachment2, attachment1] - queue = CommitQueue() - attachments.sort(queue._patch_cmp) - self.assertEqual(attachments, expected_sort) - def test_auto_retry(self): queue = CommitQueue() options = Mock() @@ -282,7 +290,13 @@ MOCK: update_work_items: commit-queue [106, 197] def test_manual_reject_during_processing(self): queue = SecondThoughtsCommitQueue() queue.bind_to_tool(MockTool()) - queue.process_work_item(MockPatch()) + 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 +""" + OutputCapture().assert_outputs(self, queue.process_work_item, [MockPatch()], expected_stderr=expected_stderr) class RietveldUploadQueueTest(QueuesTest): diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/queuestest.py b/WebKitTools/Scripts/webkitpy/tool/commands/queuestest.py index aa3cef4..9f3583d 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/queuestest.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/queuestest.py @@ -32,6 +32,7 @@ from webkitpy.common.net.bugzilla import Attachment from webkitpy.common.system.outputcapture import OutputCapture from webkitpy.common.system.executive import ScriptError from webkitpy.thirdparty.mock import Mock +from webkitpy.tool.commands.stepsequence import StepSequenceErrorHandler from webkitpy.tool.mocktool import MockTool @@ -100,4 +101,6 @@ class QueuesTest(unittest.TestCase): self.assert_outputs(queue.should_proceed_with_work_item, "should_proceed_with_work_item", [work_item], expected_stdout, expected_stderr, expected_exceptions) self.assert_outputs(queue.process_work_item, "process_work_item", [work_item], expected_stdout, expected_stderr, expected_exceptions) self.assert_outputs(queue.handle_unexpected_error, "handle_unexpected_error", [work_item, "Mock error message"], expected_stdout, expected_stderr, expected_exceptions) - self.assert_outputs(queue.handle_script_error, "handle_script_error", [tool, {"patch": MockPatch()}, ScriptError(message="ScriptError error message", script_args="MockErrorCommand")], expected_stdout, expected_stderr, expected_exceptions) + # Should we have a different function for testing StepSequenceErrorHandlers? + if isinstance(queue, StepSequenceErrorHandler): + self.assert_outputs(queue.handle_script_error, "handle_script_error", [tool, {"patch": MockPatch()}, ScriptError(message="ScriptError error message", script_args="MockErrorCommand")], expected_stdout, expected_stderr, expected_exceptions) diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/rebaseline.py b/WebKitTools/Scripts/webkitpy/tool/commands/rebaseline.py index 78e06c6..abfa850 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/rebaseline.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/rebaseline.py @@ -72,15 +72,15 @@ class Rebaseline(AbstractDeclarativeCommand): # FIXME: This should share more code with FailureReason._builder_to_explain def _builder_to_pull_from(self): - builder_statuses = self.tool.buildbot.builder_statuses() + builder_statuses = self._tool.buildbot.builder_statuses() red_statuses = [status for status in builder_statuses if not status["is_green"]] print "%s failing" % (pluralize("builder", len(red_statuses))) builder_choices = [status["name"] for status in red_statuses] - chosen_name = self.tool.user.prompt_with_list("Which builder to pull results from:", builder_choices) + chosen_name = self._tool.user.prompt_with_list("Which builder to pull results from:", builder_choices) # FIXME: prompt_with_list should really take a set of objects and a set of names and then return the object. for status in red_statuses: if status["name"] == chosen_name: - return (self.tool.buildbot.builder_with_name(chosen_name), status["build_number"]) + return (self._tool.buildbot.builder_with_name(chosen_name), status["build_number"]) def _replace_expectation_with_remote_result(self, local_file, remote_file): (downloaded_file, headers) = urllib.urlretrieve(remote_file) @@ -90,7 +90,8 @@ class Rebaseline(AbstractDeclarativeCommand): parsed_results = build.layout_test_results().parsed_results() # FIXME: This probably belongs as API on LayoutTestResults # but .failing_tests() already means something else. - return parsed_results[LayoutTestResults.fail_key] + failing_tests = parsed_results[LayoutTestResults.fail_key] + return self._tool.user.prompt_with_list("Which test(s) to rebaseline:", failing_tests, can_choose_multiple=True) def _results_url_for_test(self, build, test): test_base = os.path.splitext(test)[0] diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/sheriffbot.py b/WebKitTools/Scripts/webkitpy/tool/commands/sheriffbot.py index 24c8517..23d013d 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/sheriffbot.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/sheriffbot.py @@ -50,9 +50,9 @@ class SheriffBot(AbstractQueue, StepSequenceErrorHandler): def begin_work_queue(self): AbstractQueue.begin_work_queue(self) - self._sheriff = Sheriff(self.tool, self) - self._irc_bot = SheriffIRCBot(self.tool, self._sheriff) - self.tool.ensure_irc_connected(self._irc_bot.irc_delegate()) + self._sheriff = Sheriff(self._tool, self) + 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]) @@ -86,12 +86,12 @@ class SheriffBot(AbstractQueue, StepSequenceErrorHandler): self._update() # We do one read from buildbot to ensure a consistent view. - revisions_causing_failures = self.tool.buildbot.revisions_causing_failures() + 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): + if self._tool.status_server.svn_revision(svn_revision): old_failing_svn_revisions.append(svn_revision) new_failures = self._new_failures(revisions_causing_failures, @@ -108,7 +108,7 @@ class SheriffBot(AbstractQueue, StepSequenceErrorHandler): blame_list = new_failures.keys() for svn_revision, builders in new_failures.items(): try: - commit_info = self.tool.checkout().commit_info_for_revision(svn_revision) + commit_info = self._tool.checkout().commit_info_for_revision(svn_revision) if not commit_info: print "FAILED to fetch CommitInfo for r%s, likely missing ChangeLog" % revision continue @@ -120,7 +120,7 @@ class SheriffBot(AbstractQueue, StepSequenceErrorHandler): builders) finally: for builder in builders: - self.tool.status_server.update_svn_revision(svn_revision, + self._tool.status_server.update_svn_revision(svn_revision, builder.name()) return True diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/upload.py b/WebKitTools/Scripts/webkitpy/tool/commands/upload.py index 4a15ed6..107d8db 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/upload.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/upload.py @@ -82,14 +82,14 @@ class CleanPendingCommit(AbstractDeclarativeCommand): def execute(self, options, args, tool): committers = CommitterList() for bug_id in tool.bugs.queries.fetch_bug_ids_from_pending_commit_list(): - bug = self.tool.bugs.fetch_bug(bug_id) + bug = self._tool.bugs.fetch_bug(bug_id) patches = bug.patches(include_obsolete=True) for patch in patches: flags_to_clear = self._flags_to_clear_on_patch(patch) if not flags_to_clear: continue message = "Cleared %s from obsolete attachment %s so that this bug does not appear in http://webkit.org/pending-commit." % (flags_to_clear, patch.id()) - self.tool.bugs.obsolete_attachment(patch.id(), message) + self._tool.bugs.obsolete_attachment(patch.id(), message) class AssignToCommitter(AbstractDeclarativeCommand): @@ -104,7 +104,7 @@ class AssignToCommitter(AbstractDeclarativeCommand): def _assign_bug_to_last_patch_attacher(self, bug_id): committers = CommitterList() - bug = self.tool.bugs.fetch_bug(bug_id) + bug = self._tool.bugs.fetch_bug(bug_id) if not bug.is_unassigned(): assigned_to_email = bug.assigned_to_email() log("Bug %s is already assigned to %s (%s)." % (bug_id, assigned_to_email, committers.committer_by_email(assigned_to_email))) @@ -128,7 +128,7 @@ class AssignToCommitter(AbstractDeclarativeCommand): return reassign_message = "Attachment %s was posted by a committer and has review+, assigning to %s for commit." % (latest_patch.id(), committer.full_name) - self.tool.bugs.reassign_bug(bug_id, committer.bugzilla_email(), reassign_message) + self._tool.bugs.reassign_bug(bug_id, committer.bugzilla_email(), reassign_message) def execute(self, options, args, tool): for bug_id in tool.bugs.queries.fetch_bug_ids_from_pending_commit_list(): |