-
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: use common.skip for tap skip output and test cleanup #8841
Conversation
Changed `var` to `const`, strings to template literals, and assert.equal to assert.strictEqual where appropriate.
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.
LGTM
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.
LGTM with nits addressed or not.
var filename = path.join(common.tmpDir, '/readfile_pipe_large_test.txt'); | ||
var dataExpected = new Array(1000000).join('a'); | ||
const filename = path.join(common.tmpDir, '/readfile_pipe_large_test.txt'); | ||
const dataExpected = new Array(1000000).join('a'); |
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: how about using 'a'.repeat(1e6 - 1)
?
var filename = path.join(common.tmpDir, '/readfilesync_pipe_large_test.txt'); | ||
var dataExpected = new Array(1000000).join('a'); | ||
const filename = path.join(common.tmpDir, '/readfilesync_pipe_large_test.txt'); | ||
const dataExpected = new Array(1000000).join('a'); |
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.
Ditto.
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.
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.
LGTM
Changed `var` to `const`, strings to template literals, and assert.equal to assert.strictEqual where appropriate. PR-URL: #8841 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Thanks, landed in 5e6bd84...b838e5f! Like @lpinca said, I think cleaning up the functionality may be best saved for another PR. |
@jasnell sorry, didn't catch your sign-off in the commits because GitHub hadn't refreshed or something, not worth to force-push imo. :) |
Changed `var` to `const`, strings to template literals, and assert.equal to assert.strictEqual where appropriate. PR-URL: #8841 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Changed `var` to `const`, strings to template literals, and assert.equal to assert.strictEqual where appropriate. PR-URL: #8841 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Checklist
make -j8 test
(UNIX), orvcbuild test nosign
(Windows) passesAffected core subsystem(s)
test
Description of change
Using
common.skip
instead ofconsole.log
for test output. Updated JS syntax to ES6const
and template strings where appropriate.