Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add retry feature for failed tests #214

Closed

Conversation

theredfish
Copy link
Contributor

WIP

@theredfish
Copy link
Contributor Author

theredfish commented May 24, 2022

@ilslv I think what I want to implement is very similar to WhichScenarioFn in runner::Basic. I started to explore an implementation with this signature :

pub type RetryableFn = fn(&gherkin::Feature, &gherkin::Scenario) -> Option<u8>;

The implementation will :

  1. Check if there is a tag retry(n) on the feature, parse the digit and save it in a mutable variable retries. If there is no tag, we set the variable to None.
  2. Check if there is a tag on the scenario and mutate the variable to Some(n) if any. Since the scenario prevails on the feature. If not do nothing.
  3. Last check if n is 0 then mutate the variable to None (if anything different from unsigned, should be None due to bad parsing from the regex)

Regarding the user input (the retry tag) I don't think we have a clean way to prevent bad inputs. So everything that doesn't match the regex will be None and not retried.

Also I will try to extract this closure to its own function so I can do some unit tests. But I didn't explore this yet. I suppose I need to keep this function RetryableFn public so it can be extended by custom runners?

For the next steps :

  • I will explore a solution to keep the retries associated to scenarios in memory
  • For each retry, launch the scenario again (question : maybe we should add a default timeout between retries with tokio::time::sleep for example? But in case of async it could probably be the source of bugs + I'll need to adapt it for @serial scenarios).

@ilslv
Copy link
Member

ilslv commented May 25, 2022

@theredfish

I think what I want to implement is very similar to WhichScenarioFn in runner::Basic

Yes, this functionality should definitely be in Runner. And WhichScenarioFn is a good starting point. I imaging RetryableFn looking something like this:

enum RetryOrder {
     /// Reties `Scenario` right after the failure.
     Immediately,
     /// Retries `Scenario` in the end. 
     Postponed,
}

pub type RetryableFn = fn(
    &gherkin::Feature,
    Option<&gherkin::Rule>,
    &gherkin::Scenario,
) -> (RetryOrder, usize);

Also we should modify the existing Scenarios storage to also contain number of retries:

cucumber/src/runner/basic.rs

Lines 1404 to 1429 in cf055ac

/// [`Scenario`]s storage.
///
/// [`Scenario`]: gherkin::Scenario
type Scenarios = HashMap<
ScenarioType,
Vec<(
Arc<gherkin::Feature>,
Option<Arc<gherkin::Rule>>,
Arc<gherkin::Scenario>,
)>,
>;
/// Storage sorted by [`ScenarioType`] [`Feature`]'s [`Scenario`]s.
///
/// [`Feature`]: gherkin::Feature
/// [`Scenario`]: gherkin::Scenario
#[derive(Clone, Default)]
struct Features {
/// Storage itself.
scenarios: Arc<Mutex<Scenarios>>,
/// Indicates whether all parsed [`Feature`]s are sorted and stored.
///
/// [`Feature`]: gherkin::Feature
finished: Arc<AtomicBool>,
}

type RetiesLeft = usize;
type IsRetried = bool;

type Scenarios = HashMap<
    ScenarioType,
    Vec<(
        Arc<gherkin::Feature>,
        Option<Arc<gherkin::Rule>>,
        Arc<gherkin::Scenario>,
        RetriesLeft,
        RetryOrder,
        IsRetried, // Used to emit right events
    )>,
>;

So we should re-insert failed Scenarios in the front or back based on the RetryOrder value.

For each retry, launch the scenario again (question : maybe we should add a default timeout between retries with tokio::time::sleep for example? But in case of async it could probably be the source of bugs + I'll need to adapt it for @serial scenarios).

This should be discussed separately, as currently we are runtime-agnostic and introducing tokio dependency would break it. Maybe this would be possible with separate feature enabled, but this is definitely out of the current PRs scope.

@theredfish
Copy link
Contributor Author

Thank you for your inputs @ilslv ! I will follow them for the implementation and see how to implement the events 👍

@tyranron tyranron added the enhancement Improvement of existing features or bugfix label May 26, 2022
@tyranron tyranron added this to the 0.14.0 milestone May 26, 2022
} else {
// For some reason features.scenarios contains an empty vector for the storage.
// It will fail the rest of the execution. How to access the storage so we can
// read and update the retries?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ilslv can you maybe help me to understand this part? I try to read and update the storage. Seems like it's empty at this step (the scenario type is set to Concurrent but there is no vector as a value).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@theredfish I don't understand why empty Features::scenarios might fail the rest of the execution. Features::scenarios is an Arc<Mutex<HashMap<_, _>>> where execute() function you are working with is getting some amount of Scenarios from via Features::get(). It may get all the available Scenarios, in that case HashMap will indeed be empty.
Scenarios are inserted via insert_features() function. This is done to abstract Parser as a Stream of Features.
Basically you have 2 Futures: one is getting Stream of Features, and inserts them into Features storage, while another one extracts them and executes Scenarios.

@tyranron tyranron mentioned this pull request Jul 24, 2022
15 tasks
@theredfish
Copy link
Contributor Author

Closing in favor of #223

@theredfish theredfish closed this Aug 14, 2022
@tyranron tyranron added duplicate This issue or pull request already exists wontfix This will not be worked on labels Aug 15, 2022
@tyranron tyranron linked an issue Sep 8, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists enhancement Improvement of existing features or bugfix wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replay failed tests
3 participants