Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2608923002:
[WPT Export] Delete remote branch, add label, additional refactoring (Closed)
Description[WPT Export] Delete remote branch, add label, additional refactoring
This CL
- Refactors TestExporter.run into 3 methods
- Has a number of other fixes to get things working
- Adds the `chromium-export` label to PRs after creation
- Deletes remote branch after uploading
(5/5) This CL succeeds https://codereview.chromium.org/2595143002 and is the last of the pipelined CLs broken out from https://codereview.chromium.org/2544173002.
BUG=657117, 676426
R=qyearsley@chromium.org
Review-Url: https://codereview.chromium.org/2608923002
Cr-Commit-Position: refs/heads/master@{#442198}
Committed: https://chromium.googlesource.com/chromium/src/+/64ea253d7b62b7a193f7ba9865e66b52174c98a8
Patch Set 1 #Patch Set 2 : Fix test failures #
Total comments: 18
Patch Set 3 : Address CL feedback #
Messages
Total messages: 25 (14 generated)
Description was changed from ========== Make additional changes to get things working Address CL feedback Change ChromiumCommit.format_patch to use a filtered list of files Expand abbreviated variable names Add label to PR after creating Add missing constant Merge ChromiumWPT functionality into TestExporter, use ChromiumCommits Address CL feedback Change ChromiumCommit.format_patch to use a filtered list of files BUG= ========== to ========== This CL - Refactors TestExporter.run into 3 methods - Has a number of other fixes to get things working BUG= ==========
Description was changed from ========== This CL - Refactors TestExporter.run into 3 methods - Has a number of other fixes to get things working BUG= ========== to ========== This CL - Refactors TestExporter.run into 3 methods - Has a number of other fixes to get things working - Adds the `chromium-export` label to PRs after creation BUG= ==========
Description was changed from ========== This CL - Refactors TestExporter.run into 3 methods - Has a number of other fixes to get things working - Adds the `chromium-export` label to PRs after creation BUG= ========== to ========== This CL - Refactors TestExporter.run into 3 methods - Has a number of other fixes to get things working - Adds the `chromium-export` label to PRs after creation - Deletes remote branch after uploading BUG=657117 R=qyearsley@chromium.org ==========
jeffcarp@chromium.org changed reviewers: + qyearsley@chromium.org
https://codereview.chromium.org/2608923002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py (right): https://codereview.chromium.org/2608923002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:30: # The script assumes it will be run every 10 minutes or so I broke up the run() function into 2 separate functions that are called based on whether an in-flight PR exists: `merge_in_flight_pull_request` and `export_first_exportable_commit`.
Description was changed from ========== This CL - Refactors TestExporter.run into 3 methods - Has a number of other fixes to get things working - Adds the `chromium-export` label to PRs after creation - Deletes remote branch after uploading BUG=657117 R=qyearsley@chromium.org ========== to ========== This CL - Refactors TestExporter.run into 3 methods - Has a number of other fixes to get things working - Adds the `chromium-export` label to PRs after creation - Deletes remote branch after uploading (5/5) This CL succeeds https://codereview.chromium.org/2595143002 and is the last of the pipelined CLs broken out from https://codereview.chromium.org/2544173002. BUG=657117 R=qyearsley@chromium.org ==========
Description was changed from ========== This CL - Refactors TestExporter.run into 3 methods - Has a number of other fixes to get things working - Adds the `chromium-export` label to PRs after creation - Deletes remote branch after uploading (5/5) This CL succeeds https://codereview.chromium.org/2595143002 and is the last of the pipelined CLs broken out from https://codereview.chromium.org/2544173002. BUG=657117 R=qyearsley@chromium.org ========== to ========== This CL - Refactors TestExporter.run into 3 methods - Has a number of other fixes to get things working - Adds the `chromium-export` label to PRs after creation - Deletes remote branch after uploading (5/5) This CL succeeds https://codereview.chromium.org/2595143002 and is the last of the pipelined CLs broken out from https://codereview.chromium.org/2544173002. BUG=657117,676426 R=qyearsley@chromium.org ==========
On 2017/01/03 at 20:42:31, jeffcarp wrote: > https://codereview.chromium.org/2608923002/diff/20001/third_party/WebKit/Tool... > File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py (right): > > https://codereview.chromium.org/2608923002/diff/20001/third_party/WebKit/Tool... > third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:30: # The script assumes it will be run every 10 minutes or so > I broke up the run() function into 2 separate functions that are called based on whether an in-flight PR exists: `merge_in_flight_pull_request` and `export_first_exportable_commit`. This should be ready for review btw - this is the last of the broken-up CLs (although I have another one coming after this to fix the git commit author issue)
On 2017/01/06 at 18:48:13, jeffcarp wrote: > On 2017/01/03 at 20:42:31, jeffcarp wrote: > > https://codereview.chromium.org/2608923002/diff/20001/third_party/WebKit/Tool... > > File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py (right): > > > > https://codereview.chromium.org/2608923002/diff/20001/third_party/WebKit/Tool... > > third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:30: # The script assumes it will be run every 10 minutes or so > > I broke up the run() function into 2 separate functions that are called based on whether an in-flight PR exists: `merge_in_flight_pull_request` and `export_first_exportable_commit`. > > This should be ready for review btw - this is the last of the broken-up CLs (although I have another one coming after this to fix the git commit author issue) Excellent, will review now
Initial comments/questions https://codereview.chromium.org/2608923002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py (right): https://codereview.chromium.org/2608923002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:75: pass What kind of circumstance would result in a ScriptError here? Would this always mean that the branch has already been removed or there's some messed-up state? Maybe it makes sense to abort with an error message? https://codereview.chromium.org/2608923002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:90: remote_branch_name = 'remotes/github/%s' % self.branch_name Where is the remote name "github" determined? https://codereview.chromium.org/2608923002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:96: # TODO(jeffcarp): this is broken Should be more specific: What's broken, how, and what's the next thing "to do" here? https://codereview.chromium.org/2608923002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:135: _log.info('Patch did not apply cleanly, skipping...') This could also be "warning" rather than "info". https://codereview.chromium.org/2608923002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py (right): https://codereview.chromium.org/2608923002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:30: # The script assumes it will be run every 10 minutes or so On 2017/01/03 at 20:42:31, jeffcarp wrote: > I broke up the run() function into 2 separate functions that are called based on whether an in-flight PR exists: `merge_in_flight_pull_request` and `export_first_exportable_commit`. Excellent, those names sound clearer than run(). The result of export_first_exportable_commit is that it will create an in-flight PR but not necessarily merge right? Also, my personal preference is to end all full-sentence comments with periods. https://codereview.chromium.org/2608923002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:40: def merge_in_flight_pull_request(self, pull_request): Optionally you could add a one-line docstring here. https://codereview.chromium.org/2608923002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:52: # TODO(jeffcarp): Make branch name more available What does "more available" mean? https://codereview.chromium.org/2608923002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:54: _log.info('Branch deleted!') The else clause could be removed if a return is added in the `if self.dry_run` clause. https://codereview.chromium.org/2608923002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:59: # If there was an issue merging, it should have errored out For consistency, this should be made into a docstring with a one-line summary and more detailed notes below. https://codereview.chromium.org/2608923002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github.py (right): https://codereview.chromium.org/2608923002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github.py:21: class WPTGitHub(object): The purpose of this class is contain everything related to integration with Github, right? Even if we're not expecting to send requests for pushes to other repos, do you think it would make sense to later just call this module github.py, and make the specific repo (w3c/web-platform-tests) a parameter that's passed in to methods on this class? If this sounds like it makes sense, you could add a TODO. https://codereview.chromium.org/2608923002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github.py:106: raise Exception('PR could not be merged: %d' % pull_request_number) Optional re-phrasing: if <condition>: return <value> else: <else-clause> can always be changed to: if <condition>: return <value> <else-clause> Also, I wonder if there's a more specific exception we could raise. You could even add an exception class to this file called MergeError or some such, which could also be used above in merge_pull_request.
https://codereview.chromium.org/2608923002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py (right): https://codereview.chromium.org/2608923002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:75: pass On 2017/01/06 at 19:20:24, qyearsley wrote: > What kind of circumstance would result in a ScriptError here? Would this always mean that the branch has already been removed or there's some messed-up state? Maybe it makes sense to abort with an error message? If the branch doesn't exist, it throws a ScriptError. Only deleting it if it exists might fix the problem, but I vaguely remember there was a reason I did it this way. https://codereview.chromium.org/2608923002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:90: remote_branch_name = 'remotes/github/%s' % self.branch_name On 2017/01/06 at 19:20:24, qyearsley wrote: > Where is the remote name "github" determined? Right now it's set manually when I add it to the local WPT repo. It should probably be automated. I'll make a bug. https://codereview.chromium.org/2608923002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py (right): https://codereview.chromium.org/2608923002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:30: # The script assumes it will be run every 10 minutes or so On 2017/01/06 at 19:20:24, qyearsley wrote: > On 2017/01/03 at 20:42:31, jeffcarp wrote: > > I broke up the run() function into 2 separate functions that are called based on whether an in-flight PR exists: `merge_in_flight_pull_request` and `export_first_exportable_commit`. > > Excellent, those names sound clearer than run(). The result of export_first_exportable_commit is that it will create an in-flight PR but not necessarily merge right? It will necessarily not merge in export_first_exportable_commit. Since it takes time for the PR to be verified on Travis CI, the script will only ever do one of (merge PR, create PR) one each invocation. > Also, my personal preference is to end all full-sentence comments with periods. Got it, I wish there was a lint rule for this https://codereview.chromium.org/2608923002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:52: # TODO(jeffcarp): Make branch name more available On 2017/01/06 at 19:20:24, qyearsley wrote: > What does "more available" mean? I honestly have no idea what I meant by that, haha. I'll remove this. https://codereview.chromium.org/2608923002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:54: _log.info('Branch deleted!') On 2017/01/06 at 19:20:24, qyearsley wrote: > The else clause could be removed if a return is added in the `if self.dry_run` clause. Great!
Addressed CL feedback. https://codereview.chromium.org/2608923002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py (right): https://codereview.chromium.org/2608923002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:96: # TODO(jeffcarp): this is broken On 2017/01/06 at 19:20:24, qyearsley wrote: > Should be more specific: What's broken, how, and what's the next thing "to do" here? Removing this functionality. Since we're deleting the remote branch now after merge, it should not exist.
lgtm CL description nit: The first line of the description should be the same as the CL title (at least for now, with Rietveld).
The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== This CL - Refactors TestExporter.run into 3 methods - Has a number of other fixes to get things working - Adds the `chromium-export` label to PRs after creation - Deletes remote branch after uploading (5/5) This CL succeeds https://codereview.chromium.org/2595143002 and is the last of the pipelined CLs broken out from https://codereview.chromium.org/2544173002. BUG=657117,676426 R=qyearsley@chromium.org ========== to ========== [WPT Export] Delete remote branch, add label, additional refactoring This CL - Refactors TestExporter.run into 3 methods - Has a number of other fixes to get things working - Adds the `chromium-export` label to PRs after creation - Deletes remote branch after uploading (5/5) This CL succeeds https://codereview.chromium.org/2595143002 and is the last of the pipelined CLs broken out from https://codereview.chromium.org/2544173002. BUG=657117,676426 R=qyearsley@chromium.org ==========
On 2017/01/08 at 23:24:18, qyearsley wrote: > lgtm > > CL description nit: The first line of the description should be the same as the CL title (at least for now, with Rietveld). Oops, thanks for the reminder!
The CQ bit was checked by jeffcarp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1483940394198810,
"parent_rev": "eef2fbb63ea6f4d96cbc2f01f922f9122a453a77", "commit_rev":
"64ea253d7b62b7a193f7ba9865e66b52174c98a8"}
Message was sent while issue was closed.
Description was changed from ========== [WPT Export] Delete remote branch, add label, additional refactoring This CL - Refactors TestExporter.run into 3 methods - Has a number of other fixes to get things working - Adds the `chromium-export` label to PRs after creation - Deletes remote branch after uploading (5/5) This CL succeeds https://codereview.chromium.org/2595143002 and is the last of the pipelined CLs broken out from https://codereview.chromium.org/2544173002. BUG=657117,676426 R=qyearsley@chromium.org ========== to ========== [WPT Export] Delete remote branch, add label, additional refactoring This CL - Refactors TestExporter.run into 3 methods - Has a number of other fixes to get things working - Adds the `chromium-export` label to PRs after creation - Deletes remote branch after uploading (5/5) This CL succeeds https://codereview.chromium.org/2595143002 and is the last of the pipelined CLs broken out from https://codereview.chromium.org/2544173002. BUG=657117,676426 R=qyearsley@chromium.org Review-Url: https://codereview.chromium.org/2608923002 Cr-Commit-Position: refs/heads/master@{#442198} Committed: https://chromium.googlesource.com/chromium/src/+/64ea253d7b62b7a193f7ba9865e6... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/64ea253d7b62b7a193f7ba9865e6... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
