From 102a1a31aa7e1a64caf9612688b7cd38a140069a Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Mon, 16 Sep 2024 11:51:14 +0200 Subject: [PATCH 1/5] Use a random `${test:logging.path}` The `@TempLoggingDir` JUnit 5 extensions create a totally deterministic directory to hold the log files for tests. Since the directory is deleted only if all the tests succeed, this causes a problem with test reruns. --- .../test/junit/TempLoggingDirectory.java | 44 ++++++++++++++----- .../test/junit/TempLoggingDirectoryTest.java | 6 ++- 2 files changed, 36 insertions(+), 14 deletions(-) diff --git a/log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/TempLoggingDirectory.java b/log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/TempLoggingDirectory.java index d222f6dc173..ef97ff2378e 100644 --- a/log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/TempLoggingDirectory.java +++ b/log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/TempLoggingDirectory.java @@ -51,7 +51,8 @@ public void beforeAll(ExtensionContext context) throws Exception { Path loggingPath = null; for (final Field field : fields) { if (loggingPath != null) { - StatusLogger.getLogger().warn("Multiple fields with @TempLoggingDir annotation are not supported."); + StatusLogger.getLogger() + .warn("Multiple static fields with @TempLoggingDir annotation are not supported."); } else { final CleanupMode cleanup = determineCleanupMode(field); loggingPath = createLoggingPath(context, cleanup).getPath(); @@ -63,8 +64,8 @@ public void beforeAll(ExtensionContext context) throws Exception { @Override public void beforeEach(ExtensionContext context) throws Exception { - // JUnit 5 does not set an error on the parent context if one of the children - // fail. We record the list of children. + // JUnit 5 does not set an error on the parent context if one of the children fails. + // We record the list of children. context.getParent().ifPresent(c -> { final PathHolder holder = ExtensionContextAnchor.getAttribute(PathHolder.class, PathHolder.class, c); if (holder != null) { @@ -78,7 +79,8 @@ public void beforeEach(ExtensionContext context) throws Exception { final Object instance = context.getRequiredTestInstance(); for (final Field field : fields) { if (loggingPath != null) { - StatusLogger.getLogger().warn("Multiple fields with @TempLoggingDir annotation are not supported."); + StatusLogger.getLogger() + .warn("Multiple instance fields with @TempLoggingDir annotation are not supported."); } else { final CleanupMode cleanup = determineCleanupMode(field); loggingPath = createLoggingPath(context, cleanup).getPath(); @@ -102,7 +104,7 @@ public Object resolveParameter(ParameterContext parameterContext, ExtensionConte throws ParameterResolutionException { final TempLoggingDir annotation = parameterContext.findAnnotation(TempLoggingDir.class).get(); - // Get or create temporary directory + // Get or create a temporary directory PathHolder holder = ExtensionContextAnchor.getAttribute(PathHolder.class, PathHolder.class, extensionContext); if (holder == null || !extensionContext.equals(holder.getMainContext())) { final CleanupMode mode = determineCleanupMode(annotation); @@ -111,14 +113,9 @@ public Object resolveParameter(ParameterContext parameterContext, ExtensionConte return holder.getPath(); } - private PathHolder createLoggingPath(final ExtensionContext context, final CleanupMode cleanup) { + private PathHolder createLoggingPath(ExtensionContext context, CleanupMode cleanup) { final TestProperties props = TestPropertySource.createProperties(context); - // Create temporary directory - final String baseDir = System.getProperty("basedir"); - final Path basePath = (baseDir != null ? Paths.get(baseDir, "target") : Paths.get(".")).resolve("logs"); - final Class clazz = context.getRequiredTestClass(); - final String dir = clazz.getName().replaceAll("[.$]", File.separatorChar == '\\' ? "\\\\" : File.separator); - final Path perClassPath = basePath.resolve(dir); + final Path perClassPath = determinePerClassPath(context); // Per test subfolder final Path loggingPath = context.getTestMethod() .map(m -> perClassPath.resolve(m.getName())) @@ -135,6 +132,29 @@ private PathHolder createLoggingPath(final ExtensionContext context, final Clean return holder; } + private Path determinePerClassPath(ExtensionContext context) { + // Check if the parent context already created a folder + PathHolder holder = ExtensionContextAnchor.getAttribute(PathHolder.class, PathHolder.class, context); + if (holder == null) { + try { + // Create temporary per-class directory + final String baseDir = System.getProperty("basedir"); + final Path basePath = (baseDir != null ? Paths.get(baseDir, "target") : Paths.get(".")).resolve("logs"); + final Class clazz = context.getRequiredTestClass(); + final Package pkg = clazz.getPackage(); + final String dir = + pkg.getName().replaceAll("[.$]", File.separatorChar == '\\' ? "\\\\" : File.separator); + // Create a temporary directory that uses the simple class name as prefix + Path packagePath = basePath.resolve(dir); + Files.createDirectories(packagePath); + return Files.createTempDirectory(packagePath, clazz.getSimpleName()); + } catch (final IOException e) { + throw new ExtensionContextException("Failed to create temporary directory.", e); + } + } + return holder.getPath(); + } + private CleanupMode determineCleanupMode(final TempLoggingDir annotation) { final CleanupMode mode = annotation.cleanup(); // TODO: use JupiterConfiguration diff --git a/log4j-api-test/src/test/java/org/apache/logging/log4j/test/junit/TempLoggingDirectoryTest.java b/log4j-api-test/src/test/java/org/apache/logging/log4j/test/junit/TempLoggingDirectoryTest.java index 846a044e798..0a1dd2f15f3 100644 --- a/log4j-api-test/src/test/java/org/apache/logging/log4j/test/junit/TempLoggingDirectoryTest.java +++ b/log4j-api-test/src/test/java/org/apache/logging/log4j/test/junit/TempLoggingDirectoryTest.java @@ -20,13 +20,14 @@ import java.nio.file.Path; import java.nio.file.Paths; +import java.util.regex.Pattern; import org.apache.logging.log4j.test.TestProperties; import org.junit.jupiter.api.Test; @UsingTestProperties public class TempLoggingDirectoryTest { - private static final Path PER_CLASS_PATH = Paths.get("TempLoggingDirectoryTest"); + private static final Pattern PER_CLASS_PATH = Pattern.compile("TempLoggingDirectoryTest\\d+"); private static final Path PER_TEST_PATH = Paths.get("testInjectedFields"); @TempLoggingDir @@ -37,7 +38,8 @@ public class TempLoggingDirectoryTest { @Test void testInjectedFields(final @TempLoggingDir Path parameterLoggingPath, final TestProperties props) { - assertThat(staticLoggingPath).exists().endsWith(PER_CLASS_PATH); + assertThat(staticLoggingPath).exists(); + assertThat(staticLoggingPath.getFileName().toString()).matches(PER_CLASS_PATH); assertThat(instanceLoggingPath).exists().startsWith(staticLoggingPath).endsWith(PER_TEST_PATH); assertThat(parameterLoggingPath).isEqualTo(instanceLoggingPath); assertThat(props.getProperty(TestProperties.LOGGING_PATH)).isEqualTo(instanceLoggingPath.toString()); From e3da579dcfa9adbfe7b2d4aacdf652ca30697016 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Mon, 7 Oct 2024 15:46:58 +0200 Subject: [PATCH 2/5] Bump `junit-jupiter` to version `5.11.1` --- log4j-parent/pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/log4j-parent/pom.xml b/log4j-parent/pom.xml index 969c18523c5..1686a06a9b5 100644 --- a/log4j-parent/pom.xml +++ b/log4j-parent/pom.xml @@ -112,7 +112,7 @@ 1.37 2.40.1 4.13.2 - 5.10.3 + 5.11.1 1.9.1 3.8.0 0.2.0 From 463c5e8f0231f136685d5fb56c3cb418820b3157 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Mon, 7 Oct 2024 20:48:09 +0200 Subject: [PATCH 3/5] Address review suggestions --- .../test/junit/TempLoggingDirectory.java | 27 ++++++++++++------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/TempLoggingDirectory.java b/log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/TempLoggingDirectory.java index ef97ff2378e..003649a8c8f 100644 --- a/log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/TempLoggingDirectory.java +++ b/log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/TempLoggingDirectory.java @@ -28,6 +28,7 @@ import java.util.List; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicInteger; import org.apache.logging.log4j.status.StatusLogger; import org.apache.logging.log4j.test.TestProperties; import org.junit.jupiter.api.extension.BeforeAllCallback; @@ -39,11 +40,14 @@ import org.junit.jupiter.api.extension.ParameterResolutionException; import org.junit.jupiter.api.extension.ParameterResolver; import org.junit.jupiter.api.io.CleanupMode; +import org.junit.platform.commons.PreconditionViolationException; import org.junit.platform.commons.support.AnnotationSupport; import org.junit.platform.commons.support.ModifierSupport; public class TempLoggingDirectory implements BeforeAllCallback, BeforeEachCallback, ParameterResolver { + private final AtomicInteger counter = new AtomicInteger(); + @Override public void beforeAll(ExtensionContext context) throws Exception { final List fields = AnnotationSupport.findAnnotatedFields( @@ -51,8 +55,8 @@ public void beforeAll(ExtensionContext context) throws Exception { Path loggingPath = null; for (final Field field : fields) { if (loggingPath != null) { - StatusLogger.getLogger() - .warn("Multiple static fields with @TempLoggingDir annotation are not supported."); + throw new PreconditionViolationException( + "Multiple static fields with @TempLoggingDir annotation are not supported."); } else { final CleanupMode cleanup = determineCleanupMode(field); loggingPath = createLoggingPath(context, cleanup).getPath(); @@ -79,8 +83,8 @@ public void beforeEach(ExtensionContext context) throws Exception { final Object instance = context.getRequiredTestInstance(); for (final Field field : fields) { if (loggingPath != null) { - StatusLogger.getLogger() - .warn("Multiple instance fields with @TempLoggingDir annotation are not supported."); + throw new PreconditionViolationException( + "Multiple instance fields with @TempLoggingDir annotation are not supported."); } else { final CleanupMode cleanup = determineCleanupMode(field); loggingPath = createLoggingPath(context, cleanup).getPath(); @@ -102,8 +106,11 @@ public boolean supportsParameter(ParameterContext parameterContext, ExtensionCon @Override public Object resolveParameter(ParameterContext parameterContext, ExtensionContext extensionContext) throws ParameterResolutionException { - final TempLoggingDir annotation = - parameterContext.findAnnotation(TempLoggingDir.class).get(); + final TempLoggingDir annotation = parameterContext + .findAnnotation(TempLoggingDir.class) + .orElseThrow(() -> new PreconditionViolationException(String.format( + "Missing `%s` annotation on parameter `%s`", + TempLoggingDir.class.getSimpleName(), parameterContext))); // Get or create a temporary directory PathHolder holder = ExtensionContextAnchor.getAttribute(PathHolder.class, PathHolder.class, extensionContext); if (holder == null || !extensionContext.equals(holder.getMainContext())) { @@ -142,12 +149,14 @@ private Path determinePerClassPath(ExtensionContext context) { final Path basePath = (baseDir != null ? Paths.get(baseDir, "target") : Paths.get(".")).resolve("logs"); final Class clazz = context.getRequiredTestClass(); final Package pkg = clazz.getPackage(); - final String dir = - pkg.getName().replaceAll("[.$]", File.separatorChar == '\\' ? "\\\\" : File.separator); + final String dir = pkg.getName() + .replaceAll("org\\.apache\\.(logging\\.)?log4j\\.", "") + .replaceAll("[.$]", File.separatorChar == '\\' ? "\\\\" : File.separator); // Create a temporary directory that uses the simple class name as prefix Path packagePath = basePath.resolve(dir); Files.createDirectories(packagePath); - return Files.createTempDirectory(packagePath, clazz.getSimpleName()); + return Files.createTempDirectory( + packagePath, String.format("%s_%02d_", clazz.getSimpleName(), counter.incrementAndGet())); } catch (final IOException e) { throw new ExtensionContextException("Failed to create temporary directory.", e); } From 4c3c4c79bdadc70b1d8feeeae9a389372871cf3f Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Mon, 7 Oct 2024 21:08:24 +0200 Subject: [PATCH 4/5] Upgrades JUnit to version 5.11.x Due to a minor bug in JUnit 5.11.x (junit-team/junit5#4054), fields meta-annotated with multiple `@ExtendWith` annotations are no longer recognized. We combine those annotations into single `@ExtendWith` annotations, which also looks better. Closes #2843. --- .../logging/log4j/test/junit/SetTestProperty.java | 5 ++--- .../logging/log4j/test/junit/TempLoggingDir.java | 3 +-- .../logging/log4j/test/junit/UsingStatusListener.java | 4 +--- .../logging/log4j/test/junit/UsingTestProperties.java | 3 +-- .../apache/logging/log4j/test/junit/package-info.java | 4 +--- .../log4j/core/test/junit/LoggerContextSource.java | 10 ++++++---- .../logging/log4j/core/test/junit/package-info.java | 2 +- log4j-parent/pom.xml | 2 +- 8 files changed, 14 insertions(+), 19 deletions(-) diff --git a/log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/SetTestProperty.java b/log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/SetTestProperty.java index ff9a175d7a4..9989235165c 100644 --- a/log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/SetTestProperty.java +++ b/log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/SetTestProperty.java @@ -39,8 +39,7 @@ @Target({TYPE, METHOD}) @Inherited @Documented -@ExtendWith(ExtensionContextAnchor.class) -@ExtendWith(TestPropertyResolver.class) +@ExtendWith({ExtensionContextAnchor.class, TestPropertyResolver.class}) @Repeatable(SetTestProperty.SetTestProperties.class) @ReadsSystemProperty @ReadsEnvironmentVariable @@ -54,7 +53,7 @@ @Target({TYPE, METHOD}) @Documented @Inherited - public @interface SetTestProperties { + @interface SetTestProperties { SetTestProperty[] value(); } diff --git a/log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/TempLoggingDir.java b/log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/TempLoggingDir.java index 8a6af4c8209..dab49ddd4d2 100644 --- a/log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/TempLoggingDir.java +++ b/log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/TempLoggingDir.java @@ -37,8 +37,7 @@ @Target({FIELD, PARAMETER}) @Inherited @Documented -@ExtendWith(ExtensionContextAnchor.class) -@ExtendWith(TempLoggingDirectory.class) +@ExtendWith({ExtensionContextAnchor.class, TempLoggingDirectory.class}) public @interface TempLoggingDir { CleanupMode cleanup() default CleanupMode.DEFAULT; diff --git a/log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/UsingStatusListener.java b/log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/UsingStatusListener.java index 2ca8124b03a..7fa198c509b 100644 --- a/log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/UsingStatusListener.java +++ b/log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/UsingStatusListener.java @@ -35,7 +35,5 @@ @Retention(RUNTIME) @Target({TYPE, METHOD}) @Documented -@ExtendWith(ExtensionContextAnchor.class) -@ExtendWith(TestPropertyResolver.class) -@ExtendWith(StatusListenerExtension.class) +@ExtendWith({ExtensionContextAnchor.class, TestPropertyResolver.class, StatusListenerExtension.class}) public @interface UsingStatusListener {} diff --git a/log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/UsingTestProperties.java b/log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/UsingTestProperties.java index 1a37b998321..6d66dd4e032 100644 --- a/log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/UsingTestProperties.java +++ b/log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/UsingTestProperties.java @@ -37,8 +37,7 @@ @Target({TYPE, METHOD}) @Inherited @Documented -@ExtendWith(ExtensionContextAnchor.class) -@ExtendWith(TestPropertyResolver.class) +@ExtendWith({ExtensionContextAnchor.class, TestPropertyResolver.class}) @ReadsSystemProperty @ReadsEnvironmentVariable public @interface UsingTestProperties {} diff --git a/log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/package-info.java b/log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/package-info.java index e14cd8ee188..2a37b2933ff 100644 --- a/log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/package-info.java +++ b/log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/package-info.java @@ -15,10 +15,8 @@ * limitations under the license. */ @Export -@Version("2.24.0") -@BaselineIgnore("2.24.0") +@Version("2.24.1") package org.apache.logging.log4j.test.junit; -import aQute.bnd.annotation.baseline.BaselineIgnore; import org.osgi.annotation.bundle.Export; import org.osgi.annotation.versioning.Version; diff --git a/log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/junit/LoggerContextSource.java b/log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/junit/LoggerContextSource.java index 3367d6f0958..152766c95ce 100644 --- a/log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/junit/LoggerContextSource.java +++ b/log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/junit/LoggerContextSource.java @@ -52,10 +52,12 @@ @Documented @Inherited @Tag("functional") -@ExtendWith(TempLoggingDirectory.class) -@ExtendWith(LoggerContextResolver.class) -@ExtendWith(ConfigurationResolver.class) -@ExtendWith(AppenderResolver.class) +@ExtendWith({ + TempLoggingDirectory.class, + LoggerContextResolver.class, + ConfigurationResolver.class, + AppenderResolver.class +}) public @interface LoggerContextSource { /** * Specifies the name of the configuration file to use for the annotated test. diff --git a/log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/junit/package-info.java b/log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/junit/package-info.java index 0fedb91e3a2..4103b148043 100644 --- a/log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/junit/package-info.java +++ b/log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/junit/package-info.java @@ -20,7 +20,7 @@ * @see org.junit.rules.TestRule */ @Export -@Version("2.23.0") +@Version("2.23.1") package org.apache.logging.log4j.core.test.junit; import org.osgi.annotation.bundle.Export; diff --git a/log4j-parent/pom.xml b/log4j-parent/pom.xml index cb81b04b2c7..4c82d04e146 100644 --- a/log4j-parent/pom.xml +++ b/log4j-parent/pom.xml @@ -109,7 +109,7 @@ 3.5.12 1.37 4.13.2 - 5.10.3 + 5.11.2 1.9.1 3.8.0 0.2.0 From a9edc056217f8269e56b1fd4854a687925956c22 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Mon, 7 Oct 2024 21:20:45 +0200 Subject: [PATCH 5/5] Fix failing test --- .../logging/log4j/test/junit/TempLoggingDirectoryTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/log4j-api-test/src/test/java/org/apache/logging/log4j/test/junit/TempLoggingDirectoryTest.java b/log4j-api-test/src/test/java/org/apache/logging/log4j/test/junit/TempLoggingDirectoryTest.java index 0a1dd2f15f3..2c3a673ecb8 100644 --- a/log4j-api-test/src/test/java/org/apache/logging/log4j/test/junit/TempLoggingDirectoryTest.java +++ b/log4j-api-test/src/test/java/org/apache/logging/log4j/test/junit/TempLoggingDirectoryTest.java @@ -27,7 +27,7 @@ @UsingTestProperties public class TempLoggingDirectoryTest { - private static final Pattern PER_CLASS_PATH = Pattern.compile("TempLoggingDirectoryTest\\d+"); + private static final Pattern PER_CLASS_PATH = Pattern.compile("TempLoggingDirectoryTest_\\d{2}_\\d+"); private static final Path PER_TEST_PATH = Paths.get("testInjectedFields"); @TempLoggingDir