Test: stdio.js SIGWINCH test coverage

[sarahmeyer]

Checklist

• make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

Affected core subsystem(s)

test

Description of change

Provides coverage for calling process._refreshSize when a SIGWINCH is emitted within stderr and stdout functions in stdio.js

[sarahmeyer]

@Fishrock123 peep it 👀 -- is this what you were going for?

[MylesBorins]

ci: https://ci.nodejs.org/job/node-test-pull-request/5056/

@sarahmeyer great work! you will need to update the title of the first commit to follow our commit guidelines

[Trott]

SmartOS failure looks related.

[Fishrock123]

Hmmm, the following output occurs on SmartOS (Solaris):

not ok 1244 pseudo-tty/test-stderr-stdout-handle-sigwinch
---
duration_ms: 0.319
severity: fail
stack: |-
calling stdout._refreshSize
events.js:160
throw er; // Unhandled 'error' event
^
Error: getWindowSize EINVAL
at exports._errnoException (util.js:1022:11)
at WriteStream._refreshSize (tty.js:78:24)
at WriteStream.refreshSizeWrapperStdout [as _refreshSize] (/home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos16-64/test/pseudo-tty/test-stderr-stdout-handle-sigwinch.js:15:36)
at process.on (internal/process/stdio.js:16:43)
at emitNone (events.js:91:20)
at process.emit (events.js:185:7)
at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos16-64/test/pseudo-tty/test-stderr-stdout-handle-sigwinch.js:21:9)
at Module._compile (module.js:571:32)
at Object.Module._extensions..js (module.js:580:10)
at Module.load (module.js:488:32)

I suspect that Python does not correctly emulate this on SmartOS but I'll try to take a deeper look soon.

Skipping on SmartOS may be the appropriate solution by the looks, though.

[Fishrock123]

@sarahmeyer As we briefly discussed, it would be useful to check the properties modifiable by tty.WriteStream#_refreshSize before and after we send the SIGWINCH.

I think everything should be the same so it also should not call the resize event, so I suppose we could listen and ensure that is not called.

We could also check for an error event on SmartOS rather than skipping, which would be a bit more robust.

Edit: Forgot to say most things like checking what platform we are running on have helpers in the common "module", such as isSunOS.

[Fishrock123]

Got the coverage to (mostly) work locally, after some tweaking... You can find it at nodejs/testing#46 if you are interested.

(To run it, run (from the node repo) the pre-coverage.sh script, then node's make -j4 test, then the coverage.sh script. It will output to coverage-out in your $HOME directory currently.)

Suffice to say, this hits the correct paths (aside from the fact that the pty emulation always has the same values).

[Fishrock123]

@sarahmeyer Think you'll be able to take a look this weekend?

[jbergstroem]

/cc @nodejs/platform-smartos

[Trott]

Ping @sarahmeyer: Are you still working on this? (No rush. Just want to make sure it's not stalled and, if it is, see if there's anything we can do to get it un-stalled.)

[Trott]

I took the liberty of adding a few lines of code to swallow EINVAL error on SmartOS. Hope that's OK. Will remove or alter if CI doesn't pass (or if @sarahmeyer objects to the code).

CI: https://ci.nodejs.org/job/node-test-pull-request/5583/

(If CI passes and this gets approval, will squash my commit away on landing.)

[Trott]

CI is green. Reviews/comments? @sarahmeyer@Fishrock123 @nodejs/testing @nodejs/platform-smartos

[jasnell]

LGTM with a nit

[jasnell]

A comment indicating that the console.log is part of the test would be helpful.

[Trott]

Sure, done. Will run Ci again because there was some refactoring that accompanied it.

[Trott]

CI: https://ci.nodejs.org/job/node-test-pull-request/5593/

[Trott]

Not to bikeshed this thing forever, but it seems like we could wrap the functions in common.mustCall() and get rid of the console.log() statements and have an empty .out file. Any reason not to go that route? /cc @Fishrock123

[Trott]

Possible additional advantage to going that route: I think using the .status file will work for skipping the test on SmartOS. It will be more visible there (people will look there to see skipped tests rather than look through individual tests). And it keeps the test itself very simple, getting rid of the try...catch logic.

[Trott]

Going to land this working version. We can always switch to common.mustCall() instead in a subsequent change. Here's what that would look like:

'use strict';constcommon=require('../common');constoriginalRefreshSizeStderr=process.stderr._refreshSize;constoriginalRefreshSizeStdout=process.stdout._refreshSize;constwrap=(fn,ioStream,string)=>{returncommon.mustCall(()=>{try{fn.call(ioStream);}catch(e){// EINVAL happens on SmartOS if emulation is incompleteif(!common.isSunOS||e.code!=='EINVAL')throwe;}});};process.stderr._refreshSize=wrap(originalRefreshSizeStderr,process.stderr);process.stdout._refreshSize=wrap(originalRefreshSizeStdout,process.stdout);process.emit('SIGWINCH');

In that case, test/pseudo-tty/test-stderr-stdout-handle-sigwinch.out would be an empty file.

[Trott]

Landed in a9b59ff.

Thanks for the contribution, @sarahmeyer! 🎉

[Fishrock123]

heh, forgot to get back to this

This doesn't really test all that it should IIRC. I can give pointers for improvement if anyone else wants or otherwise I'll just do it myself I guess.