test: fix Buffer.from(ArrayBufferView) call
#43614
Conversation
nodejs-github-bot
added
needs-ci
github-actions
bot
removed
the
request-ci
This comment was marked as outdated.
This comment was marked as outdated.
| @@ -42,6 +42,7 @@ assert.strictEqual(d.length, 0); | |||
| } | |||
|
|
|||
| // Test creating a Buffer from a Uint32Array | |||
| // Note: it is implicitly interpreted as Array of integers modulo 256 | |||
There was a problem hiding this comment.
Honestly, that's not the behavior I'd have expected...
There was a problem hiding this comment.
Buffer.from() is not defined by documentation for any ArrayBufferView except for Uint8Array and Buffer.
I agree that it's not what anyone would expect. There were related bugs in node and undici, and who knows how many similar bugs in ecosystem.
Not sure if this test was intentional or not, so probably it shouldn't be removed for now, just marked with a comment.
I'll add uint8-based test in case this is what was intended.
This comment was marked as outdated.
This comment was marked as outdated.
| @@ -47,7 +47,7 @@ for (const ctor of intTypedConstructors) { | |||
| } | |||
|
|
|||
| { | |||
| const buf = new Uint16Array(10); | |||
| const buf = new Uint8Array(10); | |||
There was a problem hiding this comment.
This test looks like it might be using Uint16Array intentionally. There are a couple of ways to work around this issue (for example, use buf.toString() instead of Buffer.from(buf).toString('hex') or use Buffer.from(buf.buffer)), but it seems to me that this test is performing the exact same test as the loop above for ctor === Uint16Array. So I'd suggest removing this block (line 50 to 56) entirely.
There was a problem hiding this comment.
I'm not sure if I understand a potential intention here. Using Uint16Array in context of this method makes sense in terms of generating random utf16 strings (i.e. String.fromCharCode(...crypto.getRandomValues(new Uint16Array(charLength)))) but it's probably not worth testing explicitly.
Technically, this test was worse than ctor === Uint16Array above, as it tested only 10 bytes out of 20 generated. :)
I agree that at this point it doesn't serve any purpose. Documentation allows to use instances of Buffer and the test for it was lacking, so I guess Buffer.alloc(10) will make it useful.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
PR-URL: nodejs#43614 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
LiviaMedeiros
force-pushed
the
test-buffer-from-abv
branch
from
26a5429
to
5c6eee4
Compare
|
Landed in 5c6eee4 |
PR-URL: #43614 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
PR-URL: #43614 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
PR-URL: #43614 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
PR-URL: nodejs/node#43614 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
No description provided.