-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Fix ExtensionContext on instance creation #4032
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
/* | ||
* Copyright 2015-2024 the original author or authors. | ||
* | ||
* All rights reserved. This program and the accompanying materials are | ||
* made available under the terms of the Eclipse Public License v2.0 which | ||
* accompanies this distribution and is available at | ||
* | ||
* https://www.eclipse.org/legal/epl-v20.html | ||
*/ | ||
|
||
package org.junit.jupiter.api.extension; | ||
|
||
import static org.apiguardian.api.API.Status.MAINTAINED; | ||
|
||
import java.lang.annotation.ElementType; | ||
import java.lang.annotation.Inherited; | ||
import java.lang.annotation.Retention; | ||
import java.lang.annotation.RetentionPolicy; | ||
import java.lang.annotation.Target; | ||
|
||
import org.apiguardian.api.API; | ||
import org.junit.jupiter.api.TestInstance; | ||
import org.junit.jupiter.api.extension.ExtensionContext.Store; | ||
import org.junit.jupiter.api.extension.ExtensionContext.Store.CloseableResource; | ||
|
||
/** | ||
* {@code @EnableTestScopedConstructorContext} allows | ||
* {@link Extension Extensions} to use a test-scoped {@link ExtensionContext} | ||
* during creation of test instances. | ||
* | ||
* <p>The annotation should be used on extension classes. | ||
* JUnit will call the following extension callbacks of annotated extensions | ||
* with a test-scoped {@link ExtensionContext}, unless the test class is | ||
* annotated with {@link TestInstance @TestInstance(Lifecycle.PER_CLASS)}. | ||
* | ||
* <ul> | ||
* <li>{@link TestInstancePreConstructCallback}</li> | ||
* <li>{@link TestInstancePostProcessor}</li> | ||
* <li>{@link TestInstanceFactory}</li> | ||
* </ul> | ||
* | ||
* <p>Implementations of these extension callbacks can observe the following | ||
* differences if they are using {@code @EnableTestScopedConstructorContext}. | ||
* | ||
* <ul> | ||
* <li>{@link ExtensionContext#getElement() getElement()} may refer to the test | ||
* method and {@link ExtensionContext#getTestClass() getTestClass()} may refer | ||
* to a nested test class. Use {@link TestInstanceFactoryContext#getTestClass()} | ||
* to get the class under construction.</li> | ||
* <li>{@link ExtensionContext#getTestMethod() getTestMethod()} is no-longer | ||
* empty, unless the test class is annotated with | ||
* {@link TestInstance @TestInstance(Lifecycle.PER_CLASS)}.</li> | ||
* <li>If the callback adds a new {@link CloseableResource CloseableResource} to | ||
* the {@link Store Store}, the resource is closed just after the instance is | ||
* destroyed.</li> | ||
* <li>The callbacks can now access data previously stored by | ||
* {@link TestTemplateInvocationContext}, unless the test class is annotated | ||
* with {@link TestInstance @TestInstance(Lifecycle.PER_CLASS)}.</li> | ||
* </ul> | ||
* | ||
* <p><strong>Note</strong>: The behavior which is enabled by this annotation is | ||
* expected to become the default in future versions of JUnit Jupiter. To ensure | ||
* future compatibility, extension vendors are therefore advised to annotate | ||
* their extensions, even if they don't need the new functionality. | ||
* | ||
* @since 5.12 | ||
* @see TestInstancePreConstructCallback | ||
* @see TestInstancePostProcessor | ||
* @see TestInstanceFactory | ||
*/ | ||
@Target(ElementType.TYPE) | ||
@Retention(RetentionPolicy.RUNTIME) | ||
@Inherited | ||
@API(status = MAINTAINED, since = "5.12") | ||
public @interface EnableTestScopedConstructorContext { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just realized that you have already suggested |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,6 +39,7 @@ | |
import org.junit.jupiter.api.TestInstance.Lifecycle; | ||
import org.junit.jupiter.api.extension.AfterAllCallback; | ||
import org.junit.jupiter.api.extension.BeforeAllCallback; | ||
import org.junit.jupiter.api.extension.EnableTestScopedConstructorContext; | ||
import org.junit.jupiter.api.extension.Extension; | ||
import org.junit.jupiter.api.extension.ExtensionConfigurationException; | ||
import org.junit.jupiter.api.extension.ExtensionContext; | ||
|
@@ -66,6 +67,7 @@ | |
import org.junit.jupiter.engine.extension.ExtensionRegistry; | ||
import org.junit.jupiter.engine.extension.MutableExtensionRegistry; | ||
import org.junit.platform.commons.JUnitException; | ||
import org.junit.platform.commons.util.AnnotationUtils; | ||
import org.junit.platform.commons.util.ExceptionUtils; | ||
import org.junit.platform.commons.util.ReflectionUtils; | ||
import org.junit.platform.commons.util.StringUtils; | ||
|
@@ -201,8 +203,7 @@ public JupiterEngineExecutionContext before(JupiterEngineExecutionContext contex | |
// and store the instance in the ExtensionContext. | ||
ClassExtensionContext extensionContext = (ClassExtensionContext) context.getExtensionContext(); | ||
throwableCollector.execute(() -> { | ||
TestInstances testInstances = context.getTestInstancesProvider().getTestInstances( | ||
context.getExtensionRegistry(), throwableCollector); | ||
TestInstances testInstances = context.getTestInstancesProvider().getTestInstances(context); | ||
extensionContext.setTestInstances(testInstances); | ||
}); | ||
} | ||
|
@@ -273,52 +274,61 @@ private TestInstanceFactory resolveTestInstanceFactory(ExtensionRegistry registr | |
} | ||
|
||
private TestInstancesProvider testInstancesProvider(JupiterEngineExecutionContext parentExecutionContext, | ||
ClassExtensionContext extensionContext) { | ||
ClassExtensionContext ourExtensionContext) { | ||
|
||
return (registry, registrar, throwableCollector) -> extensionContext.getTestInstances().orElseGet( | ||
() -> instantiateAndPostProcessTestInstance(parentExecutionContext, extensionContext, registry, registrar, | ||
throwableCollector)); | ||
// For Lifecycle.PER_CLASS, ourExtensionContext.getTestInstances() is used to store the instance. | ||
// Otherwise, extensionContext.getTestInstances() is always empty and we always create a new instance. | ||
return (registry, context) -> ourExtensionContext.getTestInstances().orElseGet( | ||
() -> instantiateAndPostProcessTestInstance(parentExecutionContext, ourExtensionContext, registry, | ||
context)); | ||
} | ||
|
||
private TestInstances instantiateAndPostProcessTestInstance(JupiterEngineExecutionContext parentExecutionContext, | ||
ExtensionContext extensionContext, ExtensionRegistry registry, ExtensionRegistrar registrar, | ||
ThrowableCollector throwableCollector) { | ||
ClassExtensionContext ourExtensionContext, ExtensionRegistry registry, | ||
JupiterEngineExecutionContext context) { | ||
|
||
TestInstances instances = instantiateTestClass(parentExecutionContext, registry, registrar, extensionContext, | ||
throwableCollector); | ||
throwableCollector.execute(() -> { | ||
invokeTestInstancePostProcessors(instances.getInnermostInstance(), registry, extensionContext); | ||
TestInstances instances = instantiateTestClass(parentExecutionContext, ourExtensionContext, registry, context); | ||
context.getThrowableCollector().execute(() -> { | ||
invokeTestInstancePostProcessors(instances.getInnermostInstance(), registry, context.getExtensionContext(), | ||
ourExtensionContext); | ||
// In addition, we initialize extension registered programmatically from instance fields here | ||
// since the best time to do that is immediately following test class instantiation | ||
// and post-processing. | ||
registrar.initializeExtensions(this.testClass, instances.getInnermostInstance()); | ||
context.getExtensionRegistry().initializeExtensions(this.testClass, instances.getInnermostInstance()); | ||
}); | ||
return instances; | ||
} | ||
|
||
protected abstract TestInstances instantiateTestClass(JupiterEngineExecutionContext parentExecutionContext, | ||
ExtensionRegistry registry, ExtensionRegistrar registrar, ExtensionContext extensionContext, | ||
ThrowableCollector throwableCollector); | ||
ExtensionContext ourExtensionContext, ExtensionRegistry registry, JupiterEngineExecutionContext context); | ||
|
||
protected TestInstances instantiateTestClass(Optional<TestInstances> outerInstances, ExtensionRegistry registry, | ||
ExtensionContext extensionContext) { | ||
ExtensionContext extensionContext, ExtensionContext ourExtensionContext) { | ||
|
||
Optional<Object> outerInstance = outerInstances.map(TestInstances::getInnermostInstance); | ||
invokeTestInstancePreConstructCallbacks(new DefaultTestInstanceFactoryContext(this.testClass, outerInstance), | ||
registry, extensionContext); | ||
registry, extensionContext, ourExtensionContext); | ||
Object instance = this.testInstanceFactory != null // | ||
? invokeTestInstanceFactory(outerInstance, extensionContext) // | ||
: invokeTestClassConstructor(outerInstance, registry, extensionContext); | ||
? invokeTestInstanceFactory(outerInstance, extensionContext, ourExtensionContext) // | ||
: invokeTestClassConstructor(outerInstance, registry, extensionContext, ourExtensionContext); | ||
return outerInstances.map(instances -> DefaultTestInstances.of(instances, instance)).orElse( | ||
DefaultTestInstances.of(instance)); | ||
} | ||
|
||
private Object invokeTestInstanceFactory(Optional<Object> outerInstance, ExtensionContext extensionContext) { | ||
private Object invokeTestInstanceFactory(Optional<Object> outerInstance, ExtensionContext extensionContext, | ||
ExtensionContext ourExtensionContext) { | ||
Object instance; | ||
|
||
try { | ||
instance = this.testInstanceFactory.createTestInstance( | ||
new DefaultTestInstanceFactoryContext(this.testClass, outerInstance), extensionContext); | ||
if (AnnotationUtils.isAnnotated(this.testInstanceFactory.getClass(), | ||
EnableTestScopedConstructorContext.class)) { | ||
Comment on lines
+323
to
+324
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am a little bit concerned about performance with all these calls to |
||
instance = this.testInstanceFactory.createTestInstance( | ||
new DefaultTestInstanceFactoryContext(this.testClass, outerInstance), extensionContext); | ||
} | ||
else { | ||
instance = this.testInstanceFactory.createTestInstance( | ||
new DefaultTestInstanceFactoryContext(this.testClass, outerInstance), ourExtensionContext); | ||
} | ||
} | ||
catch (Throwable throwable) { | ||
UnrecoverableExceptions.rethrowIfUnrecoverable(throwable); | ||
|
@@ -358,24 +368,36 @@ private Object invokeTestInstanceFactory(Optional<Object> outerInstance, Extensi | |
} | ||
|
||
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); | ||
Comment on lines
370
to
375
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Calls to I just wanted to confirm that I should adjust this method as well. I was a bit reluctant to change |
||
} | ||
|
||
private void invokeTestInstancePreConstructCallbacks(TestInstanceFactoryContext factoryContext, | ||
ExtensionRegistry registry, ExtensionContext context) { | ||
registry.stream(TestInstancePreConstructCallback.class).forEach( | ||
extension -> executeAndMaskThrowable(() -> extension.preConstructTestInstance(factoryContext, context))); | ||
ExtensionRegistry registry, ExtensionContext context, ExtensionContext ourContext) { | ||
registry.stream(TestInstancePreConstructCallback.class).forEach(extension -> { | ||
if (AnnotationUtils.isAnnotated(extension.getClass(), EnableTestScopedConstructorContext.class)) { | ||
executeAndMaskThrowable(() -> extension.preConstructTestInstance(factoryContext, context)); | ||
} | ||
else { | ||
executeAndMaskThrowable(() -> extension.preConstructTestInstance(factoryContext, ourContext)); | ||
} | ||
}); | ||
} | ||
|
||
private void invokeTestInstancePostProcessors(Object instance, ExtensionRegistry registry, | ||
ExtensionContext context) { | ||
private void invokeTestInstancePostProcessors(Object instance, ExtensionRegistry registry, ExtensionContext context, | ||
ClassExtensionContext ourContext) { | ||
|
||
registry.stream(TestInstancePostProcessor.class).forEach( | ||
extension -> executeAndMaskThrowable(() -> extension.postProcessTestInstance(instance, context))); | ||
registry.stream(TestInstancePostProcessor.class).forEach(extension -> { | ||
if (AnnotationUtils.isAnnotated(extension.getClass(), EnableTestScopedConstructorContext.class)) { | ||
executeAndMaskThrowable(() -> extension.postProcessTestInstance(instance, context)); | ||
} | ||
else { | ||
executeAndMaskThrowable(() -> extension.postProcessTestInstance(instance, ourContext)); | ||
} | ||
}); | ||
} | ||
|
||
private void executeAndMaskThrowable(Executable executable) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 😄)