Changeset 53298 in webkit


Ignore:
Timestamp:
Jan 14, 2010 4:50:37 PM (14 years ago)
Author:
eric@webkit.org
Message:

2010-01-14 Eric Seidel <​eric@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.
Location:
trunk/WebKitTools
Files:
8 edited

Legend:

Unmodified
Added
Removed

  • trunk/WebKitTools/ChangeLog

    r53281 r53298  
     12010-01-14  Eric Seidel  <eric@webkit.org>
     2
     3        Reviewed by Adam Barth.
     4
     5        REGRESSION(53133): commit-queue no longer rejects patches with invalid committers, instead it hangs
     6        https://bugs.webkit.org/show_bug.cgi?id=33638
     7
     8        * Scripts/webkitpy/bugzilla.py:
     9         - Add Bug.id() to match Attachment.id()
     10         - Give Bug.reviewed_patches and commit_queued_patches the option to return patches with invalid committers/reviewers.
     11         - Add back a missing variable to _validate_setter_email found by the new unit tests!
     12        * Scripts/webkitpy/commands/queries.py:
     13         - Add FIXMEs about the commands being confusingly named.
     14        * Scripts/webkitpy/commands/queries_unittest.py:
     15         - Update results to reflect the newly restructured mock bug cache.
     16        * Scripts/webkitpy/commands/queues.py:
     17         - Add a new _validate_patches_in_commit_queue method (this is what fixes the regression).
     18         - Add a FIXME about eventually sorting the patches into some order.
     19        * Scripts/webkitpy/commands/queues_unittest.py:
     20         - Update results now that with the newly restructure mock bug cache we're testing cq+'d patches with an invalid committer.
     21        * Scripts/webkitpy/commands/upload_unittest.py:
     22         - Update results to match the newly restructured mock bug cache.
     23        * Scripts/webkitpy/mock_bugzillatool.py:
     24         - 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.
     25         - Add a few more attachments and bug dictionaries for use by the tests.
     26
    1272010-01-13  Diego Gonzalez  <diego.gonzalez@openbossa.org>
    228

  • trunk/WebKitTools/Scripts/webkitpy/bugzilla.py

    r53133 r53298  
    131131        self._bugzilla = bugzilla
    132132
     133    def id(self):
     134        return self.bug_dictionary["id"]
     135
    133136    def assigned_to_email(self):
    134137        return self.bug_dictionary["assigned_to_email"]
    … …  
    147150        return [patch for patch in self.patches() if patch.review() == "?"]
    148151
    149     def reviewed_patches(self):
     152    def reviewed_patches(self, include_invalid=False):
     153        patches = [patch for patch in self.patches() if patch.review() == "+"]
     154        if include_invalid:
     155            return patches
    150156        # Checking reviewer() ensures that it was both reviewed and has a valid reviewer.
    151         return [patch for patch in self.patches() if patch.review() == "+" and patch.reviewer()]
    152 
    153     def commit_queued_patches(self):
     157        return filter(lambda patch: patch.reviewer(), patches)
     158
     159    def commit_queued_patches(self, include_invalid=False):
     160        patches = [patch for patch in self.patches() if patch.commit_queue() == "+"]
     161        if include_invalid:
     162            return patches
    154163        # Checking committer() ensures that it was both commit-queue+'d and has a valid committer.
    155         return [patch for patch in self.patches() if patch.commit_queue() == "+" and patch.committer()]
     164        return filter(lambda patch: patch.committer(), patches)
    156165
    157166
    … …  
    196205        return self._fetch_bug_ids_advanced_query(commit_queue_url)
    197206
     207    # This function will only return patches which have valid committers set.  It won't reject patches with invalid committers/reviewers.
    198208    def fetch_patches_from_commit_queue(self):
    199209        return sum([self._fetch_bug(bug_id).commit_queued_patches() for bug_id in self.fetch_bug_ids_from_commit_queue()], [])
    … …  
    237247        committer = getattr(patch, result_key)()
    238248        # If the flag is set, and we don't recognize the setter, reject the flag!
    239         if patch._attachment_dictionary.get("%s_email" % result_key) and not committer:
     249        setter_email = patch._attachment_dictionary.get("%s_email" % result_key)
     250        if setter_email and not committer:
    240251            rejection_function(patch.id(), self._flag_permission_rejection_message(setter_email, result_key))
    241252            return False

  • trunk/WebKitTools/Scripts/webkitpy/commands/queries.py

    r53133 r53298  
    4343
    4444    def execute(self, options, args, tool):
     45        # 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).
    4546        bug_ids = tool.bugs.queries.fetch_bug_ids_from_commit_queue()
    4647        for bug_id in bug_ids:
    … …  
    5354
    5455    def execute(self, options, args, tool):
     56        # 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).
    5557        patches = tool.bugs.queries.fetch_patches_from_commit_queue()
    5658        log("Patches in commit queue:")

  • trunk/WebKitTools/Scripts/webkitpy/commands/queries_unittest.py

    r53133 r53298  
    3737class QueryCommandsTest(CommandsTest):
    3838    def test_bugs_to_commit(self):
    39         self.assert_execute_outputs(BugsToCommit(), None, "42\n75\n")
     39        expected_stderr = "Warning, attachment 128 on bug 42 has invalid committer (non-committer@example.com)\n"
     40        self.assert_execute_outputs(BugsToCommit(), None, "42\n77\n", expected_stderr)
    4041
    4142    def test_patches_to_commit(self):
    42         expected_stdout = "http://example.com/197\nhttp://example.com/128\n"
    43         expected_stderr = "Patches in commit queue:\n"
     43        expected_stdout = "http://example.com/197\nhttp://example.com/103\n"
     44        expected_stderr = "Warning, attachment 128 on bug 42 has invalid committer (non-committer@example.com)\nPatches in commit queue:\n"
    4445        self.assert_execute_outputs(PatchesToCommit(), None, expected_stdout, expected_stderr)
    4546
    4647    def test_patches_to_commit_queue(self):
    47         expected_stdout = "http://example.com/197&action=edit\n"
    48         expected_stderr = "128 committer = \"Eric Seidel\" <eric@webkit.org>\n"
     48        expected_stdout = "http://example.com/104&action=edit\n"
     49        expected_stderr = "197 already has cq=+\n128 already has cq=+\n105 committer = \"Eric Seidel\" <eric@webkit.org>\n"
    4950        options = Mock()
    5051        options.bugs = False
    5152        self.assert_execute_outputs(PatchesToCommitQueue(), None, expected_stdout, expected_stderr, options=options)
    5253
    53         expected_stdout = "http://example.com/42\n"
     54        expected_stdout = "http://example.com/77\n"
    5455        options.bugs = True
    5556        self.assert_execute_outputs(PatchesToCommitQueue(), None, expected_stdout, expected_stderr, options=options)
    5657
    5758    def test_patches_to_review(self):
    58         expected_stdout = "197\n128\n"
     59        expected_stdout = "103\n"
    5960        expected_stderr = "Patches pending review:\n"
    6061        self.assert_execute_outputs(PatchesToReview(), None, expected_stdout, expected_stderr)

  • trunk/WebKitTools/Scripts/webkitpy/commands/queues.py

    r53133 r53298  
    144144        self.committer_validator = CommitterValidator(self.tool.bugs)
    145145
     146    def _validate_patches_in_commit_queue(self):
     147        # Not using BugzillaQueries.fetch_patches_from_commit_queue() so we can reject patches with invalid committers/reviewers.
     148        bug_ids = self.tool.bugs.queries.fetch_bug_ids_from_commit_queue()
     149        all_patches = sum([self.tool.bugs.fetch_bug(bug_id).commit_queued_patches(include_invalid=True) for bug_id in bug_ids], [])
     150        return self.committer_validator.patches_after_rejecting_invalid_commiters_and_reviewers(all_patches)
     151
    146152    def next_work_item(self):
    147         patches = self.tool.bugs.queries.fetch_patches_from_commit_queue()
    148         patches = self.committer_validator.patches_after_rejecting_invalid_commiters_and_reviewers(patches)
     153        patches = self._validate_patches_in_commit_queue()
     154        # FIXME: We could sort the patches in a specific order here, was suggested by https://bugs.webkit.org/show_bug.cgi?id=33395
    149155        if not patches:
    150156            self._update_status("Empty queue")

  • trunk/WebKitTools/Scripts/webkitpy/commands/queues_unittest.py

    r52907 r53298  
    6565
    6666class CommitQueueTest(QueuesTest):
    67     def test_style_queue(self):
     67    def test_commit_queue(self):
    6868        expected_stderr = {
    6969            "begin_work_queue" : "CAUTION: commit-queue will discard all local changes in \"%s\"\nRunning WebKit commit-queue.\n" % os.getcwd(),
    70             "next_work_item" : "2 patches in commit-queue [197, 128]\n",
     70            # 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.
     71            "next_work_item" : """Warning, attachment 128 on bug 42 has invalid committer (non-committer@example.com)
     72Warning, attachment 128 on bug 42 has invalid committer (non-committer@example.com)
     732 patches in commit-queue [197, 106]
     74""",
    7175        }
    7276        self.assert_queue_outputs(CommitQueue(), expected_stderr=expected_stderr)

  • trunk/WebKitTools/Scripts/webkitpy/commands/upload_unittest.py

    r53120 r53298  
    4545    def test_assign_to_committer(self):
    4646        tool = MockBugzillaTool()
    47         expected_stderr = "Bug 75 is already assigned to foo@foo.com (None).\nBug 76 has no non-obsolete patches, ignoring.\n"
     47        expected_stderr = "Bug 77 is already assigned to foo@foo.com (None).\nBug 76 has no non-obsolete patches, ignoring.\n"
    4848        self.assert_execute_outputs(AssignToCommitter(), [], expected_stderr=expected_stderr, tool=tool)
    4949        tool.bugs.reassign_bug.assert_called_with(42, "eric@webkit.org", "Attachment 128 was posted by a committer and has review+, assigning to Eric Seidel for commit.")
    … …  
    7575        options = Mock()
    7676        options.bug_id = 42
    77         expected_stderr = "Bug: <http://example.com/42> The first bug\nRevision: 9876\nAdding comment to Bug 42.\n"
     77        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"
    7878        self.assert_execute_outputs(MarkBugFixed(), [], expected_stderr=expected_stderr, tool=tool, options=options)
    7979

  • trunk/WebKitTools/Scripts/webkitpy/mock_bugzillatool.py

    r53228 r53298  
    4949    "review" : "+",
    5050    "reviewer_email" : "foo@bar.com",
     51    "commit-queue" : "+",
     52    "committer_email" : "foo@bar.com",
    5153    "attacher_email" : "Contributer1",
    5254}
    … …  
    5961    "review" : "+",
    6062    "reviewer_email" : "foo@bar.com",
    61     "attacher_email" : "eric@webkit.org",
    62 }
     63    "commit-queue" : "+",
     64    "committer_email" : "non-committer@example.com",
     65    "attacher_email" : "eric@webkit.org",
     66}
     67_patch3 = {
     68    "id" : 103,
     69    "bug_id" : 75,
     70    "url" : "http://example.com/103",
     71    "is_obsolete" : False,
     72    "is_patch" : True,
     73    "review" : "?",
     74    "attacher_email" : "eric@webkit.org",
     75}
     76_patch4 = {
     77    "id" : 104,
     78    "bug_id" : 77,
     79    "url" : "http://example.com/103",
     80    "is_obsolete" : False,
     81    "is_patch" : True,
     82    "review" : "+",
     83    "commit-queue" : "?",
     84    "reviewer_email" : "foo@bar.com",
     85    "attacher_email" : "Contributer2",
     86}
     87_patch5 = {
     88    "id" : 105,
     89    "bug_id" : 77,
     90    "url" : "http://example.com/103",
     91    "is_obsolete" : False,
     92    "is_patch" : True,
     93    "review" : "+",
     94    "reviewer_email" : "foo@bar.com",
     95    "attacher_email" : "eric@webkit.org",
     96}
     97_patch6 = { # Valid committer, but no reviewer.
     98    "id" : 106,
     99    "bug_id" : 77,
     100    "url" : "http://example.com/103",
     101    "is_obsolete" : False,
     102    "is_patch" : True,
     103    "commit-queue" : "+",
     104    "committer_email" : "foo@bar.com",
     105    "attacher_email" : "eric@webkit.org",
     106}
     107_patch7 = { # Valid review, patch is marked obsolete.
     108    "id" : 107,
     109    "bug_id" : 76,
     110    "url" : "http://example.com/103",
     111    "is_obsolete" : True,
     112    "is_patch" : True,
     113    "review" : "+",
     114    "reviewer_email" : "foo@bar.com",
     115    "attacher_email" : "eric@webkit.org",
     116}
     117
    63118
    64119# This must be defined before we define the bugs, thus we don't use MockBugzilla.unassigned_email directly.
    … …  
    68123_bug1 = {
    69124    "id" : 42,
    70     "title" : "The first bug",
     125    "title" : "Bug with two r+'d and cq+'d patches, one of which has an invalid commit-queue setter.",
    71126    "assigned_to_email" : _unassigned_email,
    72127    "attachments" : [_patch1, _patch2],
    … …  
    74129_bug2 = {
    75130    "id" : 75,
    76     "title" : "The second bug",
     131    "title" : "Bug with a patch needing review.",
    77132    "assigned_to_email" : "foo@foo.com",
    78     "attachments" : [],
     133    "attachments" : [_patch3],
    79134}
    80135_bug3 = {
    … …  
    82137    "title" : "The third bug",
    83138    "assigned_to_email" : _unassigned_email,
    84     "attachments" : [],
     139    "attachments" : [_patch7],
     140}
     141_bug4 = {
     142    "id" : 77,
     143    "title" : "The fourth bug",
     144    "assigned_to_email" : "foo@foo.com",
     145    "attachments" : [_patch4, _patch5, _patch6],
    85146}
    86147
    … …  
    90151        self._bugzilla = bugzilla
    91152
     153    def _all_bugs(self):
     154        return map(lambda bug_dictionary: Bug(bug_dictionary, self._bugzilla), self._bugzilla.bug_cache.values())
     155
    92156    def fetch_bug_ids_from_commit_queue(self):
    93         return [42, 75]
     157        bugs_with_commit_queued_patches = filter(lambda bug: bug.commit_queued_patches(), self._all_bugs())
     158        return map(lambda bug: bug.id(), bugs_with_commit_queued_patches)
    94159
    95160    def fetch_attachment_ids_from_review_queue(self):
    96         return [197, 128]
    97 
    98     def fetch_patches_from_commit_queue(self, reject_invalid_patches=False):
    99         return [self._bugzilla.fetch_attachment(_patch1["id"]), self._bugzilla.fetch_attachment(_patch2["id"])]
     161        unreviewed_patches = sum([bug.unreviewed_patches() for bug in self._all_bugs()], [])
     162        return map(lambda patch: patch.id(), unreviewed_patches)
     163
     164    def fetch_patches_from_commit_queue(self):
     165        return sum([bug.commit_queued_patches() for bug in self._all_bugs()], [])
    100166
    101167    def fetch_bug_ids_from_pending_commit_list(self):
    102         return [42, 75, 76]
    103    
     168        bugs_with_reviewed_patches = filter(lambda bug: bug.reviewed_patches(), self._all_bugs())
     169        bug_ids = map(lambda bug: bug.id(), bugs_with_reviewed_patches)
     170        # NOTE: This manual hack here is to allow testing logging in test_assign_to_committer
     171        # the real pending-commit query on bugzilla will return bugs with patches which have r+, but are also obsolete.
     172        return bug_ids + [76]
     173
    104174    def fetch_patches_from_pending_commit_list(self):
    105         return [self._bugzilla.fetch_attachment(_patch1["id"]), self._bugzilla.fetch_attachment(_patch2["id"])]
     175        return sum([bug.reviewed_patches() for bug in self._all_bugs()], [])
     176
    106177
    107178
    … …  
    111182    bug_server_url = "http://example.com"
    112183    unassigned_email = _unassigned_email
    113     bug_cache = _id_to_object_dictionary(_bug1, _bug2, _bug3)
    114     attachment_cache = _id_to_object_dictionary(_patch1, _patch2)
     184    bug_cache = _id_to_object_dictionary(_bug1, _bug2, _bug3, _bug4)
     185    attachment_cache = _id_to_object_dictionary(_patch1, _patch2, _patch3, _patch4, _patch5, _patch6, _patch7)
    115186
    116187    def __init__(self):
Note: See TracChangeset for help on using the changeset viewer.