[SCM] WebKit Debian packaging branch, webkit-1.2, updated. upstream/1.1.90-6072-g9a69373
eric at webkit.org
eric at webkit.org
Thu Apr 8 01:08:57 UTC 2010
The following commit has been merged in the webkit-1.2 branch:
commit 6467469630d6f3acaa91b49738b47ef9378ce97f
Author: eric at webkit.org <eric at webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Date: Fri Jan 15 00:50:37 2010 +0000
2010-01-14 Eric Seidel <eric at webkit.org>
Reviewed by Adam Barth.
REGRESSION(53133): commit-queue no longer rejects patches with invalid committers, instead it hangs
https://bugs.webkit.org/show_bug.cgi?id=33638
* Scripts/webkitpy/bugzilla.py:
- Add Bug.id() to match Attachment.id()
- Give Bug.reviewed_patches and commit_queued_patches the option to return patches with invalid committers/reviewers.
- Add back a missing variable to _validate_setter_email found by the new unit tests!
* Scripts/webkitpy/commands/queries.py:
- Add FIXMEs about the commands being confusingly named.
* Scripts/webkitpy/commands/queries_unittest.py:
- Update results to reflect the newly restructured mock bug cache.
* Scripts/webkitpy/commands/queues.py:
- Add a new _validate_patches_in_commit_queue method (this is what fixes the regression).
- Add a FIXME about eventually sorting the patches into some order.
* Scripts/webkitpy/commands/queues_unittest.py:
- Update results now that with the newly restructure mock bug cache we're testing cq+'d patches with an invalid committer.
* Scripts/webkitpy/commands/upload_unittest.py:
- Update results to match the newly restructured mock bug cache.
* Scripts/webkitpy/mock_bugzillatool.py:
- Restructure fetch_ methods to not use a manual list of ids, but rather use Bug and Attachment classes to make real queries from all of the Bugs.
- Add a few more attachments and bug dictionaries for use by the tests.
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@53298 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
index 6b22dc5..ce81239 100644
--- a/WebKitTools/ChangeLog
+++ b/WebKitTools/ChangeLog
@@ -1,3 +1,29 @@
+2010-01-14 Eric Seidel <eric at webkit.org>
+
+ Reviewed by Adam Barth.
+
+ REGRESSION(53133): commit-queue no longer rejects patches with invalid committers, instead it hangs
+ https://bugs.webkit.org/show_bug.cgi?id=33638
+
+ * Scripts/webkitpy/bugzilla.py:
+ - Add Bug.id() to match Attachment.id()
+ - Give Bug.reviewed_patches and commit_queued_patches the option to return patches with invalid committers/reviewers.
+ - Add back a missing variable to _validate_setter_email found by the new unit tests!
+ * Scripts/webkitpy/commands/queries.py:
+ - Add FIXMEs about the commands being confusingly named.
+ * Scripts/webkitpy/commands/queries_unittest.py:
+ - Update results to reflect the newly restructured mock bug cache.
+ * Scripts/webkitpy/commands/queues.py:
+ - Add a new _validate_patches_in_commit_queue method (this is what fixes the regression).
+ - Add a FIXME about eventually sorting the patches into some order.
+ * Scripts/webkitpy/commands/queues_unittest.py:
+ - Update results now that with the newly restructure mock bug cache we're testing cq+'d patches with an invalid committer.
+ * Scripts/webkitpy/commands/upload_unittest.py:
+ - Update results to match the newly restructured mock bug cache.
+ * Scripts/webkitpy/mock_bugzillatool.py:
+ - Restructure fetch_ methods to not use a manual list of ids, but rather use Bug and Attachment classes to make real queries from all of the Bugs.
+ - Add a few more attachments and bug dictionaries for use by the tests.
+
2010-01-13 Diego Gonzalez <diego.gonzalez at openbossa.org>
Reviewed by Kenneth Christiansen.
diff --git a/WebKitTools/Scripts/webkitpy/bugzilla.py b/WebKitTools/Scripts/webkitpy/bugzilla.py
index c0bbb07..a66613c 100644
--- a/WebKitTools/Scripts/webkitpy/bugzilla.py
+++ b/WebKitTools/Scripts/webkitpy/bugzilla.py
@@ -130,6 +130,9 @@ class Bug(object):
self.bug_dictionary = bug_dictionary
self._bugzilla = bugzilla
+ def id(self):
+ return self.bug_dictionary["id"]
+
def assigned_to_email(self):
return self.bug_dictionary["assigned_to_email"]
@@ -146,13 +149,19 @@ class Bug(object):
def unreviewed_patches(self):
return [patch for patch in self.patches() if patch.review() == "?"]
- def reviewed_patches(self):
+ def reviewed_patches(self, include_invalid=False):
+ patches = [patch for patch in self.patches() if patch.review() == "+"]
+ if include_invalid:
+ return patches
# Checking reviewer() ensures that it was both reviewed and has a valid reviewer.
- return [patch for patch in self.patches() if patch.review() == "+" and patch.reviewer()]
+ return filter(lambda patch: patch.reviewer(), patches)
- def commit_queued_patches(self):
+ def commit_queued_patches(self, include_invalid=False):
+ patches = [patch for patch in self.patches() if patch.commit_queue() == "+"]
+ if include_invalid:
+ return patches
# Checking committer() ensures that it was both commit-queue+'d and has a valid committer.
- return [patch for patch in self.patches() if patch.commit_queue() == "+" and patch.committer()]
+ return filter(lambda patch: patch.committer(), patches)
# A container for all of the logic for making a parsing buzilla queries.
@@ -195,6 +204,7 @@ class BugzillaQueries(object):
commit_queue_url = "buglist.cgi?query_format=advanced&bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&field0-0-0=flagtypes.name&type0-0-0=equals&value0-0-0=commit-queue%2B"
return self._fetch_bug_ids_advanced_query(commit_queue_url)
+ # This function will only return patches which have valid committers set. It won't reject patches with invalid committers/reviewers.
def fetch_patches_from_commit_queue(self):
return sum([self._fetch_bug(bug_id).commit_queued_patches() for bug_id in self.fetch_bug_ids_from_commit_queue()], [])
@@ -236,7 +246,8 @@ class CommitterValidator(object):
def _validate_setter_email(self, patch, result_key, rejection_function):
committer = getattr(patch, result_key)()
# If the flag is set, and we don't recognize the setter, reject the flag!
- if patch._attachment_dictionary.get("%s_email" % result_key) and not committer:
+ setter_email = patch._attachment_dictionary.get("%s_email" % result_key)
+ if setter_email and not committer:
rejection_function(patch.id(), self._flag_permission_rejection_message(setter_email, result_key))
return False
return True
diff --git a/WebKitTools/Scripts/webkitpy/commands/queries.py b/WebKitTools/Scripts/webkitpy/commands/queries.py
index c11fb85..2880172 100644
--- a/WebKitTools/Scripts/webkitpy/commands/queries.py
+++ b/WebKitTools/Scripts/webkitpy/commands/queries.py
@@ -42,6 +42,7 @@ class BugsToCommit(AbstractDeclarativeCommand):
help_text = "List bugs in the commit-queue"
def execute(self, options, args, tool):
+ # FIXME: This command is poorly named. It's fetching the commit-queue list here. The name implies it's fetching pending-commit (all r+'d patches).
bug_ids = tool.bugs.queries.fetch_bug_ids_from_commit_queue()
for bug_id in bug_ids:
print "%s" % bug_id
@@ -52,6 +53,7 @@ class PatchesToCommit(AbstractDeclarativeCommand):
help_text = "List patches in the commit-queue"
def execute(self, options, args, tool):
+ # FIXME: This command is poorly named. It's fetching the commit-queue list here. The name implies it's fetching pending-commit (all r+'d patches).
patches = tool.bugs.queries.fetch_patches_from_commit_queue()
log("Patches in commit queue:")
for patch in patches:
diff --git a/WebKitTools/Scripts/webkitpy/commands/queries_unittest.py b/WebKitTools/Scripts/webkitpy/commands/queries_unittest.py
index da77be1..d42f363 100644
--- a/WebKitTools/Scripts/webkitpy/commands/queries_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/commands/queries_unittest.py
@@ -36,26 +36,27 @@ from webkitpy.mock_bugzillatool import MockBugzillaTool
class QueryCommandsTest(CommandsTest):
def test_bugs_to_commit(self):
- self.assert_execute_outputs(BugsToCommit(), None, "42\n75\n")
+ expected_stderr = "Warning, attachment 128 on bug 42 has invalid committer (non-committer at example.com)\n"
+ self.assert_execute_outputs(BugsToCommit(), None, "42\n77\n", expected_stderr)
def test_patches_to_commit(self):
- expected_stdout = "http://example.com/197\nhttp://example.com/128\n"
- expected_stderr = "Patches in commit queue:\n"
+ expected_stdout = "http://example.com/197\nhttp://example.com/103\n"
+ expected_stderr = "Warning, attachment 128 on bug 42 has invalid committer (non-committer at example.com)\nPatches in commit queue:\n"
self.assert_execute_outputs(PatchesToCommit(), None, expected_stdout, expected_stderr)
def test_patches_to_commit_queue(self):
- expected_stdout = "http://example.com/197&action=edit\n"
- expected_stderr = "128 committer = \"Eric Seidel\" <eric at webkit.org>\n"
+ expected_stdout = "http://example.com/104&action=edit\n"
+ expected_stderr = "197 already has cq=+\n128 already has cq=+\n105 committer = \"Eric Seidel\" <eric at webkit.org>\n"
options = Mock()
options.bugs = False
self.assert_execute_outputs(PatchesToCommitQueue(), None, expected_stdout, expected_stderr, options=options)
- expected_stdout = "http://example.com/42\n"
+ expected_stdout = "http://example.com/77\n"
options.bugs = True
self.assert_execute_outputs(PatchesToCommitQueue(), None, expected_stdout, expected_stderr, options=options)
def test_patches_to_review(self):
- expected_stdout = "197\n128\n"
+ expected_stdout = "103\n"
expected_stderr = "Patches pending review:\n"
self.assert_execute_outputs(PatchesToReview(), None, expected_stdout, expected_stderr)
diff --git a/WebKitTools/Scripts/webkitpy/commands/queues.py b/WebKitTools/Scripts/webkitpy/commands/queues.py
index 4133e34..f5a6d91 100644
--- a/WebKitTools/Scripts/webkitpy/commands/queues.py
+++ b/WebKitTools/Scripts/webkitpy/commands/queues.py
@@ -143,9 +143,15 @@ class CommitQueue(AbstractQueue, StepSequenceErrorHandler):
AbstractQueue.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 next_work_item(self):
- patches = self.tool.bugs.queries.fetch_patches_from_commit_queue()
- patches = self.committer_validator.patches_after_rejecting_invalid_commiters_and_reviewers(patches)
+ patches = self._validate_patches_in_commit_queue()
+ # FIXME: We could sort the patches in a specific order here, was suggested by https://bugs.webkit.org/show_bug.cgi?id=33395
if not patches:
self._update_status("Empty queue")
return None
diff --git a/WebKitTools/Scripts/webkitpy/commands/queues_unittest.py b/WebKitTools/Scripts/webkitpy/commands/queues_unittest.py
index 0c912f8..a422456 100644
--- a/WebKitTools/Scripts/webkitpy/commands/queues_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/commands/queues_unittest.py
@@ -64,10 +64,14 @@ class AbstractQueueTest(CommandsTest):
class CommitQueueTest(QueuesTest):
- def test_style_queue(self):
+ def test_commit_queue(self):
expected_stderr = {
"begin_work_queue" : "CAUTION: commit-queue will discard all local changes in \"%s\"\nRunning WebKit commit-queue.\n" % os.getcwd(),
- "next_work_item" : "2 patches in commit-queue [197, 128]\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 at example.com)
+Warning, attachment 128 on bug 42 has invalid committer (non-committer at example.com)
+2 patches in commit-queue [197, 106]
+""",
}
self.assert_queue_outputs(CommitQueue(), expected_stderr=expected_stderr)
diff --git a/WebKitTools/Scripts/webkitpy/commands/upload_unittest.py b/WebKitTools/Scripts/webkitpy/commands/upload_unittest.py
index 746a61a..b0d41ee 100644
--- a/WebKitTools/Scripts/webkitpy/commands/upload_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/commands/upload_unittest.py
@@ -44,7 +44,7 @@ class UploadCommandsTest(CommandsTest):
def test_assign_to_committer(self):
tool = MockBugzillaTool()
- expected_stderr = "Bug 75 is already assigned to foo at foo.com (None).\nBug 76 has no non-obsolete patches, ignoring.\n"
+ expected_stderr = "Bug 77 is already assigned to foo at foo.com (None).\nBug 76 has no non-obsolete patches, ignoring.\n"
self.assert_execute_outputs(AssignToCommitter(), [], expected_stderr=expected_stderr, tool=tool)
tool.bugs.reassign_bug.assert_called_with(42, "eric at webkit.org", "Attachment 128 was posted by a committer and has review+, assigning to Eric Seidel for commit.")
@@ -74,7 +74,7 @@ class UploadCommandsTest(CommandsTest):
tool._scm.last_svn_commit_log = lambda: "r9876 |"
options = Mock()
options.bug_id = 42
- expected_stderr = "Bug: <http://example.com/42> The first bug\nRevision: 9876\nAdding comment to Bug 42.\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\nAdding comment to Bug 42.\n"
self.assert_execute_outputs(MarkBugFixed(), [], expected_stderr=expected_stderr, tool=tool, options=options)
def test_edit_changelog(self):
diff --git a/WebKitTools/Scripts/webkitpy/mock_bugzillatool.py b/WebKitTools/Scripts/webkitpy/mock_bugzillatool.py
index 978b8c4..6a29407 100644
--- a/WebKitTools/Scripts/webkitpy/mock_bugzillatool.py
+++ b/WebKitTools/Scripts/webkitpy/mock_bugzillatool.py
@@ -48,6 +48,8 @@ _patch1 = {
"is_patch" : True,
"review" : "+",
"reviewer_email" : "foo at bar.com",
+ "commit-queue" : "+",
+ "committer_email" : "foo at bar.com",
"attacher_email" : "Contributer1",
}
_patch2 = {
@@ -58,8 +60,61 @@ _patch2 = {
"is_patch" : True,
"review" : "+",
"reviewer_email" : "foo at bar.com",
+ "commit-queue" : "+",
+ "committer_email" : "non-committer at example.com",
"attacher_email" : "eric at webkit.org",
}
+_patch3 = {
+ "id" : 103,
+ "bug_id" : 75,
+ "url" : "http://example.com/103",
+ "is_obsolete" : False,
+ "is_patch" : True,
+ "review" : "?",
+ "attacher_email" : "eric at webkit.org",
+}
+_patch4 = {
+ "id" : 104,
+ "bug_id" : 77,
+ "url" : "http://example.com/103",
+ "is_obsolete" : False,
+ "is_patch" : True,
+ "review" : "+",
+ "commit-queue" : "?",
+ "reviewer_email" : "foo at bar.com",
+ "attacher_email" : "Contributer2",
+}
+_patch5 = {
+ "id" : 105,
+ "bug_id" : 77,
+ "url" : "http://example.com/103",
+ "is_obsolete" : False,
+ "is_patch" : True,
+ "review" : "+",
+ "reviewer_email" : "foo at bar.com",
+ "attacher_email" : "eric at webkit.org",
+}
+_patch6 = { # Valid committer, but no reviewer.
+ "id" : 106,
+ "bug_id" : 77,
+ "url" : "http://example.com/103",
+ "is_obsolete" : False,
+ "is_patch" : True,
+ "commit-queue" : "+",
+ "committer_email" : "foo at bar.com",
+ "attacher_email" : "eric at webkit.org",
+}
+_patch7 = { # Valid review, patch is marked obsolete.
+ "id" : 107,
+ "bug_id" : 76,
+ "url" : "http://example.com/103",
+ "is_obsolete" : True,
+ "is_patch" : True,
+ "review" : "+",
+ "reviewer_email" : "foo at bar.com",
+ "attacher_email" : "eric at webkit.org",
+}
+
# This must be defined before we define the bugs, thus we don't use MockBugzilla.unassigned_email directly.
_unassigned_email = "unassigned at example.com"
@@ -67,21 +122,27 @@ _unassigned_email = "unassigned at example.com"
# FIXME: The ids should be 1, 2, 3 instead of crazy numbers.
_bug1 = {
"id" : 42,
- "title" : "The first bug",
+ "title" : "Bug with two r+'d and cq+'d patches, one of which has an invalid commit-queue setter.",
"assigned_to_email" : _unassigned_email,
"attachments" : [_patch1, _patch2],
}
_bug2 = {
"id" : 75,
- "title" : "The second bug",
+ "title" : "Bug with a patch needing review.",
"assigned_to_email" : "foo at foo.com",
- "attachments" : [],
+ "attachments" : [_patch3],
}
_bug3 = {
"id" : 76,
"title" : "The third bug",
"assigned_to_email" : _unassigned_email,
- "attachments" : [],
+ "attachments" : [_patch7],
+}
+_bug4 = {
+ "id" : 77,
+ "title" : "The fourth bug",
+ "assigned_to_email" : "foo at foo.com",
+ "attachments" : [_patch4, _patch5, _patch6],
}
class MockBugzillaQueries(Mock):
@@ -89,20 +150,30 @@ class MockBugzillaQueries(Mock):
Mock.__init__(self)
self._bugzilla = bugzilla
+ def _all_bugs(self):
+ return map(lambda bug_dictionary: Bug(bug_dictionary, self._bugzilla), self._bugzilla.bug_cache.values())
+
def fetch_bug_ids_from_commit_queue(self):
- return [42, 75]
+ bugs_with_commit_queued_patches = filter(lambda bug: bug.commit_queued_patches(), self._all_bugs())
+ return map(lambda bug: bug.id(), bugs_with_commit_queued_patches)
def fetch_attachment_ids_from_review_queue(self):
- return [197, 128]
+ unreviewed_patches = sum([bug.unreviewed_patches() for bug in self._all_bugs()], [])
+ return map(lambda patch: patch.id(), unreviewed_patches)
- def fetch_patches_from_commit_queue(self, reject_invalid_patches=False):
- return [self._bugzilla.fetch_attachment(_patch1["id"]), self._bugzilla.fetch_attachment(_patch2["id"])]
+ def fetch_patches_from_commit_queue(self):
+ return sum([bug.commit_queued_patches() for bug in self._all_bugs()], [])
def fetch_bug_ids_from_pending_commit_list(self):
- return [42, 75, 76]
-
+ bugs_with_reviewed_patches = filter(lambda bug: bug.reviewed_patches(), self._all_bugs())
+ bug_ids = map(lambda bug: bug.id(), bugs_with_reviewed_patches)
+ # NOTE: This manual hack here is to allow testing logging in test_assign_to_committer
+ # the real pending-commit query on bugzilla will return bugs with patches which have r+, but are also obsolete.
+ return bug_ids + [76]
+
def fetch_patches_from_pending_commit_list(self):
- return [self._bugzilla.fetch_attachment(_patch1["id"]), self._bugzilla.fetch_attachment(_patch2["id"])]
+ return sum([bug.reviewed_patches() for bug in self._all_bugs()], [])
+
# FIXME: Bugzilla is the wrong Mock-point. Once we have a BugzillaNetwork class we should mock that instead.
@@ -110,8 +181,8 @@ class MockBugzillaQueries(Mock):
class MockBugzilla(Mock):
bug_server_url = "http://example.com"
unassigned_email = _unassigned_email
- bug_cache = _id_to_object_dictionary(_bug1, _bug2, _bug3)
- attachment_cache = _id_to_object_dictionary(_patch1, _patch2)
+ bug_cache = _id_to_object_dictionary(_bug1, _bug2, _bug3, _bug4)
+ attachment_cache = _id_to_object_dictionary(_patch1, _patch2, _patch3, _patch4, _patch5, _patch6, _patch7)
def __init__(self):
Mock.__init__(self)
--
WebKit Debian packaging
More information about the Pkg-webkit-commits
mailing list