-
Notifications
You must be signed in to change notification settings - Fork 29.3k
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
These were missed from 52bae22 PR-URL: #8841 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>
Changed `var` to `const`, strings to template literals, and assert.equal to assert.strictEqual where appropriate. PR-URL: #8841 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>
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. :) |
These were missed from 52bae22 PR-URL: #8841 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>
Changed `var` to `const`, strings to template literals, and assert.equal to assert.strictEqual where appropriate. PR-URL: #8841 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>
These were missed from 52bae22 PR-URL: #8841 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>
Changed `var` to `const`, strings to template literals, and assert.equal to assert.strictEqual where appropriate. PR-URL: #8841 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>
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.