test: refactor mkdtemp test and added async#12080
test: refactor mkdtemp test and added async#12080lucamaraschi wants to merge 1 commit intonodejs:masterfrom
Conversation
There was a problem hiding this comment.
Nit: there is common.noop, so I think it makes sense to use it.
lucamaraschi
force-pushed
the
test-mkdtemp-async-prefix-check
branch
from
6c6690f to
278d515
Compare
richardlau
left a comment
richardlau
left a comment
There was a problem hiding this comment.
I'd prefer the variables are renamed. As they currently are they sound like they refer to the values that occurred during execution and are being tested rather than the expected error or test inputs.
There was a problem hiding this comment.
Could this be expectedError?
lucamaraschi
force-pushed
the
test-mkdtemp-async-prefix-check
branch
from
278d515 to
1323058
Compare
There was a problem hiding this comment.
Should the callback be be () => common.fail('Async callback should not be called')?
There was a problem hiding this comment.
That could work!
There was a problem hiding this comment.
On second thought, I'm not sure that can happen because of the throw, but it would be good insurance anyways imo
|
Oops, didn't mean to approve. Oh well lol |
lucamaraschi
force-pushed
the
test-mkdtemp-async-prefix-check
branch
from
1323058 to
b83338c
Compare
There was a problem hiding this comment.
I think this can be common.mustNotCall()
There was a problem hiding this comment.
Semantically the two have the same result. We should probably start defining a list of guidelines to use (or maybe I just missed it) for tests.
@cjihrig I guess common.mustNotCall() should be the right pick here, I am right?
There was a problem hiding this comment.
common.mustNotCall() was added to prevent calling common.fail() in an otherwise empty function.
lucamaraschi
force-pushed
the
test-mkdtemp-async-prefix-check
branch
from
b83338c to
12cc305
Compare
This test refactored the original test for mkdtempSync prefix validation and added the test also for the async function mkdtemp.
lucamaraschi
force-pushed
the
test-mkdtemp-async-prefix-check
branch
from
12cc305 to
daa39cd
Compare
This test refactored the original test for mkdtempSync prefix validation and added the test also for the async function mkdtemp. PR-URL: #12080 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
|
Landed in 085c1f8 |
This test refactored the original test for mkdtempSync prefix validation and added the test also for the async function mkdtemp. PR-URL: nodejs#12080 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
|
This test does not pass on LTS. would you be willing to backport to v6.x-staging? |
10 participants
This test refactored the original test for mkdtempSync prefix validation
and added the test also for the async function mkdtemp.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
test fs