Issue 2845983002: [modules] Add several new WPT tests.

Issue 2845983002: [modules] Add several new WPT tests. (Closed)

Created:
3 years, 7 months ago by neis

Modified:
3 years, 6 months ago

Reviewers:
domenic, kochi

CC:
blink-reviews, blink-reviews-w3ctests_chromium.org, chromium-reviews, David Black, donnd+watch_chromium.org, Jered, jfweitz+watch_chromium.org, kmadhusu+watch_chromium.org, melevin+watch_chromium.org, samarth+watch_chromium.org, skanuj+watch_chromium.org

Target Ref:
refs/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

[modules] Add several new WPT tests. BUG=594639, 723841 Review-Url: https://codereview.chromium.org/2845983002 Cr-Commit-Position: refs/heads/master@{#475745} Committed: https://chromium.googlesource.com/chromium/src/+/db5c38ed5ffceeb949a875a17b6c3e2048b4eabb

Patch Set 1 #

Total comments: 34

Patch Set 2 : Fixed tests (hopefully), added two new ones. #

Patch Set 3 : . #

Patch Set 4 : . #

Patch Set 5 : Fix evaluation-error-* tests. #

Patch Set 6 : Test scriptEl.onload in instantiation-error-*.html. #

Total comments: 16

Patch Set 7 : Rebase. #

Patch Set 8 : More. #

Patch Set 9 : Cleanup. #

Patch Set 10 : Update TestExpectations. #

Patch Set 11 : Move on top of other CL. #

Created: 3 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+495 lines, -20 lines)			Patch
M	third_party/WebKit/LayoutTests/TestExpectations	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+7 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/bad-module-specifier.js	View	1 2 3 4 5 6 7	1 chunk	+3 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/compilation-error-1.html	View	1 2 3 4 5 6 7 8 9	1 chunk	+28 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/compilation-error-2.html	View	1 2 3 4 5 6 7 8 9	1 chunk	+28 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/cycle-tdz-access.js	View		1 chunk	+3 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/cycle-tdz-access-a.js	View		1 chunk	+3 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/cycle-unresolvable.js	View		1 chunk	+2 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/cycle-unresolvable-a.js	View		1 chunk	+2 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/evaluation-error-1.html	View	1 2 3 4 5 6 7 8	1 chunk	+27 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/evaluation-error-2.html	View	1 2 3 4 5 6 7 8	1 chunk	+27 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/evaluation-error-3.html	View	1 2 3 4 5 6 7 8	1 chunk	+28 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/evaluation-error-4.html	View	1 2 3 4 5 6 7 8	1 chunk	+28 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/export-something.js	View	1	1 chunk	+3 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/export-something-nested.js	View	1 2 3 4 5 6 7	1 chunk	+2 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/fetch-error-1.html	View	1 2 3 4 5 6 7 8	1 chunk	+15 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/fetch-error-2.html	View	1 2 3 4 5 6 7 8	1 chunk	+15 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/fetch-error-2.js	View	1 2 3 4 5 6 7	1 chunk	+2 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/import-something-namespace.js	View	1	1 chunk	+5 lines, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/instantiation-error-1.html	View	1 2 3 4 5 6 7 8 9	1 chunk	+31 lines, -20 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/instantiation-error-2.html	View	1 2 3 4 5 6 7 8 9	1 chunk	+33 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/instantiation-error-3.html	View	1 2 3 4 5 6 7 8 9	1 chunk	+29 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/late-namespace-request.html	View	1 2 3 4 5 6 7 8 9	1 chunk	+20 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/late-star-export-request.html	View	1 2 3 4 5 6 7 8 9	1 chunk	+25 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/missing-export.js	View		1 chunk	+2 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/missing-export-nested.js	View		1 chunk	+2 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/module-vs-script-1.html	View	1 2 3 4 5 6 7 8 9	1 chunk	+17 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/module-vs-script-2.html	View	1 2 3 4 5 6 7 8 9	1 chunk	+17 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/nested-missing-export.js	View		1 chunk	+2 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/nomodule-attribute.html	View	1 2 3 4 5 6 7 8 9	1 chunk	+18 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/single-evaluation-1.html	View	1 2 3 4 5 6 7 8	1 chunk	+20 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/single-evaluation-2.html	View	1 2 3 4 5 6 7 8	1 chunk	+20 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/specifier-error.html	View	1 2 3 4 5 6 7 8	1 chunk	+20 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/syntaxerror.js	View		1 chunk	+2 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/syntaxerror-nested.js	View		1 chunk	+2 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/this.js	View		1 chunk	+1 line, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/this-nested.js	View		1 chunk	+2 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/throw.js	View	1 2 3 4 5 6 7 8	1 chunk	+2 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/throw-nested.js	View		1 chunk	+2 lines, -0 lines	0 comments	Download

Messages

Total messages: 23 (7 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

domenic

Great stuff! I think with some updates this should cover https://github.com/whatwg/html/pull/2595; I'm really happy you ...

3 years, 7 months ago (2017-04-28 20:12:03 UTC) #2

neis

Thanks for the feedback. I made some changes and I believe, with the exception of ...

3 years, 7 months ago (2017-05-02 13:52:00 UTC) #3

Thanks for the feedback. I made some changes and I believe, with the exception
of evaluation-error*, the tests are now correct.

I have not yet added any documentation or description due to lack of time.

I added one test concerning bad module specifiers.
I added one test concerning module namespace objects.

I still don't understand the semantics w.r.t. evaluation error, let's chat about
that "offline". Once that has been sorted out, I will also extend all tests with
checks of load events.

https://codereview.chromium.org/2845983002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/compilation-error-1.html
(right):

https://codereview.chromium.org/2845983002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/compilation-error-1.html:20:
assert_equals(log.length, 2);
On 2017/04/28 20:12:02, domenic wrote:
> This should be 5, I believe. The script is in an errored state.

Done.

https://codereview.chromium.org/2845983002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/compilation-error-1.html:23:
}));
On 2017/04/28 20:12:02, domenic wrote:
> Since this is a single test, you can omit the async_test stuff and just call
> done() when the test is done (i.e. after the assert_true). See "Single Page
> Test" on http://web-platform-tests.org/writing-tests/testharness-api.html

I wasn't able to make this work. Even without calling done(), the test passed.

https://codereview.chromium.org/2845983002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/compilation-error-1.html:26:
<script type="module" src="./syntaxerror.js"></script>
On 2017/04/28 20:12:02, domenic wrote:
> We should add onerror="assert_unreached('1st')" etc. since onerror should
never
> be fired for these. We should do that for every test that isn't testing
fetching
> errors.

assert_unreached() didn't work for me, that's why I used my own unreachable()
function. I added it to all appropriate script tags now.

https://codereview.chromium.org/2845983002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/compilation-error-2.html
(right):

https://codereview.chromium.org/2845983002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/compilation-error-2.html:20:
assert_equals(log.length, 2);
On 2017/04/28 20:12:02, domenic wrote:
> Should be 5

Done.

https://codereview.chromium.org/2845983002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/evaluation-error-3.html
(right):

https://codereview.chromium.org/2845983002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/evaluation-error-3.html:22:
"throw",
On 2017/04/28 20:12:03, domenic wrote:
> Why does "throw" show up more than once? I thought a module could not be
> evaluated twice?

Yes, this is wrong.

https://codereview.chromium.org/2845983002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/evaluation-error-4.html
(right):

https://codereview.chromium.org/2845983002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/evaluation-error-4.html:23:
"throw",
On 2017/04/28 20:12:03, domenic wrote:
> Same question

dito

https://codereview.chromium.org/2845983002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/fetch-error-1.html
(right):

https://codereview.chromium.org/2845983002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/fetch-error-1.html:17:
assert_false(true);
On 2017/04/28 20:12:03, domenic wrote:
> What is this assert_false(true) about?

Oops, I didn't intend to include this file actually.

https://codereview.chromium.org/2845983002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/fetch-error-1.html:22:
window.addEventListener("error", test_error.step_func_done(ev => {
On 2017/04/28 20:12:03, domenic wrote:
> Per latest changes this should *not* fire; it would be good to test that it
> doesn't, in fact. I guess that's automatically accomplished by not setting
> allow_uncaught_exception.

I believe it should fire because it's not being handled by the script, right?

I changed the test now such that the script handles it. And yes, not setting
the allow_uncaught_exception ensures that there's no separate window.error
event.

Are there any properties of the event object that we'd like to test? (Is it
expected to have an "error" property?)

https://codereview.chromium.org/2845983002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/instantiation-error-1.html
(right):

https://codereview.chromium.org/2845983002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/instantiation-error-1.html:20:
assert_equals(log.length, 2);
On 2017/04/28 20:12:03, domenic wrote:
> Per latest semantics this should be 5, I believe.

Done.

https://codereview.chromium.org/2845983002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/instantiation-error-2.html
(right):

https://codereview.chromium.org/2845983002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/instantiation-error-2.html:20:
assert_equals(log.length, 2);
On 2017/04/28 20:12:03, domenic wrote:
> Should be 5

Done.

neis

The evaluation-error-* tests should now conform to the (changed) spec too.

3 years, 7 months ago (2017-05-03 10:22:29 UTC) #4

domenic

On 2017/05/02 at 13:52:00, neis wrote: > https://codereview.chromium.org/2845983002/diff/1/third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/fetch-error-1.html#newcode22 > third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/fetch-error-1.html:22: window.addEventListener("error", test_error.step_func_done(ev => { > ...

3 years, 7 months ago (2017-05-03 23:24:03 UTC) #5

domenic

https://codereview.chromium.org/2845983002/diff/100001/third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/TODO File third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/TODO (right): https://codereview.chromium.org/2845983002/diff/100001/third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/TODO#newcode2 third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/TODO:2: Should even the error events be the same? Error ...

3 years, 7 months ago (2017-05-03 23:49:30 UTC) #6

neis

On 2017/05/03 23:24:03, domenic wrote: > On 2017/05/02 at 13:52:00, neis wrote: > third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/fetch-error-1.html:22: > ...

3 years, 7 months ago (2017-05-04 12:18:48 UTC) #7

neis

Some replies below. Your other comments are not forgotten. https://codereview.chromium.org/2845983002/diff/100001/third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/evaluation-error-1.html File third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/evaluation-error-1.html (right): https://codereview.chromium.org/2845983002/diff/100001/third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/evaluation-error-1.html#newcode15 third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/evaluation-error-1.html:15: ...

3 years, 7 months ago (2017-05-04 13:07:54 UTC) #8

Some replies below. Your other comments are not forgotten.

https://codereview.chromium.org/2845983002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/evaluation-error-1.html
(right):

https://codereview.chromium.org/2845983002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/evaluation-error-1.html:15:
assert_array_equals(log, ["throw", 666]);
On 2017/05/03 23:49:30, domenic wrote:
> So now per the very latest changes this should show up in the log five times.
> Thanks for keeping pushing on this :)

Shouldn't the result now be ["throw", 666, 666, 666, 666]?

https://codereview.chromium.org/2845983002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/evaluation-error-2.html
(right):

https://codereview.chromium.org/2845983002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/evaluation-error-2.html:17:
assert_equals(log[1].constructor, ReferenceError);
On 2017/05/03 23:49:30, domenic wrote:
> And again per the very latest changes this would require log.length === 4,
> log[2] === log[1], log[3] === log[1], I believe.

Agreed.

https://codereview.chromium.org/2845983002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/evaluation-error-3.html
(right):

https://codereview.chromium.org/2845983002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/evaluation-error-3.html:15:
assert_array_equals(log, ["throw", 666, "throw-nested"]);
On 2017/05/03 23:49:30, domenic wrote:
> This time I believe it's ["throw", 666, 666, 666, 666, 666, 666]. The
> HostResolveImportedModule step that happens before evaluating any of
> throw-nested.js will (re-)throw the error, like the doc explains. (It's the
same
> as for instantiation failures.)

I would expect ["throw", 666, 666, 666, 666, 666].

https://codereview.chromium.org/2845983002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/neis/late-namespace-request.html
(right):

https://codereview.chromium.org/2845983002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/neis/late-namespace-request.html:1:
<!DOCTYPE html>
On 2017/05/03 23:49:30, domenic wrote:
> This looks like it was committed by accident

It was intentional. The idea is to test the situation where
a) the loading of a module graph does not require the creation of some
   module's namespace object (here that of export-something.js),
b) but the later loading of another module graph does require it (here
   it's requested by import-something-namespace.js).

Do you think such a test makes sense? (In V8 we don't create the
namespace object until it is requested somewhere.)

domenic

On 2017/05/04 at 12:18:48, neis wrote: > That's what I thought (I think I read ...

3 years, 7 months ago (2017-05-04 14:53:11 UTC) #9

domenic

On 2017/05/04 at 12:18:48, neis wrote: > That's what I thought (I think I read ...

3 years, 7 months ago (2017-05-04 14:53:30 UTC) #10

neis

Hi Domenic, I'd like to land this now. I've made a few updates, please have ...

3 years, 6 months ago (2017-05-30 16:03:35 UTC) #12

commit-bot: I haz the power

CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2845983002/200001

3 years, 6 months ago (2017-05-30 23:24:27 UTC) #15

commit-bot: I haz the power

No L-G-T-M from a valid reviewer yet. CQ run can only be started once the ...

3 years, 6 months ago (2017-05-30 23:24:29 UTC) #17

commit-bot: I haz the power

CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2845983002/200001

3 years, 6 months ago (2017-05-30 23:29:58 UTC) #20

commit-bot: I haz the power

3 years, 6 months ago (2017-05-31 02:22:33 UTC) #23

Message was sent while issue was closed.

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/db5c38ed5ffceeb949a875a17b6c...

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages