[SCM] WebKit Debian packaging branch, debian/experimental, updated. upstream/1.3.3-9427-gc2be6fc
eric at webkit.org
eric at webkit.org
Wed Dec 22 13:34:52 UTC 2010
The following commit has been merged in the debian/experimental branch:
commit 057fca9c4f55b357be129050acd4eab7c9575ca9
Author: eric at webkit.org <eric at webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Date: Mon Sep 20 20:49:17 2010 +0000
2010-09-20 Eric Seidel <eric at webkit.org>
Reviewed by Adam Barth.
commit-queue should check commit-queue+ again just before committing
https://bugs.webkit.org/show_bug.cgi?id=32679
Added a _revalidate_patch check, right before landing.
Since _revalidate_patch passes the patch_id from the work item
back to bugzilla, I had to fix all of the previous queue tests to
use valid attachment ids (that's the majority of this change).
In order to validate that the bug was still open, I had to teach
bugzilla.Bug about open/closed states.
* Scripts/webkitpy/common/net/bugzilla.py:
* Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:
* Scripts/webkitpy/tool/commands/queues.py:
* Scripts/webkitpy/tool/commands/queues_unittest.py:
* Scripts/webkitpy/tool/commands/queuestest.py:
* Scripts/webkitpy/tool/mocktool.py:
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@67880 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
index 9316825..9ad49d6 100644
--- a/WebKitTools/ChangeLog
+++ b/WebKitTools/ChangeLog
@@ -1,3 +1,26 @@
+2010-09-20 Eric Seidel <eric at webkit.org>
+
+ Reviewed by Adam Barth.
+
+ commit-queue should check commit-queue+ again just before committing
+ https://bugs.webkit.org/show_bug.cgi?id=32679
+
+ Added a _revalidate_patch check, right before landing.
+
+ Since _revalidate_patch passes the patch_id from the work item
+ back to bugzilla, I had to fix all of the previous queue tests to
+ use valid attachment ids (that's the majority of this change).
+
+ In order to validate that the bug was still open, I had to teach
+ bugzilla.Bug about open/closed states.
+
+ * Scripts/webkitpy/common/net/bugzilla.py:
+ * Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:
+ * Scripts/webkitpy/tool/commands/queues.py:
+ * Scripts/webkitpy/tool/commands/queues_unittest.py:
+ * Scripts/webkitpy/tool/commands/queuestest.py:
+ * Scripts/webkitpy/tool/mocktool.py:
+
2010-09-20 Mihai Parparita <mihaip at chromium.org>
Unreviewed.
diff --git a/WebKitTools/Scripts/webkitpy/common/net/bugzilla.py b/WebKitTools/Scripts/webkitpy/common/net/bugzilla.py
index 2b2258e..7e17e3b 100644
--- a/WebKitTools/Scripts/webkitpy/common/net/bugzilla.py
+++ b/WebKitTools/Scripts/webkitpy/common/net/bugzilla.py
@@ -175,6 +175,20 @@ class Bug(object):
def is_unassigned(self):
return self.assigned_to_email() in self.unassigned_emails
+ def status(self):
+ return self.bug_dictionary["bug_status"]
+
+ # Bugzilla has many status states we don't really use in WebKit:
+ # https://bugs.webkit.org/page.cgi?id=fields.html#status
+ _open_states = ["UNCONFIRMED", "NEW", "ASSIGNED", "REOPENED"]
+ _closed_states = ["RESOLVED", "VERIFIED", "CLOSED"]
+
+ def is_open(self):
+ return self.status() in self._open_states
+
+ def is_closed(self):
+ return not self.is_open()
+
# Rarely do we actually want obsolete attachments
def attachments(self, include_obsolete=False):
attachments = self.bug_dictionary["attachments"]
@@ -357,15 +371,14 @@ class CommitterValidator(object):
return False
return True
+ def _reject_patch_if_flags_are_invalid(self, patch):
+ return (self._validate_setter_email(
+ patch, "reviewer", self.reject_patch_from_review_queue)
+ and self._validate_setter_email(
+ patch, "committer", self.reject_patch_from_commit_queue))
+
def patches_after_rejecting_invalid_commiters_and_reviewers(self, patches):
- validated_patches = []
- for patch in patches:
- if (self._validate_setter_email(
- patch, "reviewer", self.reject_patch_from_review_queue)
- and self._validate_setter_email(
- patch, "committer", self.reject_patch_from_commit_queue)):
- validated_patches.append(patch)
- return validated_patches
+ return [patch for patch in patches if self._reject_patch_if_flags_are_invalid(patch)]
def reject_patch_from_commit_queue(self,
attachment_id,
diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py b/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py
index a011351..3b0ea47 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py
@@ -50,7 +50,7 @@ class EarlyWarningSytemTest(QueuesTest):
"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=345, cc=%(watchers)s\n--- Begin comment ---\\Attachment 1234 did not build on %(port)s:\nBuild output: http://dummy_url\n--- End comment ---\n\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,
}
return expected_stderr
diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/queues.py b/WebKitTools/Scripts/webkitpy/tool/commands/queues.py
index 8992407..bc9ee42 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/queues.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/queues.py
@@ -119,8 +119,8 @@ class AbstractQueue(Command, QueueEngineDelegate):
# Command methods
def execute(self, options, args, tool, engine=QueueEngine):
- self.options = options
- self.tool = tool
+ 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()
@classmethod
@@ -273,6 +273,19 @@ class CommitQueue(AbstractPatchQueue, StepSequenceErrorHandler):
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 = [
@@ -307,6 +320,11 @@ class CommitQueue(AbstractPatchQueue, StepSequenceErrorHandler):
# 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
diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py b/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py
index 81808b6..2deee76 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py
@@ -34,7 +34,7 @@ from webkitpy.common.system.outputcapture import OutputCapture
from webkitpy.thirdparty.mock import Mock
from webkitpy.tool.commands.commandtest import CommandsTest
from webkitpy.tool.commands.queues import *
-from webkitpy.tool.commands.queuestest import QueuesTest
+from webkitpy.tool.commands.queuestest import QueuesTest, MockPatch
from webkitpy.tool.commands.stepsequence import StepSequence
from webkitpy.tool.mocktool import MockTool, MockSCM, MockStatusServer
@@ -47,16 +47,10 @@ class TestReviewQueue(AbstractReviewQueue):
name = "test-review-queue"
-class MockPatch(object):
+class MockRolloutPatch(MockPatch):
def is_rollout(self):
return True
- def bug_id(self):
- return 12345
-
- def id(self):
- return 76543
-
class AbstractQueueTest(CommandsTest):
def _assert_log_progress_output(self, patch_ids, progress_output):
@@ -158,6 +152,25 @@ class AlwaysCommitQueueTool(object):
return CommitQueue
+class SecondThoughtsCommitQueue(CommitQueue):
+ def _build_and_test_patch(self, patch, first_run=True):
+ attachment_dictionary = {
+ "id": patch.id(),
+ "bug_id": patch.bug_id(),
+ "name": "Rejected",
+ "is_obsolete": True,
+ "is_patch": False,
+ "review": "-",
+ "reviewer_email": "foo at bar.com",
+ "commit-queue": "-",
+ "committer_email": "foo at bar.com",
+ "attacher_email": "Contributer1",
+ }
+ patch = Attachment(attachment_dictionary, None)
+ self.tool.bugs.set_override_patch(patch)
+ return True
+
+
class CommitQueueTest(QueuesTest):
def test_commit_queue(self):
expected_stderr = {
@@ -171,8 +184,8 @@ MOCK: update_work_items: commit-queue [106, 197]
2 patches in commit-queue [106, 197]
""",
"process_work_item": "MOCK: update_status: commit-queue Pass\n",
- "handle_unexpected_error": "MOCK setting flag 'commit-queue' to '-' on attachment '1234' with comment 'Rejecting patch 1234 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 '1234' with comment 'Rejecting patch 1234 from commit-queue.' and additional comment 'ScriptError error message'\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",
}
self.assert_queue_outputs(CommitQueue(), expected_stderr=expected_stderr)
@@ -193,16 +206,16 @@ MOCK setting flag 'commit-queue' to '-' on attachment '128' with comment 'Reject
MOCK: update_work_items: commit-queue [106, 197]
2 patches in commit-queue [106, 197]
""",
- "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', 1234, '--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', 1234]\nMOCK: update_status: commit-queue Pass\n",
- "handle_unexpected_error": "MOCK setting flag 'commit-queue' to '-' on attachment '1234' with comment 'Rejecting patch 1234 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 '1234' with comment 'Rejecting patch 1234 from commit-queue.' and additional comment 'ScriptError error message'\n",
+ "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",
}
self.assert_queue_outputs(CommitQueue(), tool=tool, expected_stderr=expected_stderr)
def test_rollout_lands(self):
tool = MockTool(log_executive=True)
tool.buildbot.light_tree_on_fire()
- rollout_patch = MockPatch()
+ 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",
@@ -217,9 +230,9 @@ MOCK setting flag 'commit-queue' to '-' on attachment '128' with comment 'Reject
MOCK: update_work_items: commit-queue [106, 197]
2 patches in commit-queue [106, 197]
""",
- "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', 76543]\nMOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'land-attachment', '--force-clean', '--non-interactive', '--ignore-builders', '--quiet', '--parent-command=commit-queue', 76543]\nMOCK: update_status: commit-queue Pass\n",
- "handle_unexpected_error": "MOCK setting flag 'commit-queue' to '-' on attachment '76543' with comment 'Rejecting patch 76543 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 '1234' with comment 'Rejecting patch 1234 from commit-queue.' and additional comment 'ScriptError error message'\n",
+ "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",
}
self.assert_queue_outputs(CommitQueue(), tool=tool, work_item=rollout_patch, expected_stderr=expected_stderr)
@@ -266,6 +279,11 @@ MOCK: update_work_items: commit-queue [106, 197]
self.assertEquals(options.build, False)
self.assertEquals(options.test, False)
+ def test_manual_reject_during_processing(self):
+ queue = SecondThoughtsCommitQueue()
+ queue.bind_to_tool(MockTool())
+ queue.process_work_item(MockPatch())
+
class RietveldUploadQueueTest(QueuesTest):
def test_rietveld_upload_queue(self):
@@ -273,8 +291,8 @@ class RietveldUploadQueueTest(QueuesTest):
"begin_work_queue": self._default_begin_work_queue_stderr("rietveld-upload-queue", MockSCM.fake_checkout_root),
"should_proceed_with_work_item": "MOCK: update_status: rietveld-upload-queue Uploading patch\n",
"process_work_item": "MOCK: update_status: rietveld-upload-queue Pass\n",
- "handle_unexpected_error": "Mock error message\nMOCK setting flag 'in-rietveld' to '-' on attachment '1234' with comment 'None' and additional comment 'None'\n",
- "handle_script_error": "ScriptError error message\nMOCK: update_status: rietveld-upload-queue ScriptError error message\nMOCK setting flag 'in-rietveld' to '-' on attachment '1234' with comment 'None' and additional comment 'None'\n",
+ "handle_unexpected_error": "Mock error message\nMOCK setting flag 'in-rietveld' to '-' on attachment '197' with comment 'None' and additional comment 'None'\n",
+ "handle_script_error": "ScriptError error message\nMOCK: update_status: rietveld-upload-queue ScriptError error message\nMOCK setting flag 'in-rietveld' to '-' on attachment '197' with comment 'None' and additional comment 'None'\n",
}
self.assert_queue_outputs(RietveldUploadQueue(), expected_stderr=expected_stderr)
@@ -287,7 +305,7 @@ class StyleQueueTest(QueuesTest):
"should_proceed_with_work_item": "MOCK: update_status: style-queue Checking style\n",
"process_work_item": "MOCK: update_status: style-queue Pass\n",
"handle_unexpected_error": "Mock error message\n",
- "handle_script_error": "MOCK: update_status: style-queue ScriptError error message\nMOCK bug comment: bug_id=345, cc=[]\n--- Begin comment ---\\Attachment 1234 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 ---\\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",
}
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 df73ec3..aa3cef4 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/queuestest.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/queuestest.py
@@ -45,16 +45,20 @@ class MockQueueEngine(object):
class MockPatch():
def id(self):
- return 1234
+ return 197
def bug_id(self):
- return 345
+ return 142
+
+ def is_rollout(self):
+ return False
class QueuesTest(unittest.TestCase):
+ # Ids match patch1 in mocktool.py
mock_work_item = Attachment({
- "id": 1234,
- "bug_id": 345,
+ "id": 197,
+ "bug_id": 142,
"name": "Patch",
"attacher_email": "adam at example.com",
}, None)
diff --git a/WebKitTools/Scripts/webkitpy/tool/mocktool.py b/WebKitTools/Scripts/webkitpy/tool/mocktool.py
index e9c8148..8a6188a 100644
--- a/WebKitTools/Scripts/webkitpy/tool/mocktool.py
+++ b/WebKitTools/Scripts/webkitpy/tool/mocktool.py
@@ -161,6 +161,7 @@ _bug1 = {
"invalid commit-queue setter.",
"assigned_to_email": _unassigned_email,
"attachments": [_patch1, _patch2],
+ "bug_status": "UNCONFIRMED",
}
@@ -169,6 +170,7 @@ _bug2 = {
"title": "Bug with a patch needing review.",
"assigned_to_email": "foo at foo.com",
"attachments": [_patch3],
+ "bug_status": "ASSIGNED",
}
@@ -177,6 +179,7 @@ _bug3 = {
"title": "The third bug",
"assigned_to_email": _unassigned_email,
"attachments": [_patch7],
+ "bug_status": "NEW",
}
@@ -185,6 +188,7 @@ _bug4 = {
"title": "The fourth bug",
"assigned_to_email": "foo at foo.com",
"attachments": [_patch4, _patch5, _patch6],
+ "bug_status": "REOPENED",
}
@@ -254,8 +258,8 @@ class MockBugzilla(Mock):
def __init__(self):
Mock.__init__(self)
self.queries = MockBugzillaQueries(self)
- self.committers = CommitterList(reviewers=[Reviewer("Foo Bar",
- "foo at bar.com")])
+ self.committers = CommitterList(reviewers=[Reviewer("Foo Bar", "foo at bar.com")])
+ self._override_patch = None
def create_bug(self,
bug_title,
@@ -277,7 +281,13 @@ class MockBugzilla(Mock):
def fetch_bug(self, bug_id):
return Bug(self.bug_cache.get(bug_id), self)
+ def set_override_patch(self, patch):
+ self._override_patch = patch
+
def fetch_attachment(self, attachment_id):
+ if self._override_patch:
+ return self._override_patch
+
# This could be changed to .get() if we wish to allow failed lookups.
attachment_dictionary = self.attachment_cache[attachment_id]
bug = self.fetch_bug(attachment_dictionary["bug_id"])
--
WebKit Debian packaging
More information about the Pkg-webkit-commits
mailing list