-
Notifications
You must be signed in to change notification settings - Fork 30k
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: only check fd 0,1,2 are used and not their access modes #10339
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a good approach to me.
process.exit(84); | ||
} | ||
}); | ||
fs.fstatSync(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should check 0
as well (the original didn't, possibly because its hard to do if input is unavailable).
dbd9df8
to
3c49228
Compare
3c49228
to
76773e2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
76773e2
to
6f349a8
Compare
6f349a8
to
44862c0
Compare
Fixed first line of commit message and added following |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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 Line 4058 in 5d14602
Seems perfect to me. |
} catch (e) { | ||
assert.strictEqual(e.code, 'EBADF'); | ||
assert.strictEqual(e.message, 'EBADF: bad file descriptor, write'); | ||
process.exit(126); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it does, status is 1
on uncaught exception
assert.doesNotThrow(function() { | ||
fs.fstatSync(2) | ||
}); | ||
return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 0
is unnecessary here.
Another CI just to confirm: https://ci.nodejs.org/job/node-test-commit/7423/ |
}); | ||
assert.doesNotThrow(function() { | ||
fs.fstatSync(2) | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
04aeaf5
to
a9df4f2
Compare
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: 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: 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: 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>
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: #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: #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.
What this test needs to only ensure is that they are used at startup.
Checklist
make -j4 test
(UNIX)Affected core subsystem(s)
test
Description of change
Fixes: #10234