Improve buffer test coverage #8552
Conversation
targos
added
buffer
nodejs-github-bot
added
the
buffer
|
LGTM if CI passes |
| @@ -8,6 +8,8 @@ const SlowBuffer = require('buffer').SlowBuffer; | |||
|
|
|||
| const b = Buffer.allocUnsafe(1024); | |||
| assert.strictEqual(1024, b.length); | |||
| assert.strictEqual(0, b.byteOffset); | |||
| assert.strictEqual(0, b.offset); | |||
There was a problem hiding this comment.
Um. Since when are unsafe buffers not sliced from a pool? Did I miss something?
There was a problem hiding this comment.
I ran this test locally 1000 times and it never failed. It works because it is the first allocation in this test and so will use the start of the pool. The goal here is to cover Buffer.prototype.offset so I can change it to assert.strictEqual(b.byteOffset, b.offset) if you prefer.
There was a problem hiding this comment.
Perhaps you should test .offset and .byteOffset to be equal to 0 on buffers created using Buffer.alloc(number) and/or Buffer.allocUnsafeSlow(number) — those are not pooled.
There was a problem hiding this comment.
Btw,
It works because it is the first allocation in this test and so will use the start of the pool.
is most likely not valid, as many buffers are allocated internally, so this is not the first allocation. Try removing const common = require('../common');, moving it below this check, or adding some other requires, for example.
There was a problem hiding this comment.
I changed it to use Buffer.alloc, PTAL
|
First commit ( The second one — not sure what happened to the |
targos
force-pushed
the
buffer-test-coverage
branch
from
0b9a8cb to
95a9e3e
Compare
|
|
It is actually testing something specific to the constructor. I couldn't find any relevant place in the existing tests so I created this file and gave it a not too specific name so that we could add tests to it in the future. |
In Buffer.prototype.compare, the first check makes sure that target is an instance of Buffer. The value cannot be falsy after that so we can safely get its length. PR-URL: nodejs#8552 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Add tests for untested branches and statements. Also convert some lines to const. PR-URL: nodejs#8552 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]>
targos
force-pushed
the
buffer-test-coverage
branch
from
95a9e3e to
8699ecd
Compare
|
Thanks, landed in 4c76881...8699ecd |
|
@targos should this be backported? |
|
@thealphanerd If #8256 is backported, the test commit can be. Otherwise I think it's not worth the trouble. |
In Buffer.prototype.compare, the first check makes sure that target is an instance of Buffer. The value cannot be falsy after that so we can safely get its length. PR-URL: #8552 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Add tests for untested branches and statements. Also convert some lines to const. PR-URL: #8552 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]>
|
The alloc tests probably should be backported to v4.x. |
|
@ChALkeR can you take a lead on that? |
Checklist
make -j4 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
buffer
Description of change
cc @nodejs/testing