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

Add some initial tests for JSON modules. #16734

Merged
merged 1 commit into from
May 20, 2019
Merged

Add some initial tests for JSON modules. #16734

merged 1 commit into from
May 20, 2019

Conversation

Ms2ger
Copy link
Contributor

@Ms2ger Ms2ger commented May 8, 2019

@Ms2ger
Copy link
Contributor Author

Ms2ger commented May 9, 2019

Note to self: some CORS tests could be useful too.

CC @littledan

@annevk
Copy link
Member

annevk commented May 10, 2019

Chrome added some logic for application/json+... if I remember correctly for CORB. Might be good to verify those fail.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as a start. For posterity, additional ideas for tests (potentially best done by the first implementer):

  • Non-JSON content types get rejected, including tricky cases like @annevk pointed out.
  • Test that there are no non-default imports. (Maybe easiest to test with import().)
  • Test non-Window contexts. (Easy: using .any.js + import(). Harder: using module workers which nobody ships yet, but Chrome has behind a flag.)
  • Test JSON files containing non-objects (null, true, false, array)

@@ -0,0 +1,21 @@
<!DOCTYPE html>
<meta charset=utf-8>
<title>JSON modules: Content-Type</title>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong title

@domenic
Copy link
Member

domenic commented May 10, 2019

Also there are a lot of tests in the JS modules directory that might benefit from porting. For example testing that if you have module A with side effects, that imports JSON module B containing invalid JSON, asserting that the side effects of A do not happen.

/cc @nyaxt and @hiroshige-g as folks who helped with a lot of the test coverage for JS modules.

@Ms2ger Ms2ger merged commit f510720 into master May 20, 2019
@Ms2ger Ms2ger deleted the json-modules branch May 20, 2019 14:52
@annevk annevk mentioned this pull request Sep 28, 2019
2 tasks
annevk added a commit to whatwg/html that referenced this pull request Oct 2, 2019
As explained at WICG/webcomponents#839 the current setup is insecure.

Tests are now marked as "tentative": web-platform-tests/wpt#16734.

This reverts db03474.
zcorpan pushed a commit to whatwg/html that referenced this pull request Nov 6, 2019
As explained at WICG/webcomponents#839 the current setup is insecure.

Tests are now marked as "tentative": web-platform-tests/wpt#16734.

This reverts db03474.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants