Changeset 67880 in webkit


Ignore:
Timestamp:
Sep 20, 2010 1:49:17 PM (14 years ago)
Author:
eric@webkit.org
Message:

2010-09-20 Eric Seidel <​eric@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:
Location:
trunk/WebKitTools
Files:
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/WebKitTools/ChangeLog

    r67876 r67880  
     12010-09-20  Eric Seidel  <eric@webkit.org>
     2
     3        Reviewed by Adam Barth.
     4
     5        commit-queue should check commit-queue+ again just before committing
     6        https://bugs.webkit.org/show_bug.cgi?id=32679
     7
     8        Added a _revalidate_patch check, right before landing.
     9
     10        Since _revalidate_patch passes the patch_id from the work item
     11        back to bugzilla, I had to fix all of the previous queue tests to
     12        use valid attachment ids (that's the majority of this change).
     13
     14        In order to validate that the bug was still open, I had to teach
     15        bugzilla.Bug about open/closed states.
     16
     17        * Scripts/webkitpy/common/net/bugzilla.py:
     18        * Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:
     19        * Scripts/webkitpy/tool/commands/queues.py:
     20        * Scripts/webkitpy/tool/commands/queues_unittest.py:
     21        * Scripts/webkitpy/tool/commands/queuestest.py:
     22        * Scripts/webkitpy/tool/mocktool.py:
     23
    1242010-09-20  Mihai Parparita  <mihaip@chromium.org>
    225

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

    r66571 r67880  
    176176        return self.assigned_to_email() in self.unassigned_emails
    177177
     178    def status(self):
     179        return self.bug_dictionary["bug_status"]
     180
     181    # Bugzilla has many status states we don't really use in WebKit:
     182    # https://bugs.webkit.org/page.cgi?id=fields.html#status
     183    _open_states = ["UNCONFIRMED", "NEW", "ASSIGNED", "REOPENED"]
     184    _closed_states = ["RESOLVED", "VERIFIED", "CLOSED"]
     185
     186    def is_open(self):
     187        return self.status() in self._open_states
     188
     189    def is_closed(self):
     190        return not self.is_open()
     191
    178192    # Rarely do we actually want obsolete attachments
    179193    def attachments(self, include_obsolete=False):
    … …  
    358372        return True
    359373
     374    def _reject_patch_if_flags_are_invalid(self, patch):
     375        return (self._validate_setter_email(
     376                patch, "reviewer", self.reject_patch_from_review_queue)
     377            and self._validate_setter_email(
     378                patch, "committer", self.reject_patch_from_commit_queue))
     379
    360380    def patches_after_rejecting_invalid_commiters_and_reviewers(self, patches):
    361         validated_patches = []
    362         for patch in patches:
    363             if (self._validate_setter_email(
    364                     patch, "reviewer", self.reject_patch_from_review_queue)
    365                 and self._validate_setter_email(
    366                     patch, "committer", self.reject_patch_from_commit_queue)):
    367                 validated_patches.append(patch)
    368         return validated_patches
     381        return [patch for patch in patches if self._reject_patch_if_flags_are_invalid(patch)]
    369382
    370383    def reject_patch_from_commit_queue(self,

  • trunk/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py

    r67582 r67880  
    5151            "next_work_item": "MOCK: update_work_items: %(name)s [103]\n" % string_replacemnts,
    5252            "process_work_item": "MOCK: update_status: %(name)s Pass\n" % string_replacemnts,
    53             "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,
     53            "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,
    5454        }
    5555        return expected_stderr

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

    r67582 r67880  
    120120
    121121    def execute(self, options, args, tool, engine=QueueEngine):
    122         self.options = options
    123         self.tool = tool
     122        self.options = options  # FIXME: This code is wrong.  Command.options is a list, this assumes an Options element!
     123        self.tool = tool  # FIXME: This code is wrong too!  Command.bind_to_tool handles this!
    124124        return engine(self.name, self, self.tool.wakeup_event).run()
    125125
    … …  
    274274            raise
    275275
     276    def _revalidate_patch(self, patch):
     277        # Bugs might get closed, or patches might be obsoleted or r-'d while the
     278        # commit-queue is processing.  Do one last minute check before landing.
     279        patch = self.tool.bugs.fetch_attachment(patch.id())
     280        if patch.is_obsolete():
     281            return None
     282        if patch.bug().is_closed():
     283            return None
     284        if not patch.committer():
     285            return None
     286        # Reviewer is not required.  Misisng reviewers will be caught during the ChangeLog check during landing.
     287        return patch
     288
    276289    def _land(self, patch):
    277290        try:
    … …  
    308321            # sure its a bad test and re can reject it outright.
    309322            self._build_and_test_patch(patch)
     323        # Do one last check to catch any bug changes (cq-, closed, reviewer changed, etc.)
     324        # This helps catch races between the bots if locks expire.
     325        patch = self._revalidate_patch(patch)
     326        if not patch:
     327            return False
    310328        self._land(patch)
    311329        return True

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

    r67582 r67880  
    3535from webkitpy.tool.commands.commandtest import CommandsTest
    3636from webkitpy.tool.commands.queues import *
    37 from webkitpy.tool.commands.queuestest import QueuesTest
     37from webkitpy.tool.commands.queuestest import QueuesTest, MockPatch
    3838from webkitpy.tool.commands.stepsequence import StepSequence
    3939from webkitpy.tool.mocktool import MockTool, MockSCM, MockStatusServer
    … …  
    4848
    4949
    50 class MockPatch(object):
     50class MockRolloutPatch(MockPatch):
    5151    def is_rollout(self):
    5252        return True
    53 
    54     def bug_id(self):
    55         return 12345
    56 
    57     def id(self):
    58         return 76543
    5953
    6054
    … …  
    159153
    160154
     155class SecondThoughtsCommitQueue(CommitQueue):
     156    def _build_and_test_patch(self, patch, first_run=True):
     157        attachment_dictionary = {
     158            "id": patch.id(),
     159            "bug_id": patch.bug_id(),
     160            "name": "Rejected",
     161            "is_obsolete": True,
     162            "is_patch": False,
     163            "review": "-",
     164            "reviewer_email": "foo@bar.com",
     165            "commit-queue": "-",
     166            "committer_email": "foo@bar.com",
     167            "attacher_email": "Contributer1",
     168        }
     169        patch = Attachment(attachment_dictionary, None)
     170        self.tool.bugs.set_override_patch(patch)
     171        return True
     172
     173
    161174class CommitQueueTest(QueuesTest):
    162175    def test_commit_queue(self):
    … …  
    172185""",
    173186            "process_work_item": "MOCK: update_status: commit-queue Pass\n",
    174             "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",
    175             "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",
     187            "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",
     188            "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",
    176189        }
    177190        self.assert_queue_outputs(CommitQueue(), expected_stderr=expected_stderr)
    … …  
    1942072 patches in commit-queue [106, 197]
    195208""",
    196             "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",
    197             "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",
    198             "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",
     209            "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",
     210            "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",
     211            "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",
    199212        }
    200213        self.assert_queue_outputs(CommitQueue(), tool=tool, expected_stderr=expected_stderr)
    … …  
    203216        tool = MockTool(log_executive=True)
    204217        tool.buildbot.light_tree_on_fire()
    205         rollout_patch = MockPatch()
     218        rollout_patch = MockRolloutPatch()
    206219        expected_stderr = {
    207220            "begin_work_queue": self._default_begin_work_queue_stderr("commit-queue", MockSCM.fake_checkout_root),
    … …  
    2182312 patches in commit-queue [106, 197]
    219232""",
    220             "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",
    221             "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",
    222             "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",
     233            "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",
     234            "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",
     235            "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",
    223236        }
    224237        self.assert_queue_outputs(CommitQueue(), tool=tool, work_item=rollout_patch, expected_stderr=expected_stderr)
    … …  
    267280        self.assertEquals(options.test, False)
    268281
     282    def test_manual_reject_during_processing(self):
     283        queue = SecondThoughtsCommitQueue()
     284        queue.bind_to_tool(MockTool())
     285        queue.process_work_item(MockPatch())
     286
    269287
    270288class RietveldUploadQueueTest(QueuesTest):
    … …  
    274292            "should_proceed_with_work_item": "MOCK: update_status: rietveld-upload-queue Uploading patch\n",
    275293            "process_work_item": "MOCK: update_status: rietveld-upload-queue Pass\n",
    276             "handle_unexpected_error": "Mock error message\nMOCK setting flag 'in-rietveld' to '-' on attachment '1234' with comment 'None' and additional comment 'None'\n",
    277             "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",
     294            "handle_unexpected_error": "Mock error message\nMOCK setting flag 'in-rietveld' to '-' on attachment '197' with comment 'None' and additional comment 'None'\n",
     295            "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",
    278296        }
    279297        self.assert_queue_outputs(RietveldUploadQueue(), expected_stderr=expected_stderr)
    … …  
    288306            "process_work_item": "MOCK: update_status: style-queue Pass\n",
    289307            "handle_unexpected_error": "Mock error message\n",
    290             "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",
     308            "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",
    291309        }
    292310        expected_exceptions = {

  • trunk/WebKitTools/Scripts/webkitpy/tool/commands/queuestest.py

    r67582 r67880  
    4646class MockPatch():
    4747    def id(self):
    48         return 1234
     48        return 197
    4949
    5050    def bug_id(self):
    51         return 345
     51        return 142
     52
     53    def is_rollout(self):
     54        return False
    5255
    5356
    5457class QueuesTest(unittest.TestCase):
     58    # Ids match patch1 in mocktool.py
    5559    mock_work_item = Attachment({
    56         "id": 1234,
    57         "bug_id": 345,
     60        "id": 197,
     61        "bug_id": 142,
    5862        "name": "Patch",
    5963        "attacher_email": "adam@example.com",

  • trunk/WebKitTools/Scripts/webkitpy/tool/mocktool.py

    r67572 r67880  
    162162    "assigned_to_email": _unassigned_email,
    163163    "attachments": [_patch1, _patch2],
     164    "bug_status": "UNCONFIRMED",
    164165}
    165166
    … …  
    170171    "assigned_to_email": "foo@foo.com",
    171172    "attachments": [_patch3],
     173    "bug_status": "ASSIGNED",
    172174}
    173175
    … …  
    178180    "assigned_to_email": _unassigned_email,
    179181    "attachments": [_patch7],
     182    "bug_status": "NEW",
    180183}
    181184
    … …  
    186189    "assigned_to_email": "foo@foo.com",
    187190    "attachments": [_patch4, _patch5, _patch6],
     191    "bug_status": "REOPENED",
    188192}
    189193
    … …  
    255259        Mock.__init__(self)
    256260        self.queries = MockBugzillaQueries(self)
    257         self.committers = CommitterList(reviewers=[Reviewer("Foo Bar",
    258                                                             "foo@bar.com")])
     261        self.committers = CommitterList(reviewers=[Reviewer("Foo Bar", "foo@bar.com")])
     262        self._override_patch = None
    259263
    260264    def create_bug(self,
    … …  
    278282        return Bug(self.bug_cache.get(bug_id), self)
    279283
     284    def set_override_patch(self, patch):
     285        self._override_patch = patch
     286
    280287    def fetch_attachment(self, attachment_id):
     288        if self._override_patch:
     289            return self._override_patch
     290
    281291        # This could be changed to .get() if we wish to allow failed lookups.
    282292        attachment_dictionary = self.attachment_cache[attachment_id]
Note: See TracChangeset for help on using the changeset viewer.