-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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 known_issues test for GH-2148 #5920
Conversation
const execSync = require('child_process').execSync; | ||
|
||
const longLine = 'foo bar baz quux quuz aaa bbb ccc'.repeat(80); | ||
const expectedLength = (longLine.length * 999) + 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.
Won't this technically be + 2
on Windows?
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.
Probably. Good point. Maybe the thing to do is compare stdout.trim().length
in the assertion so as to avoid any assumptions whatsoever about the line separator?
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.
Yeah, and we may want to compare the (trimmed) output instead of comparing the lengths just to be extra sure we get what we expect.
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.
OK, done. PTAL
} | ||
|
||
const stdout = execSync(`${process.execPath} ${__filename} child`) | ||
.toString() |
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.
nit: maybe these would look a little less oddly placed if they were lined up with the first e
in execSync()
?
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.
Maybe just do this?
const argv = `${process.execPath} ${__filename} child`
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.
Thanks! Nit addressed.
One nit but otherwise LGTM. |
PR-URL: nodejs#5920 Refs: nodejs#2148 Reviewed-By: Brian White <mscdex@mscdex.net>
Landed in 33c27f8 |
Pull Request check-list
make -j8 test
(UNIX) orvcbuild test nosign
(Windows) pass withthis change (including linting)?
test (or a benchmark) included?
existing APIs, or introduces new ones)?
Affected core subsystem(s)
test, process
Description of change
test: add known_issues test for GH-2148
Refs: #2148