From 3c1472f03a4ab1e5be5578c73ac1522eb4e41c01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Michael?= Date: Fri, 22 Dec 2023 09:07:37 +0100 Subject: [PATCH] Code review amendments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Björn Michael --- .../asciidoc/user-guide/writing-tests.adoc | 2 +- .../java/org/junit/jupiter/api/AutoClose.java | 34 ++++++------ .../engine/extension/AutoCloseExtension.java | 36 ++++++------- .../engine/extension/AutoCloseTests.java | 52 +++++++++++-------- 4 files changed, 63 insertions(+), 61 deletions(-) diff --git a/documentation/src/docs/asciidoc/user-guide/writing-tests.adoc b/documentation/src/docs/asciidoc/user-guide/writing-tests.adoc index 2bcf6e4ad3da..3cd6a9a33353 100644 --- a/documentation/src/docs/asciidoc/user-guide/writing-tests.adoc +++ b/documentation/src/docs/asciidoc/user-guide/writing-tests.adoc @@ -45,7 +45,7 @@ in the `junit-jupiter-api` module. | `@ExtendWith` | Used to <>. Such annotations are _inherited_. | `@RegisterExtension` | Used to <> via fields. Such fields are _inherited_ unless they are _shadowed_. | `@TempDir` | Used to supply a <> via field injection or parameter injection in a lifecycle method or test method; located in the `org.junit.jupiter.api.io` package. -| @AutoClose | Indicates that a field in a JUnit 5 test class represents a resource that should be automatically closed after test execution. +| `@AutoClose` | Denotes that the annotated field represents a resource that should be automatically closed after test execution. |=== WARNING: Some annotations may currently be _experimental_. Consult the table in diff --git a/junit-jupiter-api/src/main/java/org/junit/jupiter/api/AutoClose.java b/junit-jupiter-api/src/main/java/org/junit/jupiter/api/AutoClose.java index 5dac568dece0..6e53f2786473 100644 --- a/junit-jupiter-api/src/main/java/org/junit/jupiter/api/AutoClose.java +++ b/junit-jupiter-api/src/main/java/org/junit/jupiter/api/AutoClose.java @@ -18,29 +18,24 @@ import org.apiguardian.api.API; /** - * The {@code AutoClose} annotation is used to automatically close resources used in JUnit 5 tests. + * The {@code AutoClose} annotation is used to automatically close resources + * used in tests. * - *

- * This annotation should be applied to fields within JUnit 5 test classes. It indicates that the annotated - * resource should be automatically closed after the test execution. The annotation targets - * {@link java.lang.annotation.ElementType#FIELD} elements, allowing it to be applied to instance variables. - *

+ *

This annotation should be applied to fields within test classes. It + * indicates that the annotated resource should be automatically closed after + * the test execution. * - *

- * By default, the {@code AutoClose} annotation expects the annotated resource to provide a {@code close()} method - * that will be invoked for closing the resource. However, developers can customize the closing behavior by providing - * a different method name through the {@code value} attribute. For example, setting {@code value = "shutdown"} will - * look for a method named {@code shutdown()} to close the resource. - * When multiple annotated resources exist the order of closing them is unspecified. - *

- * - *

- * The {@code AutoClose} annotation is retained at runtime, allowing it to be accessed and processed during test execution. - *

+ *

By default, the {@code AutoClose} annotation expects the annotated + * resource to provide a {@code close()} method that will be invoked for closing + * the resource. However, developers can customize the closing behavior by + * providing a different method name through the {@link #value} attribute. For + * example, setting {@code value = "shutdown"} will look for a method named + * {@code shutdown()} to close the resource. When multiple annotated resources + * exist the order of closing them is unspecified. * + * @since 5.11 * @see java.lang.annotation.Retention * @see java.lang.annotation.Target - * @since 5.11 */ @Retention(RetentionPolicy.RUNTIME) @Target(ElementType.FIELD) @@ -49,7 +44,8 @@ /** * Specifies the name of the method to invoke for closing the resource. - * The default value is "close". + * + *

The default value is {@code close}. * * @return the method name for closing the resource */ diff --git a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/extension/AutoCloseExtension.java b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/extension/AutoCloseExtension.java index 965f89a168d2..269fadc6d6fb 100644 --- a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/extension/AutoCloseExtension.java +++ b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/extension/AutoCloseExtension.java @@ -16,7 +16,6 @@ import java.lang.reflect.Method; import java.util.function.Predicate; -import org.apiguardian.api.API; import org.junit.jupiter.api.AutoClose; import org.junit.jupiter.api.extension.AfterAllCallback; import org.junit.jupiter.api.extension.AfterEachCallback; @@ -30,20 +29,20 @@ import org.junit.platform.commons.util.ReflectionUtils; /** - * {@code AutoCloseExtension} is a JUnit Jupiter extension that closes resources if a field in a test class is annotated - * with {@link AutoClose @AutoClose}. + * {@code AutoCloseExtension} is a JUnit Jupiter extension that closes resources + * if a field in a test class is annotated with {@link AutoClose @AutoClose}. * - *

Consult the Javadoc for {@link AutoClose} for details on the contract. + *

Consult the Javadoc for {@link AutoClose @AutoClose} for details on the + * contract. * * @since 5.11 * @see AutoClose * @see AutoCloseable */ -@API(status = API.Status.EXPERIMENTAL, since = "5.11") -public class AutoCloseExtension implements AfterAllCallback, AfterEachCallback { +class AutoCloseExtension implements AfterAllCallback, AfterEachCallback { private static final Logger logger = LoggerFactory.getLogger(AutoCloseExtension.class); - static final Namespace NAMESPACE = Namespace.create(AutoClose.class); + private static final Namespace NAMESPACE = Namespace.create(AutoClose.class); @Override public void afterAll(ExtensionContext context) { @@ -62,9 +61,9 @@ public void afterEach(ExtensionContext context) { } } - private void registerCloseables(Store contextStore, Class testClass, /* @Nullable */ Object testInstance) { - Predicate isStatic = testInstance == null ? ReflectionUtils::isStatic : ReflectionUtils::isNotStatic; - findAnnotatedFields(testClass, AutoClose.class, isStatic).forEach(field -> { + private void registerCloseables(Store contextStore, Class testClass, Object testInstance) { + Predicate predicate = testInstance == null ? ReflectionUtils::isStatic : ReflectionUtils::isNotStatic; + findAnnotatedFields(testClass, AutoClose.class, predicate).forEach(field -> { try { contextStore.put(field, asCloseableResource(testInstance, field)); } @@ -74,28 +73,29 @@ private void registerCloseables(Store contextStore, Class testClass, /* @Null }); } - private static Store.CloseableResource asCloseableResource(/* @Nullable */ Object testInstance, Field field) { + private static Store.CloseableResource asCloseableResource(Object testInstance, Field field) { return () -> { Object toBeClosed = ReflectionUtils.tryToReadFieldValue(field, testInstance).get(); if (toBeClosed == null) { - logger.warn(() -> "@AutoClose: Field " + getQualifiedFieldName(field) - + " couldn't be closed because it was null."); + logger.warn(() -> "@AutoClose couldn't close object for field " + getQualifiedFieldName(field) + + " because it was null."); return; } - getAndDestroy(field, toBeClosed); + invokeCloseMethod(field, toBeClosed); }; } - private static void getAndDestroy(Field field, Object toBeClosed) { + private static void invokeCloseMethod(Field field, Object toBeClosed) { String methodName = field.getAnnotation(AutoClose.class).value(); Method destroyMethod = ReflectionUtils.findMethod(toBeClosed.getClass(), methodName).orElseThrow( - () -> new ExtensionConfigurationException("@AutoClose: Cannot resolve the destroy method " + methodName - + "() at " + getQualifiedFieldName(field) + ": " + field.getType().getSimpleName())); + () -> new ExtensionConfigurationException( + "@AutoClose failed to close object for field " + getQualifiedFieldName(field) + " because the " + + methodName + "() method could not be " + "resolved.")); ReflectionUtils.invokeMethod(destroyMethod, toBeClosed); } private static String getQualifiedFieldName(Field field) { - return field.getDeclaringClass().getSimpleName() + "." + field.getName(); + return field.getDeclaringClass().getName() + "." + field.getName(); } } diff --git a/junit-jupiter-engine/src/test/java/org/junit/jupiter/engine/extension/AutoCloseTests.java b/junit-jupiter-engine/src/test/java/org/junit/jupiter/engine/extension/AutoCloseTests.java index 71dd5a8ff3bb..e051d93ebe56 100644 --- a/junit-jupiter-engine/src/test/java/org/junit/jupiter/engine/extension/AutoCloseTests.java +++ b/junit-jupiter-engine/src/test/java/org/junit/jupiter/engine/extension/AutoCloseTests.java @@ -11,11 +11,11 @@ package org.junit.jupiter.engine.extension; import static java.util.Arrays.asList; +import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.junit.jupiter.api.extension.ExtensionContext.Namespace; import static org.junit.platform.testkit.engine.EventConditions.finishedWithFailure; import static org.junit.platform.testkit.engine.TestExecutionResultConditions.cause; import static org.junit.platform.testkit.engine.TestExecutionResultConditions.message; @@ -32,7 +32,8 @@ import org.junit.platform.testkit.engine.Events; /** - * Integration tests for the behavior of the {@link AutoCloseExtension} to release resources after test execution. + * Integration tests for the behavior of the {@link AutoCloseExtension} to + * release resources after test execution. * * @since 5.11 */ @@ -52,16 +53,18 @@ void fieldsAreProperlyClosed() { Events tests = engineExecutionResults.testEvents(); tests.assertStatistics(stats -> stats.succeeded(2)); // @formatter:off - assertEquals(asList( + assertThat(recorder).containsExactly( "afterEach-close()", "afterEach-run()", "afterEach-close()", "afterEach-run()", - "afterAll-close()"), recorder); - // @formatter:onf + "afterAll-close()"); + // @formatter:on } @Test void noCloseMethod() { - String msg = "@AutoClose: Cannot resolve the destroy method close() at AutoCloseNoCloseMethodFailingTestCase.resource: String"; + String msg = "@AutoClose failed to close object for field " + + "org.junit.jupiter.engine.extension.AutoCloseTests$AutoCloseNoCloseMethodFailingTestCase.resource " + + "because the close() method could not be resolved."; Events tests = executeTestsForClass(AutoCloseNoCloseMethodFailingTestCase.class).testEvents(); assertFailingWithMessage(tests, msg); @@ -69,17 +72,14 @@ void noCloseMethod() { @Test void noShutdownMethod() { - String msg = "@AutoClose: Cannot resolve the destroy method shutdown() at AutoCloseNoShutdownMethodFailingTestCase.resource: String"; + String msg = "@AutoClose failed to close object for field " + + "org.junit.jupiter.engine.extension.AutoCloseTests$AutoCloseNoShutdownMethodFailingTestCase.resource " + + "because the shutdown() method could not be resolved."; Events tests = executeTestsForClass(AutoCloseNoShutdownMethodFailingTestCase.class).testEvents(); assertFailingWithMessage(tests, msg); } - @Test - void namespace() { - assertEquals(Namespace.create(AutoClose.class), AutoCloseExtension.NAMESPACE); - } - @Test void spyPermitsOnlyASingleAction() { AutoCloseSpy spy = new AutoCloseSpy(""); @@ -98,12 +98,22 @@ private static void assertFailingWithMessage(Events testEvent, String msg) { static class AutoCloseTestCase { - private static @AutoClose AutoCloseable staticClosable; - private static @AutoClose AutoCloseable nullStatic; + @AutoClose + private static AutoCloseable staticClosable; + @AutoClose + private static AutoCloseable nullStatic; - private final @AutoClose AutoCloseable closable = new AutoCloseSpy("afterEach-"); - private final @AutoClose("run") Runnable runnable = new AutoCloseSpy("afterEach-"); - private @AutoClose AutoCloseable nullField; + @AutoClose + private final AutoCloseable closable = new AutoCloseSpy("afterEach-"); + @AutoClose("run") + private final Runnable runnable = new AutoCloseSpy("afterEach-"); + @AutoClose + private AutoCloseable nullField; + + @BeforeAll + static void setup() { + staticClosable = new AutoCloseSpy("afterAll-"); + } @Test void justPass() { @@ -124,11 +134,6 @@ private void assertFields() { assertNull(nullField); } - @BeforeAll - static void setup() { - staticClosable = new AutoCloseSpy("afterAll-"); - } - } static class AutoCloseNoCloseMethodFailingTestCase { @@ -177,8 +182,9 @@ public void close() { } private void checkIfAlreadyInvoked() { - if (!invokedMethod.isEmpty()) + if (!invokedMethod.isEmpty()) { throw new IllegalStateException(); + } } private void recordInvocation(String methodName) {