1214783 - Create a mach command to create new web-platform-tests

Status: RESOLVED FIXED

(Testing :: web-platform-tests, defect)

Description

[James Graham [:jgraham]]

The command creates a template test, opens it in the editor and in the local build, and updates the test list (MANIFEST.json) to include the new test. This is designed to remove some of the boring or poorly understood steps required to create a new web-platform-test.

Comment 1

[James Graham [:jgraham]]

Attachment: wpt-create.diff

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8673836 [details] [diff] [review]
wpt-create.diff

Review of attachment 8673836 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/mach_commands.py
@@ +117,5 @@
>              logger.info(item.id)
>  
> +class WebPlatformTestsCreator(MozbuildObject):
> +    template_prefix = """<!doctype html>
> +<meta name=charset content=utf-8>

Did you mean `<meta charset=utf-8>`?

@@ +124,5 @@
> +
> +    template_body = """<title></title>
> +<script src=/resources/testharness.js></script>
> +<script src=/resources/testharnessreport.js></script>
> +<script>

I'd add a log div above this.

@@ +139,5 @@
> +        if not (rel_path.startswith("testing/web-platform/tests") or
> +                rel_path.startswith("testing/web-platform/mozilla/tests")):
> +            print("""Test path is not in wpt directories:
> +testing/web-platform/tests for tests that may be shared
> +testing.web-platform/mozilla/tests for Gecko only tests""")

`.` should be `/`

@@ +147,5 @@
> +            print("Test path already exists, pass --overwrite to replace")
> +            return 1
> +
> +        with open(path, "w") as f:
> +            template = self.template_prefix

Doesn't really matter, but I probably would have computed the template before the open()

@@ +155,5 @@
> +            f.write(template)
> +
> +        if kwargs["no_editor"]:
> +            editor = None
> +        if kwargs["editor"]:

Should `-e` override `--no-editor`?

@@ +159,5 @@
> +        if kwargs["editor"]:
> +            editor = kwargs["editor"]
> +        if "VISUAL" in os.environ:
> +            editor = os.environ["VISUAL"]
> +        elif "EDITOR" in os.environ:

Either way, I'm pretty confident this shouldn't override `--no-editor`.

@@ +165,5 @@
> +        else:
> +            editor = None
> +
> +        if editor:
> +            proc = subprocess.Popen("%s %s" % (editor, path), shell=True)

`shell=True` is somewhat unfortunate.

@@ +170,5 @@
> +
> +        if not kwargs["no_run"]:
> +            p = create_parser_wpt()
> +            wpt_kwargs = vars(p.parse_args(["--manifest-update", path]))
> +            res = context.commands.dispatch("web-platform-tests", context, **wpt_kwargs)

`res` is lost?

Comment 3

[James Graham [:jgraham]]

Attachment: wpt-create.diff

Comment 4

[James Graham [:jgraham]]

(In reply to :Ms2ger from comment #2)
> Comment on attachment 8673836 [details] [diff] [review]
> wpt-create.diff
> 
> Review of attachment 8673836 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/web-platform/mach_commands.py
> @@ +117,5 @@
> >              logger.info(item.id)
> >  
> > +class WebPlatformTestsCreator(MozbuildObject):
> > +    template_prefix = """<!doctype html>
> > +<meta name=charset content=utf-8>
> 
> Did you mean `<meta charset=utf-8>`?

What, it's not 1995 anymore?

> @@ +124,5 @@
> > +
> > +    template_body = """<title></title>
> > +<script src=/resources/testharness.js></script>
> > +<script src=/resources/testharnessreport.js></script>
> > +<script>
> 
> I'd add a log div above this.

This shouldn't be needed to get output these days.

> @@ +155,5 @@
> > +            f.write(template)
> > +
> > +        if kwargs["no_editor"]:
> > +            editor = None
> > +        if kwargs["editor"]:
> 
> Should `-e` override `--no-editor`?

I don't think so… But honestly if you use the two together then you can't really expect sensible behaviour.
 
> @@ +165,5 @@
> > +        else:
> > +            editor = None
> > +
> > +        if editor:
> > +            proc = subprocess.Popen("%s %s" % (editor, path), shell=True)
> 
> `shell=True` is somewhat unfortunate.

Yeah, but if you have e.g. EDITOR set to 'emacsclient -t' or similar then you can't just use Popen([EDITOR, path]). And I imagine that environment variables can contain quoting so that splitting on space doesn't work. Suggestions welcome.

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8674187 [details] [diff] [review]
wpt-create.diff

Review of attachment 8674187 [details] [diff] [review]:
-----------------------------------------------------------------

Assuming you tested this and it works, lgtm.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e940ae9e0f71

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e940ae9e0f71