Initial tests for Dynamic Imports #1588

rwaldron · 2018-06-05T18:20:05Z

~~This is not ready for in depth review yet~~

dnalborczyk · 2018-08-21T15:47:06Z

hey guys, sorry for hijacking this PR.

we're trying to make esm https://github.com/standard-things/esm (a node module supporting ES modules, dynamic import, and also a bridge to commonjs) ECMA-262 compliant as much as possible, and we are running a selection of test262 tests, mainly everything from module-code, import and export.

we also started running against this branch/PR to get some test coverage for dynamic import, although we know it might be not in it's final stages, and hasn't been reviewed yet.

there are a couple of tests involving import(''), and I was wondering how the result should be evaluated? meaning, what determines the test to be successful? since browsers, and node --experimental-modules, as well as esm are returning a rejected Promise not being able to find a module.

for example: module-code/dynamic-import/syntax-nested-arrow-assignment-expression-empty-str-arg.js

any hints or pointers are much appreciated. thank you in advance!

ljharb · 2018-08-21T15:57:16Z

Since it’s a URL, there’s no reason that an empty string necessarily won’t resolve to a module.

leobalter · 2018-08-21T16:05:34Z

Rick is out until September and I didn't have the chance to review this PR yet. I'm not sure at what point this PR is still a WIP with parts not done yet and I'm afraid I could be over reviewing things Rick already has plans to change.

My current work efforts are based on class fields related stuff but I hope we can be back to this PR anytime.

dnalborczyk · 2018-08-21T16:22:54Z

@ljharb thank you, good point. I 'm not sure about the intricacies of dynamic imports, but I thought the test-262 suite is conclusive, and would contain everything needed to run a test. meaning, either I would assume a flag as some indicator, or some harness helper method, or the expected '' module (although I'm not sure how this can be accomplished).

@leobalter makes sense, no worries! it can wait, there's no rush. thank you for getting back though!

Meanwhile I might just blacklist those tests for the time being, or find another temporary workaround, e.g. parsing for import(''), and expect a rejected Promise.

ljharb · 2018-08-21T16:35:29Z

Module loading is engine-specific and it’d be very hard to cover this fully with test262.

rwaldron · 2018-09-10T16:16:25Z

Module loading is engine-specific and it’d be very hard to cover this fully with test262.

Presently we use engine-specific machinery, provided via eshost runtime definitions, to ensure that module loading operations occur within the scope of each host's capabilities. This is how both module code and dynamic import test material is executed.

leobalter · 2018-10-02T14:50:53Z

I just rebased the branch to verify the CI again. No other changes introduced here so far.

rwaldron · 2018-10-02T18:02:58Z

@dnalborczyk I think your question is more appropriate for the proposal/authors.

Meanwhile I might just blacklist those tests for the time being, or find another temporary workaround, e.g. parsing for import(''), and expect a rejected Promise.

"parsing for import('')", and evaluating what it returns are two very distinct aspects of the feature, which occur during different phases of a JS program's lifetime.

…xtures

leobalter · 2018-10-03T22:16:14Z

I finally fixed the duplicate tests, they should pass on the CI process now.

git grep -h -E 'path\: (.*)' src/dynamic-import/ | sort | uniq -d should be helpful as a future reference

jdalton · 2018-10-03T23:45:27Z

@leobalter The file module-code/dynamic-import/catch/nested-arrow-import-catch-instn-iee-err-ambiguous-import.js is trying to import a missing file (maybe a path mixup)

leobalter · 2018-10-04T00:45:17Z

Thanks for catching this up. This is still WIP but you definitely caught some commit mixup. I’ll restore this file tomorrow.

Please, Let me know if you find anything else from the tests. Looking forward to complete this coverage.

jdalton · 2018-10-04T00:52:41Z

Np. There's a couple more to investigate for missing files:

test/language/module-code/dynamic-import/catch/nested-async-function-await-instn-iee-err-ambiguous-import.js
test/language/module-code/dynamic-import/catch/nested-async-function-instn-iee-err-ambiguous-import.js
test/language/module-code/dynamic-import/catch/nested-async-function-return-await-instn-iee-err-ambiguous-import.js
test/language/module-code/dynamic-import/catch/nested-block-import-catch-instn-iee-err-ambiguous-import.js
test/language/module-code/dynamic-import/catch/nested-block-labeled-instn-iee-err-ambiguous-import.js
test/language/module-code/dynamic-import/catch/nested-do-while-instn-iee-err-ambiguous-import.js
test/language/module-code/dynamic-import/catch/nested-else-import-catch-instn-iee-err-ambiguous-import.js
test/language/module-code/dynamic-import/catch/nested-function-import-catch-instn-iee-err-ambiguous-import.js
test/language/module-code/dynamic-import/catch/nested-if-import-catch-instn-iee-err-ambiguous-import.js
test/language/module-code/dynamic-import/catch/nested-while-import-catch-instn-iee-err-ambiguous-import.js
test/language/module-code/dynamic-import/catch/top-level-import-catch-instn-iee-err-ambiguous-import.js

leobalter · 2018-10-04T18:37:19Z

@jdalton this should be fixed for now

leobalter · 2018-10-04T20:05:22Z

ok, with the latest commits I'm able to run all the tests just OK w/ JSC and V8.

[~/dev/test262] dynamic-import
test262-harness --hostType=jsc --hostPath=`which jsc` -- 'test/language/module-code/dynamic-import/**/*'
Ran 272 tests
272 passed
0 failed
[~/dev/test262] dynamic-import
test262-harness --hostType=d8 --hostPath=`which v8` -- 'test/language/module-code/dynamic-import/**/*'
Ran 272 tests
272 passed
0 failed

leobalter · 2018-10-04T20:08:12Z

I'm also migrating the checklist to #1164

leobalter · 2018-10-04T20:10:52Z

Everything should be all set for an initial merge and get feedback from implementations while we continue working on the missing coverage found (see #1164).

@rwaldron ok to land this from your side?

rwaldron · 2018-10-04T20:16:25Z

Just a note to test262-harness users: you MUST be upgraded to >=test262-harness@4.5.0 for these tests.

jdalton · 2018-10-04T20:21:35Z

test/language/module-code/dynamic-import/eval-self-once-script.js

+  await import('./eval-self-once-script.js');
+
+  assert.sameValue(global.evaluated, 2, 'global property was defined and incremented only once');
+}).then($DONE, $DONE);


☝️ The global.evaluated value should be 1 and not 2.

Update:

Is this a dup of test/language/module-code/dynamic-import/eval-self-once-module.js?

This is intended to be this way. It's a very similar test but it is evaluated as a script and it loads itself again following this:

Success path The completion value of any subsequent call to HostResolveImportedModule after FinishDynamicImport has completed, given the arguments referencingScriptOrModule and specifier, must be a module which has already been evaluated, i.e. whose Evaluate concrete method has already been called and returned a normal completion.

you can find this normative text here: https://tc39.github.io/proposal-dynamic-import/#sec-hostimportmoduledynamically

a script file loading it self ends up being evaluated twice. The first time it was not evaluated as a Module Record or using HostResolveImportedModule. This is tricky.

Also, the info in the metadata says the file is meant to run as a script code (strict or non strict) and not as a module code, which has a different evaluation.

The description of the assert should probably be changed

'global property was defined and incremented only once'

yes, thanks!

@jdalton thanks for providing such quick feedback!

jdalton · 2018-10-04T20:24:46Z

test/language/module-code/dynamic-import/eval-self-once-module.js

 includes: [fnGlobalObject.js]
-flags: [async]
+flags: [async, module]


~~While this test was renamed the resource it's loading wasn't (in source of eval-self-once-module).~~

Update:

Fixed in another commit.

Sorry for that, I left the changes in the editor without saving to the file and pushed the other commit.

jdalton · 2018-10-04T21:29:39Z

...ge/module-code/dynamic-import/usage/nested-if-braceless-eval-gtbndng-indirect-update-dflt.js

+
+---*/
+
+if (true) import('./eval-gtbndng-indirect-update-dflt_FIXTURE.js');


☝️ Based on INTERPRETING async flagged tests should print 'Test262:AsyncTestComplete' or 'Test262:AsyncTestFailure: ' + reason by way of doneprintHandle. Without printing a complete or fail the interpretation can hit a test timeout and mark it as a test failure.

Tests affected:

test/language/module-code/dynamic-import/usage/nested-if-braceless-eval-gtbndng-indirect-update-dflt.js test/language/module-code/dynamic-import/usage/nested-if-braceless-eval-gtbndng-indirect-update.js test/language/module-code/dynamic-import/usage/nested-if-braceless-returns-promise.js

Nice catch! I think these are all from my original batch of generated tests, which definitely need work. Please keep the feedback and review coming <3

rwaldron force-pushed the dynamic-import branch 3 times, most recently from 80ab186 to 7ce4958 Compare June 5, 2018 19:47

rwaldron force-pushed the dynamic-import branch from 7ce4958 to 1d49c09 Compare June 20, 2018 16:43

leobalter self-requested a review June 28, 2018 16:42

leobalter self-assigned this Jun 28, 2018

This was referenced Aug 4, 2018

tc39/test262 standard-things/esm#543

Closed

WIP: initial attempt to run 'esm' against tc39/test262 standard-things/esm#550

Merged

rwaldron force-pushed the dynamic-import branch 3 times, most recently from e68c51d to 269908b Compare August 10, 2018 19:15

rwaldron changed the title ~~WIP: Dynamic Imports: initial cases, templates, non-generated tests and fixtures~~ WIP: Dynamic Imports Sep 12, 2018

leobalter force-pushed the dynamic-import branch from 269908b to 0a9a037 Compare October 2, 2018 14:50

rwaldron added 4 commits October 2, 2018 14:17

Dynamic Imports: initial cases, templates, non-generated tests and fi…

613e448

…xtures

Generate tests

df987b9

Dynamic Imports: eval export default class imports

f0d9b94

Dynamic Imports: remove [module] flag

4b90333

rwaldron force-pushed the dynamic-import branch from 0a9a037 to 4b90333 Compare October 2, 2018 18:17

leobalter added 4 commits October 2, 2018 14:19

split valid and invalid syntax templates

66c691b

remove duplicated

554aab2

Improve case clarify for syntax templates

1fb7bde

move flags to templates

bde5462

leobalter added 7 commits October 3, 2018 14:41

Regenerate tests

b49de44

remove duplicates, fix fixture folder

965e282

fix fixtures folders and nit picking on tests

98b2962

Checklist + quick template renaming

e3f3d77

Regenerate tests

cfc2278

Fix duplicate template paths

8de1ba9

Regenerate tests

e067be1

leobalter added 2 commits October 4, 2018 14:35

Recover Fixture files

5b87de7

Generate tests

33d6c2c

leobalter added 2 commits October 4, 2018 15:56

fix test in module/script modes

ff8cc74

missing fixup

1da2f2d

leobalter changed the title ~~WIP: Dynamic Imports~~ Initial tests for Dynamic Imports Oct 4, 2018

Remove checklist, now at #1164

377f07f

leobalter approved these changes Oct 4, 2018

View reviewed changes

rwaldron merged commit 8e8a593 into master Oct 4, 2018

leobalter deleted the dynamic-import branch October 4, 2018 20:16

jdalton reviewed Oct 4, 2018

View reviewed changes

jdalton mentioned this pull request Oct 4, 2018

Leftover bugs in dynamic-import tests #1826

Closed

jdalton reviewed Oct 4, 2018

View reviewed changes

jdalton mentioned this pull request Oct 5, 2018

Several dynamic-import tests flagged as async don't call $DONE #1829

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial tests for Dynamic Imports #1588

Initial tests for Dynamic Imports #1588

rwaldron commented Jun 5, 2018 •

edited by leobalter

dnalborczyk commented Aug 21, 2018 •

edited

ljharb commented Aug 21, 2018

leobalter commented Aug 21, 2018

dnalborczyk commented Aug 21, 2018

ljharb commented Aug 21, 2018

rwaldron commented Sep 10, 2018

leobalter commented Oct 2, 2018

rwaldron commented Oct 2, 2018

leobalter commented Oct 3, 2018 •

edited

jdalton commented Oct 3, 2018 •

edited

leobalter commented Oct 4, 2018

jdalton commented Oct 4, 2018 •

edited

leobalter commented Oct 4, 2018

leobalter commented Oct 4, 2018

leobalter commented Oct 4, 2018

leobalter commented Oct 4, 2018

rwaldron commented Oct 4, 2018

jdalton Oct 4, 2018 •

edited

leobalter Oct 4, 2018

leobalter Oct 4, 2018

jdalton Oct 4, 2018

leobalter Oct 4, 2018

rwaldron Oct 5, 2018

jdalton Oct 4, 2018 •

edited

leobalter Oct 4, 2018

jdalton Oct 4, 2018 •

edited

rwaldron Oct 5, 2018


		---*/

		if (true) import('./eval-gtbndng-indirect-update-dflt_FIXTURE.js');

Initial tests for Dynamic Imports #1588

Initial tests for Dynamic Imports #1588

Conversation

rwaldron commented Jun 5, 2018 • edited by leobalter

dnalborczyk commented Aug 21, 2018 • edited

ljharb commented Aug 21, 2018

leobalter commented Aug 21, 2018

dnalborczyk commented Aug 21, 2018

ljharb commented Aug 21, 2018

rwaldron commented Sep 10, 2018

leobalter commented Oct 2, 2018

rwaldron commented Oct 2, 2018

leobalter commented Oct 3, 2018 • edited

jdalton commented Oct 3, 2018 • edited

leobalter commented Oct 4, 2018

jdalton commented Oct 4, 2018 • edited

leobalter commented Oct 4, 2018

leobalter commented Oct 4, 2018

leobalter commented Oct 4, 2018

leobalter commented Oct 4, 2018

rwaldron commented Oct 4, 2018

jdalton Oct 4, 2018 • edited

Choose a reason for hiding this comment

leobalter Oct 4, 2018

Choose a reason for hiding this comment

leobalter Oct 4, 2018

Choose a reason for hiding this comment

jdalton Oct 4, 2018

Choose a reason for hiding this comment

leobalter Oct 4, 2018

Choose a reason for hiding this comment

rwaldron Oct 5, 2018

Choose a reason for hiding this comment

jdalton Oct 4, 2018 • edited

Choose a reason for hiding this comment

leobalter Oct 4, 2018

Choose a reason for hiding this comment

jdalton Oct 4, 2018 • edited

Choose a reason for hiding this comment

rwaldron Oct 5, 2018

Choose a reason for hiding this comment

rwaldron commented Jun 5, 2018 •

edited by leobalter

dnalborczyk commented Aug 21, 2018 •

edited

leobalter commented Oct 3, 2018 •

edited

jdalton commented Oct 3, 2018 •

edited

jdalton commented Oct 4, 2018 •

edited

jdalton Oct 4, 2018 •

edited

jdalton Oct 4, 2018 •

edited

jdalton Oct 4, 2018 •

edited