Skip to content

Conversation

@pietrygamat
Copy link
Contributor

@pietrygamat pietrygamat commented Apr 22, 2019

Overview

This change is an alternative take on issue #1454 based on still not merged PR #1509. In this version there are separate interfaces supporting exception handling in each of @BeforeAll, @BeforeEach, @AfterEach and @AfterAll, so that it is not forced on the handler to implement handleTestExecutionException in addition to handle(Before/After)(/All/Each)ExecutionException


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


Definition of Done

@pietrygamat pietrygamat force-pushed the issue_1454 branch 4 times, most recently from 15d4bca to 083a233 Compare April 22, 2019 13:59
@pietrygamat pietrygamat changed the title Issue 1454 Issue #1454 Introduce extension APIs to handle exceptions from lifecycle callback methods Apr 22, 2019
@marcphilipp marcphilipp added this to the 5.5 Backlog milestone May 1, 2019
@marcphilipp
Copy link
Member

@JockX Thanks for the PR! We'll discuss it in the next team meeting this Friday.

@marcphilipp marcphilipp modified the milestones: 5.5 Backlog, 5.5 M2 May 3, 2019
@sbrannen sbrannen changed the title Issue #1454 Introduce extension APIs to handle exceptions from lifecycle callback methods Introduce extension APIs to handle exceptions from lifecycle callback methods May 5, 2019
@marcphilipp
Copy link
Member

Team Decision: Please put all of the new methods into a single LifecycleMethodExecutionExceptionHandler interface, rename the methods to handle[...]MethodExecutionException (add Method), make them default methods with empty implementations, and throw them into the kitchen sink.

@marcphilipp
Copy link
Member

@JockX Thanks for updating the PR! Please run ./gradlew spotlessApply to fix the build.

@pietrygamat pietrygamat force-pushed the issue_1454 branch 2 times, most recently from d1cf4cc to 857a1db Compare May 28, 2019 22:02
@pietrygamat
Copy link
Contributor Author

@marcphilipp I would appreciate some help regarding the failure in azure tests. Does not seem to be related to the changes introduced in this PR

@marcphilipp
Copy link
Member

No, that's related to me merging #1885, I'm afraid. I'll try to fix it ASAP.

@marcphilipp
Copy link
Member

@JockX It's fixed in master now. Please rebase once again.

Implement LifecycleMethodExecutionExceptionHandler allowing extensions
to intercept the exceptions thrown during execution of methods annotated
with `@BeforeAll`, `@BeforeEach`, `@AfterEach` and `@AfterAll` before
they propagate.

Issue junit-team#1454
Copy link
Member

@marcphilipp marcphilipp 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 rebasing! This looks very promising!

Copy link
Member

@sormuras sormuras 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 to me.

Especially the LifecycleMethodExecutionExceptionHandlersTests class. 👍

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 all of the work on this PR.

I think this is good enough to be merged into master now. The implementation looks solid, and we can polish documentation, etc. after the merge.

@JockX, can you please provide the source for the PNG you have supplied?

There are some inconsistencies in the image, and it would be great if anyone in the community can update the image.

Also, please rebase on the current master and resolve conflicts. The PR should then be ready to merge.

@sbrannen
Copy link
Member

sbrannen commented Jun 6, 2019

Actually, I'll see if I can merge this PR and resolve the conflicts myself.

So, please hold off on making any further changes at this time.

@sbrannen
Copy link
Member

sbrannen commented Jun 6, 2019

This has been merged into master in 2844ee4.

However, @JockX, it would be great if you could provide the source for the PNG you supplied.

Thanks in advance!

@pietrygamat
Copy link
Contributor Author

@sbrannen, the png is a screenshot of a word document I created to more or less mimic the original. Should I put it in the repo? How is this usually handled?

@sbrannen
Copy link
Member

sbrannen commented Jun 6, 2019

@sbrannen, the png is a screenshot of a word document I created to more or less mimic the original. Should I put it in the repo?

Yes, that would be great! 👍

How is this usually handled?

Funny you should ask... for this particular graphic, the original author lost the original source, meaning we've had no way to edit it. So now that you have a "source", we would be very grateful to gain access to it. 😉

@sbrannen
Copy link
Member

sbrannen commented Jun 6, 2019

Note that this PR was further refined in 8aaac85.

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.

4 participants