test: add a test to make sure the modules can be required independently #24402

joyeecheung · 2018-11-16T20:41:11Z

The first patch comes from #24396

So, I am not entirely sure if it's worth adding a test that launches ~75 child processes just to make sure we have a cleaner dependency graph in the internal modules..so just open this to see what others think.

test: add a test to make sure the modules can be required

This patch adds a test that makes sure all the modules (internal
or not) can be independently required - that is, when there is
circular dependency, one module does not rely on another module
being loaded first to initialize, instead it should lazily load
that dependency after initialization.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

The default encoding can be retrieved via `require('internal/crypto/util').getDefaultEncoding` instead of the deprecated crypto.DEFAULT_ENCODING which triggers a warning. Background: The require chain goes like this: ``` internal/streams/lazy_transform.js -> crypto.js -> internal/crypto/cipher.js (uses LazyTransform in the global scope) -> internal/streams/lazy_transform.js ``` So when `internal/streams/lazy_transform.js` is required before `lib/crypto.js`, we have a circular dependency and since `internal/crypto/cipher.js` uses destructuring to use LazyTransform we will get an error. And it can also trigger a warning if lazy_transform.js is the first file that touches crypto.DEFAULT_ENCODING.

This patch adds a test that makes sure all the modules (internal or not) can be independently required - that is, when there is circular dependency, one module does not rely on another module being loaded first to initialize, instead it should lazily load that dependency after initialization.

nodejs-github-bot · 2018-11-16T20:41:13Z

@joyeecheung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1639/pipeline

bnoordhuis

Test seems fine to me.

addaleax · 2018-11-20T13:36:30Z

Looks like the linter failures are related?

joyeecheung · 2018-11-20T13:44:23Z

@addaleax Yes, I didn't run the linters before submitting this, my bad, will fix later

…pendently

joyeecheung · 2018-11-22T07:21:16Z

Fixed linter: https://ci.nodejs.org/job/node-test-pull-request/18872/

refack · 2018-11-23T02:52:15Z

AIX is strange ¯\(ツ)/¯

test.sequential/test-native-module-deps

/home/iojs/build/workspace/node-test-commit-aix/nodes/aix61-ppc64/test/sequential/test-native-module-deps.js:30
  child.stdout.on('data', (data) => (stdout += data.toString()));
               ^

TypeError: Cannot read property 'on' of undefined
    at run (/home/iojs/build/workspace/node-test-commit-aix/nodes/aix61-ppc64/test/sequential/test-native-module-deps.js:30:16)
    at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-aix/nodes/aix61-ppc64/test/sequential/test-native-module-deps.js:19:3)
    at Module._compile (internal/modules/cjs/loader.js:722:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:733:10)
    at Module.load (internal/modules/cjs/loader.js:620:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:560:12)
    at Function.Module._load (internal/modules/cjs/loader.js:552:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:775:12)
    at startup (internal/bootstrap/node.js:300:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:826:3)

refack · 2018-11-23T03:02:29Z

So on a slow platform like debian8-docker-armv7, this test takes 19s
https://ci.nodejs.org/job/node-test-commit-arm/20217/nodes=debian8-docker-armv7/testReport/(root)/test/sequential_test_native_module_deps/

I think it's a good test to have, but maybe move it to one of the suites that run only one a day (ATM it's internet and benchmark)

/CC @nodejs/testing

lib/internal/streams/lazy_transform.js

refack · 2018-11-23T03:10:30Z

test/sequential/test-native-module-deps.js

+  let stderr = '';
+  child.stdout.on('data', (data) => (stdout += data.toString()));
+  child.stderr.on('data', (data) => (stderr += data.toString()));
+  child.on('close', (code) => {


add common.mustCall

refack

Some suggestions

refack · 2018-11-23T03:11:56Z

test/sequential/test-native-module-deps.js

+    console.log(stderr);
+    console.log('----- stdout ----');
+    console.log(stdout);
+    assert.strictEqual(code, 0);


~~IIUC this doesn't match L33~~
Ahh you want this to fail, so assert.fail(`exit code: ${code}`).

Another thought, this will lead to the test failing for the first bad module masking, any other possible fails. Maybe replace with

common.mustCall(() => `exit code: ${code} for module: ${key}`);

The returned function will never get called, but will make the test fail when the process exits.

@refack In general I think it's fine to just fail when we encounter the first module without a clean dependency graph, and fix them one by one?

refack · 2018-11-23T03:22:27Z

test/sequential/test-native-module-deps.js

+}
+
+function run(key) {
+  const child = fork(__filename,


Maybe use:

Suggested change

const child = fork(__filename,

const child = execFile(process.execPath, ['-e', `require('${key}')])`

this way you don't need the self-reference, and eliminate L6-L9

@refack -e introduces noise in the dependency graph because that option also leads to additional module loads. Same goes to -p. It somewhat weakens the test.

Then maybe use a fixture?
It seems to me like in this case the self referenced part makes the file look awkward. And the usual benefit of having all the test code in one place, is not that beneficial since the child code is just one expression.
But it's just a style nit, and I defer to your decision.

New idea, a variation on (either with shell or by using child.stdin):

const child = exec(`echo "require('${key}')" | `${process.execPath}` --`, {shell: true});

joyeecheung · 2018-11-23T04:54:04Z

I think it's a good test to have, but maybe move it to one of the suites that run only one a day (ATM it's internet and benchmark)

I thought about that, but this test looks weird in either of those places...maybe we can create a new folder for slower tests and just move them all there? Maybe also some of the slow but less critical tests identified in #23251

refack · 2018-11-23T05:15:40Z

maybe we can create a new folder for slower tests and just move them all there? Maybe also some of the slow but less critical tests identified in #23251

I'll put it in my list of goals

BridgeAR · 2019-12-20T01:33:21Z

I like the test. It is pretty heavy thought.

@Trott do we have more tests that only run once a day?

github-actions · 2020-11-06T00:03:05Z

Closing this because it has stalled. Feel free to reopen if this PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions.

Trott · 2020-11-09T20:50:54Z

I like the test. It is pretty heavy thought.

@Trott do we have more tests that only run once a day?

Sorry for the late reply, but pummel tests run once a day.

joyeecheung added 2 commits November 17, 2018 00:28

bnoordhuis approved these changes Nov 17, 2018

View reviewed changes

fixup! test: add a test to make sure the modules can be required inde…

7925ff0

…pendently

refack added module Issues and PRs related to the module subsystem. test Issues and PRs related to the tests. cli Issues and PRs related to the Node.js command line interface. labels Nov 23, 2018

refack reviewed Nov 23, 2018

View reviewed changes

lib/internal/streams/lazy_transform.js Show resolved Hide resolved

refack reviewed Nov 23, 2018

View reviewed changes

Trott force-pushed the master branch from 1ecc406 to 49cf67e Compare September 17, 2019 16:51

devnexen force-pushed the master branch from e8a4568 to 5289f80 Compare December 26, 2019 19:46

BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19

jasnell added the stalled Issues and PRs that are stalled. label Jul 7, 2020

aduh95 mentioned this pull request Oct 19, 2020

Stale action skips PRs with no clear reason actions/stale#198

Closed

github-actions bot closed this Nov 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add a test to make sure the modules can be required independently #24402

test: add a test to make sure the modules can be required independently #24402

joyeecheung commented Nov 16, 2018 •

edited

Loading

nodejs-github-bot commented Nov 16, 2018

bnoordhuis left a comment

addaleax commented Nov 20, 2018

joyeecheung commented Nov 20, 2018

joyeecheung commented Nov 22, 2018

refack commented Nov 23, 2018 •

edited

Loading

refack commented Nov 23, 2018

refack Nov 23, 2018

refack left a comment

refack Nov 23, 2018 •

edited

Loading

joyeecheung Nov 23, 2018

refack Nov 23, 2018

joyeecheung Nov 23, 2018 •

edited

Loading

refack Nov 23, 2018

refack Nov 25, 2018

joyeecheung commented Nov 23, 2018 •

edited

Loading

refack commented Nov 23, 2018

BridgeAR commented Dec 20, 2019

github-actions bot commented Nov 6, 2020

Trott commented Nov 9, 2020

	const child = fork(__filename,
	const child = execFile(process.execPath, ['-e', `require('${key}')])`

test: add a test to make sure the modules can be required independently #24402

test: add a test to make sure the modules can be required independently #24402

Conversation

joyeecheung commented Nov 16, 2018 • edited Loading

test: add a test to make sure the modules can be required

Checklist

nodejs-github-bot commented Nov 16, 2018

bnoordhuis left a comment

Choose a reason for hiding this comment

addaleax commented Nov 20, 2018

joyeecheung commented Nov 20, 2018

joyeecheung commented Nov 22, 2018

refack commented Nov 23, 2018 • edited Loading

refack commented Nov 23, 2018

refack Nov 23, 2018

Choose a reason for hiding this comment

refack left a comment

Choose a reason for hiding this comment

refack Nov 23, 2018 • edited Loading

Choose a reason for hiding this comment

joyeecheung Nov 23, 2018

Choose a reason for hiding this comment

refack Nov 23, 2018

Choose a reason for hiding this comment

joyeecheung Nov 23, 2018 • edited Loading

Choose a reason for hiding this comment

refack Nov 23, 2018

Choose a reason for hiding this comment

refack Nov 25, 2018

Choose a reason for hiding this comment

joyeecheung commented Nov 23, 2018 • edited Loading

refack commented Nov 23, 2018

BridgeAR commented Dec 20, 2019

github-actions bot commented Nov 6, 2020

Trott commented Nov 9, 2020

joyeecheung commented Nov 16, 2018 •

edited

Loading

refack commented Nov 23, 2018 •

edited

Loading

refack Nov 23, 2018 •

edited

Loading

joyeecheung Nov 23, 2018 •

edited

Loading

joyeecheung commented Nov 23, 2018 •

edited

Loading