diff options
Diffstat (limited to 'WebKitTools/Scripts/webkitpy/tool/commands')
13 files changed, 356 insertions, 183 deletions
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): |