test: buffer read functions tests for noAssert in one file #10713
Conversation
|
-1 We already have tests that cover the
etc. ... If anything, we should at least be combining these tests into two files: one for reads (e.g. 'test-buffer-read-numbers') and one for writes (e.g. 'test-buffer-write-numbers'). Perhaps we might even combine them all into one file (e.g. 'test-buffer-read-write-numbers'). |
None of the existing tests cover I think we can use this as a starting point and combine the other read* tests into it. Either that or rename this test to add
|
|
(My vote would be add @larissayvette The commit message (and the PR description) don't contain any useful information. Can you please provide a description of the change? Perhaps something like this?: "This change tests the functionality of |
|
|
||
| assert.doesNotThrow( | ||
| () => assert.strictEqual(buff[funx](...args, true), expected), | ||
| 'noAssert does not change return value' |
There was a problem hiding this comment.
Nit: This would probably be better as something like: noAssert should not change return value for valid ranges
There was a problem hiding this comment.
Alright doing those changes
larissayvette
force-pushed
the
test-double
branch
from
0bb9642
to
1a4769e
Compare
larissayvette
changed the title
|
@Trott I have updated the PR |
|
@mscdex Are you still opposed to this given what I wrote above? If so, do you have an opinion as to a better way to introduce testing for the |
|
@Trott Additional code coverage is fine if that's really what this PR is doing, but at the same time I'm not keen on exacerbating the issue of test fragmentation for |
PR-URL: nodejs#10713 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>
|
Landed in c8ed5f2. Thanks, @larissayvette! 🎉 Next steps:
|
PR-URL: nodejs#10713 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#10713 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#10713 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#10713 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #10713 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #10713 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>
Checklist
Affected core subsystem(s)
test
noAsserttested inbuffer.read()functions