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

Conversation

XadillaX
Copy link
Contributor

@XadillaX XadillaX commented Jun 3, 2017

Add common.hijackStdout and common.hijackStderr to provide monitor
for console output.

Refs:

// TODO: Figure out how to validate console. Currently,
// there is no obvious way of validating that console
// exists here exactly when it should.

Checklist
  • make -j4 test (UNIX)
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jun 3, 2017
@XadillaX
Copy link
Contributor Author

XadillaX commented Jun 3, 2017

/cc @nodejs/testing

@gibfahn
Copy link
Member

gibfahn commented Jun 3, 2017

cc/ @nodejs/testing

@mscdex
Copy link
Contributor

mscdex commented Jun 3, 2017

Why is this being added to the test/common.js for a single test? It seems like this is very test-specific and should belong in the test file itself?

@refack
Copy link
Contributor

refack commented Jun 3, 2017

Why is this being added to the test/common.js for a single test? It seems like this is very test-specific and should belong in the test file itself?

I'm with @mscdex.
@XadillaX I think best course of action is to lower hijackStdout and hijackStderr into the test, and open a second PR for hoisting them to common (My estimation: some non trivial push back, e.g. #13346 (comment), #12935)

@gibfahn
Copy link
Member

gibfahn commented Jun 3, 2017

and open a second PR for hoisting them to common

Better yet, hoist them to common if it turns out there's a need for them in several tests.

stream.write = function(data) {
listener(data);
_write(data);
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.

@mscdex mscdex added events Issues and PRs related to the events subsystem / EventEmitter. process Issues and PRs related to the process subsystem. labels Jun 4, 2017
@XadillaX
Copy link
Contributor Author

XadillaX commented Jun 4, 2017

@gibfahn @mscdex @refack I've found out some other situations to use these functions and updated the code.

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.

@gibfahn gibfahn requested a review from Trott June 4, 2017 10:33
@@ -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.


let current;
common.hijackStdout(function(data) {
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.

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

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


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

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Some nits
Some comments about synchronization
I'm +1 for this feature


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

👌

@@ -116,6 +116,26 @@ 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):
Copy link
Contributor

Choose a reason for hiding this comment

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

(I know everywhere else uses it this way but) if you are using this link more than one you could use [MDN-Function][], and put

[MDN-Function]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#Normal_objects_and_functions

the the bottom of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👌

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

Hijack `process.stderr` to listen `write` action. Once `process.stderr.write` is
Copy link
Contributor

@refack refack Jun 4, 2017

Choose a reason for hiding this comment

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

Hijack `process.stderr` to listen `write` action => Eavesdrop to `process.stderr.write` calls

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👌


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

Choose a reason for hiding this comment

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

... `process.stderr.writeTimes` is a count of the number of calls

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👌

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

Hijack `process.stdout` to listen `write` action. Once `process.stdout.write` is
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

IMHO since this is an "internal" guide you could merge then into
### hijackStdout/Stderr(listener)

const stdin = process.openStdin();

let current;
common.hijackStdout(common.mustCall(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.

I don't this this is an interesting use of hijackStdout, it just tests the mechanics of hijackStdout.
BTW: you should do that in explicitly in test/parallel/test-common.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👌

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

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

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

@refack
Copy link
Contributor

refack commented Jun 4, 2017

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Thanks ✔️

@refack
Copy link
Contributor

refack commented Jun 5, 2017

@XadillaX
Copy link
Contributor Author

XadillaX commented Jun 5, 2017

@refack Thanks

image

@@ -116,6 +116,14 @@ Checks whether `IPv6` is supported on this platform.

Checks if there are multiple localhosts available.

### hijackStderr/Stdout(listener)
Copy link
Member

Choose a reason for hiding this comment

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

These should be listed as separate functions (even if it means duplicating the text)

Copy link
Contributor

@refack refack Jun 5, 2017

Choose a reason for hiding this comment

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

@jasnell I suggested the the merge (since it's an internal guide) #13439 (comment)
Please 👍 / 👎 again, just so we don't ask @XadillaX to do unnecessary work

Copy link
Member

Choose a reason for hiding this comment

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

I'd rather keep the style consistent throughout, so would really prefer them separate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ack.
Sorry @XadillaX

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👌

@@ -250,6 +258,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 ()

@refack
Copy link
Contributor

refack commented Jun 5, 2017

@Trott you ok with this?

@Trott
Copy link
Member

Trott commented Jun 6, 2017

@Trott you ok with this?

I'm neutral on this change, so I guess that means I'm ok with it.

@refack refack self-assigned this Jun 6, 2017
@XadillaX
Copy link
Contributor Author

XadillaX commented Jun 8, 2017

I think I've done all of the work after reviewing. @jasnell

@DavidCai1111
Copy link
Member

@refack
Copy link
Contributor

refack commented Jun 8, 2017

/cc @nodejs/testing anyone else have an 👍 / 👎 opinion?

@XadillaX
Copy link
Contributor Author

XadillaX commented Jun 9, 2017

so could it be landed now?

@Trott
Copy link
Member

Trott commented Jun 9, 2017

so could it be landed now?

If CI failures are unrelated (I haven't looked), then, yes, this can be landed. Probable lander would be @refack since he's the sole approval right now.

@mscdex @gibfahn If you're questions/skepticism should be interpreted as being opposed to this landing, please say so and maybe use the request changes review option.

@refack
Copy link
Contributor

refack commented Jun 9, 2017

so could it be landed now?

@XadillaX technically it's ready, AFAICT CI is clean, but hust in case:
rerun arm: https://ci.nodejs.org/job/node-test-commit-arm-fanned/9322/
rerun AIX: https://ci.nodejs.org/job/node-test-commit-aix/6481/

refack pushed a commit to refack/node that referenced this pull request Jun 9, 2017
Add `common.hijackStdout` and `common.hijackStderr` to provide monitor
for console output.

PR-URL: nodejs#13439
Reviewed-By: Refael Ackermann <[email protected]>
@refack
Copy link
Contributor

refack commented Jun 9, 2017

Landed in d00e5f1

@refack refack closed this Jun 9, 2017
@refack
Copy link
Contributor

refack commented Jun 9, 2017

Extra quick sanity of master: https://ci.nodejs.org/job/node-test-commit-linuxone/6518/

addaleax pushed a commit that referenced this pull request Jun 10, 2017
Add `common.hijackStdout` and `common.hijackStderr` to provide monitor
for console output.

PR-URL: #13439
Reviewed-By: Refael Ackermann <[email protected]>
@addaleax addaleax mentioned this pull request Jun 10, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@MylesBorins
Copy link
Contributor

is this something we want to add to lts?

@MylesBorins
Copy link
Contributor

if we want to land this on 6.x it will require a manual backport

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
events Issues and PRs related to the events subsystem / EventEmitter. process Issues and PRs related to the process subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.