Fix: retried scenarios which are run after another scenario with a passed result are still counted as failed

[thukim]

Description

Issue: Retried scenarios which are run after another scenario with a passed result are still counted as failed
Tests written to ensure the fix works: https://github.com/cucumber/cucumber-ruby-core/pull/250/files#diff-9bded15ebb1a2a9b74f58b63b24f0ff35f75c5cc44037e14b976ef1f3fde87d5R85-R112

Proposed changes:

• change @previous_test_case to @previous_test_cases which is a Ruby Set object, so that it holds a collection of all the unique test scenarios which have been run, regardless whether they pass or fail.
• instead of checking if the current test case is equal to the @previous_test_case value, the current event.test_case will be checked if it's included in the set @previous_test_cases. This ensures regardless whether the retried test case is run immediately after that test case or after another test case, it is still counted as retried one instead of a new test case as with the current logic.

These changes are backward compatible and don't impact future contributions, so I only bump the patch version, from 11.1.0 to 11.1.1
Please let me know if there is anything else I need to add or fix in this PR.

Thanks!

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)

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]

The original post sounds like quite a big bug, which has not been encountered by myself in years of running this. Can you maybe share a reproducible script that generates the failures you mention.

The specs/tests are a little hard to follow, but I think I need a better understanding of where this error comes from first.

[thukim]

Hi @luke-hill ,

The original post sounds like quite a big bug, which has not been encountered by myself in years of running this.

Most of the time, if we use the built-in --retry option, it won't happen to have any issue since the failed scenario will be retried immediately, thus the condition event.test_case != @previous_test_case will be true.

However, sometimes, we might want to have a custom way to retry those failed scenarios, for example: retry the failed scenarios after all the scenarios have been run. With this case, the condition event.test_case != @previous_test_case might not be true anymore since it only stores the last scenario to the var @previous_test_case, not all the scenarios which have been run previously.

This PR improves the cucumber retry so that it works for both cases: retry immediately right after 1 failed scenario, or retry later after other scenarios too.

Can you maybe share a reproducible script that generates the failures you mention.

Sure, I have created this Github repo to test - just need to run bundle and then bundle exec cucumber to reproduce it.

I also attached the screenshots below for the differences when running the cucumber tests before and after this PR:

Before (with cucumber-core v11.1.0)

After (with this PR)

Let me know if you have any other questions!

[luke-hill]

I'll need a good few weeks owing to other responsibilities to check on this. So apologies in advance.

Everything you've given looks enough for me for now to repro

[luke-hill]

Hi @thukim

As/when I get around to releasing a v12 version of this gem (Nothing huge changing don't worry). This PR will need a rebase and slight fix / re-gen of the rubocop TODO file.

I'll then plan during the v12 phase to start clicking off some of the offenses (Which number 2k+)

[thukim]

Hi @luke-hill ,

Sorry for the late reply! I have been able to switch back to this PR and resolved the conflicts.

Please let me know if there is any thing else I need to update/add in the PR.

Thank you!

[luke-hill]

Hi there. I've not forgot about this, just I've got 7 or 8 other things I'm trying to fix up atm. I will get to reviewing this in the next week or so.

[luke-hill]

Few language tweaks and a request to re-structure the tests to help the next person coming to read them

[luke-hill]

Can you place this under a ### Fixed heading

[luke-hill]

tests when using 0 as an input number or 2+
test when using 1 as an input number

[luke-hill]

If possible it would be better to write each of these as just 1 level of nesting under context with a better description. It's a bit hard for me to work out what's different here. Admittedly I don't know the code that well.