WICG / import-maps

How to control the behavior of JavaScript imports
https://html.spec.whatwg.org/multipage/webappapis.html#import-maps
Other
2.68k stars 69 forks source link

Writing good web platform tests #170

Closed domenic closed 2 years ago

domenic commented 5 years ago

Currently the reference implementation, targeted at Node.js, has tests written in Jest. This was done purely to give me something to sanity-check my work as I wrote the spec. (I.e., I didn't want to write a spec with no check that it was correct.)

But for a web platform feature, what we really need is web platform tests, targeted at the built-in implementation.

Right now @hiroshige-g is using some clever hacks to run the reference implementation tests in Chromium. (See e.g. this CL.) This is not tenable longer-term, because:

So eventually, someone (ideally me, since I created this mess) should figure out how to port tests to idiomatic web platform tests. Here are some initial thoughts on that:

A goal then would be to have some kind of wrapper that allows running the tests against Node.js implementations like the reference implementation, so that we can continue to co-evolve the spec and reference implementation, like we do with Streams, URL, and the data: URL parser today. This works best, we've found, if we can make the test inputs a JSON data file.


I welcome thoughts or comments on the above. I think the next steps are for me to see if I can rewrite a few basic tests in non-vendor-specific, idiomatic web platform tests format.

domenic commented 5 years ago

Note for future self: if you put too many import tests in one file they can be slow. Cf. https://chromium-review.googlesource.com/c/chromium/src/+/1762260

hiroshige-g commented 4 years ago

A note on testing data: base URLs: (Context: https://chromium-review.googlesource.com/c/chromium/src/+/1864014)

I expect we can test such cases by encoding a HTML page with a import map into data: URL like: data:text/html,<script type="importmap">...</script><script>test code using postMessage()</script> and create an <iframe> with that URL.

Current WPT tests don't support data: base URLs, just because the data: URL <iframe> works asynchronously, while my Jest-WPT glue code requires everything completes synchronously. (I haven't figured out how to allow asynchronous paths in my glue code)

hiroshige-g commented 4 years ago

Resolving tests: I suspect there is some clever way to get the resolved URL of a module specifier today, as long as you're willing for it to be async. I need to think about it harder.

This can be done for HTTP/HTTPS URLs, by returning JSON responses from expected URLs, by intercepting requests via Service Worker, etc. As for non-HTTP-family URLs (e.g. data: URLs, mailto:foobar, etc.), I think testing such URLs is impossible without import.meta.resolve().

Anyway I expect it's better for tests to be async (there seems no reason to require sync testing, and testing data: base URLs will need async (https://github.com/WICG/import-maps/issues/170#issuecomment-542808784)).

hiroshige-g commented 4 years ago

It is not reasonable to put the burden on future browser/spec engineers, besides myself and @hiroshige-g, to learn Jest and a wrapper framework to maintain this feature.

Writing hard-coded idiomatic web platform tests might cause costs when testing ref-impl (e.g. convert WPT back to Jest).

One option could be writing test expectations as common JSON files (the tests/expectations applicable to both WPT and Jest can be highly declarative), and write idiomatic test helpers for each of WPT and Jest. The cost here is the additional layer of test helpers, but probably it's small (and similar test helpers are used in many other locations).

domenic commented 4 years ago

Right. I would love to use common JSON files if possible. However, I think the higher priority right now is better tests for browsers. I am OK if the reference implementation starts falling behind or becoming less testable. It was most useful during the early stages, and keeping it and its tests perfect is now a secondary priority.

So, my advice would be to work on a JSON format that can theoretically be used by both, but do not do any work on the reference implementation/Jest tests to actually integrate that JSON. Instead only work on that JSON plus the web platform tests harness for it.

hiroshige-g commented 4 years ago

Sounds perfect. Then I'll start working on the JSON approach.

hiroshige-g commented 4 years ago

As for Blink internals, there are basically three options:

  1. Keep internals for now.
  2. Introduce standardized test-only APIs (using webdriver) and replace internals with them.
  3. Introduce standardized APIs (e.g. import.meta.resolve) and replace internals with them.

I'm neutral about Options 1 or 2. I feel Option 3 is not preferrable, because import.meta.resolve has some issues (e.g. interactions with possible future proposals like fallbacks between multiple fetch-scheme URLs) and there are no non-test needs for the public APIs right now.

WDYT?

domenic commented 4 years ago

I was hoping there was a 4th option, of replacing it with clever tricks using existing APIs.

It seems like the hard part is setting the script base URL that does the import, right? I see a few possible options:

That said, I think we could do option 2 as well. It will be more work, but there is precedent.

hiroshige-g commented 4 years ago

I feel the hard part is how to test resolution results. Resolution results can be captured/tested in easy cases (by service workers, by serving certain JavaScript code for resolved URLs, etc.), but hard cases would be:

(So far I haven't dug into Option 2. I'll take a look at how much work would be needed for Option 2 and then compare it with other options' costs later)

hiroshige-g commented 4 years ago

I was feeling that script base URL can be mocked by <base> tags (for non-data base URLs) and import "data:text/javascript,import('specifier-to-be-tested');";.

(I also haven't thoroughly thought about this though... Ah, and this can't test complicated data: base URLs around scoping...?)

domenic commented 4 years ago

Oh, you're right, that's way easier than my ideas.

I think it is totally fine if our initial batch of tests does not deal with data: URLs in a cross-browser-compatible way. That is a relatively small corner case that we can work on fixing over time.

hiroshige-g commented 4 years ago

I think it is totally fine if our initial batch of tests does not deal with data: URLs in a cross-browser-compatible way. That is a relatively small corner case that we can work on fixing over time.

+1 for data: URLs + scoping (using a wrong scope isn't so harmful e.g. in terms of security, especially because import maps and the scoping mechanism are not for security purposes).

I feel data: URLs + resolution (like #166) is more important, but I feel we can figure out test coverage here.

hiroshige-g commented 4 years ago

Memo: we might need two kinds of tests around resolution:

domenic commented 3 years ago

The status as of today is that I am reasonably comfortable with our web platform tests. In particular, as of https://github.com/web-platform-tests/wpt/pull/26064 and some previous PRs, the tests:

There remain some tests in https://github.com/web-platform-tests/wpt/tree/master/import-maps/common which rely on Chromium internals, namely for resolution of non-service-worker-interceptable URLs, and tests which check how the import map was parsed. However, those are more "unit tests" than WPT-style integration tests; I believe we're testing the majority of the observable consequences of the import maps spec for web developers.

I will do some work to rearrange https://github.com/web-platform-tests/wpt/tree/master/import-maps and probably move the Chromium-internal tests out of that folder in https://github.com/web-platform-tests/wpt/pull/26239, but overall we're in good shape.