-
-
Notifications
You must be signed in to change notification settings - Fork 650
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
Conversation
Thanks for investing so much time in this! I'll try to take a look tomorrow and get back to you! |
Some replies inline:
Makes sense!
Also makes sense
Of these I prefer
This sounds correct to me.
I think if
Yes these would be independent. If you have a spec with
Because if it fails then it is treated as a failure and appears as such. The intent is to convey "These specs passed, but only because they were retried."
I think for specs with RepeatAttempts we probably want something like:
I'll comment on this in the commit but I think rather than two bools it would be useful to record the configuration used when the spec ran in the
I think something like: countRepeat := 0
It("fails eventually", func() {
countRepeat += 1
if countRepeat >=3 {
Fail(fmt.Sprintf("failed on attempt #%d", countRepeat)
}
}, RepeatAttempts(3)) and then in the integration spec we have: Eventually(session).Should(gbytes.Say("failed on attempt #3")) If you want you can also assert that the Eventually(session).Should(gbytes.Say("Ginkgo: Attempt #1 Passed. Repeating..."))
Eventually(session).Should(gbytes.Say("Ginkgo: Attempt #2 Passed. Repeating..."))
Eventually(session).Should(gbytes.Say("failed on attempt #3"))
There are actually tested here granted it's a bit messy but it's validating that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left some thoughts on the commit!
Fail("fail") | ||
} | ||
}, FlakeAttempts(3)) | ||
|
||
// how to/should we test negative test cases? |
There was a problem hiding this comment.
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
decorator_dsl.go
Outdated
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 |
There was a problem hiding this comment.
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
docs/index.md
Outdated
@@ -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: |
There was a problem hiding this comment.
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 :)
internal/group.go
Outdated
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? |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
internal/group.go
Outdated
break maxAttemptForLoop | ||
} | ||
case t == reflect.TypeOf(RepeatAttempts(0)): | ||
if g.suite.currentSpecReport.State.Is(types.SpecStateFailed | types.SpecStateSkipped | types.SpecStateAborted | types.SpecStateInterrupted) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about types.SpecStatePanicked
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure that works too :)
internal/group.go
Outdated
break | ||
switch t := multipleExecutionDecorator; { | ||
case t == reflect.TypeOf(FlakeAttempts(0)): | ||
if g.suite.currentSpecReport.State.Is(types.SpecStatePassed | types.SpecStateSkipped | types.SpecStateAborted | types.SpecStateInterrupted) { |
There was a problem hiding this comment.
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.
reporters/default_reporter_test.go
Outdated
@@ -109,6 +109,12 @@ func S(options ...interface{}) types.SpecReport { | |||
report.AdditionalFailures = append(report.AdditionalFailures, option.(types.AdditionalFailure)) | |||
case reflect.TypeOf(0): | |||
report.NumAttempts = option.(int) | |||
case reflect.TypeOf(true): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, instead, we can have a new type MaxFlakeAttempts int
and type RequiredPasses int
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can I assume then that NumAttemps will hold the actual amount of executions whereas the new types would hold the requests (max amount NumAttempts could take)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think that's right.
Given the phrasing I think the defaults would be MaxFlakeAttempts = 0
and RequiredPasses = 1
right? If we want the defaults to be zero it could be RequiredAdditionalPasses
reporters/default_reporter_test.go
Outdated
@@ -1026,6 +1032,18 @@ var _ = Describe("DefaultReporter", func() { | |||
DELIMITER, | |||
"", | |||
), | |||
Entry("a failing test that was retried", | |||
C(), | |||
S(CTS("A"), "B", CLS(cl0), cl1, 2, false, types.SpecStateFailed), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
S(CTS("A"), "B", CLS(cl0), cl1, 2, MaxFlakeAttempts(2), types.SpecStateFailed),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ I think that would be a bit clearer
types/types.go
Outdated
NumAttempts int | ||
|
||
// IsFlakeAttempts captures whether the spec has been retried with ginkgo --flake-attempts=N or the use of the FlakeAttempts decorator. |
There was a problem hiding this comment.
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 I think:
MaxFlakeAttempts int
RequiredPasses int
might be more informative and easier to work with.
types/types.go
Outdated
return n | ||
} | ||
|
||
// CountOfRepeatedSpecs returns the number of SpecReports that passed after multiple attempts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need this - there's no need to tell the user that a spec was repeated if they asked for it. Whereas we do want to tell the user "hey, this suite passed but only because we retried some flaky specs". (hence CountOfFlakedSpecs()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Answered in comment discussion:
Based on this feedback, I'll change the
CountOfRepeatedSpecs
to only populate theRepeated
report field when the test with theMustPassRepeatedly
decorator fails. This was the user can understand whether a test failure is associated with aMustPassRepeatedly
decorator or not.
Nice one. All references to the funcitonality have been modified to reflect chosen name:
Ok. I'll add this information in the docs for reference. Should I also add a comment in the code, stating that
Based on this feedback, I'll change the
+1, will work on this
Think I finally understood these _fixture-based tests 😄 As a solution, I broke out the Focus and Pending and Offset decorators and the FlakeAttempts and MustPassRepeatedly decorators into different Test Suites. I also changed the decoration_test.go integration test to reflect this change |
Great :)
Yes, I think it would make sense there as well.
Got it - yeah I think what you're saying makes sense.
OK thanks - I'll take a look! |
I've noticed that the report for the Successful FlakedTests contains this Retry Denoter. I don't know what this retry denoter is meant to represent, but do we also need it for the report of the failed Repeated tests? |
it's just representing that the spec was flaky and had to be rerun. I don't think we need it for retry. let me know if you think things are in a good place and I can take another look tonight and see if we can get this merged in. so you know: I'm out of town the next few days and won't be at my computer much - hopefully we can get it in before then, if not we'll aim for early next week. |
Just reviewing the last changes. Hoping to push it up tonight. |
docs/index.md
Outdated
|
||
`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: | ||
`MustPassRepeatedly` 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 `MustPassRepeatedly` until they fail. For example: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit of a nitpick (sorry!) but I've been trying to call "tests" in Ginkgo "specs" throughout the documentation (though I'm sure I've missed quite a few!). Would you be up for changing the language here from test/tests to spec/specs?
The rationale is to distinguish Ginkgo specs from regular go tests - but it's mostly about consistency tbh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - I left a couple of comments but I think once they're resolved we'll be able to merge this in. I'm going to be out of town for the next few days but can pull the PR in and cut a new release shortly after I get back!
internal/group.go
Outdated
@@ -332,8 +330,8 @@ func (g *group) run(specs Specs) { | |||
switch t := multipleExecutionDecorator; { |
There was a problem hiding this comment.
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
?
reporters/default_reporter.go
Outdated
if report.IsFlakeAttempts { | ||
header, stream = fmt.Sprintf("%s [FLAKEY TEST - TOOK %d ATTEMPTS TO PASS]", r.retryDenoter, report.NumAttempts), false | ||
} | ||
if report.MaxFlakeAttempts > 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is right. You need report.NumAttempts > 1 && report.MaxFlakeAttempts > 1
otherwise every spec marked with MaxFlakeAttempts(N)
will get this output.
The intent is to only tell the user "hey this spec was flakey" if it actually takes more than one try to succeed. Otherwise they should just see a regular passing spec.
reporters/default_reporter.go
Outdated
@@ -176,11 +174,7 @@ func (r *DefaultReporter) DidRun(report types.SpecReport) { | |||
} | |||
case types.SpecStateFailed: | |||
highlightColor, header = "{{red}}", fmt.Sprintf("%s [FAILED]", denoter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this line is leftover form an earlier merge? You should be able to remove it as highlightColor is set elsewhere now
@@ -191,6 +185,9 @@ func (r *DefaultReporter) DidRun(report types.SpecReport) { | |||
header = fmt.Sprintf("%s! [ABORTED]", denoter) | |||
} | |||
|
|||
if report.State.Is(types.SpecStateFailureStates) && report.MaxMustPassRepeatedly > 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
} else { | ||
report.IsRepeatAttempts = true | ||
} | ||
case reflect.TypeOf(FlakeAttempts(0)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice - I think this is much clearer. thanks!
At this point all review items have been addressed apart from: #1051 (comment) |
he @felipe88alves - I'm back from vacation. This all looks pretty good. It looks like a merge conflict has popped up so if you a have a minute to take a look that would be great. On this I think I'd prefer to avoid switching on the type check since there is already context available in the code via the parameters stored on the |
hey this looks great to me, now. Happy to merge it in once it's been rebased! |
New push replaces the TLDR: Order of priority: Usually not a big fan of if elsing, but this one doesn't look too terrible. Let me know if you have any suggestion. |
Looks good to me - I think we're good to go! I'll pull this in now! |
Fix: #1034
Backlog Item #183169714
Proposal is to add a new decorator to evaluate if a Spec continuously passes. The idea is that this test will fail if ANY of its iterations fails.
Most of the design decisions and subsequent tests were inspired by the FlakeAttempts decorator.
Documentation was also added near the FlakeAttempt due to their similar nature.
A couple of questions were raised while thinking about this problem, feedback on them would be appreciated:
Naming
The naming of the new decorator is a point of contention. The name RepeatAttempts was used temporarily, but I would prefer the following proposals (respectively):
RepeatSpecs
: More descriptive of what decorator's action. Execute a specific Spec repeatedly.RequiredPasses
: More descriptive of the decorator's intent. That all the test must pass a set amount of times.RepeatAttempts
: Ties the functionality to that of the FlakeAttempts.Interworking with FlakeAttemps decorator
A decision was made to forbid the use of both decorators simultaneously. A new error has been created to handle the situation where both decorators are defined for an Spec Group. Needs approval
Interworking with ginkgo CLI --flakeattempts flag
What should be the expected behavior when --flakeattempts is defined in CLI and the RepeatAttemps decorator is used?
I was inclined to add a new error here to prevent that possibility, but was fearful that it could be too much of a limitation.
This has not been handled, and is thus an Open issue that needs to be solved prior to merging this PR.
Interworking with ginkgo CLI --repeat flag
A decision was made to treat them as independent actions. The --repeat flag repeats the execution of the suite as a whole, whereas the RepeatAttempts decorator repeats the execution of a specific Spec(s). An entry was put into the docs to explain this relation (or lack thereof). Needs approval
default_reporter.go behaviour
What is the motivation for reporting the
Flaked
section of the report only when the test passes?Ref: reporters/default_reporter.go ~ line 320
The question will help answer if the
Repeated
section of the same report should be populated when the test passes or fails. Current assumption is to do so wen it passes, but I don't understand why not report theFlake
andRepeated
sections when the test passes or fails, as they would be complimentary to thePassed
andFailed
fields anyway.Testing
The following questions are related to testing. Once again, feedback on them would be appreciated:
FlakeAttempts and RepeatAttempts differentiator in Report test
I needed to find a way to distinguish the numAttempts caused by FlakAttempts or RepeatedAttempts in the S() convenience helper function of the
default_reporter_test.go
and added a bool type to do so. Not the most elegant solution, but let me know what you think.Ref: reporters/default_reporter_test.go ~ line 103
Negative test cases
Not sure how to do the test case for the repeated decorator in the decorations_fixture_suite_test.go file, as it would need to be a negative test case. Perhaps the other tests suffice and this can be removed. If not, I kindly ask for some guidance on how to do the negative tests here. The test is currently commented for now awaiting some guidance.
Ref: integration/_fixtures/decorations_fixture/decorations_fixture_suite_test.go ~ line 34
Skipped tests
I found a couple of FlakeAttempt test which are currently skipped in its suite. Should that be corrected?
Ref: integration/_fixtures/flags_fixture/flags_test.go ~ line 88