Add tests for nomodule content attribute on script elements #4611

Merged
merged 1 commit into from Jan 31, 2017

Projects

None yet

5 participants

@rniwa
Collaborator
rniwa commented Jan 24, 2017

This feature is being added in whatwg/html#2261

Converted the tests in https://trac.webkit.org/r211078 to use testharness.js,
refined the test for IDL attribute reflecting content attribute, and re-organized tests.

@wpt-pr-bot wpt-pr-bot added the html label Jan 24, 2017
+
+test(() => {
+ assert_false(document.getElementById('moduleWithoutNomodule').noModule);
+}, 'noModule IDL attribute must return true on a parser created module script element with nomodule content attribute');
@Ms2ger
Ms2ger Jan 24, 2017 Contributor

Test name is wrong.

@zcorpan
zcorpan Jan 24, 2017 Member

I suggest removing all "must return X" from the test names; they tend to get out of sync and there is little value in stating the pass condition in the test name IMO. The assertion message will say what was expected if the test fails.

@rniwa
rniwa Jan 24, 2017 Collaborator

Done that (removing must return X).

+ assert_equals(loaded, true);
+ assert_equals(errored, false);
+ });
+}, 'An external classic script element dynamically inserted after noModule was set to false must run.');
@zcorpan
zcorpan Jan 24, 2017 Member

The test doesn't actually set noModule to false; suggest "without setting noModule" or so

@rniwa
rniwa Jan 24, 2017 Collaborator

Oh that was a bug. Fixed by adding the assignment.

+ element.onerror = () => { errored = true; }
+ setTimeout(resolve, 200);
+ }).then(() => {
+ assert_equals(window.executed, false);
@zcorpan
zcorpan Jan 24, 2017 Member

Doesn't the first test set window.executed to true? Maybe they should use different properties or be separate test files?

@zcorpan
zcorpan Jan 24, 2017 Member

nomodule-set-on-async-classic-script.html looks like it tries to address this; is this file an earlier version of that one? If so, I guess this file should be removed.

@rniwa
rniwa Jan 24, 2017 Collaborator

Oops, you're right. Removed this.

@zcorpan
zcorpan Jan 26, 2017 Member

Still need to remove this file.

+ assert_equals(typeof exportedCocoa, 'object');
+ assert_equals(exportedCocoa.taste(), 'awesome');
+ });
+}, 'An inline module script with nomodule content attribute must run');
@zcorpan
zcorpan Jan 24, 2017 Member

s/inline/external/

+
+test(() => {
+ assert_true(executed);
+}, 'An inline classic script with nomodule content attribute must run');
@zcorpan
zcorpan Jan 24, 2017 Member

s/with/without/

+ assert_true(executed);
+ assert_true(loaded);
+ assert_false(errored);
+}, 'A synchronously loaded external classic script with nomodule content attribute must run');
@zcorpan
zcorpan Jan 24, 2017 Member

s/with/without/

@@ -0,0 +1 @@
+window.executed = true;
@zcorpan
zcorpan Jan 24, 2017 Member

Maybe this could set document.currentScript.executed = true instead, to avoid keeping track of ordering in tests?

@rniwa
rniwa Jan 24, 2017 Collaborator

document.currentScript doesn't work for module scripts, does it?

@zcorpan
Member
zcorpan commented Jan 24, 2017

FAIL: 20/10 says https://travis-ci.org/w3c/web-platform-tests/jobs/194747170

@jgraham what is the cause there?

@zcorpan
Member
zcorpan commented Jan 24, 2017

@sideshowbarker says the cause is duplicate test names.

@rniwa
Collaborator
rniwa commented Jan 26, 2017

Ping reviewers. I've pushed a new commit after rebasing.

+ assert_true(loaded);
+ assert_false(errored);
+ });
+}, 'An asynchronously loaded classic script without noModule set to false must run');
@zcorpan
zcorpan Jan 26, 2017 Member

s/without/with/

@rniwa
rniwa Jan 30, 2017 Collaborator

Oops, fixed.

+ assert_false(loaded);
+ assert_false(errored);
+ });
+}, 'An asynchronously loaded classic script without noModule set to true must not run');
@zcorpan
zcorpan Jan 26, 2017 Member

s/without/with/

@rniwa
rniwa Jan 30, 2017 Collaborator

Fixed.

+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<!-- Load this script synchronously to ensure test cases below can load it in 200ms -->
+<script src="resources/set-window-executed.js"></script>
@zcorpan
zcorpan Jan 26, 2017 Member

All of these should be set-script-executed.js (or rename the file to set-window-executed.js)

@rniwa
rniwa Jan 30, 2017 Collaborator

Fixed

@zcorpan
Member
zcorpan commented Jan 30, 2017

LGTM except this comment appears to be unaddressed:
#4611 (comment)

@rniwa rniwa Add tests for nomodule content attribute on script elements
This feature is being added in whatwg/html#2261

Converted the tests in https://trac.webkit.org/r211078 to use testharness.js,
refined the test for IDL attribute reflecting content attribute, and re-organized tests.
aedf8ac
@rniwa
Collaborator
rniwa commented Jan 31, 2017

Oh strange. Removed for sure this time.

@domenic domenic was assigned by zcorpan Jan 31, 2017
@domenic
Collaborator
domenic commented Jan 31, 2017

Thanks so much!!

@domenic domenic merged commit c4725e5 into w3c:master Jan 31, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment