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

Add around() to Node to allow intercepting the whole execution #1705

Closed
wants to merge 1 commit into from

Conversation

leonard84
Copy link
Collaborator

@leonard84 leonard84 commented Dec 19, 2018

Overview

as discussed with @marcphilipp


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


Definition of Done

@ghost ghost assigned leonard84 Dec 19, 2018
@ghost ghost added the status: in progress label Dec 19, 2018
Co-authored-by: Marc Philipp <mail@marcphilipp.de>
@codecov
Copy link

codecov bot commented Dec 19, 2018

Codecov Report

Merging #1705 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1705      +/-   ##
============================================
+ Coverage     88.44%   88.45%   +<.01%     
- Complexity     3801     3804       +3     
============================================
  Files           345      345              
  Lines          9335     9342       +7     
  Branches        751      751              
============================================
+ Hits           8256     8263       +7     
  Misses          883      883              
  Partials        196      196
Impacted Files Coverage Δ Complexity Δ
...nit/platform/engine/support/hierarchical/Node.java 80.76% <100%> (+1.6%) 8 <1> (+1) ⬆️
...form/engine/support/hierarchical/NodeTestTask.java 97.5% <100%> (+0.16%) 27 <7> (+2) ⬆️

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 9bb7786...7aa1bdb. Read the comment docs.

3 similar comments
@codecov
Copy link

codecov bot commented Dec 19, 2018

Codecov Report

Merging #1705 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1705      +/-   ##
============================================
+ Coverage     88.44%   88.45%   +<.01%     
- Complexity     3801     3804       +3     
============================================
  Files           345      345              
  Lines          9335     9342       +7     
  Branches        751      751              
============================================
+ Hits           8256     8263       +7     
  Misses          883      883              
  Partials        196      196
Impacted Files Coverage Δ Complexity Δ
...nit/platform/engine/support/hierarchical/Node.java 80.76% <100%> (+1.6%) 8 <1> (+1) ⬆️
...form/engine/support/hierarchical/NodeTestTask.java 97.5% <100%> (+0.16%) 27 <7> (+2) ⬆️

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 9bb7786...7aa1bdb. Read the comment docs.

@codecov
Copy link

codecov bot commented Dec 19, 2018

Codecov Report

Merging #1705 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1705      +/-   ##
============================================
+ Coverage     88.44%   88.45%   +<.01%     
- Complexity     3801     3804       +3     
============================================
  Files           345      345              
  Lines          9335     9342       +7     
  Branches        751      751              
============================================
+ Hits           8256     8263       +7     
  Misses          883      883              
  Partials        196      196
Impacted Files Coverage Δ Complexity Δ
...nit/platform/engine/support/hierarchical/Node.java 80.76% <100%> (+1.6%) 8 <1> (+1) ⬆️
...form/engine/support/hierarchical/NodeTestTask.java 97.5% <100%> (+0.16%) 27 <7> (+2) ⬆️

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 9bb7786...7aa1bdb. Read the comment docs.

@codecov
Copy link

codecov bot commented Dec 19, 2018

Codecov Report

Merging #1705 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1705      +/-   ##
============================================
+ Coverage     88.44%   88.45%   +<.01%     
- Complexity     3801     3804       +3     
============================================
  Files           345      345              
  Lines          9335     9342       +7     
  Branches        751      751              
============================================
+ Hits           8256     8263       +7     
  Misses          883      883              
  Partials        196      196
Impacted Files Coverage Δ Complexity Δ
...nit/platform/engine/support/hierarchical/Node.java 80.76% <100%> (+1.6%) 8 <1> (+1) ⬆️
...form/engine/support/hierarchical/NodeTestTask.java 97.5% <100%> (+0.16%) 27 <7> (+2) ⬆️

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 9bb7786...7aa1bdb. Read the comment docs.

@sbrannen
Copy link
Member

as discussed with @marcphilipp

Can you please provide a rationale for the proposed change (perhaps linking to an existing issue or discussion) and include that in the commit message as well?

Thanks in advance!

@sbrannen
Copy link
Member

Tentatively slated for 5.4 M1 for team discussion.

@marcphilipp
Copy link
Member

The rationale is to support Spock's extension model that allows to intercept, i.e. wrap, the whole execution of a test class, method, or invocation.

@sbrannen
Copy link
Member

The rationale is to support Spock's extension model that allows to intercept, i.e. wrap, the whole execution of a test class, method, or invocation.

OK. Thanks for the background.

But... doesn't this change then relate to #80 and #157?

And if so, has there been any effort made to ensure that this change would work nicely with those (once implemented)?

@marcphilipp
Copy link
Member

I think it's related in a way because both #80 and #157 will require a similar wrapping pattern. However, in both case on a much more fine-grained level, e.g. only the @BeforeEach method or the @Test method. I don't think the Jupiter Nodes will override this around() method and therefore I don't think it will make it any easier or harder to implement #80 and #157.

@sormuras
Copy link
Member

Team Decision: Merge it.

@sormuras
Copy link
Member

sormuras commented Dec 21, 2018

Merge it ... after remaining deliverables are finished:

  • Change is documented in the Release Notes

@sbrannen
Copy link
Member

I think it's related in a way because both #80 and #157 will require a similar wrapping pattern.

Thanks for the feedback.

However, in both case on a much more fine-grained level, e.g. only the @BeforeEach method or the @Test method. I don't think the Jupiter Nodes will override this around() method and therefore I don't think it will make it any easier or harder to implement #80 and #157.

Well... let's see how it all plays out. 😉

@ghost ghost removed the status: in progress label Dec 22, 2018
@marcphilipp
Copy link
Member

I've added release notes and manually pushed this PR to master in 77c39f2.

@leonard84 leonard84 deleted the around-node branch March 30, 2019 13:00
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.

4 participants