fix: Ensure parallel rules aren't bypassed on retries

[tristanzander]

🤔 What's changed?

When a cucumber worker sends the testCaseFinished event back to the coordinator, do not mark the worker as "not in progress" until after all retries have finished being processed by the worker.

⚡️ What's your motivation?

Howdy, I started getting feedback from other people that Cucumber wasn't respecting parallel rules when retrying long-running tests in certain situations. I was skeptical at first, but I was able to replicate it and find the source of the problem.

The following test minimally reproduces the issue:

// example.feature
@parallelTesting
Feature: Testing retries not in parallel

    Scenario: I wait awhile SHOULD FAIL
        When I print "Start waiting 3"
        And I wait 3
        And I fail
    Example: I wait awhile again
        When I print "Start waiting <number>"
        When I wait <number>
        And I print "Waited <number>"
        Examples:
            | number |
            | 5      |

    @notInParallel
    Example: NO PARALLEL I wait awhile
        When I print "Start waiting <number>"
        When I wait <number>
        And I print "Waited <number>"
        Examples:
            | number |
            | 1      |
            | 1      |
            | 1      |
            | 1      |
            | 1      |

For this example, I had retries set to 4 to really hammer the point home. I started realizing that the NO PARALLEL tests actually started running, even when the SHOULD FAIL test was still doing a retry. This only occurs if the subsequent parallel scenarios finish and non-parallel tests need to run. Because the this.inProgressWorkers map is empty, it forces the notInParallel test to run while the retries are being attempted. This is causing flakiness in some of our tests.

You can remove the changes from src/runtime/parallel/coordinator.ts and the test case provided in the PR will fail, demonstrating the issue and ensuring the issue is not encountered again.

🏷️ What kind of change is this?

• 🐛 Bug fix (non-breaking change which fixes a defect)

♻️ Anything particular you want feedback on?

The scenario and step definition look kinda ugly, so I'm very much open to ideas to make the code look a bit simpler. Filtering through the envelopes is always a bit difficult to do cleanly, especially without chaining higher order functions.

📋 Checklist:

• I agree to respect and uphold the Cucumber Community Code of Conduct
• I've changed the behaviour of the code
  • I have added/updated tests to cover my changes.
• My change requires a change to the documentation.
  • I have updated the documentation accordingly.
• Users should know about my change
  • I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

---

This text was originally generated from a template, then edited by hand. You can modify the template here.

[coveralls]

coverage: 98.234% (+0.001%) from 98.233%
when pulling 1e08b0e on tristanzander:main
into b02a90a on cucumber:main.

[davidjgoss]

Great catch @tristanzander, and thanks for the very complete PR.

The scenario and step definition look kinda ugly, so I'm very much open to ideas to make the code look a bit simpler.

This is fair but I think you've made it about as simple as you could have. The underlying issue here is more that the parallel coordination code is not itself very testable so we have to resort to these kind of gymnastics in scenarios - this is something I'm aiming to improve on soon.

[davidjgoss]

Released in https://github.com/cucumber/cucumber-js/releases/tag/v10.7.0