New option '--retry-total' as a circuit breaker for option '--retry'

[Enceradeira]

Description

This PR adds a new option --retry-total which can be used in conjunction with the existing option --retry.

--retry-total acts as a circuit breaker in case a larger number of tests are failing, and it comes apparent that the problem introduced is not the usual 'flakiness' that --retry deals with. For example, when a bad commit leads to many tests failing it is very inefficient and slow to retry all these tests. The same is true if a temporary infrastructure problem causes all tests to fail.

In our test suite of 24'000 tests, we observe a predictable number of tests being randomly flakey. This number is below 5 and therefore we would set --retry 3 --retry-total 5. In case of a bad commit with e.g. 3000 tests failing we would save the effort and time to repeat all of them 3 times.

Type of change

• New feature (non-breaking change which adds new behaviour)

Checklist:

Your PR is ready for review once the following checklist is
complete. You can also add some checks if you want to.

• Tests have been added for any changes to behaviour of the code
• New and existing tests are passing locally and on CI
• bundle exec rubocop reports no offenses
• RDoc comments have been updated
• CHANGELOG.md has been updated

[luke-hill]

I'm just putting this here as a placeholder. Not directly rejecting this outright.

We need to get more info / discussion on this at a wider level. This likely should come in as behaviour for all cucumbers imo. If possible could we get your feedback/input on our thursday calls?

EDIT: IMO This is a good change. It's just we need to be very careful of the mechanic we use to introduce a sweeping change like this.

[luke-hill]

@Enceradeira if possible could you connect with us on slack potentially our slack endpoint is cucumberbdd.slack.com

Would be good to elicit some collaborative feedback on this. If we were to implement this it would be good to get a consensus around this across flavours.

[luke-hill]

Do we need these changes here? Just trying to think how we can diet down the complexity?

[Enceradeira]

To make my new scenario retry_failing_tests.feature:99 pass I only would need to fix scenarios_and_steps.rb:43 and not all.

The problem was that the steps could only be used once per scenario, my fix makes it possible to use such a step several times in one scenario.

I therefore could only fix scenarios_and_steps.rb:43 and not all but I thought it's better if all steps behave and are implemented the same.

Is that ok or do you want me to only fix the step that caused the problem?

[Enceradeira]

@luke-hill I tried to connect to cucumberbdd.slack.com but it seems I need an account there. Can you send me invite to [email protected] please?

[luke-hill]

As an update and from what others have said it's likely we'll just implement this here as we've done for now. I'll take a look and review this in the coming week.

You should be able to sign up on your own to the slack channel

[mpkorstanje]

This likely should come in as behaviour for all cucumbers imo.

I don't see that happen. Interaction with unreliable systems should be handled by the test code that directly interacts with those systems, not the test framework that is one layer removed.

Anything involving retry/unreliable systems is complex and because of that has the potential for endless feature creep. It seems a waste of our complexity budget. Rather I would expect the owners of the unreliable systems to deal with that.

Now unfortunately Cucumber Ruby already has a --retry option. So people have already been tricked into thinking that retrying a test is a good solution.

So consider instead of adding yet another --option to make the logic for making retry decisions a programmatic extension point. It will allow end users to describe their decisions programmatically rather then hard code their specific strategy.

This should also allow more nuanced decisions such as:

• Skip all after first failure
• Retry at most N tests globally, retry individual tests at most M times
• Try all tests atleast once, retry the failures at the end unless more then X% failed.

That said, do bear in mind that we currently also don't distinguish between test cases and execution attempts in the message protocol. So reporting tools have to make assumptions about the retry strategy we used. This currently blocks future improvements to retry.

I do believe the current implementation in this PR would report nothing about the skipped cases after the circuit breaker trips. So we would create the impression that fewer tests were selected for execution then we intended. But that is probably fixable.

[mattwynne]

This likely should come in as behaviour for all cucumbers imo.

I'm with @mpkorstanje on this. We don't need to let this change snowball into worrying about making this work for all Cucumbers, some of which don't currently have any support for retry.

So consider instead of adding yet another --option to make the logic for making retry decisions a programmatic extension point. It will allow end users to describe their decisions programmatically rather then hard code their specific strategy.

Strategically, I think this is a good idea for sure. However this change is pretty minimal and seems to give the user a lot of benefit, so I don't see the harm in merging it.

I do believe the current implementation in this PR would report nothing about the skipped cases after the circuit breaker trips. So we would create the impression that fewer tests were selected for execution then we intended. But that is probably fixable.

My reading of the code is that this doesn't really change the reporting behaviour around retries, it just puts a cap on the number of retries Cucumber will attempt. I don't think anything will be skipped - each scenario will be tested at least once, but once the max retries limit has been hit, it will just stop doing 2nd, 3rd etc. retry attempts if they fail. See how [here] the "fails twice" and "solid" scenarios are both tested once, after the circuit breaker has tripped.

[mattwynne]

One thing I notice about this code is that, because the retry count state is held in this Cucumber::Filters::Retry class instead of on the Cucumber::Core::Test::Case object, we're forced to add this new logic into this class.

I'd be comfortable adding a previous_attempts property or something onto the Core::Test::Case so we could keep that state there, and then we'd be able to add this logic in a separate little filter class. It would also put us in a better position for opening up a programmatic interface for this as @mpkorstanje suggests.

[mattwynne]

Also, I forgot to say, welcome @Enceradeira and thanks for your contribution! The tests and code look excellent!

[mpkorstanje]

@mattwynne I don't have a stake in Cucumber Ruby's maintenance. So I don't object to this PR. I will object to little hacks like this once it spills in to other parts such as messages. I'm already unhappy about the lack of distinction between attempts and testcases there.

[mattwynne]

I don't have a stake in Cucumber Ruby's maintenance. So I don't object to this PR.

👍

...other parts such as messages. I'm already unhappy about the lack of distinction between attempts and testcases there.

Is there a specific ticket already where we can talk about that?

[aslakhellesoy]

Hi @Enceradeira,

Thanks for your making your first contribution to Cucumber, and welcome to the Cucumber committers team! You can now push directly to this repo and all other repos under the cucumber organization! 🍾

In return for this generous offer we hope you will:

• ✅ Continue to use branches and pull requests. When someone on the core team approves a pull request (yours or someone else's), you're welcome to merge it yourself.
• 💚 Commit to setting a good example by following and upholding our code of conduct in your interactions with other collaborators and users.
• 💬 Join the community Slack channel to meet the rest of the team and make yourself at home.
• ℹ️ Don't feel obliged to help, just do what you can if you have the time and the energy.
• 🙋 Ask if you need anything. We're looking for feedback about how to make the project more welcoming, so please tell us!

On behalf of the Cucumber core team,
Aslak Hellesøy
Creator of Cucumber