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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

@RepeatFailedTest #110

Merged
merged 12 commits into from
Nov 30, 2019
Merged

@RepeatFailedTest #110

merged 12 commits into from
Nov 30, 2019

Conversation

nipafx
Copy link
Member

@nipafx nipafx commented Sep 7, 2018

First draft of @RepeatIfFailedTest. Code contains lots of TODOs and the test is more of a demo - it needs to be wrapped in the usual Jupiter test engine execution and assertions.

Feedback and code is welcome! 馃槂


I hereby agree to the terms of the JUnit Pioneer Contributor License Agreement.

@jbduncan
Copy link
Contributor

jbduncan commented Sep 7, 2018

I wonder if it would make sense to make @RetryIfFailedTest an annotation that would be applied on top of @Test, @ParameterizedTest and @TestFactory for flexibility, and thus rename @RetryIfFailedTest to @RetryIfFailed.

I imagine that for @ParameterizedTest and @TestFactory, @RetryIfFailed would retry each subtest/testlet that fails, independently from one another. So if a @TestFactory returned 3 dynamic tests, and if dynamic test 2 failed, then @RetryIfFail would retry that one and none of the others.

@nicolaiparlog, what do you think?

@smoyer64
Copy link
Member

smoyer64 commented Sep 8, 2018

@jbduncan I think at some point I had proposed the generic idea of @TestDecorator - it would be interesting to see if it's possible to generalize this feature to all "test types". If so, perhaps the boilerplate could in fact be extracted to make writing other test decorators easier.

@jbduncan
Copy link
Contributor

jbduncan commented Sep 8, 2018

@smoyer Interesting idea! Seems to me like a long-term goal, though. So I'd suggest that someone approaches this idea as a separate PR in the future.

I'd be interested to hear if @nicolaiparlog has any other thoughts or opinions about this idea. :)

@nipafx
Copy link
Member Author

nipafx commented Sep 21, 2018

@jbduncan I don't see a way to implement @RetryIfFail in the general way you describe it. Unless I'm missing something, there is no extension point to ...

  • intercept and rerun failing tests
  • interact with dynamic tests
  • interact with parameterized tests

So the only way I found to do this is to use the test template extension point (TestTemplateInvocationContextProvider) and generate a stream of test executions that stops as soon as one execution passes (-> test passes) or the number of retries was reached (-> test fails).

@marcphilipp
Copy link
Member

@nicolaiparlog Please be sure to tests this with parallel execution enabled. I think the Iterator will probably have to block in next() until the previous execution has finished.

@nipafx
Copy link
Member Author

nipafx commented Sep 22, 2018

@marcphilipp Thanks for pointing that out - I didn't think about parallel execution at all. I also didn't yet look into that feature. Is there any way for a test or test template (besides blocking, which 馃あ) to signal during parallel test execution that it doesn't want to be parallelized?

@marcphilipp
Copy link
Member

@Execution(SAME_THREAD) should work.

@nipafx nipafx force-pushed the nicolai/lab/repeat-if-failed branch from 6396c53 to 8b999dd Compare October 4, 2019 20:55
@nipafx
Copy link
Member Author

nipafx commented Oct 4, 2019

Turned this into a minimal viable feature. Well, almost... logging intermittent test failures is not exactly a core feature but it's already implemented. Once I find a good way to test it, I will document it.

Here are my remaining feature ideas:

  • make safe for parallel execution (currently, a commend states that this shouldn't be combined)
  • specify on which exceptions (not) to retry
  • specify a delay (linear, exponential, ...?) between test executions
  • make RepeatFailedTestInvocationContext injectable and add fields:
    • max count
    • current invocation count
    • exceptions so far
  • create an "always fail" mode where tests are never repeated (e.g. by configuration via property)

Before I do that, though, I'm interested to hear from the rest, whether this looks like a worthwhile feature. @sormuras, @marcphilipp, what do you think?

@marcphilipp
Copy link
Member

I think instead of logging intermittent failures, we should wrap them in TestAbortedExceptions so that the test is reported as aborted but not failed.

@nipafx
Copy link
Member Author

nipafx commented Oct 6, 2019

Wow, I didn't know about TestAbortedExceptions! That's much better, will do!

@nipafx
Copy link
Member Author

nipafx commented Oct 8, 2019

Thank you @marcphilipp, that improved the ease of use and feedback of the feature by a lot. Can you take another look?

@nipafx nipafx force-pushed the nicolai/lab/repeat-if-failed branch from ebb454c to 2b93c51 Compare October 8, 2019 08:18
@nipafx nipafx changed the title [WIP] @RepeatFailedTest @RepeatFailedTest Oct 8, 2019
@nipafx
Copy link
Member Author

nipafx commented Oct 8, 2019

Err, GitHub, Y U No link builds?

@@ -1,5 +1,5 @@
---
Copy link
Member

Choose a reason for hiding this comment

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

Is this change actually related to the new feature? (it's really hard to see what's changed on that line).

Copy link
Member Author

Choose a reason for hiding this comment

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

The change removes a <p> tag around the first sentence, but... I'm not sure why, the site seems to be fine as it is. Nevermind, I will remove the change.

import org.junit.jupiter.api.TestTemplate;
import org.junit.jupiter.api.extension.ExtendWith;

/**
Copy link
Member

Choose a reason for hiding this comment

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

Since we're putting the examples and longer documentation in adoc files, the javadocs should probably provide a link to them.

Copy link
Member Author

Choose a reason for hiding this comment

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

See #149.

docs/repeat-failed-test.adoc Outdated Show resolved Hide resolved
@nipafx nipafx force-pushed the nicolai/lab/repeat-if-failed branch from 2b93c51 to 4ae0fba Compare October 21, 2019 19:08
@nipafx nipafx force-pushed the nicolai/lab/repeat-if-failed branch 2 times, most recently from e5fbeba to 9432dfc Compare October 21, 2019 19:31
return true;

// if we caught an exception in each repetition, each repetition failed, including the previous one
boolean previousFailed = repetitionsSoFar == exceptionsSoFar;
Copy link
Member

Choose a reason for hiding this comment

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

In the case of parallel execution this should block until the previous invocation is done. Alternatively, we could meta-annotate @RepeatFailedTest with @Execution(SAME_THREAD). However, the latter would cause all @RepeatFailedTest methods in a test class to run sequentially.

Copy link
Member Author

@nipafx nipafx Oct 21, 2019

Choose a reason for hiding this comment

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

I think making this extension thread-safe in a way that makes sense and interacts well with Jupiter is not trivial and should be done in a separate issue. My questions:

  • should all repetitions be executed in parallel?
  • if not, would a block during the traversal of the stream returned by provideTestTemplateInvocationContexts not be unexpected for Jupiter?

(While thinking about this, I wanted to write a test that executes @RepeatFailedTest in parallel. Is there a good way to do that?)

For now, I want to go with @Execution(SAME_THREAD).

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, that requires bumping the Jupiter dependency from 5.1.1 to 5.3. I'm okay with that. Does anybody have any qualms about this?

@nipafx
Copy link
Member Author

nipafx commented Oct 21, 2019

I think this is done. Once @marcphilipp made his review, I will merge. Or he can. :)

Proposed commit message:

Implement @RepeatFailedTest

It is occasionally helpful to repeat failing tests a few times before
giving up on them. The annotation `@RepeatFailedTest(n)` does just
that:

1. run the annotated method as a test
2. if it fails:
	a) if this was the n-th run, mark test as failed
	b) otherwise, mark as aborted and go to 1

PR: #110

@marcphilipp
Copy link
Member

@nicolaiparlog Please see my review above.

@nipafx
Copy link
Member Author

nipafx commented Oct 21, 2019

I'd like to bump the dependency on Jupiter from 5.1.1 to 5.3 (reason). Any nays?

@marcphilipp
Copy link
Member

A clear "yay" from my side. 馃槈

@nipafx nipafx force-pushed the nicolai/lab/repeat-if-failed branch from e074e2b to 95f7810 Compare November 30, 2019 19:53
@nipafx nipafx merged commit 0aad1f2 into master Nov 30, 2019
@nipafx nipafx deleted the nicolai/lab/repeat-if-failed branch November 30, 2019 20:00
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.

None yet

4 participants