Skip to content

Conversation

@marcphilipp
Copy link
Member

@marcphilipp marcphilipp commented May 17, 2019

This PR introduces declarative timeouts for testable and lifecycle methods. The API surface consists of a new @Timeout implementation that can be applied to @Test, @TestTemplate, @TestFactory, and lifecycle methods. In addition, it can be applied to a test class which has the same meaning as declaring it on all of its testable, but not its lifecycle methods.

Moreover, default timeouts can be defined using configuration parameters for all of the above methods, all lifecycle methods, all testable methods, or each type individually.

The timeout is implemented in a preemptive manner, but without having to execute the test in a different thread. Instead, a single-thread ExecutorService is used to schedule interrupting the original test thread. This way, ThreadLocal will continue to work which is important for integration tests that use frameworks like Spring and its @Transactional support.

Resolves #80.


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


Definition of Done

@marcphilipp marcphilipp requested review from sbrannen and sormuras May 17, 2019 18:48
@marcphilipp marcphilipp self-assigned this May 17, 2019
@codecov
Copy link

codecov bot commented May 17, 2019

Codecov Report

Merging #1885 into master will increase coverage by 0.16%.
The diff coverage is 96.12%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1885      +/-   ##
============================================
+ Coverage     91.42%   91.58%   +0.16%     
- Complexity     4149     4225      +76     
============================================
  Files           351      356       +5     
  Lines          9978    10133     +155     
  Branches        802      814      +12     
============================================
+ Hits           9122     9280     +158     
+ Misses          659      654       -5     
- Partials        197      199       +2
Impacted Files Coverage Δ Complexity Δ
...it/jupiter/engine/extension/ExtensionRegistry.java 94.44% <ø> (ø) 24 <0> (ø) ⬇️
...it/jupiter/engine/extension/TimeoutInvocation.java 100% <100%> (ø) 7 <7> (?)
...jupiter/engine/extension/TimeoutConfiguration.java 100% <100%> (ø) 17 <17> (?)
...upiter/engine/extension/TimeoutDurationParser.java 100% <100%> (ø) 5 <5> (?)
...unit/jupiter/engine/extension/TimeoutDuration.java 89.47% <89.47%> (ø) 14 <14> (?)
...nit/jupiter/engine/extension/TimeoutExtension.java 93.1% <93.1%> (ø) 27 <27> (?)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e602724...13af1f4. Read the comment docs.

@marcphilipp marcphilipp added this to the 5.5 RC1 milestone May 23, 2019
@marcphilipp marcphilipp force-pushed the issues/80-timeouts branch 2 times, most recently from 39052ec to 8fb796d Compare May 23, 2019 19:49
Copy link
Member

@sbrannen sbrannen left a comment

Choose a reason for hiding this comment

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

First pass at a review. Haven't reviewed the extension implementation or tests in detail yet.

.isNotEqualTo("foo") //
.isNotEqualTo(new TimeoutDuration(2, SECONDS)) //
.isNotEqualTo(new TimeoutDuration(1, MINUTES));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@marcphilipp Have you considered using EqualsVerifier here? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

ping :)

Copy link
Member

@sbrannen sbrannen left a comment

Choose a reason for hiding this comment

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

This is coming along very nicely. Great work!

I've requested a few minor changes.

@marcphilipp marcphilipp force-pushed the issues/80-timeouts branch from 275c102 to f78e9cc Compare May 27, 2019 06:08
@marcphilipp marcphilipp changed the title [WIP] Introduce @Timeout extension Introduce @Timeout extension May 27, 2019
@marcphilipp marcphilipp changed the title Introduce @Timeout extension Introduce declarative timeouts May 27, 2019
@marcphilipp marcphilipp marked this pull request as ready for review May 27, 2019 06:11
@marcphilipp marcphilipp requested a review from sbrannen May 27, 2019 18:31
@marcphilipp marcphilipp force-pushed the issues/80-timeouts branch from c1d5583 to 26bb440 Compare May 27, 2019 19:07
@marcphilipp marcphilipp force-pushed the issues/80-timeouts branch from 26bb440 to 2a2850c Compare May 27, 2019 19:57
Copy link
Member

@sbrannen sbrannen left a comment

Choose a reason for hiding this comment

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

Thanks for the previous round of updates!

Hopefully this is my final set of review comments and questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Introduce global timeouts for tests

5 participants