Add partial tests for `import defer` #4278

nicolo-ribaudo · 2024-10-25T16:34:04Z

Plan: #4215 (I checked the checkbox for tests added by this PR)

This PR includes tests for:

syntax
Deferred namespace objects
Behavior of synchronous and asynchronous deferred modules
Errors for synchronous and asynchronous cases

I will open another PR (or push another commit here) for tests about async modules, since I still have to finish polishing it up.

There are a few details of the proposal that are still in flux because of some bugs, so this PR does not include tests for them:

Symbol.toStringTag of deferred namespace objects
test/language/import/import-defer/evaluation-sync/evaluation-* for the other object operations
import.defer, which currently doesn't actually defer due to a problem with .then

- syntax - Deferred namespace objects - Behavior of synchronous deferred modules - Errors for synchronous cases I will open another PR (or push another commit here) for tests about async modules, since I still have to finish polishing it up. There are a few details of the proposal that are still in flux because of some bugs, so this PR does not include tests for them: - `Symbol.toStringTag` of deferred namespace objects - `test/language/import/import-defer/evaluation-sync/evaluation-*` for the other object operations - `import.defer`, which currently doesn't actually defer due to a problem with `.then`

takikawa · 2024-10-28T00:57:56Z

I tried running these in my WebKit implementation and found a few minor issues in the tests, such as missing module flags. This commit has the fixes I made: takikawa/WebKit@d536aa2

After the fixes I had about 8 failures but at least some of these are due to bugs in my implementation, I think.

nicolo-ribaudo · 2024-10-28T14:06:33Z

Thanks! I imported your patch.

After the fixes I had about 8 failures but at least some of these are due to bugs in my implementation, I think.

~~If you could share the list, I can double-check that the tests are correct.~~

(EDIT: Confirmed that they are bugs in Asumu's implementation)

* Add deepEqual.js include where needed * Add module flag where needed * Fix some typos * In evaluation-ignore-set, add a try block to catch typeerrors from assignment Need the bug URL (OOPS!). Reviewed by NOBODY (OOPS!). Explanation of why this fixes the bug (OOPS!). * test/language/import/import-defer/deferred-namespace-object/dep-defer-ns_FIXTURE.js: * test/language/import/import-defer/deferred-namespace-object/identity.js: * test/language/import/import-defer/deferred-namespace-object/object-properties.js: * test/language/import/import-defer/errors/get-other-while-dep-evaluating/main.js: * test/language/import/import-defer/errors/get-other-while-evaluating/main.js: * test/language/import/import-defer/errors/get-self-while-defer-evaluating/main.js: * test/language/import/import-defer/errors/get-self-while-evaluating.js: * test/language/import/import-defer/errors/module-throws/defer-import-after-evaluation.js: * test/language/import/import-defer/errors/module-throws/third-party-evaluation-after-defer-import.js: * test/language/import/import-defer/errors/module-throws/trigger-evaluation.js: * test/language/import/import-defer/errors/resolution-error/import-defer-of-syntax-error-fails.js: * test/language/import/import-defer/errors/syntax-error/import-defer-of-syntax-error-fails.js: * test/language/import/import-defer/evaluation-sync/evaluation-ignore-get-symbol.js: * test/language/import/import-defer/evaluation-sync/evaluation-ignore-getPrototypeOf.js: * test/language/import/import-defer/evaluation-sync/evaluation-ignore-isExtensible.js: * test/language/import/import-defer/evaluation-sync/evaluation-ignore-preventExtensions.js: * test/language/import/import-defer/evaluation-sync/evaluation-ignore-set.js: * test/language/import/import-defer/evaluation-sync/evaluation-ignore-setPrototypeOf.js: * test/language/import/import-defer/evaluation-sync/evaluation-trigger-get-string.js: * test/language/import/import-defer/evaluation-sync/import-defer-does-not-evaluate.js: * test/language/import/import-defer/evaluation-sync/module-imported-defer-and-eager.js: * test/language/import/import-defer/syntax/import-attributes.js: * test/language/import/import-defer/syntax/valid-default-binding-named-defer.js: * test/language/import/import-defer/syntax/valid-defer-namespace.js:

ptomato

I haven't gotten most of the way through this, but here are some initial comments.

test/language/import/import-defer/syntax/invalid-defer-default-and-namespace copy.js

ptomato · 2024-10-30T01:29:09Z

test/language/import/import-defer/syntax/valid-default-binding-named-defer.js

+features: [import-defer]
+---*/
+
+import defer from "./dep_FIXTURE.js";


Do we have a test elsewhere (not in syntax) that ensures this syntax is treated as a sync default binding named defer and not as a deferred import? e.g. the imported module performs a side effect that is tested for.

No. Should I just add it in this file, given that we don't really have the concept of "syntax tests"?

Sure, that would be fine too. By "in syntax" I meant in this syntax/ folder where the test is currently placed; if the test no longer purely tests syntax, you could move it, but I don't think it matters that much.

test/language/import/import-defer/deferred-namespace-object/identity.js

test/language/import/import-defer/deferred-namespace-object/object-properties.js

test/language/import/import-defer/errors/get-other-while-evaluating/main.js

...nguage/import/import-defer/errors/module-throws/third-party-evaluation-after-defer-import.js

nicolo-ribaudo · 2024-10-31T12:20:15Z

I noticed that I cannot use assert in _FIXTURE files (https://github.com/tc39/test262/blob/main/CONTRIBUTING.md#rules-for-module-_fixture-files). It's a bit weird because they are running in the same global scope as the non-fixture files, but I pushed a commit to fix it anyway.

ptomato

Thanks! In general these tests are pretty easy to read and understand.

There were some things that I think are errors in assertions, like the tags pushed to globalThis.evaluations in some cases, so if your implementation is passing these tests in their current state, you might want to check if that's correct.

test/language/import/import-defer/deferred-namespace-object/object-properties.js

ptomato · 2024-11-01T20:21:46Z

test/language/import/import-defer/errors/get-self-while-evaluating-async/main.js

+assert(globalThis["error on ns.foo"] instanceof TypeError, "ns.foo while evaluating throws a TypeError");
+
+ns.foo;
+


Is this order correct? It seems like the evaluation of the module should only happen during the evaluation of ns.foo in line 36, so in line 34 the error won't be present yet.
Additionally, do we not need to await Promise.resolve() in between evaluating ns.foo and checking for the TypeError, since the error will be caught in a microtask?

These comments may just be my misunderstanding of the proposal! I have similar comments on several other files which probably indicates that I'm missing some piece of comprehension.

(If I'm misunderstanding and microtasks in a deferred module import happen synchronously, then does this test and similar ones really need to be async though?)

What is happening here is that ./dep_FIXTURE.js is not deferred, because it contains top-level await. So what is happening in this test is that:

the deferred module is not actually deferred, so dep_FIXTURE.js starts executing and goes in its evaluating state

it has access to a deferred namespace of itself

once it reaches the await, the state changes to evaluating-async

the test tries then to access a property from the deferred namespace while it's evaluating-async (which is what this test it testing). It should throw.

dep_FIXTURE.js is done, and becomes evaluated

main.js starts evaluating, and the error is already there

ns.foo now works, because ns is evaluated and not evaluating-async

Should I put this comment in the "info" section, even if usually it just lists spec algorithms?

I think it'd be helpful to spell this out, in the info section, but feel free to use your own judgement; it might already be obvious to someone who's implementing the proposal.

test/language/import/import-defer/errors/resolution-error/import-defer-of-syntax-error-fails.js

ptomato · 2024-11-01T20:23:25Z

test/language/import/import-defer/errors/resolution-error/import-defer-of-syntax-error-fails.js

Name might have been copied from the one in syntax-error/, rename to import-defer-of-missing-module-fails.js?

I think this got resolved accidentally, the name still references SyntaxError.

test/language/import/import-defer/errors/syntax-error/import-defer-of-syntax-error-fails.js

...ort/import-defer/evaluation-top-level-await/sync-dependency-of-deferred-async-module/main.js

test/language/import/import-defer/evaluation-sync/evaluation-ignore-isExtensible.js

test/language/import/import-defer/evaluation-sync/evaluation-ignore-preventExtensions.js

test/language/import/import-defer/evaluation-sync/evaluation-ignore-setPrototypeOf.js

.../import/import-defer/evaluation-top-level-await/import-defer-transitive-async-module/main.js

test/language/import/import-defer/evaluation-top-level-await/flattening-order/main.js

nicolo-ribaudo · 2024-11-07T16:00:37Z

Thanks both for the reviews :) Comments addressed.

test/language/import/import-defer/evaluation-top-level-await/flattening-order/main.js

takikawa · 2024-11-07T21:24:10Z

test/language/import/import-defer/errors/module-throws/trigger-evaluation.js

+
+let err1, err2;
+try { ns.foo } catch (e) { err1 = e };
+assert.deepEqual(err1, { someError: "the error from throws_FIXTURE" }, "Evaluation errors are thrown when evaluating");


Looks like this needs the deepEqual include or be written to avoid it

Could we instead write the test to avoid deepEqual? We are trying to avoid it in new tests, see #3476.
The thrown value could be a primitive, like "the error from throws_FIXTURE", since we are not checking for object identity anyway.

I want to throw an object to test that it's the same, but I'll remove deepEqual

test/language/import/import-defer/errors/module-throws/defer-import-after-evaluation.js

...nguage/import/import-defer/errors/module-throws/third-party-evaluation-after-defer-import.js

nicolo-ribaudo · 2024-11-08T09:27:05Z

All fixed, thanks @takikawa!

...anguage/import/import-defer/evaluation-top-level-await/flattening-order/dep-2.2.1_FIXTURE.js

...age/import/import-defer/evaluation-top-level-await/flattening-order/dep-2.1.1-tla_FIXTURE.js

test/language/import/import-defer/errors/get-other-while-evaluating-async/dep-1-tla_FIXTURE.js

...anguage/import/import-defer/errors/get-other-while-dep-evaluating-async/dep-1-tla_FIXTURE.js

takikawa · 2024-11-09T01:13:54Z

Aside from the flattening-order test which might need more adjustment, I ran the tests in JSC and saw 7 failures. I think these are likely all due to three existing issues with JSC's module loader (doesn't re-throw after exception during eval, module namespace object properties aren't writable, and certain cyclic module graphs cause debug assertions to fail) that aren't related to import defer.

ptomato

Just minor comments this time.

@takikawa You mentioned that the flattening-order test needs more adjustments, can you elaborate? I'll approve when that's resolved.

ptomato · 2024-11-18T18:09:50Z

test/language/import/import-defer/deferred-namespace-object/exotic-object-behavior.js

+assert.sameValue(Reflect.setPrototypeOf(ns, null), true, "Deferred namespaces' prototype can be 'set' to null");
+
+assert.throws(TypeError, () => Reflect.apply(ns, null, []), "Deferred namespaces are not callable");
+assert.throws(TypeError, () => Reflect.construct(ns, null, []), "Deferred namespaces are not constructable");


Sorry I didn't notice this before:

Suggested change

assert.throws(TypeError, () => Reflect.construct(ns, null, []), "Deferred namespaces are not constructable");

assert.throws(TypeError, () => Reflect.construct(ns, [], ns), "Deferred namespaces are not constructable");

(or Reflect.construct(function () {}, [], ns) or use harness/isConstructor.js)

ptomato · 2024-11-18T18:49:45Z

test/language/import/import-defer/errors/module-throws/trigger-evaluation.js

+
+let err1, err2;
+try { ns.foo } catch (e) { err1 = e };
+assert.deepEqual(err1, { someError: "the error from throws_FIXTURE" }, "Evaluation errors are thrown when evaluating");


Could we instead write the test to avoid deepEqual? We are trying to avoid it in new tests, see #3476.
The thrown value could be a primitive, like "the error from throws_FIXTURE", since we are not checking for object identity anyway.

ptomato · 2024-11-18T18:52:36Z

test/language/import/import-defer/errors/get-self-while-evaluating-async/main.js

+assert(globalThis["error on ns.foo"] instanceof TypeError, "ns.foo while evaluating throws a TypeError");
+
+ns.foo;
+


I think it'd be helpful to spell this out, in the info section, but feel free to use your own judgement; it might already be obvious to someone who's implementing the proposal.

ptomato · 2024-11-18T18:55:17Z

test/language/import/import-defer/errors/resolution-error/import-defer-of-syntax-error-fails.js

I think this got resolved accidentally, the name still references SyntaxError.

takikawa · 2024-11-18T22:36:56Z

@takikawa You mentioned that the flattening-order test needs more adjustments, can you elaborate? I'll approve when that's resolved.

I think Nic made some fixes in 2e0982e which was what I was referring to. But I haven't run it since that modification, so I can take another look at it

nicolo-ribaudo · 2024-11-22T10:58:45Z

@ptomato handled all of your remaining comments

takikawa · 2024-11-22T21:23:47Z

I ran the flattening order test again, and these are the results I got

(formatted to make it easier to read)

[294! NEW FAIL test/language/import/import-defer/evaluation-top-level-await/flattening-order/main.js (module)
    Exception: Test262Error: 
      Actual [1, 2.1.1 start, 2.1.1 end, 2.2.1, 2.2 start, 2.2 end, 3, 4.1 start, 4.1 end, 5] and expected
             [1, 2.1.1 start, 2.2.1, 2.2 start, 3, 5, 2.1.1 end, 2.2 end, 4.1 start, 4.1 end] should have the same contents.

I suspect the differences may be due to how JSC's module loader is implemented (in particular, how it runs async dependencies here); basically the TLA doesn't seem to yield to the other imports. Not 100% sure though.

Edit: I originally said something about 5 being missing but I just didn't notice it, it's there but the ordering is different due to async yielding.

nicolo-ribaudo · 2024-11-26T15:47:57Z

@takikawa We indeed need to add 5 at the end. The position of the end logs is correct as-is, and it's a known JSC bug: https://bugs.webkit.org/show_bug.cgi?id=264400

nicolo-ribaudo requested review from a team as code owners October 25, 2024 16:34

nicolo-ribaudo force-pushed the import-defer branch from b38f80f to fda523d Compare October 25, 2024 16:41

nicolo-ribaudo force-pushed the import-defer branch from 620655a to 79c364c Compare October 28, 2024 17:24

Add tests for async cases

8ab7c25

ptomato reviewed Oct 30, 2024

View reviewed changes

nicolo-ribaudo added 4 commits October 31, 2024 12:21

Use better assertion functions

d5ea87e

Remove copy from filename

fe7a185

Fix wrong descriptions

fe5db3c

Do not use assert in _FIXTURE files

0e5df8b

Add missing $DONOTEVALUATE() calls

612c920

ptomato requested changes Nov 1, 2024

View reviewed changes

takikawa reviewed Nov 3, 2024

View reviewed changes

.../import/import-defer/evaluation-top-level-await/import-defer-transitive-async-module/main.js Outdated Show resolved Hide resolved

takikawa reviewed Nov 3, 2024

View reviewed changes

test/language/import/import-defer/evaluation-top-level-await/flattening-order/main.js Outdated Show resolved Hide resolved

nicolo-ribaudo added 2 commits November 7, 2024 16:28

Partial fixes for ptomato review

af3d19b

Rest of review by ptomato and asumu

acd2924

Lint

a873634

takikawa reviewed Nov 7, 2024

View reviewed changes

test/language/import/import-defer/evaluation-top-level-await/flattening-order/main.js Outdated Show resolved Hide resolved

takikawa reviewed Nov 7, 2024

View reviewed changes

test/language/import/import-defer/errors/module-throws/defer-import-after-evaluation.js Outdated Show resolved Hide resolved

takikawa reviewed Nov 7, 2024

View reviewed changes

...nguage/import/import-defer/errors/module-throws/third-party-evaluation-after-defer-import.js Outdated Show resolved Hide resolved

Typos and add missing include

75a6e1c

takikawa reviewed Nov 9, 2024

View reviewed changes

...anguage/import/import-defer/evaluation-top-level-await/flattening-order/dep-2.2.1_FIXTURE.js Outdated Show resolved Hide resolved

takikawa reviewed Nov 9, 2024

View reviewed changes

...age/import/import-defer/evaluation-top-level-await/flattening-order/dep-2.1.1-tla_FIXTURE.js Outdated Show resolved Hide resolved

takikawa reviewed Nov 9, 2024

View reviewed changes

test/language/import/import-defer/errors/get-other-while-evaluating-async/dep-1-tla_FIXTURE.js Show resolved Hide resolved

takikawa reviewed Nov 9, 2024

View reviewed changes

...anguage/import/import-defer/errors/get-other-while-dep-evaluating-async/dep-1-tla_FIXTURE.js Show resolved Hide resolved

More fixes

2e0982e

nicolo-ribaudo requested a review from ptomato November 11, 2024 09:50

ptomato reviewed Nov 18, 2024

View reviewed changes

Latest review

dd44fcb

nicolo-ribaudo force-pushed the import-defer branch from 45e8d4e to dd44fcb Compare November 22, 2024 11:00

Add missing 5 to flattening-order test

23b4781

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add partial tests for `import defer` #4278

Add partial tests for `import defer` #4278

nicolo-ribaudo commented Oct 25, 2024 •

edited

Loading

takikawa commented Oct 28, 2024

nicolo-ribaudo commented Oct 28, 2024 •

edited

Loading

ptomato left a comment

ptomato Oct 30, 2024

nicolo-ribaudo Oct 31, 2024

ptomato Oct 31, 2024

nicolo-ribaudo commented Oct 31, 2024

ptomato left a comment

ptomato Nov 1, 2024

nicolo-ribaudo Nov 7, 2024 •

edited

Loading

ptomato Nov 18, 2024

ptomato Nov 1, 2024

ptomato Nov 18, 2024

nicolo-ribaudo commented Nov 7, 2024

takikawa Nov 7, 2024

ptomato Nov 18, 2024

nicolo-ribaudo Nov 22, 2024

nicolo-ribaudo commented Nov 8, 2024

takikawa commented Nov 9, 2024

ptomato left a comment

ptomato Nov 18, 2024

ptomato Nov 18, 2024

ptomato Nov 18, 2024

ptomato Nov 18, 2024

takikawa commented Nov 18, 2024

nicolo-ribaudo commented Nov 22, 2024

takikawa commented Nov 22, 2024 •

edited

Loading

nicolo-ribaudo commented Nov 26, 2024

		assert(globalThis["error on ns.foo"] instanceof TypeError, "ns.foo while evaluating throws a TypeError");

		ns.foo;

	assert.throws(TypeError, () => Reflect.construct(ns, null, []), "Deferred namespaces are not constructable");
	assert.throws(TypeError, () => Reflect.construct(ns, [], ns), "Deferred namespaces are not constructable");

Add partial tests for import defer #4278

Are you sure you want to change the base?

Add partial tests for import defer #4278

Conversation

nicolo-ribaudo commented Oct 25, 2024 • edited Loading

takikawa commented Oct 28, 2024

nicolo-ribaudo commented Oct 28, 2024 • edited Loading

ptomato left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolo-ribaudo commented Oct 31, 2024

ptomato left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolo-ribaudo Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolo-ribaudo commented Nov 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolo-ribaudo commented Nov 8, 2024

takikawa commented Nov 9, 2024

ptomato left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takikawa commented Nov 18, 2024

nicolo-ribaudo commented Nov 22, 2024

takikawa commented Nov 22, 2024 • edited Loading

nicolo-ribaudo commented Nov 26, 2024

Add partial tests for `import defer` #4278

Add partial tests for `import defer` #4278

nicolo-ribaudo commented Oct 25, 2024 •

edited

Loading

nicolo-ribaudo commented Oct 28, 2024 •

edited

Loading

nicolo-ribaudo Nov 7, 2024 •

edited

Loading

takikawa commented Nov 22, 2024 •

edited

Loading