-
Notifications
You must be signed in to change notification settings - Fork 72
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
Writing good web platform tests #170
Comments
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 |
A note on testing data: base URLs: I expect we can test such cases by encoding a HTML page with a import map into data: URL like: Current WPT tests don't support data: base URLs, |
This can be done for HTTP/HTTPS URLs, by returning JSON responses from expected URLs, by intercepting requests via Service Worker, etc. Anyway I expect it's better for tests to be async (there seems no reason to require sync testing, and testing |
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. |
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. |
Sounds perfect. Then I'll start working on the JSON approach. |
As for Blink
I'm neutral about Options 1 or 2. WDYT? |
This CL - Defines JSON test object format (see README.md) that describes the configurations and specifiers to be tested, - Implements the WPT test helper for executing the tests based on the JSON test objects (common-test-helper.js), - Converts Jest-based resolution tests into JSONs (with some refinement), and - Removes imported resolution tests. Bug: 1026809, WICG/import-maps#170 Change-Id: I993cc9cd7746d7175142f8296ac434571f5d7157
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. |
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) |
I was feeling that script base URL can be mocked by (I also haven't thoroughly thought about this though... Ah, and this can't test complicated |
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. |
+1 for I feel |
This CL - Defines JSON test object format (see README.md) that describes the configurations and specifiers to be tested, - Implements the WPT test helper for executing the tests based on the JSON test objects (common-test-helper.js), - Converts Jest-based resolution tests into JSONs (with some refinement), and - Removes imported resolution tests. The dependency to Blink internals remains after this CL. Removing this dependency is planned and discussed at WICG/import-maps#170. Bug: 1026809, WICG/import-maps#170 Change-Id: I993cc9cd7746d7175142f8296ac434571f5d7157
This CL - Defines JSON test object format (see README.md) that describes the configurations and specifiers to be tested, - Implements the WPT test helper for executing the tests based on the JSON test objects (common-test-helper.js), - Converts Jest-based resolution tests into JSONs (with some refinement, and removing test cases depending on interoperability issues of underlying URL parsers), and - Removes imported resolution tests. The dependency to Blink internals remains after this CL. Removing this dependency is planned and discussed at WICG/import-maps#170. Bug: 1026809, WICG/import-maps#170 Change-Id: I993cc9cd7746d7175142f8296ac434571f5d7157
This CL - Defines JSON test object format (see README.md) that describes the configurations and specifiers to be tested, - Implements the WPT test helper for executing the tests based on the JSON test objects (common-test-helper.js), - Converts Jest-based resolution tests into JSONs (with some refinement, and removing test cases depending on interoperability issues of underlying URL parsers), and - Removes imported resolution tests. The dependency to Blink internals remains after this CL. Removing this dependency is planned and discussed at WICG/import-maps#170. Bug: 1026809, WICG/import-maps#170 Change-Id: I993cc9cd7746d7175142f8296ac434571f5d7157 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1927852 Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org> Reviewed-by: Domenic Denicola <domenic@chromium.org> Cr-Commit-Position: refs/heads/master@{#721239}
This CL - Defines JSON test object format (see README.md) that describes the configurations and specifiers to be tested, - Implements the WPT test helper for executing the tests based on the JSON test objects (common-test-helper.js), - Converts Jest-based resolution tests into JSONs (with some refinement, and removing test cases depending on interoperability issues of underlying URL parsers), and - Removes imported resolution tests. The dependency to Blink internals remains after this CL. Removing this dependency is planned and discussed at WICG/import-maps#170. Bug: 1026809, WICG/import-maps#170 Change-Id: I993cc9cd7746d7175142f8296ac434571f5d7157 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1927852 Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org> Reviewed-by: Domenic Denicola <domenic@chromium.org> Cr-Commit-Position: refs/heads/master@{#721239}
…common Quick draft. Note: https://web-platform-tests.org/writing-tests/testdriver-extension-tutorial.html provides two methods, `testdriver-vendor.js` and `testdriver-extra.js`. I'm not sure which method to use, but in this CL I used `testdriver-vendor.js` method, because - It was easier to implement. - So far the API is expected only for testing and not needed to be exposed for automation. This CL contradicts with: https://web-platform-tests.org/writing-tests/testdriver.html > NB: presently, testdriver.js only works in the top-level > test browsing context (and not therefore in any frame or > window opened from it). because `test_driver` is accessed from inside `<iframe>`s in this CL. Perhaps this is OK in this case, as the `resolve_module_specifier` implementation works inside <iframe>s, while other `test_driver` method might stop working there. Bug: 1026809, WICG/import-maps#170 Change-Id: I8684ec544382e3036b9885e09f0c3a4d97208da5
Memo: we might need two kinds of tests around resolution:
|
…common Quick draft. Note: https://web-platform-tests.org/writing-tests/testdriver-extension-tutorial.html provides two methods, `testdriver-vendor.js` and `testdriver-extra.js`. I'm not sure which method to use, but in this CL I used `testdriver-vendor.js` method, because - It was easier to implement. - So far the API is expected only for testing and not needed to be exposed for automation. This CL contradicts with: https://web-platform-tests.org/writing-tests/testdriver.html > NB: presently, testdriver.js only works in the top-level > test browsing context (and not therefore in any frame or > window opened from it). because `test_driver` is accessed from inside `<iframe>`s in this CL. Perhaps this is OK in this case, as the `resolve_module_specifier` implementation works inside <iframe>s, while other `test_driver` method might stop working there. Bug: 1026809, WICG/import-maps#170 Change-Id: I8684ec544382e3036b9885e09f0c3a4d97208da5
…ion tests into JSON-based tests, a=testonly Automatic update from web-platform-tests [Import Maps] Migrate Jest-based resolution tests into JSON-based tests This CL - Defines JSON test object format (see README.md) that describes the configurations and specifiers to be tested, - Implements the WPT test helper for executing the tests based on the JSON test objects (common-test-helper.js), - Converts Jest-based resolution tests into JSONs (with some refinement, and removing test cases depending on interoperability issues of underlying URL parsers), and - Removes imported resolution tests. The dependency to Blink internals remains after this CL. Removing this dependency is planned and discussed at WICG/import-maps#170. Bug: 1026809, WICG/import-maps#170 Change-Id: I993cc9cd7746d7175142f8296ac434571f5d7157 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1927852 Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org> Reviewed-by: Domenic Denicola <domenic@chromium.org> Cr-Commit-Position: refs/heads/master@{#721239} -- wpt-commits: de73fe8cc89c55dea4fee1fc46d98d73dad2b21b wpt-pr: 20574
…ion tests into JSON-based tests, a=testonly Automatic update from web-platform-tests [Import Maps] Migrate Jest-based resolution tests into JSON-based tests This CL - Defines JSON test object format (see README.md) that describes the configurations and specifiers to be tested, - Implements the WPT test helper for executing the tests based on the JSON test objects (common-test-helper.js), - Converts Jest-based resolution tests into JSONs (with some refinement, and removing test cases depending on interoperability issues of underlying URL parsers), and - Removes imported resolution tests. The dependency to Blink internals remains after this CL. Removing this dependency is planned and discussed at WICG/import-maps#170. Bug: 1026809, WICG/import-maps#170 Change-Id: I993cc9cd7746d7175142f8296ac434571f5d7157 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1927852 Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org> Reviewed-by: Domenic Denicola <domenic@chromium.org> Cr-Commit-Position: refs/heads/master@{#721239} -- wpt-commits: de73fe8cc89c55dea4fee1fc46d98d73dad2b21b wpt-pr: 20574
…ion tests into JSON-based tests, a=testonly Automatic update from web-platform-tests [Import Maps] Migrate Jest-based resolution tests into JSON-based tests This CL - Defines JSON test object format (see README.md) that describes the configurations and specifiers to be tested, - Implements the WPT test helper for executing the tests based on the JSON test objects (common-test-helper.js), - Converts Jest-based resolution tests into JSONs (with some refinement, and removing test cases depending on interoperability issues of underlying URL parsers), and - Removes imported resolution tests. The dependency to Blink internals remains after this CL. Removing this dependency is planned and discussed at WICG/import-maps#170. Bug: 1026809, WICG/import-maps#170 Change-Id: I993cc9cd7746d7175142f8296ac434571f5d7157 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1927852 Commit-Queue: Hiroshige Hayashizaki <hiroshigechromium.org> Reviewed-by: Domenic Denicola <domenicchromium.org> Cr-Commit-Position: refs/heads/master{#721239} -- wpt-commits: de73fe8cc89c55dea4fee1fc46d98d73dad2b21b wpt-pr: 20574 UltraBlame original commit: 655a6e51517f24120040773fa1aa41f69532af6c
…ion tests into JSON-based tests, a=testonly Automatic update from web-platform-tests [Import Maps] Migrate Jest-based resolution tests into JSON-based tests This CL - Defines JSON test object format (see README.md) that describes the configurations and specifiers to be tested, - Implements the WPT test helper for executing the tests based on the JSON test objects (common-test-helper.js), - Converts Jest-based resolution tests into JSONs (with some refinement, and removing test cases depending on interoperability issues of underlying URL parsers), and - Removes imported resolution tests. The dependency to Blink internals remains after this CL. Removing this dependency is planned and discussed at WICG/import-maps#170. Bug: 1026809, WICG/import-maps#170 Change-Id: I993cc9cd7746d7175142f8296ac434571f5d7157 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1927852 Commit-Queue: Hiroshige Hayashizaki <hiroshigechromium.org> Reviewed-by: Domenic Denicola <domenicchromium.org> Cr-Commit-Position: refs/heads/master{#721239} -- wpt-commits: de73fe8cc89c55dea4fee1fc46d98d73dad2b21b wpt-pr: 20574 UltraBlame original commit: 655a6e51517f24120040773fa1aa41f69532af6c
…ion tests into JSON-based tests, a=testonly Automatic update from web-platform-tests [Import Maps] Migrate Jest-based resolution tests into JSON-based tests This CL - Defines JSON test object format (see README.md) that describes the configurations and specifiers to be tested, - Implements the WPT test helper for executing the tests based on the JSON test objects (common-test-helper.js), - Converts Jest-based resolution tests into JSONs (with some refinement, and removing test cases depending on interoperability issues of underlying URL parsers), and - Removes imported resolution tests. The dependency to Blink internals remains after this CL. Removing this dependency is planned and discussed at WICG/import-maps#170. Bug: 1026809, WICG/import-maps#170 Change-Id: I993cc9cd7746d7175142f8296ac434571f5d7157 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1927852 Commit-Queue: Hiroshige Hayashizaki <hiroshigechromium.org> Reviewed-by: Domenic Denicola <domenicchromium.org> Cr-Commit-Position: refs/heads/master{#721239} -- wpt-commits: de73fe8cc89c55dea4fee1fc46d98d73dad2b21b wpt-pr: 20574 UltraBlame original commit: 655a6e51517f24120040773fa1aa41f69532af6c
This CL - Converts Jest-based parsing tests into JSONs (with minor refinement, and removing test cases depending on interoperability issues of underlying URL parsers), - Adjust the JSON schema for parsing tests, and - Removes imported resolution tests. The test failures (virtual/import-maps-without-builtin-modules/external/wpt/import-maps/common/parsing.tentative.html) are due to WICG/import-maps#184, i.e. whether `null` mappings are removed from parsed import map or not. Bug: 1026809, WICG/import-maps#170 Change-Id: If1e53cbeafd44c1de1a7fa38ef646fcd5721b495
This CL - Converts Jest-based parsing tests into JSONs (with minor refinement, and removing test cases depending on interoperability issues of underlying URL parsers), - Adjust the JSON schema for parsing tests, and - Removes imported resolution tests. The test failures (virtual/import-maps-without-builtin-modules/external/wpt/import-maps/common/parsing.tentative.html) are due to WICG/import-maps#184, i.e. whether `null` mappings are removed from parsed import map or not. Bug: 1026809, WICG/import-maps#170 Change-Id: If1e53cbeafd44c1de1a7fa38ef646fcd5721b495 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1982509 Reviewed-by: Domenic Denicola <domenic@chromium.org> Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org> Cr-Commit-Position: refs/heads/master@{#731910}
This CL - Converts Jest-based parsing tests into JSONs (with minor refinement, and removing test cases depending on interoperability issues of underlying URL parsers), - Adjust the JSON schema for parsing tests, and - Removes imported resolution tests. The test failures (virtual/import-maps-without-builtin-modules/external/wpt/import-maps/common/parsing.tentative.html) are due to WICG/import-maps#184, i.e. whether `null` mappings are removed from parsed import map or not. Bug: 1026809, WICG/import-maps#170 Change-Id: If1e53cbeafd44c1de1a7fa38ef646fcd5721b495 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1982509 Reviewed-by: Domenic Denicola <domenic@chromium.org> Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org> Cr-Commit-Position: refs/heads/master@{#731910}
This CL - Converts Jest-based parsing tests into JSONs (with minor refinement, and removing test cases depending on interoperability issues of underlying URL parsers), - Adjust the JSON schema for parsing tests, and - Removes imported resolution tests. The test failures (virtual/import-maps-without-builtin-modules/external/wpt/import-maps/common/parsing.tentative.html) are due to WICG/import-maps#184, i.e. whether `null` mappings are removed from parsed import map or not. Bug: 1026809, WICG/import-maps#170 Change-Id: If1e53cbeafd44c1de1a7fa38ef646fcd5721b495 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1982509 Reviewed-by: Domenic Denicola <domenic@chromium.org> Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org> Cr-Commit-Position: refs/heads/master@{#731910}
… tests into JSON-based, a=testonly Automatic update from web-platform-tests [Import Maps] Migrate Jest-based parsing tests into JSON-based This CL - Converts Jest-based parsing tests into JSONs (with minor refinement, and removing test cases depending on interoperability issues of underlying URL parsers), - Adjust the JSON schema for parsing tests, and - Removes imported resolution tests. The test failures (virtual/import-maps-without-builtin-modules/external/wpt/import-maps/common/parsing.tentative.html) are due to WICG/import-maps#184, i.e. whether `null` mappings are removed from parsed import map or not. Bug: 1026809, WICG/import-maps#170 Change-Id: If1e53cbeafd44c1de1a7fa38ef646fcd5721b495 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1982509 Reviewed-by: Domenic Denicola <domenic@chromium.org> Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org> Cr-Commit-Position: refs/heads/master@{#731910} -- wpt-commits: a5e2efd90b6766f33e94d92e1858a68f8c40c977 wpt-pr: 21064
… tests into JSON-based, a=testonly Automatic update from web-platform-tests [Import Maps] Migrate Jest-based parsing tests into JSON-based This CL - Converts Jest-based parsing tests into JSONs (with minor refinement, and removing test cases depending on interoperability issues of underlying URL parsers), - Adjust the JSON schema for parsing tests, and - Removes imported resolution tests. The test failures (virtual/import-maps-without-builtin-modules/external/wpt/import-maps/common/parsing.tentative.html) are due to WICG/import-maps#184, i.e. whether `null` mappings are removed from parsed import map or not. Bug: 1026809, WICG/import-maps#170 Change-Id: If1e53cbeafd44c1de1a7fa38ef646fcd5721b495 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1982509 Reviewed-by: Domenic Denicola <domenic@chromium.org> Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org> Cr-Commit-Position: refs/heads/master@{#731910} -- wpt-commits: a5e2efd90b6766f33e94d92e1858a68f8c40c977 wpt-pr: 21064
The status as of today is that I am reasonably comfortable with our web platform tests. In particular, as of web-platform-tests/wpt#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 web-platform-tests/wpt#26239, but overall we're in good shape. |
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:
<script>.onerror
. Parsing successes can be tested to see if the resolution succeeds in the expected way. Individual parsing failures could similarly be tested by seeing if the resolution fails. This sounds like a good amount of work, but it may be worthwhile in the name of getting more integration test coverage...import.meta.resolve()
; see How could import.meta.resolve() behave? #79.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.
The text was updated successfully, but these errors were encountered: