-
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: Updates to test-stream2-unpipe-drain #10033
test: Updates to test-stream2-unpipe-drain #10033
Conversation
var assert = require('assert'); | ||
var stream = require('stream'); | ||
const common = require('../common'); | ||
const stream = require('stream'); |
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.
This require()
could move down below the common.hasCrypto
check.
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.
Updates made per recommendation.
@@ -55,6 +51,6 @@ src1.once('readable', function() { | |||
|
|||
|
|||
process.on('exit', function() { | |||
assert.equal(src1.reads, 2); | |||
assert.equal(src2.reads, 2); | |||
common.mustCall(src1._read, 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.
This is incorrect. common.mustCall()
should be added prior to the process.on('exit', ...)
handler.
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.
@storytimesolutions If you want to make this land-able quickly, you can remove the common.mustCall()
additions and restore the assert.equal()
statements but use assert.strictEqual()
instead.
In thie particular case, the common.mustCall()
approach (which is generally preferred) is a bit trickier than it usually is.
@Trott Thanks for the advice. Implemented. |
@cjihrig Code has been changed to address your comment. PTAL at your convenience and update your review if appropriate. Thanks! |
@storytimesolutions This code fails hard because the |
@@ -30,12 +27,11 @@ function TestReader(id) { | |||
util.inherits(TestReader, stream.Readable); | |||
|
|||
TestReader.prototype._read = function(size) { | |||
this.reads += 1; | |||
this.push(crypto.randomBytes(size)); | |||
this.push(Buffer.alloc(size)); |
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 isn't using crypto to push data to the stream, the common.hasCrypto
check is still needed?
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.
Nope, it can go too. Good catch.
...and the assertions are comparing functions to integers, so the assertions will always fire... |
1. Change var to const 2. Remove dependency crypto
da86dab
to
f04fb60
Compare
Since the fixes were relatively small, I went ahead and made them and pushed them to this PR. Hope that's OK, @storytimesolutions! @cjihrig Can you confirm that this corrects the issues you flagged and, if so, please update your review? Thanks! New CI: https://ci.nodejs.org/job/node-test-pull-request/5611/ |
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 if the CI is happy
Landed 627fa93 |
- Change var to const - Remove dependency crypto PR-URL: #10033 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
- Change var to const - Remove dependency crypto PR-URL: #10033 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
- Change var to const - Remove dependency crypto PR-URL: #10033 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
- Change var to const - Remove dependency crypto PR-URL: #10033 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
- Change var to const - Remove dependency crypto PR-URL: #10033 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
- Change var to const - Remove dependency crypto PR-URL: #10033 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
- Change var to const - Remove dependency crypto PR-URL: #10033 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
- Change var to const - Remove dependency crypto PR-URL: #10033 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
- Change var to const - Remove dependency crypto PR-URL: #10033 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
- Change var to const - Remove dependency crypto PR-URL: #10033 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Checklist
make -j8 test
(UNIX), orvcbuild test nosign
(Windows) passesAffected core subsystem(s)
Description of change