test: refactor test runner run plan tests #57304
Conversation
|
Review requested:
|
nodejs-github-bot
added
needs-ci
pmarchini
changed the title
|
|
||
| test('planning should FAIL when wait time expires before plan is met', (t) => { | ||
| t.plan(2, { wait: 500 }); | ||
| setTimeout(() => { |
There was a problem hiding this comment.
Does this need to be unrefed so the event loop isn't forced to stay alive?
There was a problem hiding this comment.
In this case, unfortunately, forceExit is needed in order to prevent the test from waiting for the setTimeout
const stream = run({
files: [join(testFixtures, 'plan', 'timeout-expired.mjs')],
forceExit: true
});There was a problem hiding this comment.
@pmarchini why though? can't we just unref it?
test('planning should FAIL when wait time expires before plan is met', (t) => {
t.plan(2, { wait: 500 });
setTimeout(() => {
t.assert.ok(true);
}, 50_000_000).unref();
});There was a problem hiding this comment.
just to clarify, do you typically expect users to do this, or is it mainly to improve the test? 😁
There was a problem hiding this comment.
The reason why I used forceExit is that it seemed more "generic" to me, but I didn't think about unreferencing the timeout.
So, no problem at all changing the test 😁
There was a problem hiding this comment.
do you typically expect users to do this
No, I was thinking of the test. IMHO having the timeout unrefed here is more explicit & readable to the flow of the test than having the forceExit flag elsewhere.
If you disagree I am OK with it staying this way 🙂
There was a problem hiding this comment.
nono absolutely, I agree, thanks for your suggestion 😁
I'm pushing now a new commit!
github-actions
bot
removed
the
request-ci
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #57304 +/- ##
==========================================
- Coverage 90.34% 90.22% -0.13%
==========================================
Files 629 630 +1
Lines 184403 185166 +763
Branches 36040 36226 +186
==========================================
+ Hits 166607 167064 +457
- Misses 10919 11114 +195
- Partials 6877 6988 +111 |
| test('deeply nested tests', async (t) => { | ||
| t.plan(1); | ||
|
|
||
| await t.test('level 1', async (t) => { |
There was a problem hiding this comment.
| await t.test('level 1', async (t) => { | |
| t.test('level 1', async (t) => { |
There was a problem hiding this comment.
It might be worth keeping the awaits so that the commit can be backported.
| await t.test('level 1', async (t) => { | ||
| t.plan(1); | ||
|
|
||
| await t.test('level 2', (t) => { |
There was a problem hiding this comment.
| await t.test('level 2', (t) => { | |
| t.test('level 2', (t) => { |
There was a problem hiding this comment.
following
It might be worth keeping the
awaits so that the commit can be backported.
Do you think we can resolve these nits? 😁
|
|
||
| test('parent test', async (t) => { | ||
| t.plan(1); | ||
| await t.test('child test', (t) => { |
There was a problem hiding this comment.
| await t.test('child test', (t) => { | |
| t.test('child test', (t) => { |
+100 on |
This is a followup refactor related to #56765 (comment) , #56765 (comment)