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

Always call pre-destroy callbacks if instance was created #2316

Conversation

marcphilipp
Copy link
Member

Prior to this commit, pre-destroy callbacks were not called when an
instance was created but post-processing it failed. Now, they are always
called if the instance was created, regardless if any subsequent step
failed.

We previously had a test that asserted the old behavior. However, IMHO
the new behavior is more consistent and changing it is ok since
TestInstancePreDestroyCallback is still experimental.

Resolves #2312.

Prior to this commit, pre-destroy callbacks were not called when an
instance was created but post-processing it failed. Now, they are always
called if the instance was created, regardless if any subsequent step
failed.

Resolves #2312.
@marcphilipp marcphilipp requested a review from sbrannen June 6, 2020 12:24
@marcphilipp marcphilipp self-assigned this Jun 6, 2020
@codecov
Copy link

codecov bot commented Jun 6, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@be2aa24). Click here to learn what that means.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2316   +/-   ##
=========================================
  Coverage          ?   90.40%           
  Complexity        ?     4524           
=========================================
  Files             ?      393           
  Lines             ?    11045           
  Branches          ?      891           
=========================================
  Hits              ?     9985           
  Misses            ?      792           
  Partials          ?      268           
Impacted Files Coverage Δ Complexity Δ
...jupiter/engine/descriptor/ClassTestDescriptor.java 100.00% <ø> (ø) 6.00 <0.00> (?)
...r/engine/descriptor/NestedClassTestDescriptor.java 100.00% <ø> (ø) 6.00 <0.00> (?)
...er/engine/descriptor/ClassBasedTestDescriptor.java 96.80% <100.00%> (ø) 71.00 <5.00> (?)
...er/engine/descriptor/TestMethodTestDescriptor.java 97.90% <100.00%> (ø) 53.00 <4.00> (?)
...upiter/engine/execution/TestInstancesProvider.java 100.00% <100.00%> (ø) 1.00 <1.00> (?)
...upiter/engine/descriptor/LifecycleMethodUtils.java 100.00% <0.00%> (ø) 17.00% <0.00%> (?%)
...g/junit/platform/commons/util/ToStringBuilder.java 100.00% <0.00%> (ø) 7.00% <0.00%> (?%)
.../java/org/junit/platform/runner/JUnitPlatform.java 97.91% <0.00%> (ø) 47.00% <0.00%> (?%)
...ter/params/ParameterizedTestParameterResolver.java 100.00% <0.00%> (ø) 8.00% <0.00%> (?%)
...va/org/junit/jupiter/api/DisplayNameGenerator.java 100.00% <0.00%> (ø) 1.00% <0.00%> (?%)
... and 388 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 be2aa24...18c27aa. Read the comment docs.

@sbrannen sbrannen added this to the 5.7 M2 milestone Jun 15, 2020
@marcphilipp marcphilipp changed the base branch from master to main June 21, 2020 16:57
@marcphilipp marcphilipp removed this from the 5.7 M2 milestone Jun 26, 2020
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 PR, @marcphilipp!

I reviewed the changes in the browser, and I think it's safe to say: LGTM.

@marcphilipp marcphilipp merged commit bd94e4f into main Jul 3, 2020
@marcphilipp marcphilipp deleted the issues/2312-call-predestroy-callbacks-whenever-testinstance-created branch July 3, 2020 10:19
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.

preDestroyTestInstance not able to get test instance from context if postProcessTestInstance failed
2 participants