[WPT Export] Add "github" remote repo in LocalWPT constructor if it doesn't exist.

[WPT Export] Add "github" remote repo in LocalWPT constructor if it doesn't exist.

BUG=679084
R=qyearsley@chromium.org

Review-Url: https://codereview.chromium.org/2620103002
Cr-Commit-Position: refs/heads/master@{#442729}
Committed: https://chromium.googlesource.com/chromium/src/+/9422f34b57066a40cea5b40c1e022adeb4712859

[qyearsley, 2017-01-10 20:42:47]

https://codereview.chromium.org/2620103002/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py (right):

https://codereview.chromium.org/2620103002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:28: use_github:
Optional, passing true will check if the GitHub remote is enabled
Is this argument now unused?

https://codereview.chromium.org/2620103002/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt_unittest.py
(right):

https://codereview.chromium.org/2620103002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt_unittest.py:15: def
test_fetches_if_wpt_exists(self):
Thought about naming: the purpose of the first three test methods could be
clarified if the name noted that it was testing the constructor behavior, e.g.:

  test_constructor_fetches_if_wpt_dir_exists
  test_constructor_clones_if_wpt_dir_does_not_exist
  test_constructor_no_fetch_flag

https://codereview.chromium.org/2620103002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt_unittest.py:25:
['fetch', 'checkout', 'remote', 'remote'])
Possible rephrasing:

  subcommands = [argv[1] for argv in host.executive.calls]
  self.assertEqual(subcommands, ['fetch', 'checkout', 'remote', 'remote'])

Or alternatively you could also change it to assert the complete contents of
host.executive.calls, if this doesn't make it to fragile to changes.

https://codereview.chromium.org/2620103002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt_unittest.py:88:
self.assertEqual(len(host.executive.calls), 12)
Note, asserting the length is redundant if you assert the whole value of the
list below.

[jeffcarp, 2017-01-10 22:11:58]

Addressed CL feedback

https://codereview.chromium.org/2620103002/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt_unittest.py
(right):

https://codereview.chromium.org/2620103002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt_unittest.py:25:
['fetch', 'checkout', 'remote', 'remote'])
On 2017/01/10 at 20:42:46, qyearsley wrote:
> Possible rephrasing:
> 
>   subcommands = [argv[1] for argv in host.executive.calls]
>   self.assertEqual(subcommands, ['fetch', 'checkout', 'remote', 'remote'])
> 
> Or alternatively you could also change it to assert the complete contents of
host.executive.calls, if this doesn't make it to fragile to changes.

Sure (to your last point). We do it elsewhere so I'll make this consistent. The
change in fragility is not that high imo.

https://codereview.chromium.org/2620103002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt_unittest.py:88:
self.assertEqual(len(host.executive.calls), 12)
On 2017/01/10 at 20:42:46, qyearsley wrote:
> Note, asserting the length is redundant if you assert the whole value of the
list below.

Good catch

[qyearsley, 2017-01-10 22:23:05]

lgtm

Final nit: The CL title/description could be changed to something like:
Add "github" remote repo in LocalWPT constructor if it doesn't exist.

Also, if you think it's helpful, in a small follow-up CL (or this CL) you could
add a global constant for the remote repo name "github", and use that instead of
the literal string.

[jeffcarp, 2017-01-10 22:29:00]

Description was changed from

==========
[WPT Export] Add github remote if not exists

BUG=679084
R=qyearsley@chromium.org
==========

to

==========
[WPT Export] Add "github" remote repo in LocalWPT constructor if it doesn't
exist.

BUG=679084
R=qyearsley@chromium.org
==========

[jeffcarp, 2017-01-10 22:29:21]

On 2017/01/10 at 22:23:05, qyearsley wrote:
> lgtm
> 
> Final nit: The CL title/description could be changed to something like:
> Add "github" remote repo in LocalWPT constructor if it doesn't exist.
> 
> Also, if you think it's helpful, in a small follow-up CL (or this CL) you
could add a global constant for the remote repo name "github", and use that
instead of the literal string.

Done

[jeffcarp, 2017-01-10 22:29:22]

The CQ bit was checked by jeffcarp@chromium.org

[jeffcarp, 2017-01-10 22:29:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
qyearsley@chromium.org
Link to the patchset: https://codereview.chromium.org/2620103002/#ps40001
(title: "Turn 'github' literal into a global constant")

[commit-bot: I haz the power, 2017-01-10 22:30:18]

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, 2017-01-10 23:38:23]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484087362711140,
"parent_rev": "17aab3f6517fd0ba74d8e5f7d9ec35440401d9e1", "commit_rev":
"9422f34b57066a40cea5b40c1e022adeb4712859"}

[commit-bot: I haz the power, 2017-01-10 23:38:54]

Description was changed from

==========
[WPT Export] Add "github" remote repo in LocalWPT constructor if it doesn't
exist.

BUG=679084
R=qyearsley@chromium.org
==========

to

==========
[WPT Export] Add "github" remote repo in LocalWPT constructor if it doesn't
exist.

BUG=679084
R=qyearsley@chromium.org

Review-Url: https://codereview.chromium.org/2620103002
Cr-Commit-Position: refs/heads/master@{#442729}
Committed:
https://chromium.googlesource.com/chromium/src/+/9422f34b57066a40cea5b40c1e02...
==========

[commit-bot: I haz the power, 2017-01-10 23:38:55]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/9422f34b57066a40cea5b40c1e02...