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.

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 hijackStdout and hijackStderr #13439

Closed
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
18 changes: 18 additions & 0 deletions test/common/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,16 @@ Checks whether `IPv6` is supported on this platform.

Checks if there are multiple localhosts available.

### hijackStderr(listener)
* `listener` [<Function>](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#Normal_objects_and_functions): An listener with a single parameter called `data`;
Copy link
Contributor

Choose a reason for hiding this comment

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

"A listener"

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, please break the lines at 80 chars if possible.


Hijack `process.stderr` to listen `write` action. Once `process.stderr.write` is called, `listener` will also be called and the `data` of `write` function will be passed to `listener`. What's more, `process.stderr.writeTimes` will plus one then.

### hijackStdout(listener)
* `listener` [<Function>](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#Normal_objects_and_functions): An listener with a single parameter called `data`;

Hijack `process.stdout` to listen `write` action. Once `process.stdout.write` is called, `listener` will also be called and the `data` of `write` function will be passed to `listener`. What's more, `process.stdout.writeTimes` will plus one then.

### inFreeBSDJail
* return [<Boolean>](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#Boolean_type)

Expand Down Expand Up @@ -250,6 +260,14 @@ Port tests are running on.

Deletes the 'tmp' dir and recreates it

### restoreStderr
Copy link
Member

Choose a reason for hiding this comment

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

If these are functions, then please include the ()


Restore the original `process.stderr.write`.

### restoreStdout

Restore the original `process.stdout.write`.

### rootDir
* return [<String>](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#String_type)

Expand Down
24 changes: 24 additions & 0 deletions test/common/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -759,3 +759,27 @@ exports.getTTYfd = function getTTYfd() {
}
return tty_fd;
};

// Hijack stdout and stderr
const stdWrite = {};
function hijackStdWritable(name, listener) {
const stream = process[name];
const _write = stdWrite[name] = stream.write.bind(stream);

stream.writeTimes = 0;
stream.write = function(data, callback) {
listener(data);
_write(data, callback);
stream.writeTimes++;
Copy link
Member

Choose a reason for hiding this comment

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

This logic seem not go well with the writable.write specification:

callback (Function) :Callback for when this chunk of data is flushed

Your logic will cause:

  • Issue the completion handler before the actual write (_write)
  • Listener will be called in the same loop tick, making it a direct call, not a callback.

I propose:
_wite(data, listener)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👌

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact, listener is something like .emit('write') but not .emit('written'), so it's sync.

And I've added the callback in _write now.

};
}

function restoreWritable(name) {
process[name].write = stdWrite[name];
delete process[name].writeTimes;
}

exports.hijackStdout = hijackStdWritable.bind(null, 'stdout');
exports.hijackStderr = hijackStdWritable.bind(null, 'stderr');
exports.restoreStdout = restoreWritable.bind(null, 'stdout');
exports.restoreStderr = restoreWritable.bind(null, 'stderr');
8 changes: 7 additions & 1 deletion test/fixtures/echo-close-check.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,15 @@ const fs = require('fs');

process.stdout.write('hello world\r\n');

var stdin = process.openStdin();
const stdin = process.openStdin();

let current;
common.hijackStdout(function(data) {
Copy link
Contributor

Choose a reason for hiding this comment

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

As there is an assertion in the listener function, it is better to have it wrapped in common.mustCall

assert.equal(data, current);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use strict equal.

});

stdin.on('data', function(data) {
current = data;
process.stdout.write(data.toString());
});

Expand Down
6 changes: 6 additions & 0 deletions test/fixtures/echo.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@ process.stdout.write('hello world\r\n');

var stdin = process.openStdin();

let current;
stdin.on('data', function(data) {
current = data;
process.stdout.write(data.toString());
});

common.hijackStdout(function(data) {
Copy link
Contributor

Choose a reason for hiding this comment

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

same

assert.equal(data, current);
});
33 changes: 20 additions & 13 deletions test/parallel/test-console.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,16 +45,14 @@ assert.doesNotThrow(function() {
// an Object with a custom .inspect() function
const custom_inspect = { foo: 'bar', inspect: () => 'inspect' };

const stdout_write = global.process.stdout.write;
const stderr_write = global.process.stderr.write;
const strings = [];
const errStrings = [];
global.process.stdout.write = function(string) {
strings.push(string);
};
global.process.stderr.write = function(string) {
errStrings.push(string);
};
common.hijackStdout(function(data) {
strings.push(data);
});
common.hijackStderr(function(data) {
errStrings.push(data);
});

// test console.log() goes to stdout
console.log('foo');
Expand Down Expand Up @@ -105,8 +103,10 @@ console.timeEnd('constructor');
console.time('hasOwnProperty');
console.timeEnd('hasOwnProperty');

global.process.stdout.write = stdout_write;
global.process.stderr.write = stderr_write;
assert.strictEqual(strings.length, process.stdout.writeTimes);
Copy link
Contributor

Choose a reason for hiding this comment

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

won't this fail after you implemented @gireeshpunathil's async comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact, listener is sync which means a write action is emitted but not finished.

I just added the callback to the original _write function to make it may receivecallback.

assert.strictEqual(errStrings.length, process.stderr.writeTimes);
common.restoreStdout();
common.restoreStderr();

// verify that console.timeEnd() doesn't leave dead links
const timesMapSize = console._times.size;
Expand Down Expand Up @@ -146,9 +146,6 @@ assert.ok(/^hasOwnProperty: \d+\.\d{3}ms$/.test(strings.shift().trim()));
assert.strictEqual('Trace: This is a {"formatted":"trace"} 10 foo',
errStrings.shift().split('\n').shift());

assert.strictEqual(strings.length, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Keep this.
But I assume you'll need to solve async issues with this whole block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same which mentioned before.

And assert.strictEqual(strings.length, 0) equals above assert.strictEqual(strings.length, process.stdout.writeTimes);.

assert.strictEqual(errStrings.length, 0);

assert.throws(() => {
console.assert(false, 'should throw');
}, common.expectsError({
Expand All @@ -159,3 +156,13 @@ assert.throws(() => {
assert.doesNotThrow(() => {
console.assert(true, 'this should not throw');
});

// hijack stderr to catch `process.emitWarning` which using `process.nextTick`
Copy link
Contributor

Choose a reason for hiding this comment

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

which is using

common.hijackStderr(common.mustCall(function(data) {
common.restoreStderr();

// stderr.write will catch sync error, so use `process.nextTick` here
process.nextTick(function() {
assert(/no such label/.test(data));
Copy link
Contributor

Choose a reason for hiding this comment

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

I like assert.strictEqual(data.includes('no such label'), true) better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👌

});
}));
31 changes: 12 additions & 19 deletions test/parallel/test-global-console-exists.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,37 +7,30 @@
const common = require('../common');
const assert = require('assert');
const EventEmitter = require('events');
const leak_warning = /EventEmitter memory leak detected\. 2 hello listeners/;
const leakWarning = /EventEmitter memory leak detected\. 2 hello listeners/;

let write_calls = 0;
common.hijackStderr(function(data) {
Copy link
Contributor

Choose a reason for hiding this comment

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

common.mustCall

if (process.stderr.writeTimes === 0) {
assert.ok(data.match(leakWarning));
} else {
assert.fail('stderr.write should be called only once');
}
});

process.on('warning', (warning) => {
process.on('warning', function(warning) {
// This will be called after the default internal
// process warning handler is called. The default
// process warning writes to the console, which will
// invoke the monkeypatched process.stderr.write
// below.
assert.strictEqual(write_calls, 1);
EventEmitter.defaultMaxListeners = old_default;
assert.strictEqual(process.stderr.writeTimes, 1);
EventEmitter.defaultMaxListeners = oldDefault;
// when we get here, we should be done
});

process.stderr.write = (data) => {
if (write_calls === 0)
assert.ok(data.match(leak_warning));
else
assert.fail('stderr.write should be called only once');

write_calls++;
};

const old_default = EventEmitter.defaultMaxListeners;
const oldDefault = EventEmitter.defaultMaxListeners;
EventEmitter.defaultMaxListeners = 1;

const e = new EventEmitter();
e.on('hello', common.noop);
e.on('hello', common.noop);

// TODO: Figure out how to validate console. Currently,
// there is no obvious way of validating that console
// exists here exactly when it should.
7 changes: 2 additions & 5 deletions test/parallel/test-process-raw-debug.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
// USE OR OTHER DEALINGS IN THE SOFTWARE.

'use strict';
require('../common');
const common = require('../common');
const assert = require('assert');
const os = require('os');

Expand Down Expand Up @@ -63,10 +63,7 @@ function child() {
throw new Error('No ticking!');
};

const stderr = process.stderr;
stderr.write = function() {
throw new Error('No writing to stderr!');
};
common.hijackStderr(common.mustNotCall('stderr.write mustn\'t be called.'));
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd avoid the contraction or use template strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, I don't quite understand what's your meaning

Copy link
Member

Choose a reason for hiding this comment

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

Replace 'stderr.write mustn\'t be called.' with either:

'stderr.write must not be called.'
or

`stderr.write mustn't be called.`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


process._rawDebug('I can still %s!', 'debug');
}
15 changes: 8 additions & 7 deletions test/parallel/test-util-log.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,19 +20,18 @@
// USE OR OTHER DEALINGS IN THE SOFTWARE.

'use strict';
require('../common');
const common = require('../common');
const assert = require('assert');
const util = require('util');

assert.ok(process.stdout.writable);
assert.ok(process.stderr.writable);

const stdout_write = global.process.stdout.write;
const strings = [];
global.process.stdout.write = function(string) {
strings.push(string);
};
console._stderr = process.stdout;
common.hijackStdout(function(data) {
strings.push(data);
});
common.hijackStderr(common.mustNotCall('stderr.write mustn\'t be called'));

const tests = [
{input: 'foo', output: 'foo'},
Expand All @@ -56,4 +55,6 @@ tests.forEach(function(test) {
assert.strictEqual(match[1], test.output);
});

global.process.stdout.write = stdout_write;
assert.strictEqual(process.stdout.writeTimes, tests.length);

common.restoreStdout();