Redo autogenerated metadata #6792

annevk · 2017-08-09T14:03:16Z

This change is

w3c-bots · 2017-08-09T14:24:16Z

Build BROKEN

Started: 2017-08-10 13:07:29
Finished: 2017-08-10 13:22:07

View more information about this build on:

jgraham · 2017-08-09T14:55:05Z

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

tools/manifest/sourcefile.py, line 204 at r1 (raw file):

    @property
    def name_is_multi_global(self):

FWIW I would leave .worker., .window and .any. tests working for now so you don't have to replace all of them at once.

tools/manifest/sourcefile.py, line 207 at r1 (raw file):

        """Check if the file name matches the conditions for the file to
        be a multi-global js test file"""
        return "global" in self.meta_flags and self.ext == ".js"

.global. seems like an odd flag because the technical meaning contradicts the standard meaning here (it makes it sound like the test is somehow global rather than like it runs in multiple globals.

tools/manifest/sourcefile.py, line 487 at r1 (raw file):

        elif self.name_is_global:
            rv = TestharnessTest.item_type, []
            valueTypeSuffixes = []

Variables should be formatted with underscores i.e. value_type_suffix.

tools/manifest/sourcefile.py, line 491 at r1 (raw file):

                if key == b"global":
                    valueTypes = value.split(b",")
                    for valueType in valueTypes:

So it seems like this would be clearer by building up a set e.g.

variants = {}
value_types = sorted(item.strip() for item in value.split(","), key=lambda x:(x[0] == "!", x))
expansions = {"workers": {"worker", "sharedworker", "serviceworker"}}
for item in value_types:
    values = expansions.get(item, {item})
    if item.startswith("!"):
        variants -= values
    else:
        variants |= values

for variant in variants:
    rv[1].append(TestharnessTest(self, replace_end(self.url, ".global.js", ".global.%s.html" % variant), timeout=self.timeout))

I would also put the first bit into its own method. This does mean that window tests aren't just .html, but you can special case that in the substitution if you like.

tools/manifest/sourcefile.py, line 508 at r1 (raw file):

            if valueTypeSuffixes == []:
                raise ValueError("You need to use `// META: global=...` in your resource")

This type of check should be part of the lint.

tools/serve/serve.py, line 142 at r1 (raw file):

class WindowHandler(HtmlWrapperHandler):
    path_replace = [(".window.html", ".window.js")]

Again, I would leave these ones working for now.

tools/serve/serve.py, line 142 at r1 (raw file):

    def _meta_replacement(self, key, value):
        if key == b"global":

I'm not really sure that this isn't just adding unnecessary complexity. In particular you end up duplicating a lot of logic here, so if you really want this I would prefer some way to share the logic with `sourcefile.py.

Comments from Reviewable

jgraham · 2017-08-09T14:55:59Z

You also need to update the lint to allow this metadata. At the moment it will fail because this isn't a meta type it knows about.

annevk · 2017-08-10T12:57:52Z

FWIW I would leave .worker., .window and .any. tests working for now so you don't have to replace all of them at once.

I think it would be better to do that as part of this. Having said that, it would be trivial to keep window.js working so I'll add that back in. Would save from adding some metadata. worker.js would clash with WorkerJavaScriptHandler.

.global. seems like an odd flag because the technical meaning contradicts the standard meaning here (it makes it sound like the test is somehow global rather than like it runs in multiple globals.

Okay, in that case I suggest we stick with .any. Shall I name the META variable any too?

So it seems like this would be clearer by building up a set

With the updates I pushed, do you still think so? (I don't think we should support standalone sharedworker for now btw. There's no demonstrated need. And !window and !worker also don't make sense.)

I'm not really sure that this isn't just adding unnecessary complexity. In particular you end up duplicating a lot of logic here, so if you really want this I would prefer some way to share the logic with `sourcefile.py.

How is this duplicating logic? This seems like the only place that controls whether something that gets loaded actually ends up making sense.

jgraham · 2017-08-11T13:45:06Z

Reviewed 1 of 2 files at r2, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

Comments from Reviewable

jgraham · 2017-08-11T15:04:09Z

Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

tools/manifest/sourcefile.py, line 496 at r3 (raw file):

            for (key, value) in self.script_metadata:
                if key == b"global":
                    global_values = value.split(b",")

Forgot to mention that you should .strip() each item here to allow whitespace in the values. You'll need to update the lint too.

Comments from Reviewable

annevk · 2017-08-11T15:05:11Z

Forgot to mention that you should .strip() each item here to allow whitespace in the values.

Why would we want to allow whitespace?

jgraham · 2017-08-22T14:49:09Z

I pushed some proposed changes to https://github.com/w3c/web-platform-tests/tree/meta_global

annevk · 2017-08-23T11:58:56Z

I think the main problem is that it requires all tests to be over HTTPS if you want to include a service worker test. As long as HTTPS requires more work to get done I don't think that's a good tradeoff.

jgraham · 2017-08-23T12:27:06Z

It doesn't require that.

w3c-bots · 2017-08-23T12:47:01Z

Build ERRORED

Started: 2018-01-25 13:07:55
Finished: 2018-01-25 13:24:38

Failing Jobs

lint
tools_unittest in py27
tools_unittest in pypy

View more information about this build on:

Ms2ger · 2017-10-06T11:11:07Z

What's the status here?

annevk · 2017-10-06T12:23:01Z

I want to pick this up again at some point, but I haven't made time recently. If someone else would like to take over to make this happen, be my guest. Whoever picks this up should probably take @jgraham's branch (or the commits there) as a starting point.

foolip · 2018-01-25T12:49:06Z

I want to pick this up again at some point, but I haven't made time recently. If someone else would like to take over to make this happen, be my guest. Whoever picks this up should probably take @jgraham's branch (or the commits there) as a starting point.

Looks like the commit from is actually included in this PR, so I guess just continuing here would work?

jgraham · 2018-01-25T13:03:59Z

Yes

foolip · 2018-01-25T13:09:08Z

I resolved the conflicts and made sure that wpt lint and wpt run don't just crash, although wpt lint did complain about things that probably need fixing to land this.

@annevk @jgraham is this in a state where careful review is "all" that's needed to get it landed, or was something tricky discovered above that needs fixing first?

foolip · 2018-01-25T13:32:24Z

OK, failing lints:

css/geometry/DOMMatrix-css-string.worker.js: Support file not in support directory (SUPPORT-WRONG-DIR)
css/geometry/WebKitCSSMatrix.worker.js: Support file not in support directory (SUPPORT-WRONG-DIR)
css/geometry/interfaces.worker.js: Support file not in support directory (SUPPORT-WRONG-DIR)

Seems like a regression in what is considered a support file.

annevk · 2018-01-25T13:32:39Z

@foolip OP has a list of things that need to be done before this can be merged.

Extracted from #6792.

Ms2ger · 2018-04-18T13:01:34Z

@jgraham @gsnedders I think this is ready for another round of reviews

Ms2ger · 2018-04-18T15:29:24Z

tools/manifest/sourcefile.py

@@ -49,6 +49,86 @@ def read_script_metadata(f, regexp):
        yield (m.groups()[0], m.groups()[1])


+_any_variants = {
+    b"window": {"default": True, "suffix": ".any.html"},


Thought: maybe we should instead have b"default": {"longhand": {b"window", b"dedicatedworker"}}, then you could use

// META: global=!default,blah

jgraham · 2018-04-20T16:18:08Z

This looks great, thanks for picking it up. I think this is at least somewhat documented, we should update the documentation before landing. Otherwise my only real feedback is that I prefer the design where default can be used explicitly.

Reviewed 5 of 5 files at r7.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

tools/lint/lint.py, line 653 at r7 (raw file):

                        else:
                            included_variants = set.union(get_default_any_variants(),

I might move this out of the loop.

tools/manifest/sourcefile.py, line 53 at r7 (raw file):

Previously, Ms2ger wrote…

Thought: maybe we should instead have b"default": {"longhand": {b"window", b"dedicatedworker"}}, then you could use
// META: global=!default,blah

I like that suggestion better.

Comments from Reviewable

Ms2ger · 2018-04-23T07:03:22Z

tools/manifest/sourcefile.py

+    globals = get_default_any_variants()
+    global_values = set(item.strip() for item in value.split(b","))
+
+    for item in global_values:


Note to self: check if the iteration order here is significant

Ms2ger · 2018-04-23T07:05:29Z

tools/manifest/sourcefile.py

+    assert isinstance(value, binary_type), value
+
+    globals = get_default_any_variants()
+    global_values = set(item.strip() for item in value.split(b","))


There doesn't seem to be a reason to create the set. (Could also do the strip() call inside the loop).

jgraham · 2018-04-24T10:21:01Z

Reviewed 6 of 6 files at r8.
Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

Comments from Reviewable

…r scopes. This commit is based on ideas and code by Anne van Kesteren and James Graham. The infrastructure is extended to allow tests to opt-in to being run in shared workers and service workers, while the default remains as window and dedicated worker scopes. (This default may be changed in the future.)

…dler.__call__()., a=testonly Automatic update from web-platform-testsDon't wrap HTTPExceptions in FunctionHandler.__call__(). Extracted from web-platform-tests/wpt#6792. -- wpt-commits: 4bf2fa8d17f3165df7b6582754bf00ef277e1180 wpt-pr: 10505

…dler.__call__()., a=testonly Automatic update from web-platform-testsDon't wrap HTTPExceptions in FunctionHandler.__call__(). Extracted from web-platform-tests/wpt#6792. -- wpt-commits: 4bf2fa8d17f3165df7b6582754bf00ef277e1180 wpt-pr: 10505 UltraBlame original commit: 244cbe67350998c363eb78a75fb34fe57d6be32a

wpt-pr-bot added the infra label Aug 9, 2017

wpt-pr-bot requested review from gsnedders and jgraham August 9, 2017 14:03

This was referenced Aug 9, 2017

WIP for .any replacement #6756

Closed

Make .any.js tests work for service/shared workers too #4210

Closed

gsnedders removed their request for review August 11, 2017 14:17

wpt-pr-bot requested a review from gsnedders August 11, 2017 15:04

gsnedders self-assigned this Apr 10, 2018

Ms2ger force-pushed the annevk/meta-global branch from 4b97ac7 to 2aa0ef3 Compare April 17, 2018 12:43

Ms2ger pushed a commit that referenced this pull request Apr 17, 2018

Don't wrap HTTPExceptions in FunctionHandler.__call__().

89e5314

Extracted from #6792.

Ms2ger mentioned this pull request Apr 17, 2018

Don't wrap HTTPExceptions in FunctionHandler.__call__(). #10505

Merged

Ms2ger pushed a commit that referenced this pull request Apr 17, 2018

Don't wrap HTTPExceptions in FunctionHandler.__call__().

4bf2fa8

Extracted from #6792.

Ms2ger force-pushed the annevk/meta-global branch from deef861 to 9e591d8 Compare April 17, 2018 13:35

Ms2ger assigned Ms2ger and unassigned gsnedders Apr 17, 2018

Ms2ger reviewed Apr 18, 2018

View reviewed changes

Ms2ger reviewed Apr 23, 2018

View reviewed changes

wpt-pr-bot added the docs label Apr 24, 2018

wpt-pr-bot requested review from dontcallmedom and sideshowbarker April 24, 2018 09:41

jgraham approved these changes Apr 24, 2018

View reviewed changes

Ms2ger force-pushed the annevk/meta-global branch from ecffddb to c964d1c Compare April 24, 2018 10:59

Ms2ger merged commit 75133c2 into master Apr 24, 2018

Ms2ger deleted the annevk/meta-global branch April 24, 2018 11:17

Ms2ger mentioned this pull request Apr 24, 2018

Support multiple META lines for cases where we accept multiple values #10603

Open

foolip mentioned this pull request Jun 19, 2019

Allow standalone serviceworker tests #17339

Open

jgraham mentioned this pull request Aug 6, 2019

Rewrite self.html as an any.js test. #18228

Merged

stephenmcgruer mentioned this pull request Apr 20, 2020

*.any.js global defaulting is confusing #23111

Closed

annevk mentioned this pull request Apr 21, 2020

RFC 52: Remove default and negated global support in *.any.js web-platform-tests/rfcs#52

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redo autogenerated metadata #6792

Redo autogenerated metadata #6792

annevk commented Aug 9, 2017 •

edited by wpt-reviewable-bot

Loading

w3c-bots commented Aug 9, 2017 •

edited

Loading

jgraham commented Aug 9, 2017

jgraham commented Aug 9, 2017

annevk commented Aug 10, 2017

jgraham commented Aug 11, 2017

jgraham commented Aug 11, 2017

annevk commented Aug 11, 2017

jgraham commented Aug 22, 2017

annevk commented Aug 23, 2017

jgraham commented Aug 23, 2017

w3c-bots commented Aug 23, 2017 •

edited

Loading

Ms2ger commented Oct 6, 2017

annevk commented Oct 6, 2017

foolip commented Jan 25, 2018

jgraham commented Jan 25, 2018

foolip commented Jan 25, 2018

foolip commented Jan 25, 2018

annevk commented Jan 25, 2018

Ms2ger commented Apr 18, 2018

Ms2ger Apr 18, 2018

jgraham commented Apr 20, 2018

Ms2ger Apr 23, 2018

Ms2ger Apr 23, 2018

jgraham commented Apr 24, 2018

Redo autogenerated metadata #6792

Redo autogenerated metadata #6792

Conversation

annevk commented Aug 9, 2017 • edited by wpt-reviewable-bot Loading

w3c-bots commented Aug 9, 2017 • edited Loading

Build BROKEN

jgraham commented Aug 9, 2017

jgraham commented Aug 9, 2017

annevk commented Aug 10, 2017

jgraham commented Aug 11, 2017

jgraham commented Aug 11, 2017

annevk commented Aug 11, 2017

jgraham commented Aug 22, 2017

annevk commented Aug 23, 2017

jgraham commented Aug 23, 2017

w3c-bots commented Aug 23, 2017 • edited Loading

Build ERRORED

Failing Jobs

Ms2ger commented Oct 6, 2017

annevk commented Oct 6, 2017

foolip commented Jan 25, 2018

jgraham commented Jan 25, 2018

foolip commented Jan 25, 2018

foolip commented Jan 25, 2018

annevk commented Jan 25, 2018

Ms2ger commented Apr 18, 2018

Ms2ger Apr 18, 2018

Choose a reason for hiding this comment

jgraham commented Apr 20, 2018

Ms2ger Apr 23, 2018

Choose a reason for hiding this comment

Ms2ger Apr 23, 2018

Choose a reason for hiding this comment

jgraham commented Apr 24, 2018

annevk commented Aug 9, 2017 •

edited by wpt-reviewable-bot

Loading

w3c-bots commented Aug 9, 2017 •

edited

Loading

w3c-bots commented Aug 23, 2017 •

edited

Loading