test_runner: allow retryable tests

[redyetidev]

Adds a new retries(X) method to the test_runner, which allows users to run tests multiple times (in case of failure). This value can also be hardcoded via the { retries } option.

(Fixes #48754)

[nodejs-github-bot]

Review requested:

• [ ] @nodejs/test_runner

[github-actions[bot]]

The https://github.com/nodejs/node/labels/notable-change label has been added by @RedYetiDev.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

[cjihrig]

This seems like the wrong design for this feature. We shouldn't be creating a new class. I would expect this to be an option when users create a test like:

test('foo', { retries: 5 }, () => {});

I also don't think this is something we should support on the test context object. This also needs to take test hooks into consideration. I would double check with other frameworks, but there is an expectation that certain hooks run again when a test is retried.

[redyetidev]

This seems like the wrong design for this feature. We shouldn't be creating a new class. I would expect this to be an option when users create a test like:

test('foo', { retries: 5 }, () => {});

I also don't think this is something we should support on the test context object. This also needs to take test hooks into consideration. I would double check with other frameworks, but there is an expectation that certain hooks run again when a test is retried.

Everything you said makes sense, except why wouldn't it be supported on the test context object?

[cjihrig]

Everything you said makes sense, except why wouldn't it be supported on the test context object?

Actually, let's survey how other frameworks implement this before committing to a design.

[redyetidev]

AFAIK, Mocha: this.retries(5); (On the TestContext) Jest: jest.retryTimes(5);

But I do think you are right about the attempt() method being unneeded and the use of the test() method for both is better.

[cjihrig]

I'd be OK with following Mocha on this.

[redyetidev]

I'd be OK with following Mocha on this.

I don't know, I think a config option { retries } seems more like the rest of Node.js's testings

[cjihrig]

I personally prefer that style. One nice thing about having it on the test context is that you can dynamically set it if you detect something happening while the test is running (like encountering a specific error). At the end of the day, both APIs would work the same way under the hood, so they could both be supported.

[redyetidev]

I personally prefer that style. One nice thing about having it on the test context is that you can dynamically set it if you detect something happening while the test is running (like encountering a specific error). At the end of the day, both APIs would work the same way under the hood, so they could both be supported.

True, I can implement it :-).

[redyetidev]

Moving to draft for development

[redyetidev]

@cjihrig, I've made the requested changes.

[redyetidev]

I need to address a few errors.

[MoLow]

I am -1 on adding this to node as it is an anti-pattern, and it can be achieved with a few lines of code, or a npm package

[juliangruber]

I am -1 on adding this to node as it is an anti-pattern, and it can be achieved with a few lines of code, or a npm package

Why is it considered an anti pattern?

[MoLow]

Why is it considered an anti pattern?

since it is mostly abused by encouraging tests to be non-deterministic and non-predictable (in the majority of cases i've seen). such features are often used to hide flakiness instead of fixing it.

[redyetidev]

since it is mostly abused by encouraging tests to be non-deterministic and non-predictable (in the majority of cases i've seen). such features are often used to hide flakiness instead of fixing it.

I think it'll be helpful when users know that some tests fail and want to give it a second (or third) chance to succeed.

We can see lots of instances where this will be helpful just by looking at implementations using Jest: https://github.com/search?q=%2FJest%5C.retryTimes%5C%28%5Cd%2F&type=code

Or, like @cjihrig mentioned, you could increment the retries by +1 when you encounter a specific error, such as a network issue.

[targos]

It's fine to add the notable-change label, but most important for the changelog is https://github.com/nodejs/node/labels/semver-minor

[redyetidev]

It's fine to add the notable-change label, but most important for the changelog is semver-minor PRs that contain new features and should be released in the next minor version.

Thanks for the info! I think it is a notable change because we are adding functionality to the test runner (and the functionality exists in other test runners)

[targos]

Yes I understand, but semver-minor changes are automatically considered notable by the release tool. The notable-change label is only really useful for notable semver-patch changes.

[redyetidev]

Yes I understand, but semver-minor changes are automatically considered notable by the release tool. The notable-change label is only really useful for notable semver-patch changes.

That's good to know! we learn something new every day :-)

[redyetidev]

Would it be worth it to also add a reruns value, to rerun the test despite failure/success?

[atlowChemi]

I am -1 on adding this to node as it is an anti-pattern, and it can be achieved with a few lines of code, or a npm package

I second this. I am -1 for adding this as well.

[benjamingr]

Basically https://github.com/nodejs/node/issues/48754#issuecomment-1637000885 - I'm +1 because I think that this can't be done well in userland (it's not "just a few lines of code" as implied) and otoh it's hypocritical to say it's bad while doing it ourselves in our own CI since that's a purity argument (sure, retries and flakey tests are bad but they're a fact of life and fixing tests while retrying seems like something that's pretty common)

[redyetidev]

since it is mostly abused by encouraging tests to be non-deterministic and non-predictable (in the majority of cases i've seen). such features are often used to hide flakiness instead of fixing it.

There also cases where tests have to be non-predictible. If a test is `fetching a website, and that site returns a strange response, this feature would let the test re-try to succeed (for example). There are thousands of examples of use-cases for this feature.

[cjihrig]

@benjamingr are you able to find a path forward for this? There seems to be support, but also at least two -1's. I think we should decide if this is something we will implement, or close the PR. (I'm trying to get through some of my PR review requests)

[benjamingr]

I think we wait for Moshe to change his mind (enough users asking for this for example) or for us to change our mind 😅

[cjihrig]

I just noticed this comment 😆 - https://github.com/nodejs/node/issues/48754#issuecomment-1879834139

[redyetidev]

I still think this is a step in the right direction. Test failures are unavoidable, but handling them isn't impossible.

[benjamingr]

@cjihrig yeah I pointed that out to him (that he said he wouldn't block, forgot about that and blocked :D)