Changeset 101834 in webkit


Ignore:
Timestamp:
Dec 2, 2011 11:10:30 AM (12 years ago)
Author:
eric@webkit.org
Message:

Reviewed by Adam Barth.

webkit-patch post, post-commits, upload should warn when posting to a closed bug, and offer to reopen it
https://bugs.webkit.org/show_bug.cgi?id=32006

I decided not to make it warn, and just have it re-open the bug.
That's not that different from today's behavior which will
just silently attach the patch.

This patch makes behavior between upload and land-safely consistent
(previously one would assign patches and the other would not)
as well as adds the ability for both to ensure that the bug is open.

To test this I had to add a few more methods to MockBugzilla which
(positively) affected a few other test results.

I also made AbstractStep keep a cached copy of the Bug object
and used the cached copy where appropriate (including for 'bug_title').
This should reduce the number of bug fetches we perform.

  • Scripts/webkitpy/tool/commands/download_unittest.py:
  • Scripts/webkitpy/tool/commands/upload.py:
  • Scripts/webkitpy/tool/commands/upload_unittest.py:
  • Scripts/webkitpy/tool/mocktool.py:
  • Scripts/webkitpy/tool/steps/init.py:
  • Scripts/webkitpy/tool/steps/abstractstep.py:
  • Scripts/webkitpy/tool/steps/closebug.py:
  • Scripts/webkitpy/tool/steps/ensurebugisopenandassigned.py: Added.
  • Scripts/webkitpy/tool/steps/postdiff.py:
  • Scripts/webkitpy/tool/steps/postdiffforcommit.py:
  • Scripts/webkitpy/tool/steps/preparechangelog.py:
  • Scripts/webkitpy/tool/steps/steps_unittest.py:
  • Scripts/webkitpy/tool/steps/updatechangelogswithreviewer.py:
Location:
trunk/Tools
Files:
12 edited
1 copied

Legend:

Unmodified
Added
Removed
  • trunk/Tools/ChangeLog

    r101832 r101834  
     12011-12-01  Eric Seidel  <eric@webkit.org>
     2
     3        Reviewed by Adam Barth.
     4
     5        webkit-patch post, post-commits, upload should warn when posting to a closed bug, and offer to reopen it
     6        https://bugs.webkit.org/show_bug.cgi?id=32006
     7
     8        I decided not to make it warn, and just have it re-open the bug.
     9        That's not that different from today's behavior which will
     10        just silently attach the patch.
     11
     12        This patch makes behavior between upload and land-safely consistent
     13        (previously one would assign patches and the other would not)
     14        as well as adds the ability for both to ensure that the bug is open.
     15
     16        To test this I had to add a few more methods to MockBugzilla which
     17        (positively) affected a few other test results.
     18
     19        I also made AbstractStep keep a cached copy of the Bug object
     20        and used the cached copy where appropriate (including for 'bug_title').
     21        This should reduce the number of bug fetches we perform.
     22
     23        * Scripts/webkitpy/tool/commands/download_unittest.py:
     24        * Scripts/webkitpy/tool/commands/upload.py:
     25        * Scripts/webkitpy/tool/commands/upload_unittest.py:
     26        * Scripts/webkitpy/tool/mocktool.py:
     27        * Scripts/webkitpy/tool/steps/__init__.py:
     28        * Scripts/webkitpy/tool/steps/abstractstep.py:
     29        * Scripts/webkitpy/tool/steps/closebug.py:
     30        * Scripts/webkitpy/tool/steps/ensurebugisopenandassigned.py: Added.
     31        * Scripts/webkitpy/tool/steps/postdiff.py:
     32        * Scripts/webkitpy/tool/steps/postdiffforcommit.py:
     33        * Scripts/webkitpy/tool/steps/preparechangelog.py:
     34        * Scripts/webkitpy/tool/steps/steps_unittest.py:
     35        * Scripts/webkitpy/tool/steps/updatechangelogswithreviewer.py:
     36
    1372011-12-02  Gustavo Noronha Silva  <gns@gnome.org>
    238
  • trunk/Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla_mock.py

    r99140 r101834  
    389389
    390390    def reopen_bug(self, bug_id, message):
    391         pass
     391        log("MOCK reopen_bug %s with comment '%s'" % (bug_id, message))
    392392
    393393    def close_bug_as_fixed(self, bug_id, message):
  • trunk/Tools/Scripts/webkitpy/tool/commands/download_unittest.py

    r101266 r101834  
    243243Building WebKit
    244244Committed r49824: <http://trac.webkit.org/changeset/49824>
     245MOCK reopen_bug 50000 with comment 'Reverted r852 for reason:
     246
     247Reason
     248
     249Committed r49824: <http://trac.webkit.org/changeset/49824>'
    245250"""
    246251        self.assert_execute_outputs(Rollout(), [852, "Reason"], options=self._default_options(), expected_stderr=expected_stderr)
  • trunk/Tools/Scripts/webkitpy/tool/commands/upload.py

    r97553 r101834  
    217217        steps.ObsoletePatches,
    218218        steps.SuggestReviewers,
     219        steps.EnsureBugIsOpenAndAssigned,
    219220        steps.PostDiff,
    220221    ]
     
    234235        steps.ValidateChangeLogs,
    235236        steps.ObsoletePatches,
     237        steps.EnsureBugIsOpenAndAssigned,
    236238        steps.PostDiffForCommit,
    237239    ]
     
    268270        steps.ObsoletePatches,
    269271        steps.SuggestReviewers,
     272        steps.EnsureBugIsOpenAndAssigned,
    270273        steps.PostDiff,
    271274    ]
  • trunk/Tools/Scripts/webkitpy/tool/commands/upload_unittest.py

    r97504 r101834  
    9696
    9797    def test_land_safely(self):
    98         expected_stderr = "Obsoleting 2 old patches on bug 50000\nMOCK add_patch_to_bug: bug_id=50000, description=Patch for landing, mark_for_review=False, mark_for_commit_queue=False, mark_for_landing=True\n"
     98        expected_stderr = "Obsoleting 2 old patches on bug 50000\nMOCK reassign_bug: bug_id=50000, assignee=None\nMOCK add_patch_to_bug: bug_id=50000, description=Patch for landing, mark_for_review=False, mark_for_commit_queue=False, mark_for_landing=True\n"
    9999        self.assert_execute_outputs(LandSafely(), [50000], expected_stderr=expected_stderr)
    100100
  • trunk/Tools/Scripts/webkitpy/tool/steps/__init__.py

    r96444 r101834  
    4343from webkitpy.tool.steps.createbug import CreateBug
    4444from webkitpy.tool.steps.editchangelog import EditChangeLog
     45from webkitpy.tool.steps.ensurebugisopenandassigned import EnsureBugIsOpenAndAssigned
    4546from webkitpy.tool.steps.ensurelocalcommitifneeded import EnsureLocalCommitIfNeeded
    4647from webkitpy.tool.steps.obsoletepatches import ObsoletePatches
     
    4950from webkitpy.tool.steps.postdiffforcommit import PostDiffForCommit
    5051from webkitpy.tool.steps.postdiffforrevert import PostDiffForRevert
     52from webkitpy.tool.steps.preparechangelog import PrepareChangeLog
    5153from webkitpy.tool.steps.preparechangelogfordepsroll import PrepareChangeLogForDEPSRoll
    5254from webkitpy.tool.steps.preparechangelogforrevert import PrepareChangeLogForRevert
    53 from webkitpy.tool.steps.preparechangelog import PrepareChangeLog
    5455from webkitpy.tool.steps.promptforbugortitle import PromptForBugOrTitle
    5556from webkitpy.tool.steps.reopenbugafterrollout import ReopenBugAfterRollout
     
    5758from webkitpy.tool.steps.runtests import RunTests
    5859from webkitpy.tool.steps.suggestreviewers import SuggestReviewers
     60from webkitpy.tool.steps.update import Update
    5961from webkitpy.tool.steps.updatechangelogswithreviewer import UpdateChangeLogsWithReviewer
    6062from webkitpy.tool.steps.updatechromiumdeps import UpdateChromiumDEPS
    61 from webkitpy.tool.steps.update import Update
    6263from webkitpy.tool.steps.validatechangelogs import ValidateChangeLogs
    6364from webkitpy.tool.steps.validatereviewer import ValidateReviewer
  • trunk/Tools/Scripts/webkitpy/tool/steps/abstractstep.py

    r82771 r101834  
    4141
    4242    _well_known_keys = {
    43         "bug_title": lambda self, state: self._tool.bugs.fetch_bug(state["bug_id"]).title(),
     43        # FIXME: Should this use state.get('bug_id') or state.get('patch').bug_id() like UpdateChangeLogsWithReviewer does?
     44        "bug": lambda self, state: self._tool.bugs.fetch_bug(state["bug_id"]),
     45        # bug_title can either be a new title given by the user, or one from an existing bug.
     46        "bug_title": lambda self, state: self.cached_lookup(state, 'bug').title(),
    4447        "changed_files": lambda self, state: self._tool.scm().changed_files(self._options.git_commit),
    4548        "diff": lambda self, state: self._tool.scm().create_patch(self._options.git_commit, changed_files=self._changed_files(state)),
  • trunk/Tools/Scripts/webkitpy/tool/steps/closebug.py

    r68496 r101834  
    4444        # Check to make sure there are no r? or r+ patches on the bug before closing.
    4545        # Assume that r- patches are just previous patches someone forgot to obsolete.
     46        # FIXME: Should this use self.cached_lookup('bug')?  It's unclear if
     47        # state["patch"].bug_id() always equals state['bug_id'].
    4648        patches = self._tool.bugs.fetch_bug(state["patch"].bug_id()).patches()
    4749        for patch in patches:
  • trunk/Tools/Scripts/webkitpy/tool/steps/ensurebugisopenandassigned.py

    r101833 r101834  
    11# Copyright (C) 2010 Google Inc. All rights reserved.
    2 # 
     2#
    33# Redistribution and use in source and binary forms, with or without
    44# modification, are permitted provided that the following conditions are
    55# met:
    6 # 
     6#
    77#     * Redistributions of source code must retain the above copyright
    88# notice, this list of conditions and the following disclaimer.
     
    1414# contributors may be used to endorse or promote products derived from
    1515# this software without specific prior written permission.
    16 # 
     16#
    1717# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
    1818# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
     
    2828
    2929from webkitpy.tool.steps.abstractstep import AbstractStep
    30 from webkitpy.tool.steps.options import Options
    31 from webkitpy.common.system.deprecated_logging import log
    3230
    3331
    34 class CloseBug(AbstractStep):
    35     @classmethod
    36     def options(cls):
    37         return AbstractStep.options() + [
    38             Options.close_bug,
    39         ]
     32class EnsureBugIsOpenAndAssigned(AbstractStep):
     33    def run(self, state):
     34        bug = self.cached_lookup(state, "bug")
     35        if bug.is_unassigned():
     36            self._tool.bugs.reassign_bug(bug.id())
    4037
    41     def run(self, state):
    42         if not self._options.close_bug:
    43             return
    44         # Check to make sure there are no r? or r+ patches on the bug before closing.
    45         # Assume that r- patches are just previous patches someone forgot to obsolete.
    46         patches = self._tool.bugs.fetch_bug(state["patch"].bug_id()).patches()
    47         for patch in patches:
    48             if patch.review() == "?" or patch.review() == "+":
    49                 log("Not closing bug %s as attachment %s has review=%s.  Assuming there are more patches to land from this bug." % (patch.bug_id(), patch.id(), patch.review()))
    50                 return
    51         self._tool.bugs.close_bug_as_fixed(state["patch"].bug_id(), "All reviewed patches have been landed.  Closing bug.")
     38        if bug.is_closed():
     39            # FIXME: We should probably pass this message in somehow?
     40            # Right now this step is only used before PostDiff steps, so this is OK.
     41            self._tool.bugs.reopen_bug(bug.id(), "Reopening to attach new patch.")
  • trunk/Tools/Scripts/webkitpy/tool/steps/postdiff.py

    r84575 r101834  
    4848        bug_id = state["bug_id"]
    4949
    50         # FIXME: We should find some way of caching the Bug object instead of
    51         # going back to the network here.
    52         if self._tool.bugs.fetch_bug(bug_id).is_unassigned():
    53             self._tool.bugs.reassign_bug(bug_id)
    54 
    5550        self._tool.bugs.add_patch_to_bug(bug_id, diff, description, comment_text=comment_text, mark_for_review=self._options.review, mark_for_commit_queue=self._options.request_commit)
    5651        if self._options.open_bug:
  • trunk/Tools/Scripts/webkitpy/tool/steps/preparechangelog.py

    r91210 r101834  
    6464        if state.get("bug_id"):
    6565            args.append("--bug=%s" % state["bug_id"])
    66             args.append("--description=%s" % self._tool.bugs.fetch_bug(state["bug_id"]).title())
     66            args.append("--description=%s" % self.cached_lookup(state, 'bug_title'))
    6767        if self._options.email:
    6868            args.append("--email=%s" % self._options.email)
  • trunk/Tools/Scripts/webkitpy/tool/steps/steps_unittest.py

    r98687 r101834  
    3232from webkitpy.common.config.ports import WebKitPort
    3333from webkitpy.tool.mocktool import MockOptions, MockTool
    34 from webkitpy.tool.steps.update import Update
    35 from webkitpy.tool.steps.runtests import RunTests
    36 from webkitpy.tool.steps.promptforbugortitle import PromptForBugOrTitle
    3734
     35from webkitpy.tool import steps
    3836
    3937class StepsTest(unittest.TestCase):
     
    6058        options.update = True
    6159        expected_stderr = "Updating working directory\n"
    62         OutputCapture().assert_outputs(self, self._run_step, [Update, tool, options], expected_stderr=expected_stderr)
     60        OutputCapture().assert_outputs(self, self._run_step, [steps.Update, tool, options], expected_stderr=expected_stderr)
    6361
    6462    def test_prompt_for_bug_or_title_step(self):
    6563        tool = MockTool()
    6664        tool.user.prompt = lambda message: 50000
    67         self._run_step(PromptForBugOrTitle, tool=tool)
     65        self._run_step(steps.PromptForBugOrTitle, tool=tool)
     66
     67    def _post_diff_options(self):
     68        options = self._step_options()
     69        options.git_commit = None
     70        options.description = None
     71        options.comment = None
     72        options.review = True
     73        options.request_commit = False
     74        options.open_bug = True
     75        return options
     76
     77    def _assert_step_output_with_bug(self, step, bug_id, expected_stderr, options=None):
     78        state = {'bug_id': bug_id}
     79        OutputCapture().assert_outputs(self, self._run_step, [step, MockTool(), options, state], expected_stderr=expected_stderr)
     80
     81    def _assert_post_diff_output_for_bug(self, step, bug_id, expected_stderr):
     82        self._assert_step_output_with_bug(step, bug_id, expected_stderr, self._post_diff_options())
     83
     84    def test_post_diff(self):
     85        expected_stderr = "MOCK add_patch_to_bug: bug_id=78, description=Patch, mark_for_review=True, mark_for_commit_queue=False, mark_for_landing=False\nMOCK: user.open_url: http://example.com/78\n"
     86        self._assert_post_diff_output_for_bug(steps.PostDiff, 78, expected_stderr)
     87
     88    def test_post_diff_for_commit(self):
     89        expected_stderr = "MOCK add_patch_to_bug: bug_id=78, description=Patch for landing, mark_for_review=False, mark_for_commit_queue=False, mark_for_landing=True\n"
     90        self._assert_post_diff_output_for_bug(steps.PostDiffForCommit, 78, expected_stderr)
     91
     92    def test_ensure_bug_is_open_and_assigned(self):
     93        expected_stderr = "MOCK reopen_bug 50004 with comment 'Reopening to attach new patch.'\n"
     94        self._assert_step_output_with_bug(steps.EnsureBugIsOpenAndAssigned, 50004, expected_stderr)
     95        expected_stderr = "MOCK reassign_bug: bug_id=50002, assignee=None\n"
     96        self._assert_step_output_with_bug(steps.EnsureBugIsOpenAndAssigned, 50002, expected_stderr)
    6897
    6998    def test_runtests_args(self):
    7099        mock_options = self._step_options()
    71         step = RunTests(MockTool(log_executive=True), mock_options)
     100        step = steps.RunTests(MockTool(log_executive=True), mock_options)
    72101        # FIXME: We shouldn't use a real port-object here, but there is too much to mock at the moment.
    73102        mock_port = WebKitPort()
     
    75104        tool = MockTool(log_executive=True)
    76105        tool.port = lambda: mock_port
    77         step = RunTests(tool, mock_options)
     106        step = steps.RunTests(tool, mock_options)
    78107        expected_stderr = """Running Python unit tests
    79108MOCK run_and_throw_if_fail: ['Tools/Scripts/test-webkitpy'], cwd=/mock-checkout
  • trunk/Tools/Scripts/webkitpy/tool/steps/updatechangelogswithreviewer.py

    r95257 r101834  
    4444
    4545    def _guess_reviewer_from_bug(self, bug_id):
     46        # FIXME: It's unclear if it would be safe to use self.cached_lookup(state, 'bug')
     47        # here as we don't currently have a way to invalidate a bug after making changes (like ObsoletePatches does).
    4648        patches = self._tool.bugs.fetch_bug(bug_id).reviewed_patches()
    4749        if not patches:
Note: See TracChangeset for help on using the changeset viewer.