Replay failed tests

[theredfish]

I was wondering if it's something that we can add to the framework. We already have a function to repeat failed tests output but we don't have a function to run failed tests again. It's common to have flaky tests that sometimes can be solved by running them again.

For example :

#[tokio::main]
async fn main() {
    AnimalWorld::cucumber()
        .replay_failed(2)
        .run_and_exit("tests/features/book/output/terminal_repeat_failed.feature")
        .await;
}

Where 2 is the maximum number of times the tests should be run again in case of test failures. Let's say we have the tests A, B, C and D :

1. During the first run only A passes
2. Then B, C and D are run again (1 replay left if we have new test failures)
3. Only B passes, we run again C and D (0 replay left)
4. D fails again, this one is certainly more than just unstable

Regarding the output we have two choices :

• Print all test executions : it's transparent but can be repetitive when the tests fail multiple times (like D in this example)
• Or just print the result once the last replay is done (which can be the maximum, here 2, or a previous run if all tests are passing)

[tyranron]

@theredfish I'm not sure whether it has sense. Flaky tests should not be a common thing. In our codebases we actively reworking them to remove any flakiness, because it's very hard to reason about tests when you have the one: either a test fails due to flakinees, or the test may pass due to flakiness too. So I'm somewhat against about making life easier with flaky tests instead of giving people more reasons to remove it.

Let's hear what other users think about it.

[ilslv]

@theredfish I do think, that adding retries for flaky tests is a great feature to have, but I have couple concerns about proposed implementation.

.replay_failed(2)

In addition to specifying number of times test should be retried, I think that we should retry only tests tagged as @flaky or something like that, as being explicit is better that implicit here. Maybe even allow to override this value with something like @flaky(retries = 3).
I want this library to be a tool, that is hard to misuse and with defaults that follow best practices. So adding @flaky should be good point of friction for user to think twice, why this Scenario is flaky.

1. During the first run only A passes
2. Then B, C and D are run again (1 replay left if we have new test failures)
3. Only B passes, we run again C and D (0 replay left)
4. D fails again, this one is certainly more than just unstable

I think, that this may lead to unexpected problems: on panic changes from step B may be partially applied and fully retiring it may cause some unexpected changes in World state. Consider the following Step implementation:

#[given(...)]
fn apply_then_assert(w: &mut World) {
    w.counter += 1;
    assert_eq!(w.counter, 3);
}

Retrying this Step after an assert_eq! will always lead to incrementation of the w.counter. So, as we don't impose Clone bound on our World (otherwise we would be able to .clone() the World before every Step and rollback to it, if needed), the only option left is to retry entire Scenario.

Or just print the result once the last replay is done (which can be the maximum, here 2, or a previous run if all tests are passing)

I'm not sure this can be achieved with streaming output like ours. And even if it would, I think, that we should be transparent about failed flaky tests and also include stats about them in Summarized Writer.

@tyranron

Flaky tests should not be a common thing.

I saw couple of conference talks, where people from huge FAANG-like companies argued that at this scale flaky tests are inevitable. I'm not sure I agree with them, but there is at least opinions floating around. Also other test runners provide this feature out of the box.

[tyranron]

@ilslv okay, I see that there are enough reasons for having this feature.

Speaking of the feature design...

In addition to specifying number of times test should be retried, I think that we should retry only tests tagged as @flaky or something like that, as being explicit is better that implicit here.

I think, both variants should be provided, so we have the natural tags inheritance:

• tag on a scenario enables retrying for that scenario and overwrites any other settings;
• tag on a feature enables retrying for all the scenarios of that feature;
• specifying retries on the whole runner or via CLI enables retrying for the whole set of scenarios.

I propose @retry(n) tag, --retry=n CLI option and .retry(n) runner method would be quite a consistent and ergonomic naming here.

And even if it would, I think, that we should be transparent about failed flaky tests and also include stats about them in Summarized Writer.

The detailed summarization is "must have" 👍

Questions:

• Should we retry the scenario right after its failure, or wait for the whole set to finish and only then retry failures? Should this be controlled by option (like @retry(3).postponed/@retry(3).immediately)?

[ilslv]

@tyranron I think that reasonable default should be rerunning Scenarios immediately. But it would be great to support postponed reruns or even combination of postponed and serial for really flaky ones.

[tyranron]

@ilslv yup.

It also worths to mark the retried scenarios explicitly in the output, like the following:

Feature: Animal feature
  Scenario: If we feed a hungry cat it will no longer be hungry
    ✔  Given a hungry cat
    ✔  When I feed the cat
    ✔  Then the cat is not hungry
Retry #1 | Feature: Animal feature
  Retry #1 | Scenario: If we feed a hungry cat it will no longer be hungry
    ✔  Given a hungry cat
    ✔  When I feed the cat
    ✔  Then the cat is not hungry

[theredfish]

Thank you for the feedback! Indeed the idea isn't to encourage to ignore flaky tests but have a way to handle them waiting for a fix.

The tag is a good idea so we offer a different granularity and an explicit way.

I think, that this may lead to unexpected problems: on panic changes from step B may be partially applied

My bad it wasn't clear enough but my example was about scenarios not steps.

I can try to implement this feature if you want ? If you're available to guide me during the development ?

[ilslv]

@theredfish

My bad it wasn't clear enough but my example was about scenarios not steps.

Actually Scenarios are generally run in parallel, so there is no need for additional complexities you've described. We can just rerun failed Scenarios on their own.

I can try to implement this feature if you want ? If you're available to guide me during the development ?

I'll be happy to help you with development of this feature!

[theredfish]

I was a bit busy with other projects, I'll start to work on it :)

[ilslv]

While implementing retries for failed Scenarios I've stumbled upon an inconvenience in the discussed design. In case Writer is wrapped into a Normalize I expect it to wait on current Feature/Rule until all retried Scenarios are passed or drain out their retry attempts. While In case retry.immediately this works fine, for retry.postponed this will wait until all other Scenarios start their execution and only then retry it's attempt as it places retried Scenarios at the end of the queue. This will obviously hang up output for a while.

We can use retry.postponed(3s) to specify the delay for a retry. This can be almost achieved while still being runtime-agnostic with the following algorithm: use now() + delay as a deadline and place Scenario in front of the queue. On selection we will drain out only Scenarios which deadline isn't exceeded yet. The only problem with this solution is that we can end up only with Scenarios with active deadlines. Here we can be stuck inside a busy-loop or spawn a thread, that will sleep and notify us, when shortest deadline exceeds.

Another way to deal with this problem is to output retried Scenarios as a separate brach, re-outputting Feature and Rule. Here we need to understand what will actually be outputted: only retry itself or with additional info about previous/all failures.

ack @tyranron @theredfish

[theredfish]

That's great if you can do some progress on this feature, I haven't been able to work on it since my last commit. But I can continue later if you want. Just switching between priorities for the moment.

With your current example how do you specify the number of attempts for a postponed retry? Do we remove the possibility to specify de number of attempts per retry tag?

Since postponed is a strategy selected by users, they should expect more delays, is it really an issue to have to wait more? It might even be intended to decouple the execution from the rest. Flaky tests can be due to side effects created by other tests for example.

Regarding the delay if I understand it will block the execution until the delay is reached, but will execute the test asynchronously from the rest.

Another way to deal with this problem is to output retried Scenarios as a separate brach

What do you mean by separate branch? From the execution flow? Then it will be a bit confusing with the existing feature replay_failed_tests no? What if we have both?

Maybe we can keep a "naive" implementation first and then iterate? We could provide the immediate retry as a first solution, and later work around a postponed version where we can discuss more in depth of the different solutions. It will avoid to worry about the output and existing features. A retry with a delay is still a good idea I think, most of the time we need to wait a bit before retrying. We could then have only one default mode, if there is no delay we retry without one, if there is a delay we just wait during this time. As a first implementation it might be enough.

Or we can reduce this first feature to only an immediate retry. Do we keep the number of attempts config?