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

Introduction of 'MustPassRepeatedly' decorator #1051

Merged
merged 9 commits into from Oct 19, 2022
12 changes: 10 additions & 2 deletions decorator_dsl.go
Expand Up @@ -13,13 +13,21 @@ You can learn more about decorators here: https://onsi.github.io/ginkgo/#decorat
type Offset = internal.Offset

/*
FlakeAttempts(uint N) is a decorator that allows you to mark individual specs or spec containers as flaky. Ginkgo will run them up to `N` times until they pass.
FlakeAttempts(uint N) is a decorator that allows you to mark individual specs or spec containers as flaky. Ginkgo will run them up to `N` times until they pass.

You can learn more here: https://onsi.github.io/ginkgo/#repeating-spec-runs-and-managing-flaky-specs
You can learn more here: https://onsi.github.io/ginkgo/#the-flakeattempts-decorator
You can learn more about decorators here: https://onsi.github.io/ginkgo/#decorator-reference
*/
type FlakeAttempts = internal.FlakeAttempts

/*
RepeatAttempts(uint N) is a decorator that allows you to repeat the execution of individual specs or spec containers. Ginkgo will run them up to `N` times until they fail.

You can learn more here: https://onsi.github.io/ginkgo/#the-repeatattempts-decorator
You can learn more about decorators here: https://onsi.github.io/ginkgo/#decorator-reference
*/
type RepeatAttempts = internal.RepeatAttempts
Copy link
Owner

Choose a reason for hiding this comment

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

As mentioned on the issue - I'd suggest MustPassRepeatedly


/*
Focus is a decorator that allows you to mark a spec or container as focused. Identical to FIt and FDescribe.

Expand Down
49 changes: 46 additions & 3 deletions docs/index.md
Expand Up @@ -2485,7 +2485,21 @@ One quick note on `--repeat`: when you invoke `ginkgo --repeat=N` Ginkgo will ru

Both `--until-it-fails` and `--repeat` help you identify flaky specs early. Doing so will help you debug flaky specs while the context that introduced them is fresh.

However. There are times when the cost of preventing and/or debugging flaky specs simply is simply too high and specs simply need to be retried. While this should never be the primary way of dealing with flaky specs, Ginkgo is pragmatic about this reality and provides a mechanism for retrying specs.
A more granular approach to repeating tests is by decorating individual subject or container nodes with the RepeatAttempts(N) decorator:
Copy link
Owner

Choose a reason for hiding this comment

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

fantastic! thanks for taking the time to integrate this into the docs so well :)


```go
Describe("Storing books", func() {
It("can save books to the central library", RepeatAttempts(3), func() {
// this spec has been marked and will be retried up to 3 times
})

It("can save books locally", func() {
// this spec has not been marked and will not be retired
})
})
```

However, There are times when the cost of preventing and/or debugging flaky specs simply is simply too high and specs simply need to be retried. While this should never be the primary way of dealing with flaky specs, Ginkgo is pragmatic about this reality and provides a mechanism for retrying specs.

You can retry all specs in a suite via:

Expand All @@ -2495,7 +2509,7 @@ ginkgo --flake-attempts=N

Now, when a spec fails Ginkgo will not automatically mark the suite as failed. Instead it will attempt to rerun the spec up to `N` times. If the spec succeeds during a retry, Ginkgo moves on and marks the suite as successful but reports that the spec needed to be retried.

You can take a more granular approach by decorating individual subject nodes or container nodes as potentially flaky with the `FlakeAttempts(N)` decorator:
A more granular approach is also provided for this functionality with the use of the `FlakeAttempts(N)` decorator:

```go
Describe("Storing books", func() {
Expand Down Expand Up @@ -4797,7 +4811,7 @@ In addition to `Offset`, users can decorate nodes with a `types.CodeLocation`.
Passing a `types.CodeLocation` decorator in has the same semantics as passing `Offset` in: it only applies to the node in question.

#### The FlakeAttempts Decorator
The `FlakeAttempts(uint)` decorator applies container and subject nodes. It is an error to apply `FlakeAttempts` to a setup node.
The `FlakeAttempts(uint)` decorator applies to container and subject nodes. It is an error to apply `FlakeAttempts` to a setup node.

`FlakeAttempts` allows the user to flag specific tests or groups of tests as potentially flaky. Ginkgo will run tests up to the number of times specified in `FlakeAttempts` until they pass. For example:

Expand Down Expand Up @@ -4825,6 +4839,35 @@ With this setup, `"is flaky"` and `"is also flaky"` will run up to 3 times. `"i

If `ginkgo --flake-attempts=N` is set the value passed in by the CLI will override all the decorated values. Every test will now run up to `N` times.

#### The RepeatAttempts Decorator
The `RepeatAttempts(uint)` decorator applies to container and subject nodes. It is an error to apply `RepeatAttempts` to a setup node.

`RepeatAttempts` allows the user to flag specific tests or groups of tests for debbuging flaky tests. Ginkgo will run tests up to the number of times specified in `RepeatAttempts` until they fail. For example:

```go
Describe("repeated tests", RepeatAttempts(3), func() {
It("is retried", func() {
...
})

It("is also retried", func() {
...
})

It("is retried even more", RepeatAttempts(5) func() {
...
})

It("is retried less", RepeatAttempts(1), func() {
...
})
})
```

With this setup, `"is retried"` and `"is also retried"` will run up to 3 times. `"is retried even more"` will run up to 5 times. `"is retried less"` will run only once. Note that if multiple `RepeatAttempts` appear in a spec's hierarchy, the most deeply nested `RepeatAttempts` wins. If multiple `RepeatAttempts` are passed into a given node, the last one wins.

The `ginkgo --repeat=N` value passed in by the CLI has no relation with the `RepeatAttempts` decorator. If the `--repeat` CLI flag is used and a container or subject node also contains the `RepeatAttempts` decorator, then the test will run up to `N*R` times, where `N` is the values passed to the `--repeat` CLI flag and `R` is the value passed to the RepeatAttempts decorator.


#### The SuppressProgressOutput Decorator

Expand Down
3 changes: 2 additions & 1 deletion dsl/decorators/decorators_dsl.go
@@ -1,7 +1,7 @@
/*
Ginkgo is usually dot-imported via:

import . "github.com/onsi/ginkgo/v2"
import . "github.com/onsi/ginkgo/v2"

however some parts of the DSL may conflict with existing symbols in the user's code.

Expand All @@ -18,6 +18,7 @@ import (

type Offset = ginkgo.Offset
type FlakeAttempts = ginkgo.FlakeAttempts
type RepeatAttempts = ginkgo.RepeatAttempts
type Labels = ginkgo.Labels
type PollProgressAfter = ginkgo.PollProgressAfter
type PollProgressInterval = ginkgo.PollProgressInterval
Expand Down
Expand Up @@ -12,7 +12,8 @@ func TestDecorationsFixture(t *testing.T) {
RunSpecs(t, "DecorationsFixture Suite")
}

var count = 0
var countFlake = 0
// var countRepeat = 0

var _ = Describe("some decorated tests", func() {
Describe("focused", Focus, func() {
Expand All @@ -23,13 +24,21 @@ var _ = Describe("some decorated tests", func() {

})

It("passes eventually", func() {
count += 1
if count < 3 {
FIt("passes eventually", func() {
countFlake += 1
if countFlake < 3 {
Fail("fail")
}
}, FlakeAttempts(3))

// how to/should we test negative test cases?
Copy link
Owner

Choose a reason for hiding this comment

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

Commented on an approach to this on the issue

// FIt("fails eventually", func() {
// countRepeat += 1
// if countRepeat >=3 {
// Fail("fail")
// }
// }, RepeatAttempts(3))

It("focused it", Focus, func() {
Ω(true).Should(BeTrue())
})
Expand Down

This file was deleted.

@@ -0,0 +1,19 @@
package invalid_decorations_flakeattempts_repeatattempts_test

import (
"testing"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
)

func TestInvalidDecorations(t *testing.T) {
RegisterFailHandler(Fail)
RunSpecs(t, "InvalidDecorations Suite - RepeatAttempts and FlakeAttempts")
}

var _ = Describe("invalid decorators: repeatattempts and flakeattempts", FlakeAttempts(3), RepeatAttempts(3), func() {
It("never runs", func() {

})
})
@@ -0,0 +1,19 @@
package invalid_decorations_focused_pending_test

import (
"testing"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
)

func TestInvalidDecorations(t *testing.T) {
RegisterFailHandler(Fail)
RunSpecs(t, "InvalidDecorations Suite - Focused and Pending")
}

var _ = Describe("invalid decorators: focused and pending", Focus, Pending, func() {
It("never runs", func() {

})
})
1 change: 1 addition & 0 deletions integration/_fixtures/flags_fixture/flags_test.go
Expand Up @@ -85,6 +85,7 @@ var _ = Describe("Testing various flags", func() {
})
})

// what is this testing? This test and the previous are being skipped
Describe("a flaky test", func() {
runs := 0
It("should only pass the second time it's run", func() {
Expand Down
10 changes: 8 additions & 2 deletions integration/decorations_test.go
Expand Up @@ -27,9 +27,15 @@ some decorated tests
Ω(out).ShouldNot(ContainSubstring("never_see_this_file"))
})

It("exits with a clear error if decorations are misconfigured", func() {
session := startGinkgo(fm.PathTo("decorations", "invalid_decorations"), "-v", "--no-color")
It("exits with a clear error if decorations are misconfigured - focus and pending error", func() {
session := startGinkgo(fm.PathTo("decorations", "invalid_decorations_focused_pending"), "-v", "--no-color")
Eventually(session).Should(gexec.Exit(1))
Ω(session).Should(gbytes.Say("Invalid Combination of Decorators: Focused and Pending"))
})

It("exits with a clear error if decorations are misconfigured - flakeattempts and repeatattempts error", func() {
session := startGinkgo(fm.PathTo("decorations", "invalid_decorations_flakeattempts_repeatattempts"), "-v", "--no-color")
Eventually(session).Should(gexec.Exit(1))
Ω(session).Should(gbytes.Say("Invalid Combination of Decorators: FlakeAttempts and RepeatAttempts"))
})
})
42 changes: 38 additions & 4 deletions internal/group.go
Expand Up @@ -2,6 +2,7 @@ package internal

import (
"fmt"
"reflect"
"time"

"github.com/onsi/ginkgo/v2/types"
Expand Down Expand Up @@ -118,6 +119,8 @@ func (g *group) initialReportForSpec(spec Spec) types.SpecReport {
ParallelProcess: g.suite.config.ParallelProcess,
IsSerial: spec.Nodes.HasNodeMarkedSerial(),
IsInOrderedContainer: !spec.Nodes.FirstNodeMarkedOrdered().IsZero(),
IsFlakeAttempts: spec.Nodes.HasSetFlakeAttempts(),
IsRepeatAttempts: spec.Nodes.HasSetRepeatAttempts(),
}
}

Expand Down Expand Up @@ -299,16 +302,39 @@ func (g *group) run(specs Specs) {

g.suite.currentSpecReport.StartTime = time.Now()
if !skip {
maxAttempts := max(1, spec.FlakeAttempts())

var maxAttempts = 1
var multipleExecutionDecorator interface{}

if g.suite.currentSpecReport.IsFlakeAttempts {
multipleExecutionDecorator = reflect.TypeOf(FlakeAttempts(0))
maxAttempts = max(1, spec.FlakeAttempts())
}
if g.suite.config.FlakeAttempts > 0 {
multipleExecutionDecorator = reflect.TypeOf(FlakeAttempts(0))
maxAttempts = g.suite.config.FlakeAttempts
g.suite.currentSpecReport.IsFlakeAttempts = true
}

if g.suite.currentSpecReport.IsRepeatAttempts {
multipleExecutionDecorator = reflect.TypeOf(RepeatAttempts(0))
maxAttempts = max(1, spec.RepeatAttempts())
} else if g.suite.config.FlakeAttempts > 0 {
//What should be the behavior when --flakeattempts is defined in CLI and the RepeatAttemps decorator is used?
Copy link
Owner

Choose a reason for hiding this comment

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

as mentioned in the issue - it should be ignored and only the explicit value set by the user in the decorator should be used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Answered with suggestions in the comments

}

maxAttemptForLoop:
for attempt := 0; attempt < maxAttempts; attempt++ {
g.suite.currentSpecReport.NumAttempts = attempt + 1
g.suite.writer.Truncate()
g.suite.outputInterceptor.StartInterceptingOutput()
if attempt > 0 {
fmt.Fprintf(g.suite.writer, "\nGinkgo: Attempt #%d Failed. Retrying...\n", attempt)
switch t := multipleExecutionDecorator; {
Copy link
Owner

Choose a reason for hiding this comment

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

rather than tracking multipleExecutionDecorator and doing a type switch could these be if g.suite.currentSpecReport.MaxFlakeAttampts > 0 and else if g.suite.currentSpecReport.MaxMustPassRepeatedly > 0?

case t == reflect.TypeOf(FlakeAttempts(0)):
fmt.Fprintf(g.suite.writer, "\nGinkgo: Attempt #%d Failed. Retrying...\n", attempt)
case t == reflect.TypeOf(RepeatAttempts(0)):
fmt.Fprintf(g.suite.writer, "\nGinkgo: Attempt #%d Passed. Retrying...\n", attempt)
}
}

g.attemptSpec(attempt == maxAttempts-1, spec)
Expand All @@ -318,9 +344,17 @@ func (g *group) run(specs Specs) {
g.suite.currentSpecReport.CapturedGinkgoWriterOutput += string(g.suite.writer.Bytes())
g.suite.currentSpecReport.CapturedStdOutErr += g.suite.outputInterceptor.StopInterceptingAndReturnOutput()

if g.suite.currentSpecReport.State.Is(types.SpecStatePassed | types.SpecStateSkipped | types.SpecStateAborted | types.SpecStateInterrupted) {
break
switch t := multipleExecutionDecorator; {
case t == reflect.TypeOf(FlakeAttempts(0)):
if g.suite.currentSpecReport.State.Is(types.SpecStatePassed | types.SpecStateSkipped | types.SpecStateAborted | types.SpecStateInterrupted) {
Copy link
Owner

Choose a reason for hiding this comment

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

this looks right to me.

break maxAttemptForLoop
}
case t == reflect.TypeOf(RepeatAttempts(0)):
if g.suite.currentSpecReport.State.Is(types.SpecStateFailed | types.SpecStateSkipped | types.SpecStateAborted | types.SpecStateInterrupted) {
Copy link
Owner

Choose a reason for hiding this comment

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

we need types.SpecStateTimedout here as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about types.SpecStatePanicked?

Copy link
Owner

Choose a reason for hiding this comment

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

yes. in fact... I think you want if !g.suite.currentSpecReport.State.Is(types.SpecStatePassed) { - since the idea is it has to pass cleanly each time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case if g.suite.currentSpecReport.State.Is(types.SpecStateFailureStates) { might be cleaner

Copy link
Owner

Choose a reason for hiding this comment

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

sure that works too :)

break maxAttemptForLoop
}
}

}
}

Expand Down
44 changes: 35 additions & 9 deletions internal/internal_integration/decorations_test.go
Expand Up @@ -14,23 +14,38 @@ var _ = Describe("Decorations test", func() {
customIt := func() {
It("is-offset", rt.T("is-offset"), Offset(1))
}
var count = 0
var countFlaky = 0
var countRepeat = 0
success, _ := RunFixture("happy-path decoration test", func() {
Describe("top-level-container", func() {
clForOffset = types.NewCodeLocation(0)
customIt()
It("flaky", FlakeAttempts(4), rt.T("flaky", func() {
count += 1
countFlaky += 1
outputInterceptor.AppendInterceptedOutput("so flaky\n")
writer.Println("so tasty")
if count < 3 {
if countFlaky < 3 {
F("fail")
}
}))
It("never-passes", FlakeAttempts(2), rt.T("never-passes", func() {
It("flaky-never-passes", FlakeAttempts(2), rt.T("flaky-never-passes", func() {
F("fail")
}))
It("skips", FlakeAttempts(3), rt.T("skips", func() {
It("flaky-skips", FlakeAttempts(3), rt.T("flaky-skips", func() {
Skip("skip")
}))
It("repeat", RepeatAttempts(4), rt.T("repeat", func() {
countRepeat += 1
outputInterceptor.AppendInterceptedOutput("repeats a bit\n")
writer.Println("here we go")
if countRepeat >= 3 {
F("fail")
}
}))
It("repeat-never-fails", RepeatAttempts(2), rt.T("repeat-never-passes", func() {
// F("fail")
}))
It("repeat-skips", RepeatAttempts(3), rt.T("repeat-skips", func() {
Skip("skip")
}))
})
Expand All @@ -42,8 +57,11 @@ var _ = Describe("Decorations test", func() {
Ω(rt).Should(HaveTracked(
"is-offset",
"flaky", "flaky", "flaky",
"never-passes", "never-passes",
"skips",
"flaky-never-passes", "flaky-never-passes",
"flaky-skips",
"repeat", "repeat", "repeat",
"repeat-never-passes", "repeat-never-passes",
"repeat-skips",
))
})

Expand All @@ -57,8 +75,16 @@ var _ = Describe("Decorations test", func() {
Describe("FlakeAttempts", func() {
It("reruns tests until they pass or until the number of flake attempts is exhausted, but does not rerun skipped tests", func() {
Ω(reporter.Did.Find("flaky")).Should(HavePassed(NumAttempts(3), CapturedStdOutput("so flaky\nso flaky\nso flaky\n"), CapturedGinkgoWriterOutput("so tasty\n\nGinkgo: Attempt #1 Failed. Retrying...\nso tasty\n\nGinkgo: Attempt #2 Failed. Retrying...\nso tasty\n")))
Ω(reporter.Did.Find("never-passes")).Should(HaveFailed("fail", NumAttempts(2)))
Ω(reporter.Did.Find("skips")).Should(HaveBeenSkippedWithMessage("skip", NumAttempts(1)))
Ω(reporter.Did.Find("flaky-never-passes")).Should(HaveFailed("fail", NumAttempts(2)))
Ω(reporter.Did.Find("flaky-skips")).Should(HaveBeenSkippedWithMessage("skip", NumAttempts(1)))
})
})

Describe("RepeatAttempts", func() {
It("reruns tests until they fail or until the number of repeat attempts is exhausted, but does not rerun skipped tests", func() {
Ω(reporter.Did.Find("repeat")).Should(HaveFailed(NumAttempts(3), CapturedStdOutput("repeats a bit\nrepeats a bit\nrepeats a bit\n"), CapturedGinkgoWriterOutput("here we go\n\nGinkgo: Attempt #1 Passed. Retrying...\nhere we go\n\nGinkgo: Attempt #2 Passed. Retrying...\nhere we go\n")))
Ω(reporter.Did.Find("repeat-never-fails")).Should(HavePassed("passed", NumAttempts(2)))
Ω(reporter.Did.Find("repeat-skips")).Should(HaveBeenSkippedWithMessage("skip", NumAttempts(1)))
})
})
})