-
Notifications
You must be signed in to change notification settings - Fork 30k
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: increase coverage of internal/util #10964
Conversation
|
||
if (!process.versions.openssl) { | ||
assert.throws(() => util.assertCrypto() | ||
, /Node.js is not compiled with openssl crypto support/); |
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.
Does this pass make lint
? The comma should be on the previous line.
|
||
if (!process.versions.openssl) { | ||
assert.throws(() => util.assertCrypto() | ||
, /Node.js is not compiled with openssl crypto support/); |
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.
The regex should include. ^
and $
.
assert.throws(() => util.assertCrypto() | ||
, /Node.js is not compiled with openssl crypto support/); | ||
} else { | ||
try { |
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.
Why not use assert.doesNotThrow()
?
@cjihrig OK, updated :) |
@@ -0,0 +1,12 @@ | |||
// Flags: --expose-internals |
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.
FWIW, this test won't actually increase test coverage reported at coverage.nodejs.org unless it is run with Node without crypto. And then, it would lose coverage on the else
condition.
Failures in CI appear to be unrelated. |
PR-URL: #10964 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Landed in a7172b5 |
PR-URL: #10964 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: #10964 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
I had to revert this from v6.x-staging due to an error
It would appear that |
Odd... I was certain I had run that test locally. Likely missing another commit. This one is likely safe to omit |
if this gets backported it should come with #11620 |
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
test