test: add a simple test for import defer of a CJS module

[MayaLekova]

test: add a simple test for import defer of a CJS module

This tests imports a CommonJS modules with the defer modifier.
It ensures that the imported module is not evaluated before
accessing properties form its exports.

[MayaLekova]

@nicolo-ribaudo and @joyeecheung please take a look, thanks!

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.31%. Comparing base (4bec191) to head (2595c51).
⚠️ Report is 383 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #64694 +/- ##
==========================================
+ Coverage 90.13% 90.31% +0.18% 
==========================================
Files 741 760 +19 Lines 242106 248525 +6419 Branches 45551 46892 +1341 ==========================================
+ Hits 218211 224459 +6248 - Misses 15430 15507 +77 - Partials 8465 8559 +94 

see 224 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[nicolo-ribaudo]

It would be better to swap these two statements, so that when reading somebody does not mistakenly assume that globalThis.eval_list = [] would be evaluated first (imports are hoisted and executed before code in the file).

This test right now would pass regardless of whether import defer is eager or deferred. If it's eager it passes because:

• after the eager ../fixtures/es-modules/module-cjs-deferred-eval.js evaluation, this file sets globalThis.eval_list = [];. Then it asserts that its length is 0, which is obviously true because we just set it to an empty array.
• further down, in assert.partialDeepStrictEqual(['defer-1'], globalThis.eval_list);, we are checking that ['defer-1'] contains all the elements of globalThis.eval_list. If globalThis.eval_list is empty, that's tautologically true.

I have not run this test, but I assume the reason in test/fixtures/es-modules/module-cjs-deferred-eval.js you had to add if (!globalThis.eval_list) { globalThis.eval_list = []; } is because the CJS module is indeed not being deferred, so it's running before that this file defines globalThis.eval_list = [].

Maybe better, either one of this:

• move the globalThis.eval_list = [] initialization to a separate file that is imported at the beginning of this one, before the other imports, so that we know it runs first
• use a boolean instead of an array, and then we don't need to pre-initialize it.

[MayaLekova]

Thanks for the explanation! I was trying to finish the test in a rush and didn't test properly.
The imported module doesn't need to initialize the array at all, so I just moved it to the main module. Having eval_list as an array makes it easier to check the imported module is executed only once (using deepStrictEqual as Joyee suggested), so I'm leaving it as an array.

[nicolo-ribaudo]

It'd be great to also have an assertion about imported.foo and imported.identifier, to make sure that import defer works with the cjs exports detection (like Joyee suggested in #64694 (comment))

[MayaLekova]

Thanks, I missed the part that foo and identifier are read directly from the exports, rather than the .default object (is it an exports default object?).

Uploaded a new patch, please take another look.

[nicolo-ribaudo]

Looks good 👍

[joyeecheung]

LGTM % comment suggestions

[joyeecheung]

Suggested change

	// Check that the exported properties are accessible and have the
	// expected values.

This is a bit self-evident.

[MayaLekova]

Okay, I tried rephrazing it, so that it carries more meaning.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75742/

[MayaLekova]

Hi @joyeecheung , could you re-run the CI, as I don't have permissions? I think the Mac failure is a flake.

[nodejs-github-bot]

Landed in f83e7df

[MayaLekova]

test: add a simple test for import defer of a CJS module

This tests imports a CommonJS modules with the defer modifier.
It ensures that the imported module is not evaluated before
accessing properties form its exports.

[MayaLekova]

@nicolo-ribaudo and @joyeecheung please take a look, thanks!

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.31%. Comparing base (4bec191) to head (2595c51).
⚠️ Report is 383 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #64694 +/- ##
==========================================
+ Coverage 90.13% 90.31% +0.18% 
==========================================
Files 741 760 +19 Lines 242106 248525 +6419 Branches 45551 46892 +1341 ==========================================
+ Hits 218211 224459 +6248 - Misses 15430 15507 +77 - Partials 8465 8559 +94 

see 224 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[nicolo-ribaudo]

It would be better to swap these two statements, so that when reading somebody does not mistakenly assume that globalThis.eval_list = [] would be evaluated first (imports are hoisted and executed before code in the file).

This test right now would pass regardless of whether import defer is eager or deferred. If it's eager it passes because:

• after the eager ../fixtures/es-modules/module-cjs-deferred-eval.js evaluation, this file sets globalThis.eval_list = [];. Then it asserts that its length is 0, which is obviously true because we just set it to an empty array.
• further down, in assert.partialDeepStrictEqual(['defer-1'], globalThis.eval_list);, we are checking that ['defer-1'] contains all the elements of globalThis.eval_list. If globalThis.eval_list is empty, that's tautologically true.

I have not run this test, but I assume the reason in test/fixtures/es-modules/module-cjs-deferred-eval.js you had to add if (!globalThis.eval_list) { globalThis.eval_list = []; } is because the CJS module is indeed not being deferred, so it's running before that this file defines globalThis.eval_list = [].

Maybe better, either one of this:

• move the globalThis.eval_list = [] initialization to a separate file that is imported at the beginning of this one, before the other imports, so that we know it runs first
• use a boolean instead of an array, and then we don't need to pre-initialize it.

[MayaLekova]

Thanks for the explanation! I was trying to finish the test in a rush and didn't test properly.
The imported module doesn't need to initialize the array at all, so I just moved it to the main module. Having eval_list as an array makes it easier to check the imported module is executed only once (using deepStrictEqual as Joyee suggested), so I'm leaving it as an array.

[nicolo-ribaudo]

It'd be great to also have an assertion about imported.foo and imported.identifier, to make sure that import defer works with the cjs exports detection (like Joyee suggested in #64694 (comment))

[MayaLekova]

Thanks, I missed the part that foo and identifier are read directly from the exports, rather than the .default object (is it an exports default object?).

Uploaded a new patch, please take another look.

[nicolo-ribaudo]

Looks good 👍

[joyeecheung]

LGTM % comment suggestions

[joyeecheung]

Suggested change

	// Check that the exported properties are accessible and have the
	// expected values.

This is a bit self-evident.

[MayaLekova]

Okay, I tried rephrazing it, so that it carries more meaning.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75742/

[MayaLekova]

Hi @joyeecheung , could you re-run the CI, as I don't have permissions? I think the Mac failure is a flake.

[nodejs-github-bot]

Landed in f83e7df

[MayaLekova]

test: add a simple test for import defer of a CJS module

This tests imports a CommonJS modules with the defer modifier.
It ensures that the imported module is not evaluated before
accessing properties form its exports.

[MayaLekova]

@nicolo-ribaudo and @joyeecheung please take a look, thanks!

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.31%. Comparing base (4bec191) to head (2595c51).
⚠️ Report is 383 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #64694 +/- ##
==========================================
+ Coverage 90.13% 90.31% +0.18% 
==========================================
Files 741 760 +19 Lines 242106 248525 +6419 Branches 45551 46892 +1341 ==========================================
+ Hits 218211 224459 +6248 - Misses 15430 15507 +77 - Partials 8465 8559 +94 

see 224 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[nicolo-ribaudo]

It would be better to swap these two statements, so that when reading somebody does not mistakenly assume that globalThis.eval_list = [] would be evaluated first (imports are hoisted and executed before code in the file).

This test right now would pass regardless of whether import defer is eager or deferred. If it's eager it passes because:

• after the eager ../fixtures/es-modules/module-cjs-deferred-eval.js evaluation, this file sets globalThis.eval_list = [];. Then it asserts that its length is 0, which is obviously true because we just set it to an empty array.
• further down, in assert.partialDeepStrictEqual(['defer-1'], globalThis.eval_list);, we are checking that ['defer-1'] contains all the elements of globalThis.eval_list. If globalThis.eval_list is empty, that's tautologically true.

I have not run this test, but I assume the reason in test/fixtures/es-modules/module-cjs-deferred-eval.js you had to add if (!globalThis.eval_list) { globalThis.eval_list = []; } is because the CJS module is indeed not being deferred, so it's running before that this file defines globalThis.eval_list = [].

Maybe better, either one of this:

• move the globalThis.eval_list = [] initialization to a separate file that is imported at the beginning of this one, before the other imports, so that we know it runs first
• use a boolean instead of an array, and then we don't need to pre-initialize it.

[MayaLekova]

Thanks for the explanation! I was trying to finish the test in a rush and didn't test properly.
The imported module doesn't need to initialize the array at all, so I just moved it to the main module. Having eval_list as an array makes it easier to check the imported module is executed only once (using deepStrictEqual as Joyee suggested), so I'm leaving it as an array.

[nicolo-ribaudo]

It'd be great to also have an assertion about imported.foo and imported.identifier, to make sure that import defer works with the cjs exports detection (like Joyee suggested in #64694 (comment))

[MayaLekova]

Thanks, I missed the part that foo and identifier are read directly from the exports, rather than the .default object (is it an exports default object?).

Uploaded a new patch, please take another look.

[nicolo-ribaudo]

Looks good 👍

[joyeecheung]

LGTM % comment suggestions

[joyeecheung]

Suggested change

	// Check that the exported properties are accessible and have the
	// expected values.

This is a bit self-evident.

[MayaLekova]

Okay, I tried rephrazing it, so that it carries more meaning.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75742/

[MayaLekova]

Hi @joyeecheung , could you re-run the CI, as I don't have permissions? I think the Mac failure is a flake.

[nodejs-github-bot]

Landed in f83e7df

[MayaLekova]

test: add a simple test for import defer of a CJS module

This tests imports a CommonJS modules with the defer modifier.
It ensures that the imported module is not evaluated before
accessing properties form its exports.

[MayaLekova]

@nicolo-ribaudo and @joyeecheung please take a look, thanks!

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.31%. Comparing base (4bec191) to head (2595c51).
⚠️ Report is 383 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #64694 +/- ##
==========================================
+ Coverage 90.13% 90.31% +0.18% 
==========================================
Files 741 760 +19 Lines 242106 248525 +6419 Branches 45551 46892 +1341 ==========================================
+ Hits 218211 224459 +6248 - Misses 15430 15507 +77 - Partials 8465 8559 +94 

see 224 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[nicolo-ribaudo]

It would be better to swap these two statements, so that when reading somebody does not mistakenly assume that globalThis.eval_list = [] would be evaluated first (imports are hoisted and executed before code in the file).

This test right now would pass regardless of whether import defer is eager or deferred. If it's eager it passes because:

• after the eager ../fixtures/es-modules/module-cjs-deferred-eval.js evaluation, this file sets globalThis.eval_list = [];. Then it asserts that its length is 0, which is obviously true because we just set it to an empty array.
• further down, in assert.partialDeepStrictEqual(['defer-1'], globalThis.eval_list);, we are checking that ['defer-1'] contains all the elements of globalThis.eval_list. If globalThis.eval_list is empty, that's tautologically true.

I have not run this test, but I assume the reason in test/fixtures/es-modules/module-cjs-deferred-eval.js you had to add if (!globalThis.eval_list) { globalThis.eval_list = []; } is because the CJS module is indeed not being deferred, so it's running before that this file defines globalThis.eval_list = [].

Maybe better, either one of this:

• move the globalThis.eval_list = [] initialization to a separate file that is imported at the beginning of this one, before the other imports, so that we know it runs first
• use a boolean instead of an array, and then we don't need to pre-initialize it.

[MayaLekova]

Thanks for the explanation! I was trying to finish the test in a rush and didn't test properly.
The imported module doesn't need to initialize the array at all, so I just moved it to the main module. Having eval_list as an array makes it easier to check the imported module is executed only once (using deepStrictEqual as Joyee suggested), so I'm leaving it as an array.

[nicolo-ribaudo]

It'd be great to also have an assertion about imported.foo and imported.identifier, to make sure that import defer works with the cjs exports detection (like Joyee suggested in #64694 (comment))

[MayaLekova]

Thanks, I missed the part that foo and identifier are read directly from the exports, rather than the .default object (is it an exports default object?).

Uploaded a new patch, please take another look.

[nicolo-ribaudo]

Looks good 👍

[joyeecheung]

LGTM % comment suggestions

[joyeecheung]

Suggested change

	// Check that the exported properties are accessible and have the
	// expected values.

This is a bit self-evident.

[MayaLekova]

Okay, I tried rephrazing it, so that it carries more meaning.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75742/

[MayaLekova]

Hi @joyeecheung , could you re-run the CI, as I don't have permissions? I think the Mac failure is a flake.

[nodejs-github-bot]

Landed in f83e7df

[MayaLekova]

test: add a simple test for import defer of a CJS module

This tests imports a CommonJS modules with the defer modifier.
It ensures that the imported module is not evaluated before
accessing properties form its exports.

[MayaLekova]

@nicolo-ribaudo and @joyeecheung please take a look, thanks!

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.31%. Comparing base (4bec191) to head (2595c51).
⚠️ Report is 383 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #64694 +/- ##
==========================================
+ Coverage 90.13% 90.31% +0.18% 
==========================================
Files 741 760 +19 Lines 242106 248525 +6419 Branches 45551 46892 +1341 ==========================================
+ Hits 218211 224459 +6248 - Misses 15430 15507 +77 - Partials 8465 8559 +94 

see 224 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[nicolo-ribaudo]

It would be better to swap these two statements, so that when reading somebody does not mistakenly assume that globalThis.eval_list = [] would be evaluated first (imports are hoisted and executed before code in the file).

This test right now would pass regardless of whether import defer is eager or deferred. If it's eager it passes because:

• after the eager ../fixtures/es-modules/module-cjs-deferred-eval.js evaluation, this file sets globalThis.eval_list = [];. Then it asserts that its length is 0, which is obviously true because we just set it to an empty array.
• further down, in assert.partialDeepStrictEqual(['defer-1'], globalThis.eval_list);, we are checking that ['defer-1'] contains all the elements of globalThis.eval_list. If globalThis.eval_list is empty, that's tautologically true.

I have not run this test, but I assume the reason in test/fixtures/es-modules/module-cjs-deferred-eval.js you had to add if (!globalThis.eval_list) { globalThis.eval_list = []; } is because the CJS module is indeed not being deferred, so it's running before that this file defines globalThis.eval_list = [].

Maybe better, either one of this:

• move the globalThis.eval_list = [] initialization to a separate file that is imported at the beginning of this one, before the other imports, so that we know it runs first
• use a boolean instead of an array, and then we don't need to pre-initialize it.

[MayaLekova]

Thanks for the explanation! I was trying to finish the test in a rush and didn't test properly.
The imported module doesn't need to initialize the array at all, so I just moved it to the main module. Having eval_list as an array makes it easier to check the imported module is executed only once (using deepStrictEqual as Joyee suggested), so I'm leaving it as an array.

[nicolo-ribaudo]

It'd be great to also have an assertion about imported.foo and imported.identifier, to make sure that import defer works with the cjs exports detection (like Joyee suggested in #64694 (comment))

[MayaLekova]

Thanks, I missed the part that foo and identifier are read directly from the exports, rather than the .default object (is it an exports default object?).

Uploaded a new patch, please take another look.

[nicolo-ribaudo]

Looks good 👍

[joyeecheung]

LGTM % comment suggestions

[joyeecheung]

Suggested change

	// Check that the exported properties are accessible and have the
	// expected values.

This is a bit self-evident.

[MayaLekova]

Okay, I tried rephrazing it, so that it carries more meaning.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75742/

[MayaLekova]

Hi @joyeecheung , could you re-run the CI, as I don't have permissions? I think the Mac failure is a flake.

[nodejs-github-bot]

Landed in f83e7df

[MayaLekova]

test: add a simple test for import defer of a CJS module

This tests imports a CommonJS modules with the defer modifier.
It ensures that the imported module is not evaluated before
accessing properties form its exports.

[MayaLekova]

@nicolo-ribaudo and @joyeecheung please take a look, thanks!

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.31%. Comparing base (4bec191) to head (2595c51).
⚠️ Report is 383 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #64694 +/- ##
==========================================
+ Coverage 90.13% 90.31% +0.18% 
==========================================
Files 741 760 +19 Lines 242106 248525 +6419 Branches 45551 46892 +1341 ==========================================
+ Hits 218211 224459 +6248 - Misses 15430 15507 +77 - Partials 8465 8559 +94 

see 224 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[nicolo-ribaudo]

It would be better to swap these two statements, so that when reading somebody does not mistakenly assume that globalThis.eval_list = [] would be evaluated first (imports are hoisted and executed before code in the file).

This test right now would pass regardless of whether import defer is eager or deferred. If it's eager it passes because:

• after the eager ../fixtures/es-modules/module-cjs-deferred-eval.js evaluation, this file sets globalThis.eval_list = [];. Then it asserts that its length is 0, which is obviously true because we just set it to an empty array.
• further down, in assert.partialDeepStrictEqual(['defer-1'], globalThis.eval_list);, we are checking that ['defer-1'] contains all the elements of globalThis.eval_list. If globalThis.eval_list is empty, that's tautologically true.

I have not run this test, but I assume the reason in test/fixtures/es-modules/module-cjs-deferred-eval.js you had to add if (!globalThis.eval_list) { globalThis.eval_list = []; } is because the CJS module is indeed not being deferred, so it's running before that this file defines globalThis.eval_list = [].

Maybe better, either one of this:

• move the globalThis.eval_list = [] initialization to a separate file that is imported at the beginning of this one, before the other imports, so that we know it runs first
• use a boolean instead of an array, and then we don't need to pre-initialize it.

[MayaLekova]

Thanks for the explanation! I was trying to finish the test in a rush and didn't test properly.
The imported module doesn't need to initialize the array at all, so I just moved it to the main module. Having eval_list as an array makes it easier to check the imported module is executed only once (using deepStrictEqual as Joyee suggested), so I'm leaving it as an array.

[nicolo-ribaudo]

It'd be great to also have an assertion about imported.foo and imported.identifier, to make sure that import defer works with the cjs exports detection (like Joyee suggested in #64694 (comment))

[MayaLekova]

Thanks, I missed the part that foo and identifier are read directly from the exports, rather than the .default object (is it an exports default object?).

Uploaded a new patch, please take another look.

[nicolo-ribaudo]

Looks good 👍

[joyeecheung]

LGTM % comment suggestions

[joyeecheung]

Suggested change

	// Check that the exported properties are accessible and have the
	// expected values.

This is a bit self-evident.

[MayaLekova]

Okay, I tried rephrazing it, so that it carries more meaning.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75742/

[MayaLekova]

Hi @joyeecheung , could you re-run the CI, as I don't have permissions? I think the Mac failure is a flake.

[nodejs-github-bot]

Landed in f83e7df

[MayaLekova]

test: add a simple test for import defer of a CJS module

This tests imports a CommonJS modules with the defer modifier.
It ensures that the imported module is not evaluated before
accessing properties form its exports.

[MayaLekova]

@nicolo-ribaudo and @joyeecheung please take a look, thanks!

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.31%. Comparing base (4bec191) to head (2595c51).
⚠️ Report is 383 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #64694 +/- ##
==========================================
+ Coverage 90.13% 90.31% +0.18% 
==========================================
Files 741 760 +19 Lines 242106 248525 +6419 Branches 45551 46892 +1341 ==========================================
+ Hits 218211 224459 +6248 - Misses 15430 15507 +77 - Partials 8465 8559 +94 

see 224 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[nicolo-ribaudo]

It would be better to swap these two statements, so that when reading somebody does not mistakenly assume that globalThis.eval_list = [] would be evaluated first (imports are hoisted and executed before code in the file).

This test right now would pass regardless of whether import defer is eager or deferred. If it's eager it passes because:

• after the eager ../fixtures/es-modules/module-cjs-deferred-eval.js evaluation, this file sets globalThis.eval_list = [];. Then it asserts that its length is 0, which is obviously true because we just set it to an empty array.
• further down, in assert.partialDeepStrictEqual(['defer-1'], globalThis.eval_list);, we are checking that ['defer-1'] contains all the elements of globalThis.eval_list. If globalThis.eval_list is empty, that's tautologically true.

I have not run this test, but I assume the reason in test/fixtures/es-modules/module-cjs-deferred-eval.js you had to add if (!globalThis.eval_list) { globalThis.eval_list = []; } is because the CJS module is indeed not being deferred, so it's running before that this file defines globalThis.eval_list = [].

Maybe better, either one of this:

• move the globalThis.eval_list = [] initialization to a separate file that is imported at the beginning of this one, before the other imports, so that we know it runs first
• use a boolean instead of an array, and then we don't need to pre-initialize it.

[MayaLekova]

Thanks for the explanation! I was trying to finish the test in a rush and didn't test properly.
The imported module doesn't need to initialize the array at all, so I just moved it to the main module. Having eval_list as an array makes it easier to check the imported module is executed only once (using deepStrictEqual as Joyee suggested), so I'm leaving it as an array.

[nicolo-ribaudo]

It'd be great to also have an assertion about imported.foo and imported.identifier, to make sure that import defer works with the cjs exports detection (like Joyee suggested in #64694 (comment))

[MayaLekova]

Thanks, I missed the part that foo and identifier are read directly from the exports, rather than the .default object (is it an exports default object?).

Uploaded a new patch, please take another look.

[nicolo-ribaudo]

Looks good 👍

[joyeecheung]

LGTM % comment suggestions

[joyeecheung]

Suggested change

	// Check that the exported properties are accessible and have the
	// expected values.

This is a bit self-evident.

[MayaLekova]

Okay, I tried rephrazing it, so that it carries more meaning.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75742/

[MayaLekova]

Hi @joyeecheung , could you re-run the CI, as I don't have permissions? I think the Mac failure is a flake.

[nodejs-github-bot]

Landed in f83e7df

[MayaLekova]

test: add a simple test for import defer of a CJS module

This tests imports a CommonJS modules with the defer modifier.
It ensures that the imported module is not evaluated before
accessing properties form its exports.

[MayaLekova]

@nicolo-ribaudo and @joyeecheung please take a look, thanks!

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.31%. Comparing base (4bec191) to head (2595c51).
⚠️ Report is 383 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #64694 +/- ##
==========================================
+ Coverage 90.13% 90.31% +0.18% 
==========================================
Files 741 760 +19 Lines 242106 248525 +6419 Branches 45551 46892 +1341 ==========================================
+ Hits 218211 224459 +6248 - Misses 15430 15507 +77 - Partials 8465 8559 +94 

see 224 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[nicolo-ribaudo]

It would be better to swap these two statements, so that when reading somebody does not mistakenly assume that globalThis.eval_list = [] would be evaluated first (imports are hoisted and executed before code in the file).

This test right now would pass regardless of whether import defer is eager or deferred. If it's eager it passes because:

• after the eager ../fixtures/es-modules/module-cjs-deferred-eval.js evaluation, this file sets globalThis.eval_list = [];. Then it asserts that its length is 0, which is obviously true because we just set it to an empty array.
• further down, in assert.partialDeepStrictEqual(['defer-1'], globalThis.eval_list);, we are checking that ['defer-1'] contains all the elements of globalThis.eval_list. If globalThis.eval_list is empty, that's tautologically true.

I have not run this test, but I assume the reason in test/fixtures/es-modules/module-cjs-deferred-eval.js you had to add if (!globalThis.eval_list) { globalThis.eval_list = []; } is because the CJS module is indeed not being deferred, so it's running before that this file defines globalThis.eval_list = [].

Maybe better, either one of this:

• move the globalThis.eval_list = [] initialization to a separate file that is imported at the beginning of this one, before the other imports, so that we know it runs first
• use a boolean instead of an array, and then we don't need to pre-initialize it.

[MayaLekova]

Thanks for the explanation! I was trying to finish the test in a rush and didn't test properly.
The imported module doesn't need to initialize the array at all, so I just moved it to the main module. Having eval_list as an array makes it easier to check the imported module is executed only once (using deepStrictEqual as Joyee suggested), so I'm leaving it as an array.

[nicolo-ribaudo]

It'd be great to also have an assertion about imported.foo and imported.identifier, to make sure that import defer works with the cjs exports detection (like Joyee suggested in #64694 (comment))

[MayaLekova]

Thanks, I missed the part that foo and identifier are read directly from the exports, rather than the .default object (is it an exports default object?).

Uploaded a new patch, please take another look.

[nicolo-ribaudo]

Looks good 👍

[joyeecheung]

LGTM % comment suggestions

[joyeecheung]

Suggested change

	// Check that the exported properties are accessible and have the
	// expected values.

This is a bit self-evident.

[MayaLekova]

Okay, I tried rephrazing it, so that it carries more meaning.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75742/

[MayaLekova]

Hi @joyeecheung , could you re-run the CI, as I don't have permissions? I think the Mac failure is a flake.

[nodejs-github-bot]

Landed in f83e7df