-
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
lib,test: improve validateNumber and validators coverage #33288
Conversation
Adds a min/max predicate to validateNumber (which will be used by the QUIC impl, extracted from that PR) ... and improves test coverage in test-validators.js Signed-off-by: James M Snell <jasnell@gmail.com>
89d8f25
to
9c44e2e
Compare
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.
Looks reasonable to me.
This has a pretty bad performance impact on Buffer functions: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/589/
I just copied the results with a high confidence level. |
|
||
[1, false, '', {}, [], 1n, null, undefined].forEach((i) => { | ||
assert.throws(() => validateBuffer(i, 'foo'), invalidArgTypeError); | ||
}); |
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.
Do we need these tests? I suggest to add more "integration" tests instead that test these functions where the code is actually implemented. That way we are certain that the code is properly executed by looking at the coverage report.
Closing. Will address a different way in the QUIC code to avoid the perf regression on Buffer. |
Adds a min/max predicate to validateNumber (which will be used
by the QUIC impl, extracted from that PR) ... and improves test
coverage in test-validators.js
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes