Implement retry flaky steps #684

nhv96 · 2025-03-29T13:56:35Z

🤔 What's changed?

Added support for retrying flaky steps by returning ErrRetry
Added CLI option for MaxRetries attempts
Added example of the feature

⚡️ What's your motivation?

Discussed here.

🏷️ What kind of change is this?

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

♻️ Anything particular you want feedback on?

📋 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.

* retry failed pickles * revert some changes * use example steps from cck * update example * implement retry with godog.ErrRetry * unit test for retry flaky * Update flaky feature description * add --retry to CLI option * update test * update test for --retry cli option * update test * handle err without printing retry steps * storage methods to upsert step result * rename wording * revert change * update feature test (will remove this feature) * revert unused files * revert unused files * improve readability * remove unused code * cleanup

* retry failed pickles * revert some changes * use example steps from cck * update example * implement retry with godog.ErrRetry * unit test for retry flaky * Update flaky feature description * add --retry to CLI option * update test * update test for --retry cli option * update test * handle err without printing retry steps * storage methods to upsert step result * rename wording * revert change * update feature test (will remove this feature) * revert unused files * revert unused files * improve readability * remove unused code * cleanup * update changelog

luke-hill

Some minor fixes suggested. However a larger question is, are you seeking conformance with standard cucumber execution / cck conformance (I know you might not be there yet).

The reason why, is that a lot of your implementation details surrounding the specific status is unique and not what all standard implementors do

luke-hill · 2025-03-31T14:28:52Z

_examples/retry-flaky/godogs_test.go

+	sc.Step(`^a step that passes the second time`, func(ctx context.Context) (context.Context, error) {
+		secondTimePass++
+		if secondTimePass < 2 {
+			return ctx, fmt.Errorf("unexpected network connection, %w", godog.ErrRetry)


For stuff that will inevitably become part of living documentation I would make this more informative than obfuscating.

See https://github.com/cucumber/compatibility-kit/blob/main/devkit/samples/retry/retry.feature.ts (Which in itself probably could be a bit better)

Right, I think I should just return godog.ErrRetry instead.

luke-hill · 2025-03-31T14:29:32Z

_examples/retry-flaky/godogs_test.go

+		return ctx, nil
+	})
+
+	fifthTimePass := 0


This should be removed as we want this always to fail. The conditional below should also be removed. It should always return the formatted error

Yes I agree, this folder is for examples, I should not add this case here (this case is supposed to be in unit test over here only).

luke-hill · 2025-03-31T14:31:14Z

internal/flags/flags.go

@@ -46,4 +46,6 @@ built-in formatters are:
 specify SEED to reproduce the shuffling from a previous run
  --random=5738`)
 	flagSet.Lookup(prefix + "random").NoOptDefVal = "-1"
+
+	flagSet.IntVar(&opts.MaxRetries, prefix+"retry", opts.MaxRetries, "retry n times when a step encounter error")


Suggested change

flagSet.IntVar(&opts.MaxRetries, prefix+"retry", opts.MaxRetries, "retry n times when a step encounter error")

flagSet.IntVar(&opts.MaxRetries, prefix+"retry", opts.MaxRetries, "Specify the number of times to retry failing tests (default: 0)")

Make sure this is defaulting to 0

If you want to see where the original config parser is set with all of the messages check here: https://github.com/cucumber/cucumber-ruby/blob/main/lib/cucumber/cli/options.rb

Yes it will be default 0. I'll update the message.

internal/flags/flags_test.go

luke-hill · 2025-03-31T14:35:20Z

internal/flags/options.go

@@ -80,6 +80,9 @@ type Options struct {

 	// ShowHelp enables suite to show CLI flags usage help and exit.
 	ShowHelp bool
+
+	// MaxRetries is used to retry the number of time when a step is failed. Default is 0 retry.


Suggested change

// MaxRetries is used to retry the number of time when a step is failed. Default is 0 retry.

// MaxRetries is the number of times you can retry failing tests. Default is 0.

luke-hill · 2025-03-31T14:37:03Z

retry_flaky_test.go

+
+			fifthTimePass := 0
+			ctx.Step(`^a step that always fails`, func(ctx context.Context) (context.Context, error) {
+				fifthTimePass++


This looks like duplicate info from above. Is there a way to avoid defining this twice.

Oh I forgot, this file is unit tests for the framework itself. So this case here is to cover the case step retried all attempts but still fail.

So based on what I've seen earlier, this should ideally live in a separate folder called compatibility and you should look towards CCK conformance imo.

Maybe come and discuss this on discord. See what the desired avenue is in the go community

nhv96 · 2025-04-12T13:44:18Z

Some minor fixes suggested. However a larger question is, are you seeking conformance with standard cucumber execution / cck conformance (I know you might not be there yet).

The reason why, is that a lot of your implementation details surrounding the specific status is unique and not what all standard implementors do

@luke-hill I agree that this implementation works for the specific status godog.ErrRetry, in our discussion thread we also proposed other approaches as well such as using tags @flaky in the feature files.

Do you mean that other standard implementors will also implement such approaches as well? If it is the case, then this PR is to implement the approach using RetryErr, then the following PRs can be to add other approaches.

So I do want to conform the cucumber standard for implementations, but I don't want to make so many different changes in one PR 😄

nhv96 · 2025-05-17T09:10:04Z

hi @vearutop @luke-hill if you have time, please help me review this PR.

luke-hill

Done another pass at a review but at this point I think the "goal" needs to be understood better with regards to conformance / non-conformance

luke-hill · 2025-05-20T10:53:14Z

_examples/retry-flaky/features/retry.feature

+Feature: Godog should be able to retry flaky tests
+    In order to help Go developers deal with flaky tests
+    As a test suite
+    I need to be able to return godog.Err to mark which steps should be retry


more a question, but usually in feature files if it's a technical term you're referencing you'd use backticks or something else.

But maybe you don't do that here 🤷‍♂️

luke-hill · 2025-05-20T10:53:48Z

_examples/retry-flaky/features/retry.feature

@@ -0,0 +1,19 @@
+Feature: Godog should be able to retry flaky tests


Whilst the principle here is good, I'd lean more on you implementing the CCK for things like this, so that you can use the conformance element there

luke-hill · 2025-05-20T10:55:21Z

internal/flags/options.go

@@ -80,6 +80,9 @@ type Options struct {

 	// ShowHelp enables suite to show CLI flags usage help and exit.
 	ShowHelp bool
+
+	// MaxRetries is the number of times you can retry failing tests. Default is 0 retry.


Suggested change

// MaxRetries is the number of times you can retry failing tests. Default is 0 retry.

// MaxRetries is the number of times you can retry failing tests. Default is 0 retries.

luke-hill · 2025-05-20T10:56:15Z

internal/formatters/fmt.go

@@ -37,6 +37,7 @@ var (
 	undefined = models.Undefined
 	pending   = models.Pending
 	ambiguous = models.Ambiguous
+	retry     = models.Retry


Whilst this is godog, and you are allowed to do your own thing, if your ultimate goal is cucumber conformance, this would need to go.

We have a concept called "flaky" which is something that passed after retries, maybe this can be renamed or adapted I don't know

luke-hill · 2025-05-20T10:56:39Z

internal/models/results.go

@@ -74,6 +79,8 @@ const (
 	Pending
 	// Ambiguous ...
 	Ambiguous
+	// Retry ...
+	Retry


This might / should be altered too

luke-hill · 2025-05-20T10:56:48Z

internal/models/results.go

@@ -105,6 +112,8 @@ func (st StepResultStatus) String() string {
 		return "pending"
 	case Ambiguous:
 		return "ambiguous"
+	case Retry:


luke-hill · 2025-05-20T10:57:34Z

internal/storage/storage.go

@@ -336,3 +345,15 @@ func (s *Storage) mustGet(table, index string, args ...interface{}) memdb.Result

 	return it
 }
+
+func (s *Storage) first(table, index string, args ...interface{}) interface{} {


I am writing this comment to let you know I don't understand this bit. But I'm assuming it does something important 😅

luke-hill · 2025-05-20T10:58:34Z

retry_flaky_test.go

+
+			fifthTimePass := 0
+			ctx.Step(`^a step that always fails`, func(ctx context.Context) (context.Context, error) {
+				fifthTimePass++


So based on what I've seen earlier, this should ideally live in a separate folder called compatibility and you should look towards CCK conformance imo.

Maybe come and discuss this on discord. See what the desired avenue is in the go community

nhv96 added 2 commits March 29, 2025 20:49

nhv96 mentioned this pull request Mar 29, 2025

Retry failed tests automatically #658

Open

luke-hill requested changes Mar 31, 2025

View reviewed changes

nhv96 added 2 commits April 13, 2025 21:31

make example less obfuscating

5ce208b

update text message

fd83073

luke-hill reviewed May 20, 2025

View reviewed changes

	flagSet.IntVar(&opts.MaxRetries, prefix+"retry", opts.MaxRetries, "retry n times when a step encounter error")
	flagSet.IntVar(&opts.MaxRetries, prefix+"retry", opts.MaxRetries, "Specify the number of times to retry failing tests (default: 0)")

	// MaxRetries is used to retry the number of time when a step is failed. Default is 0 retry.
	// MaxRetries is the number of times you can retry failing tests. Default is 0.

		@@ -0,0 +1,19 @@
		Feature: Godog should be able to retry flaky tests

Implement retry flaky steps #684

Are you sure you want to change the base?

Implement retry flaky steps #684

Uh oh!

Conversation

nhv96 commented Mar 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🤔 What's changed?

⚡️ What's your motivation?

🏷️ What kind of change is this?

♻️ Anything particular you want feedback on?

📋 Checklist:

Uh oh!

luke-hill left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nhv96 commented Apr 12, 2025

Uh oh!

nhv96 commented May 17, 2025

Uh oh!

luke-hill left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nhv96 commented Mar 29, 2025 •

edited

Loading