Skip to content
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

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

Closed
wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
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
7 changes: 5 additions & 2 deletions lib/internal/streams/lazy_transform.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@

const stream = require('stream');
const util = require('util');
const crypto = require('crypto');

const {
getDefaultEncoding
} = require('internal/crypto/util');

module.exports = LazyTransform;

Expand All @@ -22,7 +25,7 @@ function makeGetter(name) {
this._writableState.decodeStrings = false;

if (!this._options || !this._options.defaultEncoding) {
this._writableState.defaultEncoding = crypto.DEFAULT_ENCODING;
this._writableState.defaultEncoding = getDefaultEncoding();
refack marked this conversation as resolved.
Show resolved Hide resolved
}

return this[name];
Expand Down
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
Contributor

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
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
Contributor

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
Contributor

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
Contributor

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
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
Member Author

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?

});
}