webkit-patch: Passing --no-review should submit patch to EWS by default
authordbates@webkit.org <dbates@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 14 Aug 2017 21:14:10 +0000 (21:14 +0000)
committerdbates@webkit.org <dbates@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 14 Aug 2017 21:14:10 +0000 (21:14 +0000)
https://bugs.webkit.org/show_bug.cgi?id=148899

Reviewed by David Kilzer.

Make "webkit-patch upload --no-review" and "webkit-patch post-commits --no-review" submit
the patch(es) for EWS analysis by default. Add a new optional command line argument, --no-ews,
to these commands to not submit a non-reviewed patch(es) for EWS analysis.

Frequently I want to post a draft of a patch without review to have the EWS bots process
it to catch build and test breakage before I clean it up and post it for formal review.
Currently this requires using "webkit-patch upload --no-review --open-bug" to upload the
patch and open the bug in Safari so that I can click the Submit for EWS analysis button.
We should teach "webkit-patch upload" and "webkit-patch post-commits" how to do this.

* Scripts/webkitpy/common/net/bugzilla/bugzilla.py:
(Bugzilla._parse_attachment_id_from_add_patch_to_bug_response): Add helper function to
parse the attachment id from the response page after adding an attachment to the bug.
(Bugzilla.add_patch_to_bug): Modified to parse and return the attachment id from the
HTTP response after uploading the patch.
* Scripts/webkitpy/common/net/bugzilla/bugzilla_mock.py:
(MockBugzilla.add_patch_to_bug): Return a dummy attachment id for testing purposes.
* Scripts/webkitpy/common/net/bugzilla/bugzilla_unittest.py:
(test__parse_attachment_id_from_add_patch_to_bug_response): Added.
* Scripts/webkitpy/tool/commands/upload.py:
(Upload): Add step SubmitToEWS to the list of steps when uploading a patch.
(PostCommits.__init__): Add --no-ews option to the list of applicable option flags for
this command.
(PostCommits.execute): Submit the patch to EWS, if applicable.
* Scripts/webkitpy/tool/commands/upload_unittest.py:
(test_upload): Set options.ews to False as the upload command expects this option
to be specified.
(test_upload_with_no_review_and_ews): Added.
* Scripts/webkitpy/tool/steps/__init__.py: Import module SubmitToEWS.
* Scripts/webkitpy/tool/steps/options.py:
(Options): Added command line option --no-ews (defaults: False - submit to EWS).
* Scripts/webkitpy/tool/steps/postdiff.py:
(PostDiff.options): Add --no-ews option to the list of applicable option flags for
this command.
(PostDiff.run): Submit the patch to EWS, if applicable.
* Scripts/webkitpy/tool/steps/submittoews.py: Added.
(SubmitToEWS):
(SubmitToEWS.options): Return an empty array as we this step does not have any command line options.
(SubmitToEWS.run): Submit the specified attachment ids for EWS analysis.

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@220715 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Tools/ChangeLog
Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py
Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla_mock.py
Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla_unittest.py
Tools/Scripts/webkitpy/tool/commands/upload.py
Tools/Scripts/webkitpy/tool/commands/upload_unittest.py
Tools/Scripts/webkitpy/tool/steps/__init__.py
Tools/Scripts/webkitpy/tool/steps/options.py
Tools/Scripts/webkitpy/tool/steps/postdiff.py
Tools/Scripts/webkitpy/tool/steps/submittoews.py [new file with mode: 0644]

index 2fbf71b..e4470b8 100644 (file)
@@ -1,3 +1,50 @@
+2017-08-14  Daniel Bates  <dabates@apple.com>
+
+        webkit-patch: Passing --no-review should submit patch to EWS by default
+        https://bugs.webkit.org/show_bug.cgi?id=148899
+
+        Reviewed by David Kilzer.
+
+        Make "webkit-patch upload --no-review" and "webkit-patch post-commits --no-review" submit
+        the patch(es) for EWS analysis by default. Add a new optional command line argument, --no-ews,
+        to these commands to not submit a non-reviewed patch(es) for EWS analysis.
+
+        Frequently I want to post a draft of a patch without review to have the EWS bots process
+        it to catch build and test breakage before I clean it up and post it for formal review.
+        Currently this requires using "webkit-patch upload --no-review --open-bug" to upload the
+        patch and open the bug in Safari so that I can click the Submit for EWS analysis button.
+        We should teach "webkit-patch upload" and "webkit-patch post-commits" how to do this.
+
+        * Scripts/webkitpy/common/net/bugzilla/bugzilla.py:
+        (Bugzilla._parse_attachment_id_from_add_patch_to_bug_response): Add helper function to
+        parse the attachment id from the response page after adding an attachment to the bug.
+        (Bugzilla.add_patch_to_bug): Modified to parse and return the attachment id from the
+        HTTP response after uploading the patch.
+        * Scripts/webkitpy/common/net/bugzilla/bugzilla_mock.py:
+        (MockBugzilla.add_patch_to_bug): Return a dummy attachment id for testing purposes.
+        * Scripts/webkitpy/common/net/bugzilla/bugzilla_unittest.py:
+        (test__parse_attachment_id_from_add_patch_to_bug_response): Added.
+        * Scripts/webkitpy/tool/commands/upload.py:
+        (Upload): Add step SubmitToEWS to the list of steps when uploading a patch. 
+        (PostCommits.__init__): Add --no-ews option to the list of applicable option flags for
+        this command.
+        (PostCommits.execute): Submit the patch to EWS, if applicable.
+        * Scripts/webkitpy/tool/commands/upload_unittest.py:
+        (test_upload): Set options.ews to False as the upload command expects this option
+        to be specified.
+        (test_upload_with_no_review_and_ews): Added.
+        * Scripts/webkitpy/tool/steps/__init__.py: Import module SubmitToEWS.
+        * Scripts/webkitpy/tool/steps/options.py:
+        (Options): Added command line option --no-ews (defaults: False - submit to EWS).
+        * Scripts/webkitpy/tool/steps/postdiff.py:
+        (PostDiff.options): Add --no-ews option to the list of applicable option flags for
+        this command.
+        (PostDiff.run): Submit the patch to EWS, if applicable.
+        * Scripts/webkitpy/tool/steps/submittoews.py: Added.
+        (SubmitToEWS):
+        (SubmitToEWS.options): Return an empty array as we this step does not have any command line options.
+        (SubmitToEWS.run): Submit the specified attachment ids for EWS analysis.
+
 2017-08-14  Ryan Haddad  <ryanhaddad@apple.com>
 
         Disable two flaky VideoControlsManager API tests.
index 8203d14..b10c96f 100644 (file)
@@ -620,6 +620,14 @@ class Bugzilla(object):
             self.browser['comment'] = comment_text
         self.browser.submit()
 
+    @staticmethod
+    def _parse_attachment_id_from_add_patch_to_bug_response(response_html):
+        match = re.search('<title>Attachment (?P<attachment_id>\d+) added to Bug \d+</title>', response_html)
+        if match:
+            return match.group('attachment_id')
+        _log.warning('Unable to parse attachment id')
+        return None
+
     # FIXME: The arguments to this function should be simplified and then
     # this should be merged into add_attachment_to_bug
     def add_patch_to_bug(self,
@@ -652,7 +660,8 @@ class Bugzilla(object):
         if comment_text:
             _log.info(comment_text)
             self.browser['comment'] = comment_text
-        self.browser.submit()
+        response = self.browser.submit()
+        return self._parse_attachment_id_from_add_patch_to_bug_response(response.read())
 
     # FIXME: There has to be a more concise way to write this method.
     def _check_create_bug_response(self, response_html):
index 068d594..bb2c274 100644 (file)
@@ -461,6 +461,7 @@ class MockBugzilla(object):
             _log.info("-- Begin comment --")
             _log.info(comment_text)
             _log.info("-- End comment --")
+        return '10001'
 
     def add_cc_to_bug(self, bug_id, ccs):
         pass
index 09402b2..9b8c805 100644 (file)
@@ -342,6 +342,18 @@ Ignore this bug.  Just for testing failure modes of webkit-patch and the commit-
         title_html_bugzilla_425 = "<title>Bug 101640 Submitted &ndash; Testing webkit-patch again</title>"
         self.assertEqual(bugzilla._check_create_bug_response(title_html_bugzilla_425), '101640')
 
+    def test__parse_attachment_id_from_add_patch_to_bug_response(self):
+        bugzilla = Bugzilla()
+
+        title_html = '<title>Attachment 317591 added to Bug 175247</title>'
+        self.assertEqual(bugzilla._parse_attachment_id_from_add_patch_to_bug_response(title_html), '317591')
+
+        title_html = '<title>Attachment 317591; malformed</title>'
+        self.assertEqual(bugzilla._parse_attachment_id_from_add_patch_to_bug_response(title_html), None)
+
+        title_html = '<title>Attachment A added to Bug 175247</title>'
+        self.assertEqual(bugzilla._parse_attachment_id_from_add_patch_to_bug_response(title_html), None)
+
 
 class BugzillaQueriesTest(unittest.TestCase):
     _sample_request_page = """
index c592519..1b0fcfa 100644 (file)
@@ -284,6 +284,7 @@ class Upload(AbstractPatchUploadingCommand):
         steps.SuggestReviewers,
         steps.EnsureBugIsOpenAndAssigned,
         steps.PostDiff,
+        steps.SubmitToEWS,
     ]
     long_help = """upload uploads the current diff to bugs.webkit.org.
     If no bug id is provided, upload will create a bug.
@@ -321,6 +322,7 @@ class PostCommits(Command):
             steps.Options.obsolete_patches,
             steps.Options.review,
             steps.Options.request_commit,
+            steps.Options.ews,
         ]
         Command.__init__(self, options=options, requires_local_commits=True)
 
@@ -356,7 +358,13 @@ class PostCommits(Command):
             diff = tool.scm().create_patch(git_commit=commit_id)
             description = options.description or commit_message.description(lstrip=True, strip_url=True)
             comment_text = self._comment_text_for_commit(options, commit_message, tool, commit_id)
-            tool.bugs.add_patch_to_bug(bug_id, diff, description, comment_text, mark_for_review=options.review, mark_for_commit_queue=options.request_commit)
+            attachment_id = tool.bugs.add_patch_to_bug(bug_id, diff, description, comment_text, mark_for_review=options.review, mark_for_commit_queue=options.request_commit)
+
+            # We only need to submit --no-review patches to EWS as patches posted for review are
+            # automatically submitted to EWS by EWSFeeder.
+            if not options.review and options.ews:
+                state = {'attachment_ids': [attachment_id]}
+                steps.SubmitToEWS(tool, options).run(state)
 
 
 # FIXME: This command needs to be brought into the modern age with steps and CommitInfo.
index 61f6090..387b4be 100644 (file)
@@ -123,6 +123,7 @@ MOCK add_patch_to_bug: bug_id=50000, description=Patch for landing, mark_for_rev
         options.non_interactive = False
         options.request_commit = False
         options.review = True
+        options.submit_to_ews = False
         options.sort_xcode_project = False
         options.suggest_reviewers = False
         expected_logs = """MOCK: user.open_url: file://...
@@ -134,6 +135,29 @@ MOCK: user.open_url: http://example.com/50000
 """
         self.assert_execute_outputs(Upload(), [50000], options=options, expected_logs=expected_logs)
 
+    def test_upload_with_no_review_and_ews(self):
+        options = MockOptions()
+        options.cc = None
+        options.check_style = True
+        options.check_style_filter = None
+        options.comment = None
+        options.description = 'MOCK description'
+        options.non_interactive = False
+        options.request_commit = False
+        options.review = False
+        options.ews = True
+        options.sort_xcode_project = False
+        options.suggest_reviewers = False
+        expected_logs = """MOCK: user.open_url: file://...
+Was that diff correct?
+Obsoleting 2 old patches on bug 50000
+MOCK reassign_bug: bug_id=50000, assignee=None
+MOCK add_patch_to_bug: bug_id=50000, description=MOCK description, mark_for_review=False, mark_for_commit_queue=False, mark_for_landing=False
+MOCK: user.open_url: http://example.com/50000
+MOCK: submit_to_ews: 10001
+"""
+        self.assert_execute_outputs(Upload(), [50000], options=options, expected_logs=expected_logs)
+
     def test_mark_bug_fixed(self):
         tool = MockTool()
         tool._scm.last_svn_commit_log = lambda: "r9876 |"
index 9cabc4f..f3c170e 100644 (file)
@@ -60,6 +60,7 @@ 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.sortxcodeprojectfiles import SortXcodeProjectFiles
+from webkitpy.tool.steps.submittoews import SubmitToEWS
 from webkitpy.tool.steps.suggestreviewers import SuggestReviewers
 from webkitpy.tool.steps.update import Update
 from webkitpy.tool.steps.updatechangelogswithreviewer import UpdateChangeLogsWithReviewer
index a6ee042..7259835 100644 (file)
@@ -45,6 +45,7 @@ class Options(object):
     confirm = make_option("--no-confirm", action="store_false", dest="confirm", default=True, help="Skip confirmation steps.")
     description = make_option("-m", "--description", action="store", type="string", dest="description", help="Description string for the attachment")
     email = make_option("--email", action="store", type="string", dest="email", help="Email address to use in ChangeLogs.")
+    ews = make_option('--no-ews', action='store_false', dest='ews', default=True, help='Do not submit the patch to EWS for analysis (only applies when using --no-review).')
     force_clean = make_option("--force-clean", action="store_true", dest="force_clean", default=False, help="Clean working directory before applying patches (removes local changes and commits)")
     git_commit = make_option("-g", "--git-commit", action="store", dest="git_commit", help="Operate on a local commit. If a range, the commits are squashed into one. <ref>.... includes the working copy changes. UPSTREAM can be used for the upstream/tracking branch.")
     group = make_option("--group", action="store", dest="group", default=None, help="")
index fc5c443..b68d406 100644 (file)
@@ -39,6 +39,7 @@ class PostDiff(AbstractStep):
             Options.review,
             Options.request_commit,
             Options.open_bug,
+            Options.ews,
         ]
 
     def run(self, state):
@@ -47,6 +48,11 @@ class PostDiff(AbstractStep):
         comment_text = self._options.comment
         bug_id = state["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)
+        attachment_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))
+
+        # We only need to submit --no-review patches to EWS as patches posted for review are
+        # automatically submitted to EWS by EWSFeeder.
+        if not self._options.review and self._options.ews:
+            state['attachment_ids'] = [attachment_id]
diff --git a/Tools/Scripts/webkitpy/tool/steps/submittoews.py b/Tools/Scripts/webkitpy/tool/steps/submittoews.py
new file mode 100644 (file)
index 0000000..1eade14
--- /dev/null
@@ -0,0 +1,38 @@
+# Copyright (C) 2017 Apple 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:
+# 1. Redistributions of source code must retain the above copyright
+#    notice, this list of conditions and the following disclaimer.
+# 2. 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.
+#
+# THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS 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 APPLE INC. OR ITS 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.
+
+import logging
+
+from webkitpy.tool.steps.abstractstep import AbstractStep
+
+_log = logging.getLogger(__name__)
+
+
+class SubmitToEWS(AbstractStep):
+    @classmethod
+    def options(cls):
+        return []
+
+    def run(self, state):
+        for attachment_id in state.get('attachment_ids', []):
+            self._tool.status_server.submit_to_ews(attachment_id)