Diff
Modified: trunk/Tools/ChangeLog (101833 => 101834)
--- trunk/Tools/ChangeLog 2011-12-02 19:08:22 UTC (rev 101833)
+++ trunk/Tools/ChangeLog 2011-12-02 19:10:30 UTC (rev 101834)
@@ -1,3 +1,39 @@
+2011-12-01 Eric Seidel <[email protected]>
+
+ 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:
+
2011-12-02 Gustavo Noronha Silva <[email protected]>
Also pass --no-interact to jhbuild when updating dependencies.
Modified: trunk/Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla_mock.py (101833 => 101834)
--- trunk/Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla_mock.py 2011-12-02 19:08:22 UTC (rev 101833)
+++ trunk/Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla_mock.py 2011-12-02 19:10:30 UTC (rev 101834)
@@ -388,7 +388,7 @@
pass
def reopen_bug(self, bug_id, message):
- pass
+ log("MOCK reopen_bug %s with comment '%s'" % (bug_id, message))
def close_bug_as_fixed(self, bug_id, message):
pass
Modified: trunk/Tools/Scripts/webkitpy/tool/commands/download_unittest.py (101833 => 101834)
--- trunk/Tools/Scripts/webkitpy/tool/commands/download_unittest.py 2011-12-02 19:08:22 UTC (rev 101833)
+++ trunk/Tools/Scripts/webkitpy/tool/commands/download_unittest.py 2011-12-02 19:10:30 UTC (rev 101834)
@@ -242,6 +242,11 @@
Was that diff correct?
Building WebKit
Committed r49824: <http://trac.webkit.org/changeset/49824>
+MOCK reopen_bug 50000 with comment 'Reverted r852 for reason:
+
+Reason
+
+Committed r49824: <http://trac.webkit.org/changeset/49824>'
"""
self.assert_execute_outputs(Rollout(), [852, "Reason"], options=self._default_options(), expected_stderr=expected_stderr)
Modified: trunk/Tools/Scripts/webkitpy/tool/commands/upload.py (101833 => 101834)
--- trunk/Tools/Scripts/webkitpy/tool/commands/upload.py 2011-12-02 19:08:22 UTC (rev 101833)
+++ trunk/Tools/Scripts/webkitpy/tool/commands/upload.py 2011-12-02 19:10:30 UTC (rev 101834)
@@ -216,6 +216,7 @@
steps.ConfirmDiff,
steps.ObsoletePatches,
steps.SuggestReviewers,
+ steps.EnsureBugIsOpenAndAssigned,
steps.PostDiff,
]
@@ -233,6 +234,7 @@
steps.UpdateChangeLogsWithReviewer,
steps.ValidateChangeLogs,
steps.ObsoletePatches,
+ steps.EnsureBugIsOpenAndAssigned,
steps.PostDiffForCommit,
]
@@ -267,6 +269,7 @@
steps.ConfirmDiff,
steps.ObsoletePatches,
steps.SuggestReviewers,
+ steps.EnsureBugIsOpenAndAssigned,
steps.PostDiff,
]
long_help = """upload uploads the current diff to bugs.webkit.org.
Modified: trunk/Tools/Scripts/webkitpy/tool/commands/upload_unittest.py (101833 => 101834)
--- trunk/Tools/Scripts/webkitpy/tool/commands/upload_unittest.py 2011-12-02 19:08:22 UTC (rev 101833)
+++ trunk/Tools/Scripts/webkitpy/tool/commands/upload_unittest.py 2011-12-02 19:10:30 UTC (rev 101834)
@@ -95,7 +95,7 @@
self.assert_execute_outputs(AttachToBug(), [50000, "path/to/file.txt"], options=options, expected_stderr=expected_stderr)
def test_land_safely(self):
- 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"
+ 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"
self.assert_execute_outputs(LandSafely(), [50000], expected_stderr=expected_stderr)
def test_prepare_diff_with_arg(self):
Modified: trunk/Tools/Scripts/webkitpy/tool/steps/__init__.py (101833 => 101834)
--- trunk/Tools/Scripts/webkitpy/tool/steps/__init__.py 2011-12-02 19:08:22 UTC (rev 101833)
+++ trunk/Tools/Scripts/webkitpy/tool/steps/__init__.py 2011-12-02 19:10:30 UTC (rev 101834)
@@ -42,22 +42,23 @@
from webkitpy.tool.steps.confirmdiff import ConfirmDiff
from webkitpy.tool.steps.createbug import CreateBug
from webkitpy.tool.steps.editchangelog import EditChangeLog
+from webkitpy.tool.steps.ensurebugisopenandassigned import EnsureBugIsOpenAndAssigned
from webkitpy.tool.steps.ensurelocalcommitifneeded import EnsureLocalCommitIfNeeded
from webkitpy.tool.steps.obsoletepatches import ObsoletePatches
from webkitpy.tool.steps.options import Options
from webkitpy.tool.steps.postdiff import PostDiff
from webkitpy.tool.steps.postdiffforcommit import PostDiffForCommit
from webkitpy.tool.steps.postdiffforrevert import PostDiffForRevert
+from webkitpy.tool.steps.preparechangelog import PrepareChangeLog
from webkitpy.tool.steps.preparechangelogfordepsroll import PrepareChangeLogForDEPSRoll
from webkitpy.tool.steps.preparechangelogforrevert import PrepareChangeLogForRevert
-from webkitpy.tool.steps.preparechangelog import PrepareChangeLog
from webkitpy.tool.steps.promptforbugortitle import PromptForBugOrTitle
from webkitpy.tool.steps.reopenbugafterrollout import ReopenBugAfterRollout
from webkitpy.tool.steps.revertrevision import RevertRevision
from webkitpy.tool.steps.runtests import RunTests
from webkitpy.tool.steps.suggestreviewers import SuggestReviewers
+from webkitpy.tool.steps.update import Update
from webkitpy.tool.steps.updatechangelogswithreviewer import UpdateChangeLogsWithReviewer
from webkitpy.tool.steps.updatechromiumdeps import UpdateChromiumDEPS
-from webkitpy.tool.steps.update import Update
from webkitpy.tool.steps.validatechangelogs import ValidateChangeLogs
from webkitpy.tool.steps.validatereviewer import ValidateReviewer
Modified: trunk/Tools/Scripts/webkitpy/tool/steps/abstractstep.py (101833 => 101834)
--- trunk/Tools/Scripts/webkitpy/tool/steps/abstractstep.py 2011-12-02 19:08:22 UTC (rev 101833)
+++ trunk/Tools/Scripts/webkitpy/tool/steps/abstractstep.py 2011-12-02 19:10:30 UTC (rev 101834)
@@ -40,7 +40,10 @@
return self.cached_lookup(state, "changed_files")
_well_known_keys = {
- "bug_title": lambda self, state: self._tool.bugs.fetch_bug(state["bug_id"]).title(),
+ # FIXME: Should this use state.get('bug_id') or state.get('patch').bug_id() like UpdateChangeLogsWithReviewer does?
+ "bug": lambda self, state: self._tool.bugs.fetch_bug(state["bug_id"]),
+ # bug_title can either be a new title given by the user, or one from an existing bug.
+ "bug_title": lambda self, state: self.cached_lookup(state, 'bug').title(),
"changed_files": lambda self, state: self._tool.scm().changed_files(self._options.git_commit),
"diff": lambda self, state: self._tool.scm().create_patch(self._options.git_commit, changed_files=self._changed_files(state)),
# Absolute path to ChangeLog files.
Modified: trunk/Tools/Scripts/webkitpy/tool/steps/closebug.py (101833 => 101834)
--- trunk/Tools/Scripts/webkitpy/tool/steps/closebug.py 2011-12-02 19:08:22 UTC (rev 101833)
+++ trunk/Tools/Scripts/webkitpy/tool/steps/closebug.py 2011-12-02 19:10:30 UTC (rev 101834)
@@ -43,6 +43,8 @@
return
# Check to make sure there are no r? or r+ patches on the bug before closing.
# Assume that r- patches are just previous patches someone forgot to obsolete.
+ # FIXME: Should this use self.cached_lookup('bug')? It's unclear if
+ # state["patch"].bug_id() always equals state['bug_id'].
patches = self._tool.bugs.fetch_bug(state["patch"].bug_id()).patches()
for patch in patches:
if patch.review() == "?" or patch.review() == "+":
Copied: trunk/Tools/Scripts/webkitpy/tool/steps/ensurebugisopenandassigned.py (from rev 101833, trunk/Tools/Scripts/webkitpy/tool/steps/closebug.py) (0 => 101834)
--- trunk/Tools/Scripts/webkitpy/tool/steps/ensurebugisopenandassigned.py (rev 0)
+++ trunk/Tools/Scripts/webkitpy/tool/steps/ensurebugisopenandassigned.py 2011-12-02 19:10:30 UTC (rev 101834)
@@ -0,0 +1,41 @@
+# Copyright (C) 2010 Google Inc. All rights reserved.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+# * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+# * Redistributions in binary form must reproduce the above
+# copyright notice, this list of conditions and the following disclaimer
+# in the documentation and/or other materials provided with the
+# distribution.
+# * Neither the name of Google Inc. nor the names of its
+# contributors may be used to endorse or promote products derived from
+# this software without specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+from webkitpy.tool.steps.abstractstep import AbstractStep
+
+
+class EnsureBugIsOpenAndAssigned(AbstractStep):
+ def run(self, state):
+ bug = self.cached_lookup(state, "bug")
+ if bug.is_unassigned():
+ self._tool.bugs.reassign_bug(bug.id())
+
+ if bug.is_closed():
+ # FIXME: We should probably pass this message in somehow?
+ # Right now this step is only used before PostDiff steps, so this is OK.
+ self._tool.bugs.reopen_bug(bug.id(), "Reopening to attach new patch.")
Modified: trunk/Tools/Scripts/webkitpy/tool/steps/postdiff.py (101833 => 101834)
--- trunk/Tools/Scripts/webkitpy/tool/steps/postdiff.py 2011-12-02 19:08:22 UTC (rev 101833)
+++ trunk/Tools/Scripts/webkitpy/tool/steps/postdiff.py 2011-12-02 19:10:30 UTC (rev 101834)
@@ -47,11 +47,6 @@
comment_text = self._options.comment
bug_id = state["bug_id"]
- # FIXME: We should find some way of caching the Bug object instead of
- # going back to the network here.
- if self._tool.bugs.fetch_bug(bug_id).is_unassigned():
- self._tool.bugs.reassign_bug(bug_id)
-
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)
if self._options.open_bug:
self._tool.user.open_url(self._tool.bugs.bug_url_for_bug_id(bug_id))
Modified: trunk/Tools/Scripts/webkitpy/tool/steps/preparechangelog.py (101833 => 101834)
--- trunk/Tools/Scripts/webkitpy/tool/steps/preparechangelog.py 2011-12-02 19:08:22 UTC (rev 101833)
+++ trunk/Tools/Scripts/webkitpy/tool/steps/preparechangelog.py 2011-12-02 19:10:30 UTC (rev 101834)
@@ -63,7 +63,7 @@
args = self._tool.port().prepare_changelog_command()
if state.get("bug_id"):
args.append("--bug=%s" % state["bug_id"])
- args.append("--description=%s" % self._tool.bugs.fetch_bug(state["bug_id"]).title())
+ args.append("--description=%s" % self.cached_lookup(state, 'bug_title'))
if self._options.email:
args.append("--email=%s" % self._options.email)
Modified: trunk/Tools/Scripts/webkitpy/tool/steps/steps_unittest.py (101833 => 101834)
--- trunk/Tools/Scripts/webkitpy/tool/steps/steps_unittest.py 2011-12-02 19:08:22 UTC (rev 101833)
+++ trunk/Tools/Scripts/webkitpy/tool/steps/steps_unittest.py 2011-12-02 19:10:30 UTC (rev 101834)
@@ -31,10 +31,8 @@
from webkitpy.common.system.outputcapture import OutputCapture
from webkitpy.common.config.ports import WebKitPort
from webkitpy.tool.mocktool import MockOptions, MockTool
-from webkitpy.tool.steps.update import Update
-from webkitpy.tool.steps.runtests import RunTests
-from webkitpy.tool.steps.promptforbugortitle import PromptForBugOrTitle
+from webkitpy.tool import steps
class StepsTest(unittest.TestCase):
def _step_options(self):
@@ -59,22 +57,53 @@
options = self._step_options()
options.update = True
expected_stderr = "Updating working directory\n"
- OutputCapture().assert_outputs(self, self._run_step, [Update, tool, options], expected_stderr=expected_stderr)
+ OutputCapture().assert_outputs(self, self._run_step, [steps.Update, tool, options], expected_stderr=expected_stderr)
def test_prompt_for_bug_or_title_step(self):
tool = MockTool()
tool.user.prompt = lambda message: 50000
- self._run_step(PromptForBugOrTitle, tool=tool)
+ self._run_step(steps.PromptForBugOrTitle, tool=tool)
+ def _post_diff_options(self):
+ options = self._step_options()
+ options.git_commit = None
+ options.description = None
+ options.comment = None
+ options.review = True
+ options.request_commit = False
+ options.open_bug = True
+ return options
+
+ def _assert_step_output_with_bug(self, step, bug_id, expected_stderr, options=None):
+ state = {'bug_id': bug_id}
+ OutputCapture().assert_outputs(self, self._run_step, [step, MockTool(), options, state], expected_stderr=expected_stderr)
+
+ def _assert_post_diff_output_for_bug(self, step, bug_id, expected_stderr):
+ self._assert_step_output_with_bug(step, bug_id, expected_stderr, self._post_diff_options())
+
+ def test_post_diff(self):
+ 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"
+ self._assert_post_diff_output_for_bug(steps.PostDiff, 78, expected_stderr)
+
+ def test_post_diff_for_commit(self):
+ 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"
+ self._assert_post_diff_output_for_bug(steps.PostDiffForCommit, 78, expected_stderr)
+
+ def test_ensure_bug_is_open_and_assigned(self):
+ expected_stderr = "MOCK reopen_bug 50004 with comment 'Reopening to attach new patch.'\n"
+ self._assert_step_output_with_bug(steps.EnsureBugIsOpenAndAssigned, 50004, expected_stderr)
+ expected_stderr = "MOCK reassign_bug: bug_id=50002, assignee=None\n"
+ self._assert_step_output_with_bug(steps.EnsureBugIsOpenAndAssigned, 50002, expected_stderr)
+
def test_runtests_args(self):
mock_options = self._step_options()
- step = RunTests(MockTool(log_executive=True), mock_options)
+ step = steps.RunTests(MockTool(log_executive=True), mock_options)
# FIXME: We shouldn't use a real port-object here, but there is too much to mock at the moment.
mock_port = WebKitPort()
mock_port.name = lambda: "Mac"
tool = MockTool(log_executive=True)
tool.port = lambda: mock_port
- step = RunTests(tool, mock_options)
+ step = steps.RunTests(tool, mock_options)
expected_stderr = """Running Python unit tests
MOCK run_and_throw_if_fail: ['Tools/Scripts/test-webkitpy'], cwd=/mock-checkout
Running Perl unit tests
Modified: trunk/Tools/Scripts/webkitpy/tool/steps/updatechangelogswithreviewer.py (101833 => 101834)
--- trunk/Tools/Scripts/webkitpy/tool/steps/updatechangelogswithreviewer.py 2011-12-02 19:08:22 UTC (rev 101833)
+++ trunk/Tools/Scripts/webkitpy/tool/steps/updatechangelogswithreviewer.py 2011-12-02 19:10:30 UTC (rev 101834)
@@ -43,6 +43,8 @@
]
def _guess_reviewer_from_bug(self, bug_id):
+ # FIXME: It's unclear if it would be safe to use self.cached_lookup(state, 'bug')
+ # here as we don't currently have a way to invalidate a bug after making changes (like ObsoletePatches does).
patches = self._tool.bugs.fetch_bug(bug_id).reviewed_patches()
if not patches:
log("%s on bug %s, cannot infer reviewer." % ("No reviewed patches", bug_id))
