fix: Ensure parallel rules aren't bypassed on retries

tristanzander commented 2 months ago

🤔 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: 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:

[x] I agree to respect and uphold the Cucumber Community Code of Conduct
[X] I've changed the behaviour of the code
- [X] I have added/updated tests to cover my changes.
[ ] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly.
[X] Users should know about my change
- [X] 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 commented 2 months ago

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

davidjgoss commented 2 months ago

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

cucumber / cucumber-js