Refactor WPT Export to ensure only one PR in flight at a time

Refactor WPT Export to ensure only one PR in flight at a time

This CL:
- Moves out all export logic into its own class, TestExporter
- sync_wpt.py is still a shell that sets up all the classes and calls TestExporter
- Changes the 'exportable' logic to honor NOEXPORT=true
- Renames GitHub to WPTGitHub
- Adds a bunch more unit tests

This CL has not yet been tested on a real export. I hope to do a real export soon and improve/fix the code in the process.

BUG=666809
BUG=666884
Committed: https://crrev.com/06bc68e15d0b41c94ddc85a6b5713da14561823f
Cr-Commit-Position: refs/heads/master@{#435071}

[qyearsley, 2016-11-22 22:08:18]

qyearsley@chromium.org changed reviewers:
+ qyearsley@chromium.org

[qyearsley, 2016-11-22 22:08:20]

Quick drive-by initial review :-)

https://codereview.chromium.org/2518313003/diff/20001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/github.py (right):

https://codereview.chromium.org/2518313003/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/github.py:65: API_BASE =
'https://api.github.com'
Since this is a local variable, convention would have it still be lower case
even though it's meant to be constant.

https://codereview.chromium.org/2518313003/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/github.py:66: assert
path.startswith('/')
Note, I think that this is another good use of assert:
 * It's expressing a precondition in a readable way
 * It's checking input passed in from other code here, not validating user input

https://codereview.chromium.org/2518313003/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/github.py:74: json_body =
json.JSONEncoder().encode(body)
Is there a difference between json.JSONEncoder().encode(x) and json.dumps(x)?

https://codereview.chromium.org/2518313003/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/github.py:81: def
merge_pull_request(self, number):
What type of number? Would it make sense to call this pull_request_number?

https://codereview.chromium.org/2518313003/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/github.py:82: assert number and
number is IntType
Another way to express this would be `assert type(number) is int`; no need to
import types.IntType.

Also, it's pretty common practice to not explicitly assert types, and just rely
on a TypeError being raised below
if something goes wrong. I don't have strong opinions about this though.

https://codereview.chromium.org/2518313003/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/github.py:83: url =
'/repos/w3c/web-platform-tests/pulls/%s/merge' % number
%s could be changed to %d; also, this variable could be called path instead of
url.

https://codereview.chromium.org/2518313003/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/github.py:93: class
MockWPTGitHub(object):
In other places, I've seen fakes like this put into their own modules (e.g.
buildbot_mock, filesystem_mock, host_mock)

https://codereview.chromium.org/2518313003/diff/20001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py
(right):

https://codereview.chromium.org/2518313003/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py:14: def
test_stops_if_more_than_one_PR_is_in_flight(self):
The linter might complain about capitals in the name here.

https://codereview.chromium.org/2518313003/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py:17:
wpt_github = MockWPTGitHub(pull_requests=mock_pull_requests)
You could also pass the literal list of mock pull requests directly, e.g.

        wpt_github = MockWPTGitHub(pull_requests=[{'id': 1}, {'id': 2}])

https://codereview.chromium.org/2518313003/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py:19:
self.assertRaises(Exception, TestExporter(host, wpt_github))
An alternate way of expressing this that I've seen is:

with self.assertRaises(Exception):
    TestExporter(host, wpt_github)

[jeffcarp, 2016-11-22 22:30:15]

Thanks for the quick comments

https://codereview.chromium.org/2518313003/diff/20001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/github.py (right):

https://codereview.chromium.org/2518313003/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/github.py:74: json_body =
json.JSONEncoder().encode(body)
On 2016/11/22 at 22:08:19, qyearsley wrote:
> Is there a difference between json.JSONEncoder().encode(x) and json.dumps(x)?

No idea, in practice it doesn't look like it. I was just mirroring the previous
pattern.

https://codereview.chromium.org/2518313003/diff/20001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py
(right):

https://codereview.chromium.org/2518313003/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py:19:
self.assertRaises(Exception, TestExporter(host, wpt_github))
On 2016/11/22 at 22:08:20, qyearsley wrote:
> An alternate way of expressing this that I've seen is:
> 
> with self.assertRaises(Exception):
>     TestExporter(host, wpt_github)

Hell yes that looks much nicer

[jeffcarp, 2016-11-23 00:26:42]

Description was changed from

==========
Refactor WPT Export to ensure only one PR in flight at a time

BUG=666809
==========

to

==========
Refactor WPT Export to ensure only one PR in flight at a time

BUG=666809
BUG=666884
==========

[jeffcarp, 2016-11-23 00:27:28]

Description was changed from

==========
Refactor WPT Export to ensure only one PR in flight at a time

BUG=666809
BUG=666884
==========

to

==========
Refactor WPT Export to ensure only one PR in flight at a time

Additionally:
- Changes the 'exportable' logic to honor NOEXPORT=true


BUG=666809
BUG=666884
==========

[jeffcarp, 2016-11-23 00:54:32]

Description was changed from

==========
Refactor WPT Export to ensure only one PR in flight at a time

Additionally:
- Changes the 'exportable' logic to honor NOEXPORT=true


BUG=666809
BUG=666884
==========

to

==========
Refactor WPT Export to ensure only one PR in flight at a time

This CL has not yet been tested on a real export. I hope to do a real export
soon and improve/fix the code in the process.

Additionally:
- Changes the 'exportable' logic to honor NOEXPORT=true


BUG=666809
BUG=666884
==========

[jeffcarp, 2016-11-23 00:55:21]

Description was changed from

==========
Refactor WPT Export to ensure only one PR in flight at a time

This CL has not yet been tested on a real export. I hope to do a real export
soon and improve/fix the code in the process.

Additionally:
- Changes the 'exportable' logic to honor NOEXPORT=true


BUG=666809
BUG=666884
==========

to

==========
Refactor WPT Export to ensure only one PR in flight at a time

This CL:
- Moves out all export logic into its own class, TestExporter
- sync_wpt.py is still a shell that sets up all the classes and calls
TestExporter
- Changes the 'exportable' logic to honor NOEXPORT=true

This CL has not yet been tested on a real export. I hope to do a real export
soon and improve/fix the code in the process.


BUG=666809
BUG=666884
==========

[jeffcarp, 2016-11-23 00:55:39]

Description was changed from

==========
Refactor WPT Export to ensure only one PR in flight at a time

This CL:
- Moves out all export logic into its own class, TestExporter
- sync_wpt.py is still a shell that sets up all the classes and calls
TestExporter
- Changes the 'exportable' logic to honor NOEXPORT=true

This CL has not yet been tested on a real export. I hope to do a real export
soon and improve/fix the code in the process.


BUG=666809
BUG=666884
==========

to

==========
Refactor WPT Export to ensure only one PR in flight at a time

This CL:
- Moves out all export logic into its own class, TestExporter
- sync_wpt.py is still a shell that sets up all the classes and calls
TestExporter
- Changes the 'exportable' logic to honor NOEXPORT=true
- Renames GitHub to WPTGitHub

This CL has not yet been tested on a real export. I hope to do a real export
soon and improve/fix the code in the process.


BUG=666809
BUG=666884
==========

[jeffcarp, 2016-11-23 00:56:26]

Description was changed from

==========
Refactor WPT Export to ensure only one PR in flight at a time

This CL:
- Moves out all export logic into its own class, TestExporter
- sync_wpt.py is still a shell that sets up all the classes and calls
TestExporter
- Changes the 'exportable' logic to honor NOEXPORT=true
- Renames GitHub to WPTGitHub

This CL has not yet been tested on a real export. I hope to do a real export
soon and improve/fix the code in the process.


BUG=666809
BUG=666884
==========

to

==========
Refactor WPT Export to ensure only one PR in flight at a time

This CL:
- Moves out all export logic into its own class, TestExporter
- sync_wpt.py is still a shell that sets up all the classes and calls
TestExporter
- Changes the 'exportable' logic to honor NOEXPORT=true
- Renames GitHub to WPTGitHub
- Adds a bunch more unit tests

This CL has not yet been tested on a real export. I hope to do a real export
soon and improve/fix the code in the process.


BUG=666809
BUG=666884
==========

[jeffcarp, 2016-11-23 21:39:55]

jeffcarp@chromium.org changed reviewers:
+ foolip@chromium.org

[foolip, 2016-11-29 11:12:21]

lgtm % nits

https://codereview.chromium.org/2518313003/diff/100001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_commit.py (right):

https://codereview.chromium.org/2518313003/diff/100001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_commit.py:66: #
TODO(jeffcarp): do not include expectations files
Also exclude manifest changes.

https://codereview.chromium.org/2518313003/diff/100001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py (right):

https://codereview.chromium.org/2518313003/diff/100001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py:41: and not
message.startswith('Import ')
Is the 'Import ' check still needed?

https://codereview.chromium.org/2518313003/diff/100001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt_unittest.py
(right):

https://codereview.chromium.org/2518313003/diff/100001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt_unittest.py:33:
'Commit message\nNOEXPORT=true',  # show (message)
Also test a case with '> NOEXPORT=true' at the beginning of the line if the
intention is that a reverted commit should also be excluded.

https://codereview.chromium.org/2518313003/diff/100001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py (right):

https://codereview.chromium.org/2518313003/diff/100001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:31: # Because the
current bot doesn't use the GitHub status system
You mean the stability checker I assume? There are still other things that run
using the status checks, at least it takes a while for the merge button to go
green. Make this a TODO :)

https://codereview.chromium.org/2518313003/diff/100001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:42: raise
Exception('More than two in-flight PRs!')
Maybe link to them so that a human can close them?

https://codereview.chromium.org/2518313003/diff/100001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:50: #
TODO(jeffcarp): fetch Chromium origin/master as well
I'm thinking this could be done by the system that runs this script? If the
script updates the repo it's in, it would also have to restart itself, since it
could have changed in origin/master.

https://codereview.chromium.org/2518313003/diff/100001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:73:
_log.info('Found %d exportable commits in chromium:', len(exportable_commits))
s/chromium/Chromium/, or the other way around above.

https://codereview.chromium.org/2518313003/diff/100001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:77:
outbound_commit = ChromiumCommit(self.host, sha=exportable_commits[-1])
--reverse inside exportable_commits_since would make this a bit nicer I think,
otherwise some comment is needed to explain that the last exportable commit is
actually the first, chronologically.

https://codereview.chromium.org/2518313003/diff/100001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github.py (right):

https://codereview.chromium.org/2518313003/diff/100001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github.py:12: EXPORT_LABEL =
'Automated Chromium Export'
Is it possible to add a label not in
https://github.com/w3c/web-platform-tests/labels?

I've added a chromium-export label, that fits in with the other names and I
thought it'd also work when it's a human doing it manually.

https://codereview.chromium.org/2518313003/diff/100001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github_mock.py (right):

https://codereview.chromium.org/2518313003/diff/100001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github_mock.py:19: """Creates
a PR on GitHub.
Copying the documentation here doesn't seem useful.

[jeffcarp, 2016-11-29 19:00:48]

https://codereview.chromium.org/2518313003/diff/100001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py (right):

https://codereview.chromium.org/2518313003/diff/100001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py:41: and not
message.startswith('Import ')
On 2016/11/29 at 11:12:20, foolip wrote:
> Is the 'Import ' check still needed?

At the moment it's needed to ignore Import commits from before NOEXPORT=true was
merged. We can take this line out once we're past that point.

https://codereview.chromium.org/2518313003/diff/100001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt_unittest.py
(right):

https://codereview.chromium.org/2518313003/diff/100001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt_unittest.py:33:
'Commit message\nNOEXPORT=true',  # show (message)
On 2016/11/29 at 11:12:20, foolip wrote:
> Also test a case with '> NOEXPORT=true' at the beginning of the line if the
intention is that a reverted commit should also be excluded.

Good catch thanks!

https://codereview.chromium.org/2518313003/diff/100001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/sync_wpt.py (right):

https://codereview.chromium.org/2518313003/diff/100001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/sync_wpt.py:31: # TODO: TE unit
test for options.dry_run
This is done I will remove it.

https://codereview.chromium.org/2518313003/diff/100001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py (right):

https://codereview.chromium.org/2518313003/diff/100001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:42: raise
Exception('More than two in-flight PRs!')
On 2016/11/29 at 11:12:21, foolip wrote:
> Maybe link to them so that a human can close them?

👍

https://codereview.chromium.org/2518313003/diff/100001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:50: #
TODO(jeffcarp): fetch Chromium origin/master as well
On 2016/11/29 at 11:12:21, foolip wrote:
> I'm thinking this could be done by the system that runs this script? If the
script updates the repo it's in, it would also have to restart itself, since it
could have changed in origin/master.

Good point I'll change the TODO.

[jeffcarp, 2016-11-29 19:17:25]

The CQ bit was checked by jeffcarp@chromium.org

[jeffcarp, 2016-11-29 19:17:26]

The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2518313003/#ps120001
(title: "Address CL feedback")

[commit-bot: I haz the power, 2016-11-29 19:17:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 20:34:04]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1480447045916210,
"parent_rev": "5a425e59e0ca33f34a80491070c6b02dd2e1a221", "commit_rev":
"5f1868aac384e3c48829502ba04bd3d033475d78"}

[commit-bot: I haz the power, 2016-11-29 20:34:32]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-11-29 20:40:27]

Description was changed from

==========
Refactor WPT Export to ensure only one PR in flight at a time

This CL:
- Moves out all export logic into its own class, TestExporter
- sync_wpt.py is still a shell that sets up all the classes and calls
TestExporter
- Changes the 'exportable' logic to honor NOEXPORT=true
- Renames GitHub to WPTGitHub
- Adds a bunch more unit tests

This CL has not yet been tested on a real export. I hope to do a real export
soon and improve/fix the code in the process.


BUG=666809
BUG=666884
==========

to

==========
Refactor WPT Export to ensure only one PR in flight at a time

This CL:
- Moves out all export logic into its own class, TestExporter
- sync_wpt.py is still a shell that sets up all the classes and calls
TestExporter
- Changes the 'exportable' logic to honor NOEXPORT=true
- Renames GitHub to WPTGitHub
- Adds a bunch more unit tests

This CL has not yet been tested on a real export. I hope to do a real export
soon and improve/fix the code in the process.

BUG=666809
BUG=666884

Committed: https://crrev.com/06bc68e15d0b41c94ddc85a6b5713da14561823f
Cr-Commit-Position: refs/heads/master@{#435071}
==========

[commit-bot: I haz the power, 2016-11-29 20:40:28]

Patchset 7 (id:??) landed as
https://crrev.com/06bc68e15d0b41c94ddc85a6b5713da14561823f
Cr-Commit-Position: refs/heads/master@{#435071}

[jeffcarp, 2016-11-30 03:21:32]

https://codereview.chromium.org/2518313003/diff/100001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github.py (right):

https://codereview.chromium.org/2518313003/diff/100001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github.py:12: EXPORT_LABEL =
'Automated Chromium Export'
On 2016/11/29 at 11:12:21, foolip wrote:
> Is it possible to add a label not in
https://github.com/w3c/web-platform-tests/labels?
> 
> I've added a chromium-export label, that fits in with the other names and I
thought it'd also work when it's a human doing it manually.

Great, changed the label to `chromium-export`.

[jeffcarp, 2016-11-30 03:21:32]

https://codereview.chromium.org/2518313003/diff/100001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github.py (right):

https://codereview.chromium.org/2518313003/diff/100001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github.py:12: EXPORT_LABEL =
'Automated Chromium Export'
On 2016/11/29 at 11:12:21, foolip wrote:
> Is it possible to add a label not in
https://github.com/w3c/web-platform-tests/labels?
> 
> I've added a chromium-export label, that fits in with the other names and I
thought it'd also work when it's a human doing it manually.

Great, changed the label to `chromium-export`.