-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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: refactor test-https-truncate #10225
Conversation
* use common.mustCall() where appropriate * Buffer.allocUnsafe() -> Buffer.alloc() * do crypto check before loading any additional modules * specify 1ms duration for `setTimeout()`
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
@@ -14,7 +15,7 @@ const key = fs.readFileSync(common.fixturesDir + '/keys/agent1-key.pem'); | |||
const cert = fs.readFileSync(common.fixturesDir + '/keys/agent1-cert.pem'); | |||
|
|||
// number of bytes discovered empirically to trigger the bug | |||
const data = Buffer.allocUnsafe(1024 * 32 + 1); | |||
const data = Buffer.alloc(1024 * 32 + 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.
May I ask why alloc
is preferred over allocUnsafe
in a test?
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.
May I ask why alloc is preferred over allocUnsafe in a test?
I don't feel strongly about it, but here's the thinking:
-
If we use
Buffer.alloc()
, then we get a buffer with predictable contents. While the contents don't play a role in the test here, all things being equal, I prefer having repeatable, predictable tests over tests with unpredictable inputs involved (unless, of course, the purpose is to do something like fuzz testing). -
This is not performance-critical code, so the (small, in this case) performance hit of zero-filling is not a concern.
-
The very name of
Buffer.allocUnsafe()
seems to suggest "Don't use it unless you need to and know what you're doing." We don't need it here, so...
Again, I don't feel particularly strongly about it, but it does seem to me that if we can use either alloc()
or allocUnsafe()
in a test without any real difference, then we should default to alloc()
. (The situation is different in lib
where it stands to reason that tiny performance hits can add up to significant performance issues for some use cases. I'm only talking about test
here.)
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.
Yes, I'm fine with that, was just curious.
Landed in 8bad37a |
* use common.mustCall() where appropriate * Buffer.allocUnsafe() -> Buffer.alloc() * do crypto check before loading any additional modules * specify 1ms duration for `setTimeout()` PR-URL: #10225 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
* use common.mustCall() where appropriate * Buffer.allocUnsafe() -> Buffer.alloc() * do crypto check before loading any additional modules * specify 1ms duration for `setTimeout()` PR-URL: nodejs#10225 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
* use common.mustCall() where appropriate * Buffer.allocUnsafe() -> Buffer.alloc() * do crypto check before loading any additional modules * specify 1ms duration for `setTimeout()` PR-URL: nodejs#10225 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
* use common.mustCall() where appropriate * Buffer.allocUnsafe() -> Buffer.alloc() * do crypto check before loading any additional modules * specify 1ms duration for `setTimeout()` PR-URL: nodejs#10225 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
* use common.mustCall() where appropriate * Buffer.allocUnsafe() -> Buffer.alloc() * do crypto check before loading any additional modules * specify 1ms duration for `setTimeout()` PR-URL: nodejs#10225 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
* use common.mustCall() where appropriate * Buffer.allocUnsafe() -> Buffer.alloc() * do crypto check before loading any additional modules * specify 1ms duration for `setTimeout()` PR-URL: #10225 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
* use common.mustCall() where appropriate * Buffer.allocUnsafe() -> Buffer.alloc() * do crypto check before loading any additional modules * specify 1ms duration for `setTimeout()` PR-URL: #10225 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
* use common.mustCall() where appropriate * Buffer.allocUnsafe() -> Buffer.alloc() * do crypto check before loading any additional modules * specify 1ms duration for `setTimeout()` PR-URL: #10225 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
* use common.mustCall() where appropriate * Buffer.allocUnsafe() -> Buffer.alloc() * do crypto check before loading any additional modules * specify 1ms duration for `setTimeout()` PR-URL: #10225 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Checklist
make -j4 test
(UNIX), orvcbuild test nosign
(Windows) passesAffected core subsystem(s)
test https
Description of change
setTimeout()