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

Conversation

felipe88alves
Copy link
Contributor

@felipe88alves felipe88alves commented Oct 11, 2022

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

  1. RepeatSpecs: More descriptive of what decorator's action. Execute a specific Spec repeatedly.
  2. RequiredPasses: More descriptive of the decorator's intent. That all the test must pass a set amount of times.
  3. 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 the Flake and Repeated sections when the test passes or fails, as they would be complimentary to the Passed and Failed 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

@onsi
Copy link
Owner

onsi commented Oct 11, 2022

Thanks for investing so much time in this! I'll try to take a look tomorrow and get back to you!

@onsi
Copy link
Owner

onsi commented Oct 12, 2022

Some replies inline:

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.

Makes sense!

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.

Also makes sense

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

Of these I prefer RequiredPasses(3) but would actually suggest MustPassRepeatedly(3)

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

This sounds correct to me.

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.

I think if RepeatAttempts is provided it overrides any attempt to use FlakeAttempts:

  • In the case of a FlakeAttempts decorator an error is raised (as you've done)
  • In the case of --flake-attempts the cli configured flake attempts is silently ignored for specs that have RepeatAttempts.

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

Yes these would be independent. If you have a spec with RepeatAttempts(3) and run ginkgo --repeat=4 that spec will end up running 12 times. That seems correct to me.

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

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

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 the Flake and Repeated sections when the test passes or fails, as they would be complimentary to the Passed and Failed fields anyway.

I think for specs with RepeatAttempts we probably want something like:

  • If the spec passed on all attempts, it just passes. No need to emit anything in the reporter output. If a user is running with -v they should see "Ginkgo: Attempt Hi! #1 Passed. Repeating...". If they look at a JSON report they should see the number of repeats. (You already have this in place but I have a few tweaks to suggest)
  • If the spec fails on any attempt the failure is reported. Since the SpecReport tracks the number of repeats we can tack on the attempt to the [FAILED] headers here. For example Ginkgo could say • [TIMEDOUT] During repetition #1. This isn't super important, though, since the GinkgoWriter output will be emitted and it will include all the details.

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

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 SpecReport. So rather than have types.SpecReport.IsFlakeAttempts and types.SpecReport.IsRepeatAttempts be bools we could have types.SpecReport.MaxFlakeAttempts and `types.SpecReport.RequiredPasses" be set to the appropriate integers.

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

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 Ginkgo: Attempt #1 Passed. Repeating... lines appeared:

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"))

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

There are actually tested here

granted it's a bit messy but it's validating that --repeat-attempts works and that --focus overrides the programmatic FDescribe

Copy link
Owner

@onsi onsi left a 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?
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

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

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

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

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

@@ -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):
Copy link
Owner

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

Copy link
Contributor Author

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)?

Copy link
Owner

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

@@ -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),
Copy link
Owner

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),

Copy link
Owner

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.
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 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
Copy link
Owner

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()

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 in comment discussion:

Based on this feedback, I'll change the CountOfRepeatedSpecs to only populate the Repeated report field when the test with the MustPassRepeatedly decorator fails. This was the user can understand whether a test failure is associated with a MustPassRepeatedly decorator or not.

@felipe88alves
Copy link
Contributor Author

felipe88alves commented Oct 13, 2022

Of these I prefer RequiredPasses(3) but would actually suggest MustPassRepeatedly(3)

Nice one. All references to the funcitonality have been modified to reflect chosen name: MustPassRepeatedly

I think if RepeatAttempts is provided it overrides any attempt to use FlakeAttempts:

  • In the case of a FlakeAttempts decorator an error is raised (as you've done)
  • In the case of --flake-attempts the cli configured flake attempts is silently ignored for specs that have RepeatAttempts.

Ok. I'll add this information in the docs for reference. Should I also add a comment in the code, stating that MustPassRepeatedly takes precedence over the --flake-attempts cli config (line 318 of group.go)?

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

  • If the spec passed on all attempts, it just passes. No need to emit anything in the reporter output. If a user is running with -v they should see "Ginkgo: Attempt Hi! #1 Passed. Repeating...". If they look at a JSON report they should see the number of repeats. (You already have this in place but I have a few tweaks to suggest)
  • If the spec fails on any attempt the failure is reported. Since the SpecReport tracks the number of repeats we can tack on the attempt to the [FAILED] headers here. For example Ginkgo could say • [TIMEDOUT] During repetition Hi! #1. This isn't super important, though, since the GinkgoWriter output will be emitted and it will include all the details.

Based on this feedback, I'll change the CountOfRepeatedSpecs to only populate the Repeated report field when the test with the MustPassRepeatedly decorator fails. This was the user can understand whether a test failure is associated with a MustPassRepeatedly decorator or not. Is that ok or would you really prefer to not have any such feedback?

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 SpecReport. So rather than have types.SpecReport.IsFlakeAttempts and types.SpecReport.IsRepeatAttempts be bools we could have types.SpecReport.MaxFlakeAttempts and `types.SpecReport.RequiredPasses" be set to the appropriate integers.

+1, will work on this

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 Ginkgo: Attempt #1 Passed. Repeating... lines appeared:

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"))

Think I finally understood these _fixture-based tests 😄
In any case, according to what I was seeing, the FlakeAttempts test was being skipped in the decorations_fixture, due to the Focus decorator also being tested in the same suite.

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

@onsi
Copy link
Owner

onsi commented Oct 13, 2022

Nice one. All references to the funcitonality have been modified to reflect chosen name: MustPassRepeatedly

Great :)

Ok. I'll add this information in the docs for reference. Should I also add a comment in the code, stating that MustPassRepeatedly takes precedence over the --flake-attempts cli config (line 318 of group.go)?

Yes, I think it would make sense there as well.

Based on this feedback, I'll change the CountOfRepeatedSpecs to only populate the Repeated report field when the test with the MustPassRepeatedly decorator fails. This was the user can understand whether a test failure is associated with a MustPassRepeatedly decorator or not. Is that ok or would you really prefer to not have any such feedback?

Got it - yeah I think what you're saying makes sense.

Think I finally understood these _fixture-based tests 😄 In any case, according to what I was seeing, the FlakeAttempts test was being skipped in the decorations_fixture, due to the Focus decorator also being tested in the same suite.

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

OK thanks - I'll take a look!

@felipe88alves
Copy link
Contributor Author

If the spec fails on any attempt the failure is reported. Since the SpecReport tracks the number of repeats we can tack on the attempt to the [FAILED] headers here. For example Ginkgo could say • [TIMEDOUT] During repetition #1. This isn't super important, though, since the GinkgoWriter output will be emitted and it will include all the details.

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?

@onsi
Copy link
Owner

onsi commented Oct 13, 2022

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.

@felipe88alves
Copy link
Contributor Author

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:
Copy link
Owner

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.

Copy link
Owner

@onsi onsi left a 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!

@@ -332,8 +330,8 @@ func (g *group) run(specs Specs) {
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?

if report.IsFlakeAttempts {
header, stream = fmt.Sprintf("%s [FLAKEY TEST - TOOK %d ATTEMPTS TO PASS]", r.retryDenoter, report.NumAttempts), false
}
if report.MaxFlakeAttempts > 1 {
Copy link
Owner

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.

@@ -176,11 +174,7 @@ func (r *DefaultReporter) DidRun(report types.SpecReport) {
}
case types.SpecStateFailed:
highlightColor, header = "{{red}}", fmt.Sprintf("%s [FAILED]", denoter)
Copy link
Owner

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 {
Copy link
Owner

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)):
Copy link
Owner

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!

@felipe88alves felipe88alves changed the title Draft: introduction of RepeatAttempts decorator Introduction of 'MustPassRepeatedly' decorator Oct 14, 2022
@felipe88alves
Copy link
Contributor Author

At this point all review items have been addressed apart from: #1051 (comment)
Pending conclusion on design.

@onsi
Copy link
Owner

onsi commented Oct 18, 2022

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 currentSpecReport to decide the switch. I'd be happy to pull this in and just make that tweak myself, though if you'd prefer.

@onsi
Copy link
Owner

onsi commented Oct 19, 2022

hey this looks great to me, now. Happy to merge it in once it's been rebased!

@felipe88alves
Copy link
Contributor Author

New push replaces the multipleExecutionDecorator type switch for an if/elseif statement.
The if/elseif statement defines that MustPassRepeatedly decorator supersedes the the --flakeattempts cli flag that supersedes the FlakeAttempts decorator .

TLDR: Order of priority: MustPassRepeatedly -> --flakeattempts -> FlakeAttempts

Usually not a big fan of if elsing, but this one doesn't look too terrible. Let me know if you have any suggestion.

@onsi
Copy link
Owner

onsi commented Oct 19, 2022

Looks good to me - I think we're good to go! I'll pull this in now!

@onsi onsi merged commit 047c02f into onsi:master Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The Opposite of FlakeAttempts
2 participants