-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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.
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,console: known_issues test for issue 831 #5639
Conversation
This is probably fragile, dependent on |
The Lines 645 to 648 in a498492
|
@Trott I don't think that issue link is correct? |
@Fishrock123 Argh. You are correct. The correct issue link is #831. Will fix above too... |
This adds a test for the issue described in nodejs#831. I've added some metadata fields that I would personally find useful in these tests. Refs: nodejs#831
I believe #947 is the same issue, or closely related. I have this failing test, which I haven't opened a PR for: 'use strict';
// Refs: https://github.com/nodejs/node/issues/947
const common = require('./test/common');
const assert = require('assert');
const cp = require('child_process');
if (process.argv[2] === 'child') {
process.on('message', common.mustCall((msg) => {
assert.strictEqual(msg, 'go');
console.log('logging should not cause a crash');
process.disconnect();
}));
} else {
const child = cp.fork(__filename, ['child'], {silent: true});
child.on('close', common.mustCall((exitCode, signal) => {
assert.strictEqual(exitCode, 0);
assert.strictEqual(signal, null);
}));
child.stdout.destroy();
child.send('go');
} |
Test LGTM, will be good to get this fixed. |
Pull Request check-list
Please make sure to review and check all of these items:
make -j8 test
(UNIX) orvcbuild test nosign
(Windows) pass withthis change (including linting)?
Affected core subsystem(s)
console, test
Description of change
This adds a test for the issue described in
#831.
I've added some metadata fields that I would personally find useful in
these tests.
Refs: #831