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

import-maps tests are using window.internals object #32697

Closed
allstarschh opened this issue Feb 4, 2022 · 5 comments · Fixed by #35373
Closed

import-maps tests are using window.internals object #32697

allstarschh opened this issue Feb 4, 2022 · 5 comments · Fixed by #35373

Comments

@allstarschh
Copy link
Contributor

For the test files in import-maps, it is using browser-specific objects, like window.internals

Also, it uses (seems Chromium only) useInternalMethods, which also looks like browser-specific.

@annevk
Copy link
Member

annevk commented Feb 7, 2022

@domenic are there other ways to write these tests that do not require window.internals support?

@domenic
Copy link
Member

domenic commented Feb 7, 2022

/cc @hiroshige-g

So the current state is basically that these aren't "real" web platform tests but we thought it was slightly more useful to have broken tests in the public, shared repository than to hide them away in the Chromium source tree.

We tried to fix this but got stuck because the WPT process for testdriver stuff was very confusing. (An RFC is needed? Things work differently in iframes vs. top-level? WebDriver vs. testdriver vs. WebDriver BiDi? There are JSON endpoints involved that need to be defined as request/response pairs? Etc.) This is a bad excuse and we are sorry.

The other route we were hoping for was to introduce import.meta.resolve() as a web-exposed API, which we believe to be an independently-useful primitive which would replace the internals dependency. Honestly landing that might be easier than figuring out the WPT/testdriver process and technical architecture.

I am somewhat hopeful that TestUtils would be a reasonable place to introduce such an API since it's much easier to understand how to spec TestUtils things than WebDriver/testdriver things. I don't really know what the criteria is for deciding what goes in TestUtils vs. what goes in WebDriver. /cc @jgraham.

Finally, it is possible many of these tests could be converted to use existing technologies like service workers or server-side request detection instead of using a specific module-resolution function. However we suspect that would fail in some cases (e.g. non-HTTP(S) URLs) or be worse in certain ways (e.g. relying on timeouts for certain types of failures). So we didn't pursue that very hard.

We'd welcome guidance on the best way to fix this situation, either in terms of the ideal path forward (which might take a while) or in terms of short-term fixes that make it easier for other implementers to work with the tests.

@jgraham
Copy link
Contributor

jgraham commented Feb 8, 2022

In terms of WebDriver vs TestDriver, the question is "is this the kind of thing that's going to be useful to normal developers when testing webapps" (or, for a Chromium-specific take on that: is this the kind of thing you'd expose via CDP). If the answer is "yes" then we should figure out how to make it part of a developer-facing test API (i.e. WebDriver[-BiDi]), if not then we could add it to TestUtils.

Of course adding stuff for use by general web developers outside of the browser is a bit of a higher bar, and WebDriver has some technical limitations that cause the top-level vs frames confusion (BiDi will largely fix these problems, once it's widely available). In any case if we think this is something that is useful to developers, I can help you navigate the WebDriver/testdriver space (to give a bit of an introduction: WebDriver requires defining a HTTP endpoint that returns the data as JSON. testdriver is an abstraction layer that allows non-WebDriver implementations of the commands so that e.g. Chromium can use internals methods in content-shell, as long as they implement the same semantics as specified in WebDriver. Generally all user-facing API additions in web-platform-tests require an RFC, although this would be a formality in the case that something is specified in WebDriver).

@hiroshige-g
Copy link
Contributor

The tests depending on the internal APIs are

so most of the resolution tests (which I think most important) are not affected.

Perhaps we might want to first (re)start the discussions for (non-test) APIs, including import.meta.resolve() (whatwg/html#3871 / whatwg/html#5572), to see whether they should be exposed as non-test APIs or should be test-only APIs. import.meta.resolve() might be worth adding as a user-facing API, while I'm less sure about the parsing API to get parsed import maps.
Previously, we didn't pursue the option to expose them as non-test APIs (we postponed the decision as future work), because we had much less data/needs at that time (e.g. we haven't shipped the Chromium implementation and thus few real world experiences and feedbacks about import maps ergonomics, and we didn't have non-test blockers depending on such APIs).
Now we might be able to have more inputs from both users and browser vendors.

Another possible way to alleviate the issue might be to convert the parsing tests into service-worker-based resolution tests, because parsed import maps themselves are anyway not directly observable. (Direct tests on parsed import maps are probably easier to write anyway though)

@domenic
Copy link
Member

domenic commented Feb 9, 2022

Thanks both of you for weighing in! I had forgotten you did the heroic work of making most of the tests use service workers, @hiroshige-g; that makes me feel a bit better about the state of the test suite at least.

I suspect that resolution will be useful to developers, not just in testing web apps, but also in general. So I think finishing the spec for import.meta.resolve() is the best way to do that. Would Mozilla be interested in that? If so dropping a comment in whatwg/html#5572 would be helpful and then we could move forward.

For the parsing tests, I think @hiroshige-g's last paragraph has the right idea:

Another possible way to alleviate the issue might be to convert the parsing tests into service-worker-based resolution tests, because parsed import maps themselves are anyway not directly observable. (Direct tests on parsed import maps are probably easier to write anyway though)

We have not heard anyone wanting to get the parsed import map. And even for web developers writing their own automated tests, I don't think doing that is very useful. The actually-interesting thing is whether your import map works like you expect (which you can test directly). Whether it parses like you expect is not that interesting.

I think we can convert these tests into resolution tests without too much effort. And some of them are just not testing anything observably interesting, e.g. parsing-schema-normalization.json.

We could leave the parsing test data files around in a subdirectory, with an explicit warning that they are not maintained or used directly by any web platform tests. They might still be useful to browsers writing unit tests, or to web developer tools.

chromium-wpt-export-bot pushed a commit that referenced this issue Aug 8, 2022
* Use import.meta.resolve() and <base> trickery, instead of a service worker + sometimes Chromium internals.

* Remove the parsing tests that used Chromium internals. We should convert these into resolution tests eventually, but for now they just make wpt.fyi look sad and are confusing for other engines. We leave the JSON files here for now in anticipation of that upcoming conversion.

* Use <meta name="variant"> instead of a loop over the JSON files. This avoids the need for <meta timeout="long"> and possibly allows some parallelism.

* Renamed data-base-url.json to be data-url-prefix.json and update the test description to reflect the intent. This confusion originates from the original JS to JSON conversion at WICG/import-maps@6cb173d.

Fixes #32697.

Change-Id: I4943f4249eaec89f718c7a7fef271dbc61ec3f77
chromium-wpt-export-bot pushed a commit that referenced this issue Aug 8, 2022
* Use import.meta.resolve() and <base> trickery, instead of a service worker + sometimes Chromium internals.

* Remove the parsing tests that used Chromium internals. We should convert these into resolution tests eventually, but for now they just make wpt.fyi look sad and are confusing for other engines. We leave the JSON files here for now in anticipation of that upcoming conversion.

* Use <meta name="variant"> instead of a loop over the JSON files. This avoids the need for <meta timeout="long"> and possibly allows some parallelism.

* Renamed data-base-url.json to be data-url-prefix.json and update the test description to reflect the intent. This confusion originates from the original JS to JSON conversion at WICG/import-maps@6cb173d.

Fixes #32697.

Change-Id: I4943f4249eaec89f718c7a7fef271dbc61ec3f77
chromium-wpt-export-bot pushed a commit that referenced this issue Aug 9, 2022
* Use import.meta.resolve() and <base> trickery, instead of a service worker + sometimes Chromium internals.

* Remove the parsing tests that used Chromium internals. We should convert these into resolution tests eventually, but for now they just make wpt.fyi look sad and are confusing for other engines. We leave the JSON files here for now in anticipation of that upcoming conversion.

  These tests are partially restored, in wpt_internal, in https://chromium-review.googlesource.com/c/chromium/src/+/3815111/.

* Use <meta name="variant"> instead of a loop over the JSON files. This avoids the need for <meta timeout="long"> and possibly allows some parallelism.

* Renamed data-base-url.json to be data-url-prefix.json and update the test description to reflect the intent. This confusion originates from the original JS to JSON conversion at WICG/import-maps@6cb173d.

Fixes #32697.

Change-Id: I4943f4249eaec89f718c7a7fef271dbc61ec3f77
aarongable pushed a commit to chromium/chromium that referenced this issue Aug 9, 2022
* Use import.meta.resolve() and <base> trickery, instead of a service worker + sometimes Chromium internals.

* Remove the parsing tests that used Chromium internals. We should convert these into resolution tests eventually, but for now they just make wpt.fyi look sad and are confusing for other engines. We leave the JSON files here for now in anticipation of that upcoming conversion.

  These tests are partially restored, in wpt_internal, in https://chromium-review.googlesource.com/c/chromium/src/+/3815111/.

* Use <meta name="variant"> instead of a loop over the JSON files. This avoids the need for <meta timeout="long"> and possibly allows some parallelism.

* Renamed data-base-url.json to be data-url-prefix.json and update the test description to reflect the intent. This confusion originates from the original JS to JSON conversion at WICG/import-maps@6cb173d.

Fixes web-platform-tests/wpt#32697.

Change-Id: I4943f4249eaec89f718c7a7fef271dbc61ec3f77
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3816001
Reviewed-by: Hiroshige Hayashizaki <[email protected]>
Commit-Queue: Domenic Denicola <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1032864}
chromium-wpt-export-bot pushed a commit that referenced this issue Aug 9, 2022
* Use import.meta.resolve() and <base> trickery, instead of a service worker + sometimes Chromium internals.

* Remove the parsing tests that used Chromium internals. We should convert these into resolution tests eventually, but for now they just make wpt.fyi look sad and are confusing for other engines. We leave the JSON files here for now in anticipation of that upcoming conversion.

  These tests are partially restored, in wpt_internal, in https://chromium-review.googlesource.com/c/chromium/src/+/3815111/.

* Use <meta name="variant"> instead of a loop over the JSON files. This avoids the need for <meta timeout="long"> and possibly allows some parallelism.

* Renamed data-base-url.json to be data-url-prefix.json and update the test description to reflect the intent. This confusion originates from the original JS to JSON conversion at WICG/import-maps@6cb173d.

Fixes #32697.

Change-Id: I4943f4249eaec89f718c7a7fef271dbc61ec3f77
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3816001
Reviewed-by: Hiroshige Hayashizaki <[email protected]>
Commit-Queue: Domenic Denicola <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1032864}
chromium-wpt-export-bot pushed a commit that referenced this issue Aug 9, 2022
* Use import.meta.resolve() and <base> trickery, instead of a service worker + sometimes Chromium internals.

* Remove the parsing tests that used Chromium internals. We should convert these into resolution tests eventually, but for now they just make wpt.fyi look sad and are confusing for other engines. We leave the JSON files here for now in anticipation of that upcoming conversion.

  These tests are partially restored, in wpt_internal, in https://chromium-review.googlesource.com/c/chromium/src/+/3815111/.

* Use <meta name="variant"> instead of a loop over the JSON files. This avoids the need for <meta timeout="long"> and possibly allows some parallelism.

* Renamed data-base-url.json to be data-url-prefix.json and update the test description to reflect the intent. This confusion originates from the original JS to JSON conversion at WICG/import-maps@6cb173d.

Fixes #32697.

Change-Id: I4943f4249eaec89f718c7a7fef271dbc61ec3f77
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3816001
Reviewed-by: Hiroshige Hayashizaki <[email protected]>
Commit-Queue: Domenic Denicola <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1032864}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Sep 5, 2022
…, a=testonly

Automatic update from web-platform-tests
Import maps: simplify web platform tests

* Use import.meta.resolve() and <base> trickery, instead of a service worker + sometimes Chromium internals.

* Remove the parsing tests that used Chromium internals. We should convert these into resolution tests eventually, but for now they just make wpt.fyi look sad and are confusing for other engines. We leave the JSON files here for now in anticipation of that upcoming conversion.

  These tests are partially restored, in wpt_internal, in https://chromium-review.googlesource.com/c/chromium/src/+/3815111/.

* Use <meta name="variant"> instead of a loop over the JSON files. This avoids the need for <meta timeout="long"> and possibly allows some parallelism.

* Renamed data-base-url.json to be data-url-prefix.json and update the test description to reflect the intent. This confusion originates from the original JS to JSON conversion at WICG/import-maps@6cb173d.

Fixes web-platform-tests/wpt#32697.

Change-Id: I4943f4249eaec89f718c7a7fef271dbc61ec3f77
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3816001
Reviewed-by: Hiroshige Hayashizaki <[email protected]>
Commit-Queue: Domenic Denicola <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1032864}

--

wpt-commits: e4148cadb24fbc954dafa0a0fcfe0e342968e508
wpt-pr: 35373
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
* Use import.meta.resolve() and <base> trickery, instead of a service worker + sometimes Chromium internals.

* Remove the parsing tests that used Chromium internals. We should convert these into resolution tests eventually, but for now they just make wpt.fyi look sad and are confusing for other engines. We leave the JSON files here for now in anticipation of that upcoming conversion.

  These tests are partially restored, in wpt_internal, in https://chromium-review.googlesource.com/c/chromium/src/+/3815111/.

* Use <meta name="variant"> instead of a loop over the JSON files. This avoids the need for <meta timeout="long"> and possibly allows some parallelism.

* Renamed data-base-url.json to be data-url-prefix.json and update the test description to reflect the intent. This confusion originates from the original JS to JSON conversion at WICG/import-maps@6cb173d.

Fixes web-platform-tests/wpt#32697.

Change-Id: I4943f4249eaec89f718c7a7fef271dbc61ec3f77
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3816001
Reviewed-by: Hiroshige Hayashizaki <[email protected]>
Commit-Queue: Domenic Denicola <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1032864}
NOKEYCHECK=True
GitOrigin-RevId: 9f37e44f3924a3f6191c39784842373654e604b7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants