test_runner: allow retryable tests #52626

RedYetiDev · 2024-04-21T19:39:30Z

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 · 2024-04-21T19:39:34Z

Review requested:

@nodejs/test_runner

github-actions · 2024-04-21T19:43:55Z

The notable-change PRs with changes that should be highlighted in changelogs. 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 · 2024-04-21T19:54:29Z

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 · 2024-04-21T19:56:20Z

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 · 2024-04-21T20:01:03Z

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 · 2024-04-21T20:04:00Z

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 · 2024-04-21T20:05:24Z

I'd be OK with following Mocha on this.

RedYetiDev · 2024-04-21T20:05:59Z

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 · 2024-04-21T20:08:34Z

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 · 2024-04-21T20:09:09Z

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 · 2024-04-21T20:09:29Z

Moving to draft for development

RedYetiDev · 2024-04-21T20:42:18Z

@cjihrig, I've made the requested changes.

RedYetiDev · 2024-04-21T20:46:11Z

I need to address a few errors.

MoLow · 2024-04-21T20:49:00Z

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 · 2024-04-21T23:07:02Z

doc/api/test.md

+});
+
+test('dynamic retries', (t) => {
+  t.retries(5);


May be worth documenting whether this can be called multiple times

Sure! I'll add that documentation tomorrow!

Unresolving because I don't think this was addressed.

test/fixtures/test-runner/output/retryable.js

lib/internal/test_runner/test.js

juliangruber · 2024-04-21T23:09:34Z

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 · 2024-04-22T08:51:08Z

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 · 2024-04-22T13:57:22Z

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 · 2024-04-22T14:35:51Z

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.

RedYetiDev · 2024-04-22T14:43:21Z

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 · 2024-04-22T14:47:26Z

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 · 2024-04-22T15:05:09Z

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 · 2024-04-25T15:04:25Z

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

MoLow

adding an explicit request changes as I think this should be added

atlowChemi · 2024-04-26T11:51:01Z

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 · 2024-04-26T15:16:06Z

Basically #48754 (comment) - 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 · 2024-05-06T17:59:50Z

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 · 2024-06-28T14:12:45Z

@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 · 2024-06-28T14:28:25Z

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 · 2024-06-28T21:05:08Z

I just noticed this comment 😆 - #48754 (comment)

RedYetiDev · 2024-06-28T21:06:59Z

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

benjamingr · 2024-06-28T21:56:44Z

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

removing my block. I still think it is bad practice but I see a lot of people asking for it

MoLow · 2024-06-30T06:49:26Z

@benjamingr @cjihrig I have removed my block

cjihrig · 2024-06-30T13:50:51Z

lib/internal/test_runner/test.js

@@ -209,6 +210,19 @@ class TestContext {
    return subtest.start();
  }

+  retries(number) {
+    validateNumber(number, 'number', 0, NumberMAX_SAFE_INTEGER);


Please use validateInteger() here.

cjihrig · 2024-06-30T13:51:31Z

lib/internal/test_runner/test.js

@@ -209,6 +210,19 @@ class TestContext {
    return subtest.start();
  }

+  retries(number) {


Maybe name this parameter count.

cjihrig · 2024-06-30T13:52:41Z

lib/internal/test_runner/test.js

@@ -209,6 +210,19 @@ class TestContext {
    return subtest.start();
  }

+  retries(number) {


All three of these functions are missing proper API docs.

cjihrig · 2024-06-30T13:54:11Z

doc/api/test.md

+  - REPLACEME
+-->
+
+Failed tests can be retried via a hardcoded `{ retries }` option, or


Suggested change

Failed tests can be retried via a hardcoded `{ retries }` option, or

Failing tests can be automatically retried via the `retries` test option, or

Note, retries needs API docs and we can link to them from here.

cjihrig · 2024-06-30T13:54:55Z

doc/api/test.md

+-->
+
+Failed tests can be retried via a hardcoded `{ retries }` option, or
+via the dynamic `retries()` TestContext prototype method.


Suggested change

via the dynamic `retries()` TestContext prototype method.

via the `retries()` `TestContext` prototype method.

Please also link from here.

cjihrig · 2024-06-30T14:09:10Z

doc/api/test.md

+  - REPLACEME
+-->
+
+Failed tests can be retried via a hardcoded `{ retries }` option, or


We should also mention that hooks are re-run for each retry.

cjihrig · 2024-06-30T14:18:25Z

doc/api/test.md

+Failed tests can be retried via a hardcoded `{ retries }` option, or
+via the dynamic `retries()` TestContext prototype method.
+
+Via the `.currentAttempt` property, the current attempt (the number of times


Can you remove the leading . from the property names.

Also, currentAttempt is a bit confusing IMO. I would expect it to be one-based and not only specific to retries. For example, if I run a test with no retries, it will run once. I would expect currentAttempt to be one on that single run.

cjihrig · 2024-06-30T14:23:55Z

lib/internal/test_runner/test.js

-            kCallbackAndPromisePresent,
-          ));
-          await SafePromiseRace([ret, stopPromise]);
+    while (true) {


Using an infinite loop here makes me nervous. Would a for loop work:

for (this.currentAttempt = 1; this.currentAttempt <= this.retries + 1; this.currentAttempt++)

You could still break out of the loop if the test passes.

cjihrig · 2024-06-30T14:31:56Z

test/fixtures/test-runner/output/retryable.js

+  test('should call before/after hooks (success)', {
+    retries: 3,
+  }, (t) => {
+    if (t.attempts < 3) {


Should this be currentAttempt?

cjihrig · 2024-06-30T14:39:49Z

test/fixtures/test-runner/output/retryable.js

+const common = require('../../../common');
+const test = require('node:test');
+const assert = require('node:assert');
+


I think we need a test case where a test (not suite) has retries set and there are several subtests that also have retries, but the last subtest never passes. We need to make sure there is no duplicated reporting.

benjamingr · 2024-06-30T16:10:22Z

removing my block. I still think it is bad practice but I see a lot of people asking for it

We can discuss an alternative design like --test-flaky that outputs (or tags) tests that passed with retries as flaky or something similar.

My point is both these things are true at the same time:

Retries are useful as a mechanism to not block CI while you have flaky tests
Retries are a very bad way to deal with flaky tests

So I'm all ears for a different design for retries while acknowledging that some way is needed and that it benefits from being in core.

RedYetiDev · 2024-07-01T16:03:21Z

I'm not opposed to that idea, maybe there should be a discussion on how to best achieve this?

Maybe the TSC could decide whether to use CLI flags or programatic methods (or another way)?

test_runner: allow retryable tests

6651910

nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Apr 21, 2024

Merge branch 'main' into retryable-tests

1263b91

RedYetiDev added the notable-change PRs with changes that should be highlighted in changelogs. label Apr 21, 2024

RedYetiDev marked this pull request as draft April 21, 2024 20:09

fix: merge conflict

ef74417

RedYetiDev force-pushed the retryable-tests branch from 94d6f4a to ef74417 Compare April 21, 2024 20:41

RedYetiDev marked this pull request as ready for review April 21, 2024 20:41

RedYetiDev requested a review from cjihrig April 21, 2024 20:42

RedYetiDev force-pushed the retryable-tests branch from e0cc94a to 5ebb2a0 Compare April 21, 2024 20:45

RedYetiDev force-pushed the retryable-tests branch from c241cb4 to f95464f Compare April 21, 2024 21:01

juliangruber reviewed Apr 21, 2024

View reviewed changes

lib/internal/test_runner/test.js Show resolved Hide resolved

test_runner: allow retry-able tests

ee1ee38

RedYetiDev force-pushed the retryable-tests branch from 3964a68 to ee1ee38 Compare April 22, 2024 13:12

targos added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 22, 2024

RedYetiDev removed the notable-change PRs with changes that should be highlighted in changelogs. label Apr 22, 2024

MoLow previously requested changes Apr 26, 2024

View reviewed changes

RedYetiDev requested a review from MoLow June 28, 2024 15:05

cjihrig reviewed Jun 30, 2024

View reviewed changes

	Failed tests can be retried via a hardcoded `{ retries }` option, or
	Failing tests can be automatically retried via the `retries` test option, or

	via the dynamic `retries()` TestContext prototype method.
	via the `retries()` `TestContext` prototype method.

test_runner: allow retryable tests #52626

Are you sure you want to change the base?

test_runner: allow retryable tests #52626

Conversation

RedYetiDev commented Apr 21, 2024 • edited Loading

nodejs-github-bot commented Apr 21, 2024

github-actions bot commented Apr 21, 2024

cjihrig commented Apr 21, 2024

RedYetiDev commented Apr 21, 2024

cjihrig commented Apr 21, 2024

RedYetiDev commented Apr 21, 2024 • edited Loading

cjihrig commented Apr 21, 2024

RedYetiDev commented Apr 21, 2024

cjihrig commented Apr 21, 2024

RedYetiDev commented Apr 21, 2024

RedYetiDev commented Apr 21, 2024

RedYetiDev commented Apr 21, 2024

RedYetiDev commented Apr 21, 2024 • edited Loading

MoLow commented Apr 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliangruber commented Apr 21, 2024

MoLow commented Apr 22, 2024

RedYetiDev commented Apr 22, 2024 • edited Loading

targos commented Apr 22, 2024

RedYetiDev commented Apr 22, 2024 • edited Loading

targos commented Apr 22, 2024

RedYetiDev commented Apr 22, 2024

RedYetiDev commented Apr 25, 2024

MoLow left a comment

Choose a reason for hiding this comment

atlowChemi commented Apr 26, 2024 • edited Loading

benjamingr commented Apr 26, 2024

RedYetiDev commented May 6, 2024

cjihrig commented Jun 28, 2024

benjamingr commented Jun 28, 2024

cjihrig commented Jun 28, 2024

RedYetiDev commented Jun 28, 2024

benjamingr commented Jun 28, 2024

MoLow commented Jun 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamingr commented Jun 30, 2024

RedYetiDev commented Jul 1, 2024 • edited Loading

RedYetiDev commented Apr 21, 2024 •

edited

Loading

RedYetiDev commented Apr 21, 2024 •

edited

Loading

RedYetiDev commented Apr 21, 2024 •

edited

Loading

RedYetiDev commented Apr 22, 2024 •

edited

Loading

RedYetiDev commented Apr 22, 2024 •

edited

Loading

atlowChemi commented Apr 26, 2024 •

edited

Loading

RedYetiDev commented Jul 1, 2024 •

edited

Loading