Merge ChromiumWPT functionality into TestExporter, use ChromiumCommits

Merge ChromiumWPT functionality into TestExporter, use ChromiumCommits

(1/5)

BUG=657117
Committed: https://crrev.com/a2dc5b4f3fbf401f6e2fbf85811a0f91f84985a9
Cr-Commit-Position: refs/heads/master@{#439293}

[jeffcarp, 2016-12-16 00:28:55]

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

[jeffcarp, 2016-12-16 00:28:55]

This is the 1st of 5 CLs from https://codereview.chromium.org/2544173002/

[qyearsley, 2016-12-16 00:45:17]

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

https://codereview.chromium.org/2579193002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:11:
CHROMIUM_WPT_DIR = 'third_party/WebKit/LayoutTests/imported/wpt/'
Nit: Optional: Could add a blank line after `import logging` and after `_log =
...`.

https://codereview.chromium.org/2579193002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:107: toplevel =
self.host.executive.run_command([
toplevel is the repo root, right? Is this necessary here? Is there a better
name?

https://codereview.chromium.org/2579193002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:126: )
Doesn't make much of a difference here, but this could also be made a
class-level (non-nested) method _is_exportable. Optionally it could be a static
method.

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

https://codereview.chromium.org/2579193002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py:19:
return ''
Possible minor change:

    def run_fn(args):
        sub_command = args[1]
        return vals.get(sub_command, '')

https://codereview.chromium.org/2579193002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py:102:
self.assertIsInstance(commits[0], ChromiumCommit)
It would be helpful to assert what calls are made by the executive here, in what
order (self.host.executive.calls). Same for the test methods below.

[jeffcarp, 2016-12-16 00:59:49]

Description was changed from

==========
Merge ChromiumWPT functionality into TestExporter, use ChromiumCommits

BUG=657117
==========

to

==========
Merge ChromiumWPT functionality into TestExporter, use ChromiumCommits

(1/5)

BUG=657117
==========

[jeffcarp, 2016-12-16 20:07:10]

Thanks for the quick review, addressed your feedback.

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

https://codereview.chromium.org/2579193002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:107: toplevel =
self.host.executive.run_command([
On 2016/12/16 at 00:45:17, qyearsley wrote:
> toplevel is the repo root, right? Is this necessary here? Is there a better
name?

Yep, I can rename it to repo_root. Is there a cleaner way to achieve this?

https://codereview.chromium.org/2579193002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:126: )
On 2016/12/16 at 00:45:17, qyearsley wrote:
> Doesn't make much of a difference here, but this could also be made a
class-level (non-nested) method _is_exportable. Optionally it could be a static
method.

It seemed cleaner to have it in here because it's only used within this method,
but now that you bring it up a static is_exportable method might be useful to
other modules too.

This method will change a bit in later CLs, can we revisit this after those?

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

https://codereview.chromium.org/2579193002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py:19:
return ''
On 2016/12/16 at 00:45:17, qyearsley wrote:
> Possible minor change:
> 
>     def run_fn(args):
>         sub_command = args[1]
>         return vals.get(sub_command, '')

🙌🙌

https://codereview.chromium.org/2579193002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py:102:
self.assertIsInstance(commits[0], ChromiumCommit)
On 2016/12/16 at 00:45:17, qyearsley wrote:
> It would be helpful to assert what calls are made by the executive here, in
what order (self.host.executive.calls). Same for the test methods below.

Added. I think it makes the unit tests more brittle but I'm ok keeping them in
there and if they get annoying I'll advocate for their removal.

[qyearsley, 2016-12-17 00:26:03]

LGTM

On 2016/12/16 at 20:07:10, jeffcarp wrote:
> Thanks for the quick review, addressed your feedback.
> 
>
https://codereview.chromium.org/2579193002/diff/1/third_party/WebKit/Tools/Sc...
> File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py (right):
> 
>
https://codereview.chromium.org/2579193002/diff/1/third_party/WebKit/Tools/Sc...
> third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:107: toplevel =
self.host.executive.run_command([
> On 2016/12/16 at 00:45:17, qyearsley wrote:
> > toplevel is the repo root, right? Is this necessary here? Is there a better
name?
> 
> Yep, I can rename it to repo_root. Is there a cleaner way to achieve this?
> 

repo_root sounds good to me. I think that this way is good. In other places,
webkitpy.common.webkit_finder.WebKitFinder is used to get the chromium repo
root, but that seems strange.

One possible change to consider later would be to move many of these calls to
git to the webkitpy.common.checkout.scm.git module.

>
https://codereview.chromium.org/2579193002/diff/1/third_party/WebKit/Tools/Sc...
> third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:126: )
> On 2016/12/16 at 00:45:17, qyearsley wrote:
> > Doesn't make much of a difference here, but this could also be made a
class-level (non-nested) method _is_exportable. Optionally it could be a static
method.
> 
> It seemed cleaner to have it in here because it's only used within this
method, but now that you bring it up a static is_exportable method might be
useful to other modules too.
> 
> This method will change a bit in later CLs, can we revisit this after those?
> 
>
https://codereview.chromium.org/2579193002/diff/1/third_party/WebKit/Tools/Sc...
> File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py
(right):
> 
>
https://codereview.chromium.org/2579193002/diff/1/third_party/WebKit/Tools/Sc...
> third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py:19:
return ''
> On 2016/12/16 at 00:45:17, qyearsley wrote:
> > Possible minor change:
> > 
> >     def run_fn(args):
> >         sub_command = args[1]
> >         return vals.get(sub_command, '')
> 
> 🙌🙌
> 
>
https://codereview.chromium.org/2579193002/diff/1/third_party/WebKit/Tools/Sc...
> third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py:102:
self.assertIsInstance(commits[0], ChromiumCommit)
> On 2016/12/16 at 00:45:17, qyearsley wrote:
> > It would be helpful to assert what calls are made by the executive here, in
what order (self.host.executive.calls). Same for the test methods below.
> 
> Added. I think it makes the unit tests more brittle but I'm ok keeping them in
there and if they get annoying I'll advocate for their removal.

Yeah, that's true -- this means that any change in calls requires updating
tests. That reminds me of the recipe tests, which are usually a sort of "change
detector test" (they have to be updated after every change, even most changes
which don't affect behavior). Although, sometimes tests like this are still
helpful as long as you read them and check that it makes sense.

[jeffcarp, 2016-12-17 00:42:42]

The CQ bit was checked by jeffcarp@chromium.org

[commit-bot: I haz the power, 2016-12-17 00:43: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, 2016-12-17 02:31:14]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481935362932010,
"parent_rev": "056f38c15acecdc68285936cbbc7ac1263b47001", "commit_rev":
"e00d89da3fcef5bf55fea19a191f3b8f1486b5b3"}

[commit-bot: I haz the power, 2016-12-17 02:31:40]

Description was changed from

==========
Merge ChromiumWPT functionality into TestExporter, use ChromiumCommits

(1/5)

BUG=657117
==========

to

==========
Merge ChromiumWPT functionality into TestExporter, use ChromiumCommits

(1/5)

BUG=657117

Review-Url: https://codereview.chromium.org/2579193002
==========

[commit-bot: I haz the power, 2016-12-17 02:31:41]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-17 02:34:30]

Description was changed from

==========
Merge ChromiumWPT functionality into TestExporter, use ChromiumCommits

(1/5)

BUG=657117

Review-Url: https://codereview.chromium.org/2579193002
==========

to

==========
Merge ChromiumWPT functionality into TestExporter, use ChromiumCommits

(1/5)

BUG=657117

Committed: https://crrev.com/a2dc5b4f3fbf401f6e2fbf85811a0f91f84985a9
Cr-Commit-Position: refs/heads/master@{#439293}
==========

[commit-bot: I haz the power, 2016-12-17 02:34:31]

Patchset 2 (id:??) landed as
https://crrev.com/a2dc5b4f3fbf401f6e2fbf85811a0f91f84985a9
Cr-Commit-Position: refs/heads/master@{#439293}