Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
joyeecheung wants to merge 3 commits into from
+48 −2
Closed

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

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
152a8f6
stream: do not use crypto.DEFAULT_ENCODING in lazy_transform.js
joyeecheung Nov 16, 2018
987fe87
test: add a test to make sure the modules can be required independently
joyeecheung Nov 16, 2018
7925ff0
fixup! test: add a test to make sure the modules can be required inde…
joyeecheung Nov 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
test: add a test to make sure the modules can be required independently 
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.
  • Loading branch information
@joyeecheung
joyeecheung committed Nov 16, 2018
commit 987fe8772a15c4dc75d0973b8bebc14af072c66b
43 changes: 43 additions & 0 deletions test/sequential/test-native-module-deps.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
'use strict';

// This tests that all the native modules can be loaded independently
// Flags: --expose-internals

if (process.argv[3]) {
require(process.argv[3]);
return;
}

require('../common');
const {
cachableBuiltins
} = require('internal/bootstrap/cache');
const { fork } = require('child_process');
const assert = require('assert');

for (const key of cachableBuiltins) {
run(key);
}

function run(key) {
const child = fork(__filename,
Copy link
Copy Markdown
Contributor

@refack refack Nov 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

@joyeecheung joyeecheung Nov 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Copy Markdown
Contributor

@refack refack Nov 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@refack refack Nov 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

[ '--expose-internals', key ],
{ silent: true }
);

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

@refack refack Nov 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add common.mustCall

if (code === 0) {
return;
}
console.log(`Failed to require ${key}`)
console.log('----- stderr ----')
console.log(stderr);
console.log('----- stdout ----')
console.log(stdout);
assert.strictEqual(code, 0);
Copy link
Copy Markdown
Contributor

@refack refack Nov 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

@joyeecheung joyeecheung Nov 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

});
}