Lab crash on simple test code

[kanongil]

Support plan

• is this issue currently blocking your project? (yes/no): no
• is this issue affecting a production system? (yes/no): no

Context

• node version: v14.7.0
• module version with issue: 23.0.0
• last module version without issue:
• environment (e.g. node, browser, native): native
• used with (e.g. hapi application, another framework, standalone, ...):
• any other relevant information:

What are you trying to achieve or the steps to reproduce?

To run test suite without lab crashing. Eg. the following with lab fail.js -i 1:

fail.js:

'use strict';

const Hoek = require('@hapi/hoek');
const Lab = require('@hapi/lab');


// Test shortcuts

const lab = exports.lab = Lab.script();
const { it } = lab;


it('does not crash lab', async () => {

    await Hoek.wait(1);

    process.nextTick(() => {

        throw new Error('fail');
    });
});

it('has another test', async () => {

});

What was the result you got?

./<snip>/fail.js:19
      throw new Error('fail');
      ^

Error: fail
  at /<snip>/fail.js:19:15
  at processTicksAndRejections (internal/process/task_queues.js:75:11)

What result did you expect?

  
  .

1 tests complete
Test duration: 3 ms
Leaks: No issues

Note that lab doesn't crash when running the full suite, but it assigns the error to the second test.

This specific issue can probably be remedied by letting ticks settle before treating the test as "completed".

[geek]

I think we can handle this particular case of a pending nextTick, but I'm not certain how we would trap a setTimeout scheduled into the future. I also think this is up to the developer to handle any unfinished operations before resolving the test. That being said, here is a PR to fix this specific issue: #995

[devinivy]

Yeah, I am not sure how to reliably handle this. We can solve it for one tick, but what about two? Three? I just want to ensure that we're handling realistic/likely and not a variety of contrived cases.

If we could handle this in a generic way that would be excellent. Would that error's stack be able to point us back to the failing test? Could async hooks point us back there? Might require some research.

[gerardolima]

As I understand, lab does not intend to check for these situations and I have doubts it should be tackled, at all.

As @geek said, there will be other ways to delay exceptions and make asynchronous tests fail after their corresponding it/test completes. I believe that, if it should be possible to handle these, it will introduce arcane code, that most likely would be hard to understand -- and, so, prone to errors -- breaking one value lab has, simplicity, in favour of edge cases.

[kanongil]

@gerardolima I would say that it is fair to expect lab to at least wait until the microtask queue has been emptied, before moving on. This can be done in 1 line of code.

If the issue extends beyond the microtask queue, it should be handled by the feature I proposed in #988.

[gerardolima]

hey, @kanongil, I believe when you say this change can be done in one single line; my question is: should lab handle this? Is it expected a test runner to handle scheduled events in its normal flow? And, particularly this one, which has simplicity as one of its core values?

[kanongil]

This is not about scheduled events. I'm talking about not yet resolved nextTicks and related microtask queue events, which can only have been queued during the active test.

It is also about tests not crashing lab from an errant throw in code or test logic.

Node itself uses nextTick delays extensively.

[gerardolima]

I wasn't sure nextTicks could have side effects impacting other async/Promise it/test on the same describe/subject. If you say it's ok, I believe you.