Revert to a simplified extension model #237
Conversation
This commit separates the execution of extensions and user code with regard to test lifecycle callbacks. Issue: junit-team#232
This commit avoids unnecessary streaming and synthesizing of extensions as lambda expressions for @BeforeAll and @afterall methods. Instead, @BeforeAll and @afterall methods are now invoked directly using standard for-loops. Issue: junit-team#232
With this commit @beforeeach and @AfterEach methods are looked up and validated eagerly during the discovery phase instead of late in the game during the execution phase. This commit also extracts a new LifecycleMethodUtils class from the ClassTestDescriptor. Issue: junit-team#232
|
I did a short analysis of potential conflicts between this PR and the changes in branch issue58-dynamic-tests-with-new-resolvers. There are only 2 files changed by both branches. The difference in MethodInvoker is trivial, the conflicts in MethodTestDescriptor are more substantial but should probably be resolvable... |
|
BTW |
|
Thanks for checking for potential conflicts. I hope the merging with |
"Indentation art" --> you've coined a new term! We've already been using that technique in our tests when we want to verify extension execution order, so it only seemed fitting to do the same in the production code for exactly the same reason: visualization of the wrapping nature of extensions and lifecycle callbacks. So I'm glad you like it. 😉 |
| * | ||
| * @since 5.0 | ||
| */ | ||
| final class LifecycleMethodUtils { |
There was a problem hiding this comment.
Nice. I like the idea of having a central place of how to lookup and find the different pieces. We also do have some more of these things in extra classes. I am wondering if we could consolidate those all into a "function" library. This will not be part of this PR, just an idea I wanted to mention.
|
Great job! Thanks. I support the changes in this PR and vote for merging it into master. Let's talk about it in our call. |
| @@ -78,6 +76,8 @@ public ClassTestDescriptor(UniqueId uniqueId, Class<?> testClass) { | |||
|
|
|||
| this.beforeAllMethods = findBeforeAllMethods(testClass); | |||
| this.afterAllMethods = findAfterAllMethods(testClass); | |||
| this.beforeEachMethods = findBeforeEachMethods(testClass); | |||
| this.afterEachMethods = findAfterEachMethods(testClass); | |||
There was a problem hiding this comment.
Where are the exceptions thrown by these methods caught?
There was a problem hiding this comment.
To be addressed in #242. 😉
|
I think we need a follow-up issue to take care of exception handling in the JUnit 5 TestEngine. E.g. invalid before-each methods should not lead to a complete abortion of the discovery process. |
|
Team Decisions:
|
sbrannen
force-pushed
the
issue232-simplified-extension-model
branch
from
76193be to
744eb9b
Compare
…odel * issue232-simplified-extension-model: Update User Guide regarding renamed and new extensions Delete discussion of programmatic extension registration in User Guide Polish JUnit 5 test descriptors Eagerly load and validate @beforeeach & @AfterEach methods Do not synthesize @BeforeAll & @afterall methods as extensions Do not mix execution of extensions and user code Polish JUnit 5 test descriptors Polish Javadoc for extension APIs Remove ExtensionPointRegistry and ExtensionRegistrar Remove ExtensionPoint API in favor of Extension Simplify extension streaming and invocation algorithms Introduce before & after test method callback APIs Rename test lifecycle callback extension APIs Rename ExceptionHandlerExtensionPoint to ExceptionHandler
In order to postpone support for programmatic extension registration, the JUnit Team has decided to revert to a simplified extension model with a focus on extensions that are applied at extension points.
The commits in this pull request therefore address the following issues.
ExtensionPoint,ExtensionPointRegistry, andExtensionRegistrarExtensionPointin extension interface namesIssue: #232
I hereby agree to the terms of the JUnit Contributor License Agreement.