Fix ExtensionContext on instance creation #4032
Conversation
|
see #3445 (comment) |
| @Target(ElementType.TYPE) | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| @Inherited | ||
| @API(status = MAINTAINED, since = "5.12") |
There was a problem hiding this comment.
I wasn't sure which status to use in the API annotation. Which status do you suggest?
If we want to us EXPERIMENTAL for now, then I should probably also reformulate my note in the Javadoc. (Suggesting to switch to an experimental API for future compatibility seems a bit odd. 😄)
| @Retention(RetentionPolicy.RUNTIME) | ||
| @Inherited | ||
| @API(status = MAINTAINED, since = "5.12") | ||
| public @interface EnableTestScopedConstructorContext { |
There was a problem hiding this comment.
Feel free to suggest other names. This is the best name that came to mind. I think the name is good enough, at least I like it much more than my other ideas (e.g. NewTestInstanceConstructionContext).
There was a problem hiding this comment.
I just realized that you have already suggested @MethodLevelExtensionContextAware. I don't have a strong preference. Your suggestion is a bit shorter.
| private Object invokeTestClassConstructor(Optional<Object> outerInstance, ExtensionRegistry registry, | ||
| ExtensionContext extensionContext) { | ||
| ExtensionContext extensionContext, ExtensionContext ourExtensionContext) { | ||
|
|
||
| Constructor<?> constructor = ReflectionUtils.getDeclaredConstructor(this.testClass); | ||
| return executableInvoker.invoke(constructor, outerInstance, extensionContext, registry, | ||
| InvocationInterceptor::interceptTestClassConstructor); |
There was a problem hiding this comment.
Calls to ParameterResolver are not yet looking at the new annotation. Shall calls to ParameterResolver also consider the annotation?
I just wanted to confirm that I should adjust this method as well. I was a bit reluctant to change InterceptingExecutableInvoker for some reason. For now, I wrote the documentation matching the current behavior, I will change the documentation together with the implementation.
| if (AnnotationUtils.isAnnotated(this.testInstanceFactory.getClass(), | ||
| EnableTestScopedConstructorContext.class)) { |
There was a problem hiding this comment.
I am a little bit concerned about performance with all these calls to AnnotationUtils.isAnnotated. Do you think this could be an issue?
Overview
Resolve #1568.
Resolve #2970.
Resolve #3445.
This PR moves the instance creation into the execution context of the test method. This affects the following callbacks:
TestInstancePreConstructCallbackTestInstanceFactoryParameterResolver(when resolving parameters for the constructor)TestInstancePostProcessorAssuming the test is using
TestInstance.Lifecycle.PER_METHOD(the default), these callbacks will now receive theExtensionContextfor the currently executed test method. This has the following effects:ExtensionContext.getTestMethod()ExtensionContext.getTestClass()andTestInstanceFactoryContext.getTestClass()are no-longer interchangeableExtensionContext.getTestClass()now returns the (nested) class of the currently executed testCloseableResourcewhich are added to the store will now be closed at the end of the test executionIf the test is using
TestInstance.Lifecycle.PER_CLASS, the behavior is unaltered.Not sure if the change is considered API breaking. The previous behavior was rather unintuitive and not explicitly documented. However, the example at
TestInfoDemois no-longer valid and has been changed as part of the PR. I also think there is a good chance that there are a few extensions in the wild which rely on the previous behavior, since the behavior was in place for multiple years. I would therefore like to know how I should proceed with the PR.I hereby agree to the terms of the JUnit Contributor License Agreement.
Definition of Done
@APIannotations