Allow bailing early when test fails

[nicholaswmin]

Needs a Bail Out/Fail Fast strategy

What is the problem this feature will solve?

Not having to sift through a jumbled up mess of stack traces as I'm cleaning up tests after significant refactoring of the unit i'm testing.

Spit out the failing test and it's stack trace, then Exit 1 as soon as a test fails

Proposal

A bail flag, i.e --bail that hints to the runner that a failure should report and exit.

Alternatives

Cooking up an Abort Signal wiring but that's really me writing machinery code to test.. the code... which is just bad taste, at least in most cases.

[MoLow]

this will only work as expected in case of no parallelism/concurrency. otherwise, there is just the option for a best effort where as soon as the orchestrator knows of a failed test it tries its best to stop other processes from executing tests. a naive approach can be found here: https://www.npmjs.com/package/@reporters/bail

[nicholaswmin]

That is exactly right. Parallelisation messes up with a lot of other potential options - sorted tests being another case it doesn't play with.

I do not see parallelism as a first-class concern - I'm much more inclined to get fed up with running tests if I have to mindlessly scroll through pages of stack traces over and over.

I like parallelisation because it forces me to rethink about potential state leaks between tests but that's pretty much it. The test runner is fast enough as it is.

[marco-ippolito]

My previous (failed) attempt: https://github.com/nodejs/node/pull/48919

[nicholaswmin]

Sorry It's not quite clear to me what's going on there.

The spawned process phantoms out? Can I take a stab at it?

[marco-ippolito]

Sorry It's not quite clear to me what's going on there.

The spawned process phantoms out? Can I take a stab at it?

Go ahead 🙂

[nicholaswmin]

@MoLow Why do you consider your approach naive?

So, here:

module.exports = async function* bail(source) {
  for await (const event of source) {
    if (event.type === 'test:fail') {
      /* c8 ignore start */
      yield `\n\u001b[31m✖ Bailing on failed test: ${event.data.name}\u001b[0m\n`;
      throw new Error('Bail');
    }
    /* c8 ignore stop */
  }
};

This line:

throw new Error('Bail');

This won't report the failure, won't it? Is this why you consider this naive? I also have concerns about phantom child_process spawns;

I remember I had some issues back in 2017 with the child_process module. The spawned children could become phantoms on their own in a lot of cases.

Does it reliably detect exceptions in the spawns or does it still need special handling in case the IPC signalling get disconnected?

[marco-ippolito]

@MoLow Why do you consider your approach naive?

So, here:

module.exports = async function* bail(source) {
  for await (const event of source) {
    if (event.type === 'test:fail') {
      /* c8 ignore start */
      yield `\n\u001b[31m✖ Bailing on failed test: ${event.data.name}\u001b[0m\n`;
      throw new Error('Bail');
    }
    /* c8 ignore stop */
  }
};

This line:

throw new Error('Bail');

This won't report the failure, won't it? Is this why you consider this naive? I also have concerns about phantom child_process spawns;

I remember I had some issues back in 2017 with the child_process module. The spawned children could become phantoms on their own in a lot of cases.

Does it reliably detect exceptions in the spawns or does it still need special handling in case the IPC signalling get disconnected?

The issue I see is that test will continue to run, this will only hide the result from the reporter. Immagine a big test suite failing on the first test, will continue to run for a while

[nicholaswmin]

Thanks 🎯 ,

I suppose the main stuff lives in runner/harness.

Just off the top of my head, my plan is this:

• forward a --bail flag to the spawned children.

• main

  • Listens on subprocess:test-failed and prints the results
  • Listens on child:error
  • Signals the rest of the bunch via subprocess.kill('SIGTERM') to kill themselves.

• child

  • Listens on the 'test:fail' event:
  • signals the main via iPC, sending'test-failed'informing of it's failure; plus it's test results as payload.
  • throws an Error and dies as a result of it's exception

• This ping-pong style of wiring sounds brittle, plus it might interfere with generic error handling on child process errors. A child:error event now has 2 different meanings.

• Solution A: An exception in the child without a preceding test-failed event should be handled like a regular error and not a test failure. This is starting to sound real dodgy, depending on order of events and introducing additional state in the main.

• Solution B: The child does not throw an exception on test-failure. Instead it signals the main that it's test has failed, who is responsible for issuing a SIGTERM back to it which exits 0. This misaligns usual runner behavior, AFAIK mocha exits with 1 on test failures. However, this is a child process, not the main. I don't like this either.

I also have big concerns about errant children - I remember working with child_process back in 2017? and having to add timeouts to IPC events in case a child stops responding. Is this still a concernm and if yes, how does current parallel orchrstrator/main handle this? I'd rather not cook up my own homegrown rube-goldbergs.

Last:

• What about shards? Any special considerations?
• Do we need to report the success results of the rest of the ongoing runners?