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 tests for nomodule content attribute on script elements #4611

Merged
merged 1 commit into from Jan 31, 2017

Conversation

Projects
None yet
6 participants
@rniwa
Contributor

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

This comment has been minimized.

Collaborator

wpt-pr-bot commented Jan 24, 2017

@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');

This comment has been minimized.

@Ms2ger

Ms2ger Jan 24, 2017

Contributor

Test name is wrong.

This comment has been minimized.

@zcorpan

zcorpan Jan 24, 2017

Contributor

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.

This comment has been minimized.

@rniwa

rniwa Jan 24, 2017

Contributor

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.');

This comment has been minimized.

@zcorpan

zcorpan Jan 24, 2017

Contributor

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

This comment has been minimized.

@rniwa

rniwa Jan 24, 2017

Contributor

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

element.onerror = () => { errored = true; }
setTimeout(resolve, 200);
}).then(() => {
assert_equals(window.executed, false);

This comment has been minimized.

@zcorpan

zcorpan Jan 24, 2017

Contributor

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

This comment has been minimized.

@zcorpan

zcorpan Jan 24, 2017

Contributor

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.

This comment has been minimized.

@rniwa

rniwa Jan 24, 2017

Contributor

Oops, you're right. Removed this.

This comment has been minimized.

@zcorpan

zcorpan Jan 26, 2017

Contributor

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');

This comment has been minimized.

@zcorpan

zcorpan Jan 24, 2017

Contributor

s/inline/external/

test(() => {
assert_true(executed);
}, 'An inline classic script with nomodule content attribute must run');

This comment has been minimized.

@zcorpan

zcorpan Jan 24, 2017

Contributor

s/with/without/

assert_true(executed);
assert_true(loaded);
assert_false(errored);
}, 'A synchronously loaded external classic script with nomodule content attribute must run');

This comment has been minimized.

@zcorpan

zcorpan Jan 24, 2017

Contributor

s/with/without/

@@ -0,0 +1 @@
window.executed = true;

This comment has been minimized.

@zcorpan

zcorpan Jan 24, 2017

Contributor

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

This comment has been minimized.

@rniwa

rniwa Jan 24, 2017

Contributor

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

@zcorpan

This comment has been minimized.

Contributor

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

This comment has been minimized.

Contributor

zcorpan commented Jan 24, 2017

@sideshowbarker says the cause is duplicate test names.

@rniwa rniwa force-pushed the rniwa:add-nomodule-tests branch from d38e369 to f1e53e2 Jan 24, 2017

@rniwa

This comment has been minimized.

Contributor

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');

This comment has been minimized.

@zcorpan

zcorpan Jan 26, 2017

Contributor

s/without/with/

This comment has been minimized.

@rniwa

rniwa Jan 30, 2017

Contributor

Oops, fixed.

assert_false(loaded);
assert_false(errored);
});
}, 'An asynchronously loaded classic script without noModule set to true must not run');

This comment has been minimized.

@zcorpan

zcorpan Jan 26, 2017

Contributor

s/without/with/

This comment has been minimized.

@rniwa

rniwa Jan 30, 2017

Contributor

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>

This comment has been minimized.

@zcorpan

zcorpan Jan 26, 2017

Contributor

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

This comment has been minimized.

@rniwa

rniwa Jan 30, 2017

Contributor

Fixed

@rniwa rniwa force-pushed the rniwa:add-nomodule-tests branch from f1e53e2 to c240d2e Jan 30, 2017

@zcorpan

This comment has been minimized.

Contributor

zcorpan commented Jan 30, 2017

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

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.

@rniwa rniwa force-pushed the rniwa:add-nomodule-tests branch from c240d2e to aedf8ac Jan 31, 2017

@rniwa

This comment has been minimized.

Contributor

rniwa commented Jan 31, 2017

Oh strange. Removed for sure this time.

@domenic

This comment has been minimized.

Member

domenic commented Jan 31, 2017

Thanks so much!!

@domenic domenic merged commit c4725e5 into web-platform-tests:master Jan 31, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jmaher

This comment has been minimized.

Contributor

jmaher commented Mar 24, 2017

@rniwa we run this test in automation at Mozilla and there are intermittent failures (we set this to expect failure and it intermittently passes) as documented in https://bugzilla.mozilla.org/show_bug.cgi?id=1344486. Is there more we can do to make this test stable?

@zcorpan

This comment has been minimized.

Contributor

zcorpan commented Mar 24, 2017

You could add a feature check and fail the test early if noModule is not supported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment