Skip to content
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

Fast fail feature for the node:test context #42990

Closed
null8626 opened this issue May 6, 2022 · 19 comments
Closed

Fast fail feature for the node:test context #42990

null8626 opened this issue May 6, 2022 · 19 comments
Labels
feature request Issues that request new features to be added to Node.js. test_runner Issues and PRs related to the test runner subsystem.

Comments

@null8626
Copy link
Contributor

null8626 commented May 6, 2022

What is the problem this feature will solve?

A test's progress can sometimes be crucial for the next one, if one of them fails, say like this:

let module;
let instance;

test("importing module", async () => {
  module = await import("...");
});

test("using module feature 1", () => {
  instance = new module.SomeExportedClass("arg1", { option: true });
});

test("request to API", async () => {
  await instance.apiEndpoint({ ... });
});

While yes, if the library has checks, this would be pointless, but it would be better to have an option for the the rest of the tests to fail if one had failed.

What is the feature you are proposing to solve the problem?

Something like this:

test("test for myModule", { fastFail: true }, (t) => {
  t("test that fails", () => {
    // This should make the next tests to fail as well.
    assert(false);
  });

  t("test that will never succeed", () => {
    console.log("This won't log to the console.");
  });
});

What alternatives have you considered?

No response

@null8626 null8626 added the feature request Issues that request new features to be added to Node.js. label May 6, 2022
@targos targos moved this to Pending Triage in Node.js feature requests May 6, 2022
@targos targos added the test_runner Issues and PRs related to the test runner subsystem. label May 6, 2022
@VoltrexKeyva
Copy link
Member

@nodejs/test_runner

@RaisinTen
Copy link
Contributor

GitHub Actions does provide something like this - https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstrategyfail-fast. Not sure if this is something that's already present in other existing test runners.

@ljharb
Copy link
Member

ljharb commented May 6, 2022

I believe jest does as well. It’s typically present in CI runners with a matrix, but it’s very rare for a test runner to have it.

Usually when running your tests, you don’t want it to fail fast - you want the maximal information. Otherwise, you’re constantly running, fixing one problem, and rerunning - it’s a game of whack-a-mole.

@null8626
Copy link
Contributor Author

null8626 commented May 6, 2022

But even though it might be rare for it to exist on other existing test runners, it can be useful for it to fail and not continue further with the other tests until the one failed is also fixed

@RaisinTen
Copy link
Contributor

You could move the part you would expect to fail the rest of the tests outside the test and that should work as expected, right?

const test = require('node:test');

test("test for myModule", async (t) => {
  let module = require('node:does-not-exist');
  let instance;

  await t.test("using module feature 1", () => {
    instance = new module.SomeExportedClass("arg1", { option: true });
  });

  await t.test("request to API", async () => {
    await instance.apiEndpoint({ ... });
  });
});

@ljharb
Copy link
Member

ljharb commented May 6, 2022

@vierofernando the claim i'm making is that it's rare because it's not actually useful.

With no fast-fail, if you only want one failure, you can control-C, or mentally ignore everything after the first failure - both of which are easy.

With fast-fail, if you want more than one failure, you have to rerun your entire test suite - which is hard.

@juliangruber
Copy link
Member

I assume this depends on the test suite size.

When you have a large test suite, you want all tests to always run, because test runs are expensive.

When you have a small test suite, running it is cheap so the main point becomes convenience - you don't want to have to scroll through / see test output after the first failure, because that's already all you want to know.

@ljharb
Copy link
Member

ljharb commented May 6, 2022

@juliangruber sure! but since running is cheap, if you want the first failure, you can quickly jump to the top of the scrollback (which you cleared prior to running the test suite) and it'll be right there :-)

@juliangruber
Copy link
Member

Good point! You could also use Unix tools like less for this. Or have a test runner that offers this.

It's a question of dx vs simplicity, or docs I would say?

@juliangruber
Copy link
Member

I wouldn't be surprised if 80% of node users don't know how to clear their scrollback

@ljharb
Copy link
Member

ljharb commented May 6, 2022

agreed, but i'd also be surprised if even 1% of that set of users wanted to see one failure at a time ;-)

@himself65
Copy link
Member

I think your example is not a good idea. test API shouldn't be related to each other.

the correct API and usage should be like

let module;
let instance;

before("importing module", async () => {
  module = await import("...");
});

before("using module feature 1", () => {
  instance = new module.SomeExportedClass("arg1", { option: true });
});

test("request to API", async () => {
  await instance.apiEndpoint({ ... });
});

@github-actions
Copy link
Contributor

github-actions bot commented Nov 3, 2022

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Nov 3, 2022
@kmannislands
Copy link

It's easily possible to bail out on the first failure by implementing your own entry using the run API if that's what you desire.

It seems like unnecessary complexity to support this feature in the default CLI, IMO.

I'd suggest closing this as "Won't do."

@EndangeredMassa
Copy link

I use Fail Fast options in other test runners as a debugging tool in CI, specifically. When CI is flakey or just takes a long time to run, I want to know as soon as possible when there's a failure so that I can act. Watching fast moving logs for a single failure is annoying. Seeing the full test suite failed indicator light up is much easier. This reduces the feedback loop.

I consider this a temporary flag. I wouldn't run normal test suites with this on, because (as others have said) you usually want to know about all failing tests. However, as a tool I can use during debugging, then revert it, I find it very useful.

It's not a necessity, sure. Lots of test runners don't even have it. It is very handy when you need it, though.

@MoLow
Copy link
Member

MoLow commented Jun 5, 2023

this feature can be achieved very in a very straightforward way using reporters, so I will close now.
for convenience, I have created a user-land implementation of this: https://www.npmjs.com/package/@reporters/bail
even if you do not want to install an npm package for this, implementing this yourself is ±5 lines of code

@MoLow MoLow closed this as completed Jun 5, 2023
@extremeheat
Copy link

It's a shame a trivial, but useful, feature like this doesn't exist natively inside the test runner as a CLI flag. For codebases that have many tests inside them, waiting for an entire test suite to run before being able to read an error stack trace can be incredibly annoying.

I use mocha --bail or jest --bail all the time, if it needs hacking on some extra code to the project to get this functionality or requires installing some other 3P package, you may as well continue using mocha or jest.

@MoLow
Copy link
Member

MoLow commented Jul 2, 2023

It's a shame a trivial, but useful, feature like this doesn't exist natively inside the test runner as a CLI flag. For codebases that have many tests inside them, waiting for an entire test suite to run before being able to read an error stack trace can be incredibly annoying.

PR's attempting to add this feature are welcome. just note adding cli flags to node.js isn't as easy as adding them for standalone test runners.
I recommend @cjihrig 's blog post https://cjihrig.com/test_runner_expectations explaining why not evert "trivial" feature is going to be implemented into node.js test runner

@Pheromon
Copy link

Pheromon commented Dec 5, 2023

WTH a bail() and timeout() function to migrate almost seamlessly from mocha would have been so great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

No branches or pull requests