Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add wpt frontend for commands #6421

Merged
merged 19 commits into from
Jul 26, 2017
Merged

Add wpt frontend for commands #6421

merged 19 commits into from
Jul 26, 2017

Conversation

jgraham
Copy link
Contributor

@jgraham jgraham commented Jun 28, 2017

This change is Reviewable

@w3c-bots
Copy link

w3c-bots commented Jun 28, 2017

View the complete job log.

Firefox (nightly)

@w3c-bots
Copy link

w3c-bots commented Jun 28, 2017

View the complete job log.

Sauce (safari)

@w3c-bots
Copy link

w3c-bots commented Jun 28, 2017

View the complete job log.

Chrome (unstable)

@w3c-bots
Copy link

w3c-bots commented Jun 28, 2017

View the complete job log.

Sauce (MicrosoftEdge)

@bobholt
Copy link
Contributor

bobholt commented Jun 29, 2017

This looks like it's still a work-in-progress with (since the build's not passing). Overall, I like how this simplifies the component modules. I left a couple of line comments for things that jumped out at me in this revision, but am pretty much 👍 as long as the builds work. I'd like to see a sample flakey test run with this before merging.


Reviewed 31 of 31 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


tools/browserutils/install.py, line 18 at r1 (raw file):

def run(venv, **kwargs):
    browser = kwargs["browser"]
    if venv and kwargs["destination"] is None:

This doesn't account for the not venv and kwargs["destination"] is None case (where destination wouldn't get assigned). If the existence of venv is checked in another script, the check isn't needed here.


tools/wpt/run.py, line 18 at r1 (raw file):

<<<<<<< HEAD

Unresolved merge conflict that may be causing some errors.


Comments from Reviewable

@jgraham
Copy link
Contributor Author

jgraham commented Jun 29, 2017

Yeah, this isn't done, just pushing it here for a little visibility.

@jgraham jgraham force-pushed the wpt_files_changed branch 2 times, most recently from e08abe7 to 99216ed Compare June 30, 2017 18:47
@gsnedders
Copy link
Member

How does this relate to #6237?

@jgraham jgraham force-pushed the wpt_files_changed branch 5 times, most recently from 2da5be0 to 73b8a53 Compare July 1, 2017 00:53
@jgraham
Copy link
Contributor Author

jgraham commented Jul 2, 2017

This is more stuff on top of that one.

@jgraham jgraham force-pushed the wpt_files_changed branch 6 times, most recently from 847c430 to 56827ed Compare July 4, 2017 10:57
@bobholt
Copy link
Contributor

bobholt commented Jul 25, 2017

👍 when the tests pass. @gsnedders do you have any interest in taking another pass at this?


Reviewed 1 of 35 files at r1, 2 of 29 files at r3, 3 of 3 files at r4, 3 of 3 files at r5.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


tools/wpt/install.py, line 18 at r1 (raw file):

Previously, bobholt (Bob Holt) wrote…

Ping

LGTM


Comments from Reviewable

@jgraham jgraham force-pushed the wpt_files_changed branch 2 times, most recently from 0e2a8fb to 8405c77 Compare July 25, 2017 16:58
@gsnedders
Copy link
Member

@bobholt As I said to @jgraham an hour ago, I don't think there's much worth is spending more time on reviewing this; my quick skim before didn't really find anything notable, and I doubt I'll find much more than what you have… though I'll inevitable find stuff in six months regardless of whether I review it now or not. :)

@gsnedders
Copy link
Member

Reviewed 15 of 35 files at r1, 4 of 12 files at r2, 22 of 29 files at r3, 3 of 3 files at r4, 1 of 3 files at r5, 10 of 10 files at r6.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


Comments from Reviewable

@jgraham jgraham merged commit c9458fb into master Jul 26, 2017
@plehegar plehegar deleted the wpt_files_changed branch September 26, 2017 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants