Add |wpt| command frontend #6237

jgraham · 2017-06-14T11:15:55Z

This change is

jgraham · 2017-06-14T11:16:24Z

Not quite done yet (missing docs), but should work in the current state.

bobholt · 2017-06-14T11:57:57Z

One apparent bug, and yes, docs are welcome.

Realizing this may be a bit of a naive question: Why this approach and not something like setup.py where wpt aliases run/install/etc. commands in setup.py?

Reviewed 11 of 11 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

tools/lint/lint.py, line 768 at r1 (raw file):

if __name__ == "__main__":
    args = create_parser.parse_args()

Looks like this should be create_parser().parse_args().

Comments from Reviewable

jgraham · 2017-06-14T12:06:14Z

AFAIK if you create a setup.py then the user has to create their own virtualenv and run python setup.py install &c. Making it possible to run everything from a single command without any setup apart from cloning the repo is very desirable (we could consider adding a command to install the virtualenv tool as well, for example). This approach is based heavily — in spirit if not im implementation — on Mozilla's mach command, which has the same advantage of not requiring any python-specific knowledge to e.g. start a build.

w3c-bots · 2017-06-14T12:15:17Z

View the complete job log.

Firefox (nightly)

Testing web-platform-tests at revision 6967685
Using browser at version BuildID 20170612100241; SourceStamp 27cad9749cddf68e11fdd4e5d73dad84a8f8cf23
Starting 10 test iterations
No tests run.

w3c-bots · 2017-06-14T12:15:17Z

View the complete job log.

Sauce (safari)

Testing web-platform-tests at revision 6967685
Using browser at version 10.0
Starting 10 test iterations
No tests run.

w3c-bots · 2017-06-14T12:15:18Z

View the complete job log.

Chrome (unstable)

Testing web-platform-tests at revision 6967685
Using browser at version 61.0.3141.7 dev
Starting 10 test iterations
No tests run.

w3c-bots · 2017-06-14T12:15:19Z

View the complete job log.

Sauce (MicrosoftEdge)

Testing web-platform-tests at revision 6967685
Using browser at version 14.14393
Starting 10 test iterations
No tests run.

jugglinmike · 2017-06-14T14:55:55Z

Happy to see further consolidation happening. This patch omits at least three files: /wpt, /tools/lint/commands.json, /tools/manifest/commands.json. I recommend naming those "commands" files wpt_commands.json to give a hint about their purpose in the otherwise-unrelated directories. I'll hold off on further review until I'm able to play with this locally.

jugglinmike · 2017-06-14T16:42:55Z

README.md

+
+* `wpt run` - For running tests in a browser
+* `wpt lint` - For running the lint against all tests
+* `wpt manifest` - For updating or generating a `MANIFEST.json` test manifest


Did you intentionally omit install-browser?

jugglinmike · 2017-06-14T16:43:35Z

tools/browserutils/install.py

+
+if __name__ == '__main__':
+ args = parser.parse_args()
+ run(None, **vars(args))


With the new wpt utility exposing the functionality from a more unified
interface, I can't think of a reason to maintain this module as an executable.
What do you say we remove this and update ci_unittest.sh to use wpt
instead?

I see that you've updated ci_unittest.sh, but this script is still executable. Do you have a use case in mind for executing this file directly?

jugglinmike · 2017-06-14T16:50:58Z

tools/wpt/wpt.py

+ parser.add_argument("--venv", action="store", help="Path to an existing virtualenv to use")
+ parser.add_argument("--debug", action="store_true", help="Run the debugger in case of an exception")
+ subparsers = parser.add_subparsers(dest="command")
+ for command, props in commands.iteritems():


Other parts of WPT's infrastructure are tested in Python 3; are you interested
in supporting that platform with the wpt utility? I'm not the best choice for
reviewing for Python 3 compatibility concerns, but I could try (e.g.
iteritems isn't available there). If not, I'll refrain from pointing out
incompatibilities.

Shrug. I guess I ought to make this work with Python 3, but wptrunner doesn't, for example, so it won't be super useful.

jugglinmike · 2017-06-14T17:01:04Z

tools/wpt/run.py

@@ -1,3 +1,4 @@
+# wpt command:run:main


I'm having trouble understanding the purpose of this comment. Is it a vestige of some previous implementation strategy? Or am I missing something about the current patch?

jugglinmike · 2017-06-14T17:02:02Z

tools/manifest/update.py


- update_from_cli(**vars(opts))
+print run


Errant print statement

jugglinmike · 2017-06-14T17:03:45Z

tools/browserutils/commands.json

@@ -0,0 +1,2 @@
+{"install-browser": {"path": "install.py", "script": "run", "parser": "get_parser", "help": "Install browser components",


wpt install-browser firefox browser looks a bit redundant, while wpt install-browser firefox webdriver is a little misleading. What do you think
about calling this sub-command install instead?

jugglinmike · 2017-06-14T17:10:30Z

tools/wpt/run.py

@@ -295,10 +310,7 @@ def main():
 venv.install_requirements(os.path.join(wpt_root, "tools", "wptrunner", "requirements.txt"))
 venv.install("requests")


Is this still necessary?

Not necessary, but it allows the file to be run directly from the command line, which seems somewhat useful to keep for now.

jugglinmike · 2017-06-14T17:12:36Z

tools/wpt/commands.json

@@ -0,0 +1,2 @@
+{"run": {"path": "run.py", "script": "run", "parser": "create_parser", "help": "Run tests in a browser",
+ "virtualenv": true, "install": ["requests"], "requirements": ["../wptrunner/requirements.txt"]}}


If the "requests" module is necessary to use the wptrunner module, why isn't it listed in wptrunner/requirements.txt?

It's necessary to install the browser/webdriver/prefs/etc. so it's a requirement for wpt run but not for wptrunner itself.

jgraham · 2017-06-23T18:59:31Z

This is ready and waiting for review.

jugglinmike

The README.md file contains a few lingering references to wptrun: wptrun --help, wptrun --wptrunner-help wptrun --binary=path product (I don't think this last one is supported by wpt, though).

The lint.whitelist file includes a reference to the wptrun.py file that this patch renames. The tests pass, so I guess this can be removed (rather than updated).

Would you mind removing the wptrun, lintandwhitelist` executables from the top level of the project?

jugglinmike · 2017-06-26T15:52:35Z

tools/browserutils/install.py

+
+if __name__ == '__main__':
+ args = parser.parse_args()
+ run(None, **vars(args))


I see that you've updated ci_unittest.sh, but this script is still executable. Do you have a use case in mind for executing this file directly?

jugglinmike · 2017-06-26T15:58:00Z

tools/wpt/run.py

@@ -7,11 +7,14 @@
 import tarfile
 from distutils.spawn import find_executable

+wpt_root = os.path.abspath(os.path.join(os.path.dirname(__file__), "..", ".."))


I recommend os.pardir for consistency with the change to tools/manifest/update.py.

jugglinmike · 2017-06-26T15:58:27Z

tools/wpt/wpt.py

+
+
+here = os.path.dirname(__file__)
+wpt_root = os.path.abspath(os.path.join(here, "..", ".."))


Same comment regarding os.pardir

jgraham · 2017-06-26T23:45:58Z

I want to leave the executables for now in case downstream consumers are making use of them in scripts or integrations. They can be removed in a followup once this has been announced.

jgraham · 2017-06-28T20:10:42Z

@jugglinmike updated this.

jugglinmike · 2017-06-28T22:03:02Z

Got it. This latest version is rebased on master, but I can't execute wptrun from master at the moment, so I'll need to work that out before I can continue the review here. See #6375 (comment)

jugglinmike · 2017-06-30T19:21:32Z

A couple of the documented options for the run sub-command do not work.

Specifying --binary produces the following output:

$ ./wptrun --binary /opt/mozilla/firefox-nightly/firefox firefox url/a-element.html 
usage: wptrun [-h] [--yes] [--wptrunner-help] product [tests [tests ...]] ...
wptrun: error: unrecognized arguments: --binary

Specifying --wptrunner-help produces the following output:

$ ./wpt run --wptrunner-help
Traceback (most recent call last):
  File "./wpt", line 5, in <module>
    wpt.main()
  File "/home/mike/projects/bocoup/google-wpt/web-platform-tests/tools/wpt/wpt.py", line 100, in main
    kwargs = vars(parser.parse_args(command_args))
  File "/usr/lib/python2.7/argparse.py", line 1701, in parse_args
    args, argv = self.parse_known_args(args, namespace)
  File "/usr/lib/python2.7/argparse.py", line 1733, in parse_known_args
    namespace, args = self._parse_known_args(args, namespace)
  File "/usr/lib/python2.7/argparse.py", line 1939, in _parse_known_args
    start_index = consume_optional(start_index)
  File "/usr/lib/python2.7/argparse.py", line 1879, in consume_optional
    take_action(action, args, option_string)
  File "/usr/lib/python2.7/argparse.py", line 1807, in take_action
    action(self, namespace, argument_values, option_string)
  File "/home/mike/projects/bocoup/google-wpt/web-platform-tests/tools/wpt/run.py", line 37, in __call__
    from wptrunner import wptcommandline
  File "/home/mike/projects/bocoup/google-wpt/web-platform-tests/tools/wptrunner/wptrunner/wptcommandline.py", line 9, in <module>
    import wpttest
  File "/home/mike/projects/bocoup/google-wpt/web-platform-tests/tools/wptrunner/wptrunner/wpttest.py", line 4, in <module>
    import mozinfo
ImportError: No module named mozinfo

...although I receive similar errors when using wptrun as defined in master today.

This is intended to provide a single point of entry for all wpt commands and make it easier to share infrastructure like virtual environments. Commands are defined via a commands.json file in the directory containing the command. This isi still pretty rough, but it basically works.

gsnedders · 2017-06-30T23:35:50Z

There's a lot of new untested code here, especially given @jugglinmike found issues earlier. :/

jugglinmike · 2017-07-03T15:56:37Z

The command wptrun firefox url/a-element.html now executes as expected, but
the equivalent using the wpt executable still produces that error on my
system:

$ ./wpt run firefox url/a-element.html 
Traceback (most recent call last):
  File "./wpt", line 5, in <module>
    wpt.main()
  File "/home/mike/projects/bocoup/google-wpt/web-platform-tests/tools/wpt/wpt.py", line 98, in main
    script, parser = import_command(command, props)
  File "/home/mike/projects/bocoup/google-wpt/web-platform-tests/tools/wpt/wpt.py", line 70, in import_command
    parser = getattr(mod, props["parser"])()
  File "/home/mike/projects/bocoup/google-wpt/web-platform-tests/tools/wpt/run.py", line 45, in create_parser
    from wptrunner import wptcommandline
  File "/home/mike/projects/bocoup/google-wpt/web-platform-tests/tools/wptrunner/wptrunner/wptcommandline.py", line 9, in <module>
    import wpttest
  File "/home/mike/projects/bocoup/google-wpt/web-platform-tests/tools/wptrunner/wptrunner/wpttest.py", line 4, in <module>
    import mozinfo
ImportError: No module named mozinfo

jgraham · 2017-07-15T17:12:30Z

Closing this in favour of #6421 that ended up fixing some issues here.

jgraham requested a review from jugglinmike June 14, 2017 11:15

wpt-pr-bot added the infra label Jun 14, 2017

wpt-pr-bot requested review from bobholt and gsnedders June 14, 2017 11:16

jgraham added the do not merge yet label Jun 14, 2017

jgraham force-pushed the wpt_frontend branch from 98d95d3 to f55f875 Compare June 14, 2017 15:09

wpt-pr-bot added the wpt label Jun 14, 2017

jugglinmike reviewed Jun 14, 2017

View reviewed changes

jgraham force-pushed the wpt_frontend branch 4 times, most recently from b78831b to c2c13cd Compare June 19, 2017 17:25

wpt-pr-bot added the manifest label Jun 19, 2017

jgraham force-pushed the wpt_frontend branch 3 times, most recently from 40ef52c to 7aa63c8 Compare June 20, 2017 13:27

jgraham removed the do not merge yet label Jun 20, 2017

jgraham force-pushed the wpt_frontend branch 2 times, most recently from 5ded5b9 to 00a0fcd Compare June 23, 2017 12:12

jugglinmike reviewed Jun 26, 2017

View reviewed changes

jgraham force-pushed the wpt_frontend branch from 00a0fcd to c3f95a5 Compare June 28, 2017 20:08

jgraham force-pushed the wpt_frontend branch from c3f95a5 to 43fd506 Compare June 30, 2017 17:33

jgraham added 5 commits June 30, 2017 15:04

Convert |wptrun| command to |wpt run|

98be1e9

Add |wpt install| command for installing browser components

6419bbc

Add lint and manifest commands to |wpt|

7d3e3e9

Add a serve command to the wpt frontend

b83f581

jgraham force-pushed the wpt_frontend branch from 43fd506 to b83f581 Compare June 30, 2017 22:04

gsnedders mentioned this pull request Jun 30, 2017

Add wpt frontend for commands #6421

Merged

jgraham mentioned this pull request Jul 15, 2017

Only lint files that have changed in ci_lint.sh #6136

Closed

jgraham closed this Jul 15, 2017

jugglinmike mentioned this pull request May 18, 2018

Stop posting PR results to pulls.web-platform-tests.org #11060

Merged

sideshowbarker deleted the wpt_frontend branch November 22, 2018 22:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add |wpt| command frontend #6237

Add |wpt| command frontend #6237

jgraham commented Jun 14, 2017 •

edited by wpt-reviewable-bot

Loading

jgraham commented Jun 14, 2017

bobholt commented Jun 14, 2017

jgraham commented Jun 14, 2017

w3c-bots commented Jun 14, 2017 •

edited

Loading

w3c-bots commented Jun 14, 2017 •

edited

Loading

w3c-bots commented Jun 14, 2017 •

edited

Loading

w3c-bots commented Jun 14, 2017 •

edited

Loading

jugglinmike commented Jun 14, 2017

jugglinmike Jun 14, 2017

jugglinmike Jun 14, 2017

jugglinmike Jun 26, 2017

jugglinmike Jun 14, 2017

jgraham Jun 19, 2017

jugglinmike Jun 14, 2017

jugglinmike Jun 14, 2017

jugglinmike Jun 14, 2017

jugglinmike Jun 14, 2017

jgraham Jun 19, 2017

jugglinmike Jun 14, 2017

jgraham Jun 19, 2017

jgraham commented Jun 23, 2017

jugglinmike left a comment

jugglinmike Jun 26, 2017

jugglinmike Jun 26, 2017

jugglinmike Jun 26, 2017

jgraham commented Jun 26, 2017

jgraham commented Jun 28, 2017

jugglinmike commented Jun 28, 2017

jugglinmike commented Jun 30, 2017

gsnedders commented Jun 30, 2017

jugglinmike commented Jul 3, 2017

jgraham commented Jul 15, 2017 •

edited

Loading

		@@ -0,0 +1,2 @@
		{"install-browser": {"path": "install.py", "script": "run", "parser": "get_parser", "help": "Install browser components",

		@@ -295,10 +310,7 @@ def main():
		venv.install_requirements(os.path.join(wpt_root, "tools", "wptrunner", "requirements.txt"))
		venv.install("requests")

		@@ -0,0 +1,2 @@
		{"run": {"path": "run.py", "script": "run", "parser": "create_parser", "help": "Run tests in a browser",
		"virtualenv": true, "install": ["requests"], "requirements": ["../wptrunner/requirements.txt"]}}



		here = os.path.dirname(__file__)
		wpt_root = os.path.abspath(os.path.join(here, "..", ".."))

Add |wpt| command frontend #6237

Add |wpt| command frontend #6237

Conversation

jgraham commented Jun 14, 2017 • edited by wpt-reviewable-bot Loading

jgraham commented Jun 14, 2017

bobholt commented Jun 14, 2017

jgraham commented Jun 14, 2017

w3c-bots commented Jun 14, 2017 • edited Loading

Firefox (nightly)

w3c-bots commented Jun 14, 2017 • edited Loading

Sauce (safari)

w3c-bots commented Jun 14, 2017 • edited Loading

Chrome (unstable)

w3c-bots commented Jun 14, 2017 • edited Loading

Sauce (MicrosoftEdge)

jugglinmike commented Jun 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgraham commented Jun 23, 2017

jugglinmike left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgraham commented Jun 26, 2017

jgraham commented Jun 28, 2017

jugglinmike commented Jun 28, 2017

jugglinmike commented Jun 30, 2017

gsnedders commented Jun 30, 2017

jugglinmike commented Jul 3, 2017

jgraham commented Jul 15, 2017 • edited Loading

jgraham commented Jun 14, 2017 •

edited by wpt-reviewable-bot

Loading

w3c-bots commented Jun 14, 2017 •

edited

Loading

w3c-bots commented Jun 14, 2017 •

edited

Loading

w3c-bots commented Jun 14, 2017 •

edited

Loading

w3c-bots commented Jun 14, 2017 •

edited

Loading

jgraham commented Jul 15, 2017 •

edited

Loading