test: only check fd 0,1,2 are used and not their access modes by jBarz · Pull Request #10339 · nodejs/node · GitHub

Don't do a write on stdout/stderr because that checks for their
writability. But fd=1 could legitimately be opened with read-only access by the user.
What this test needs to only ensure is that they are used at startup.

Checklist

make -j4 test (UNIX)
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

test

Description of change

Fixes: #10234

Looks like a good approach to me.

should check 0 as well (the original didn't, possibly because its hard to do if input is unavailable).

Could you make the first line of the commit message <50 chars?

While you're at it, you can add this to the commit:

Fixes: https://github.com/nodejs/node/issues/10234

Fixed first line of commit message and added following
Fixes #10234

LGTM, CI: https://ci.nodejs.org/job/node-test-commit/6727/

Is the original behavior good to preserve for non-AIX platforms in a different test case?

I think this follows the test name but has fundamentally different behavior.

IMO, we don't need to preserve the original behavior because on any system, fd=1 doesn't have to be writable. So the original test-case was making an incorrect assumption.

The original test was asserting things that b5f25a9 did not in fact guarantee. What b5f25a9 did do is described as

Check that stdin, stdout and stderr map to open file descriptors

However, the test checked that stdout and stderr were writeable (something the c++ code does not check), and didn't do any check at all for stdin. This version of the test asserts that 0,1,2 map to open fds, which is exactly what is intended, and does it using fstat(), which is exactly what the code-under-test does:

node/src/node.cc

Line 4058 in 5d14602

if (fstat(fd, &ignored) == 0)

Seems perfect to me.

If this is being rewritten anyway, rather than try-catch could it instead be using assert.doesNotThrow? I'm also personally not a fan of the magic process exit values (42 and 126) -- Doesn't an uncaught assertion failure cause a non-zero exit code which we can check in the parent?

it does, status is 1 on uncaught exception

cc/ @sxa555 @Trott @nodejs/platform-aix for review

The 0 is unnecessary here.

Another CI just to confirm: https://ci.nodejs.org/job/node-test-commit/7423/

Extremely minor nit... this can be much more succinctly written...

[0,1,2].forEach((i) => assert.doesNotThrow(() => fs.fstatSync(i)));

But not necessary to land :-)

Will land. I like succinctness :-)

Don't do a write on stdout/stderr because that checks for their writability. But fd=1 could legitimately be opened with read-only access by the user. What this test needs to only ensure is that they are used at startup. This fixes nodejs#10234

CI: https://ci.nodejs.org/job/node-test-commit/7430/

EDIT: Windows job is hanging, looks like a machine issue, otherwise good.

Landed in 106d09a

Don't do a write on stdout/stderr because that checks for their writability. But fd=1 could legitimately be opened with read-only access by the user. All this test needs to ensure is that they are used at startup. PR-URL: #10339 Fixes: #10234 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

Don't do a write on stdout/stderr because that checks for their writability. But fd=1 could legitimately be opened with read-only access by the user. All this test needs to ensure is that they are used at startup. PR-URL: nodejs#10339 Fixes: nodejs#10234 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

Don't do a write on stdout/stderr because that checks for their writability. But fd=1 could legitimately be opened with read-only access by the user. All this test needs to ensure is that they are used at startup. PR-URL: #10339 Fixes: #10234 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added test Issues and PRs related to the tests. lts-watch-v6.x labels Dec 19, 2016

sam-github suggested changes Dec 19, 2016

View reviewed changes

jBarz force-pushed the aix-stdio-closed.2 branch from dbd9df8 to 3c49228 Compare December 19, 2016 16:03