From e4961fdccc18f7df6257d32820230d0bfc9f467f Mon Sep 17 00:00:00 2001 From: "M.P. Korstanje" Date: Wed, 25 Mar 2026 14:04:39 +0100 Subject: [PATCH 1/2] Configure defacto constant naming conventions in checkstyle#5554 * public, protected and package-private constants must be upper case * private constants may have any case * except for logger which must be lowercase. --- .../junit/api/tools/ApiReportGenerator.java | 4 +- gradle/config/checkstyle/checkstyleMain.xml | 11 ++++ .../descriptor/AbstractExtensionContext.java | 4 +- .../engine/extension/TempDirectory.java | 24 ++++---- ...adPoolHierarchicalTestExecutorService.java | 56 +++++++++---------- .../launcher/core/EngineFilterer.java | 4 +- 6 files changed, 57 insertions(+), 46 deletions(-) diff --git a/documentation/src/tools/java/org/junit/api/tools/ApiReportGenerator.java b/documentation/src/tools/java/org/junit/api/tools/ApiReportGenerator.java index ce3d3581291f..c6e7ff9bc4be 100644 --- a/documentation/src/tools/java/org/junit/api/tools/ApiReportGenerator.java +++ b/documentation/src/tools/java/org/junit/api/tools/ApiReportGenerator.java @@ -48,7 +48,7 @@ */ class ApiReportGenerator { - private static final Logger LOGGER = LoggerFactory.getLogger(ApiReportGenerator.class); + private static final Logger logger = LoggerFactory.getLogger(ApiReportGenerator.class); private static final String EOL = System.lineSeparator(); public static void main(String... args) { @@ -160,7 +160,7 @@ private static SortedSet collectTypes(ScanResult scanResult) { .filter(it -> !it.getAnnotationInfo(API.class).isInherited()) // .collect(toCollection(TreeSet::new)); - LOGGER.debug(() -> { + logger.debug(() -> { var builder = new StringBuilder("Listing of all " + types.size() + " annotated types:"); builder.append(EOL); types.forEach(e -> builder.append(e.getName()).append(EOL)); diff --git a/gradle/config/checkstyle/checkstyleMain.xml b/gradle/config/checkstyle/checkstyleMain.xml index c4be037754b7..57264283b24d 100644 --- a/gradle/config/checkstyle/checkstyleMain.xml +++ b/gradle/config/checkstyle/checkstyleMain.xml @@ -48,6 +48,17 @@ + + + + + + + + + + + diff --git a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/AbstractExtensionContext.java b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/AbstractExtensionContext.java index ebaba3ddf69e..57dd56f1c58a 100644 --- a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/AbstractExtensionContext.java +++ b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/AbstractExtensionContext.java @@ -54,7 +54,7 @@ */ abstract class AbstractExtensionContext implements ExtensionContextInternal, AutoCloseable { - private static final Logger LOGGER = LoggerFactory.getLogger(AbstractExtensionContext.class); + private static final Logger logger = LoggerFactory.getLogger(AbstractExtensionContext.class); private static final Namespace CLOSEABLE_RESOURCE_LOGGING_NAMESPACE = Namespace.create( AbstractExtensionContext.class, "CloseableResourceLogging"); @@ -113,7 +113,7 @@ private NamespacedHierarchicalStore.CloseAction createCloseAction() { if (value instanceof Store.CloseableResource resource) { if (isAutoCloseEnabled) { store.computeIfAbsent(value.getClass(), type -> { - LOGGER.warn(() -> "Type implements CloseableResource but not AutoCloseable: " + type.getName()); + logger.warn(() -> "Type implements CloseableResource but not AutoCloseable: " + type.getName()); return true; }); } diff --git a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/extension/TempDirectory.java b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/extension/TempDirectory.java index 94f9565b5a4c..4a0d0bb27e08 100644 --- a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/extension/TempDirectory.java +++ b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/extension/TempDirectory.java @@ -273,7 +273,7 @@ private static ExtensionContext.Store getContextSpecificStore(ExtensionContext c @SuppressWarnings("deprecation") static class CloseablePath implements Store.CloseableResource, AutoCloseable { - private static final Logger LOGGER = LoggerFactory.getLogger(CloseablePath.class); + private static final Logger logger = LoggerFactory.getLogger(CloseablePath.class); private final Path dir; private final TempDirFactory factory; @@ -311,7 +311,7 @@ public void close() throws IOException { try { if (this.cleanupMode == NEVER || (this.cleanupMode == ON_SUCCESS && selfOrChildFailed(this.extensionContext))) { - LOGGER.info(() -> "Skipping cleanup of temp dir %s for %s due to CleanupMode.%s.".formatted( + logger.info(() -> "Skipping cleanup of temp dir %s for %s due to CleanupMode.%s.".formatted( this.dir, descriptionFor(this.annotatedElement), this.cleanupMode.name())); return; } @@ -319,18 +319,18 @@ public void close() throws IOException { FileOperations fileOperations = this.extensionContext.getStore(NAMESPACE) // .getOrDefault(FILE_OPERATIONS_KEY, FileOperations.class, FileOperations.DEFAULT); FileOperations loggingFileOperations = file -> { - LOGGER.trace(() -> "Attempting to delete " + file); + logger.trace(() -> "Attempting to delete " + file); try { fileOperations.delete(file); - LOGGER.trace(() -> "Successfully deleted " + file); + logger.trace(() -> "Successfully deleted " + file); } catch (IOException e) { - LOGGER.trace(e, () -> "Failed to delete " + file); + logger.trace(e, () -> "Failed to delete " + file); throw e; } }; - LOGGER.trace(() -> "Cleaning up temp dir " + this.dir); + logger.trace(() -> "Cleaning up temp dir " + this.dir); SortedMap failures = deleteAllFilesAndDirectories(loggingFileOperations); if (!failures.isEmpty()) { throw createIOExceptionWithAttachedFailures(failures); @@ -383,7 +383,7 @@ private SortedMap deleteAllFilesAndDirectories(FileOperations @Override public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) throws IOException { - LOGGER.trace(() -> "preVisitDirectory: " + dir); + logger.trace(() -> "preVisitDirectory: " + dir); if (isLinkWithTargetOutsideTempDir(dir)) { warnAboutLinkWithTargetOutsideTempDir("link", dir); delete(dir); @@ -397,7 +397,7 @@ public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) th @Override public FileVisitResult visitFileFailed(Path file, IOException exc) { - LOGGER.trace(exc, () -> "visitFileFailed: " + file); + logger.trace(exc, () -> "visitFileFailed: " + file); if (exc instanceof NoSuchFileException && !Files.exists(file, LinkOption.NOFOLLOW_LINKS)) { return CONTINUE; } @@ -408,7 +408,7 @@ public FileVisitResult visitFileFailed(Path file, IOException exc) { @Override public FileVisitResult visitFile(Path file, BasicFileAttributes attributes) throws IOException { - LOGGER.trace(() -> "visitFile: " + file); + logger.trace(() -> "visitFile: " + file); if (Files.isSymbolicLink(file) && isLinkWithTargetOutsideTempDir(file)) { warnAboutLinkWithTargetOutsideTempDir("symbolic link", file); } @@ -418,7 +418,7 @@ public FileVisitResult visitFile(Path file, BasicFileAttributes attributes) thro @Override public FileVisitResult postVisitDirectory(Path dir, IOException exc) { - LOGGER.trace(exc, () -> "postVisitDirectory: " + dir); + logger.trace(exc, () -> "postVisitDirectory: " + dir); delete(dir); return CONTINUE; } @@ -430,7 +430,7 @@ private boolean isLinkWithTargetOutsideTempDir(Path path) { return !path.toRealPath().startsWith(rootRealPath); } catch (IOException e) { - LOGGER.trace(e, + logger.trace(e, () -> "Failed to determine real path for " + path + "; assuming it is not a link"); return false; } @@ -438,7 +438,7 @@ private boolean isLinkWithTargetOutsideTempDir(Path path) { private void warnAboutLinkWithTargetOutsideTempDir(String linkType, Path file) throws IOException { Path realPath = file.toRealPath(); - LOGGER.warn(() -> """ + logger.warn(() -> """ Deleting %s from location inside of temp dir (%s) \ to location outside of temp dir (%s) but not the target file/directory""".formatted( linkType, file, realPath)); diff --git a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/WorkerThreadPoolHierarchicalTestExecutorService.java b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/WorkerThreadPoolHierarchicalTestExecutorService.java index e0417ef65ab2..6e8b9770be95 100644 --- a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/WorkerThreadPoolHierarchicalTestExecutorService.java +++ b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/WorkerThreadPoolHierarchicalTestExecutorService.java @@ -95,7 +95,7 @@ public final class WorkerThreadPoolHierarchicalTestExecutorService implements Hi directly. */ - private static final Logger LOGGER = LoggerFactory.getLogger(WorkerThreadPoolHierarchicalTestExecutorService.class); + private static final Logger logger = LoggerFactory.getLogger(WorkerThreadPoolHierarchicalTestExecutorService.class); private final WorkQueue workQueue = new WorkQueue(); private final ExecutorService executor; @@ -137,18 +137,18 @@ public final class WorkerThreadPoolHierarchicalTestExecutorService implements Hi executor = new ThreadPoolExecutor(configuration.getCorePoolSize(), configuration.getMaxPoolSize(), configuration.getKeepAliveSeconds(), SECONDS, new SynchronousQueue<>(), threadFactory, rejectedExecutionHandler); - LOGGER.trace(() -> "initialized thread pool for parallelism of " + configuration.getParallelism()); + logger.trace(() -> "initialized thread pool for parallelism of " + configuration.getParallelism()); } @Override public void close() { - LOGGER.trace(() -> "shutting down thread pool"); + logger.trace(() -> "shutting down thread pool"); executor.shutdownNow(); } @Override public Future<@Nullable Void> submit(TestTask testTask) { - LOGGER.trace(() -> "submit: " + testTask); + logger.trace(() -> "submit: " + testTask); var workerThread = WorkerThread.get(); if (workerThread == null) { @@ -173,7 +173,7 @@ public void close() { */ @Override public void invokeAll(List testTasks) { - LOGGER.trace(() -> "invokeAll: " + testTasks); + logger.trace(() -> "invokeAll: " + testTasks); var workerThread = WorkerThread.get(); Preconditions.condition(workerThread != null && workerThread.executor() == this, @@ -219,13 +219,13 @@ private record RunLeaseAwareWorker(WorkerLease workerLease, BooleanSupplier pare @Override public void run() { - LOGGER.trace(() -> "starting worker"); + logger.trace(() -> "starting worker"); try { WorkerThread.getOrThrow().processQueueEntries(workerLease, parentDoneCondition); } finally { workerLease.release(parentDoneCondition); - LOGGER.trace(() -> "stopping worker"); + logger.trace(() -> "stopping worker"); } } } @@ -286,11 +286,11 @@ void processQueueEntries(WorkerLease workerLease, BooleanSupplier doneCondition) this.workerLease = workerLease; while (!executor.isShutdown()) { if (doneCondition.getAsBoolean()) { - LOGGER.trace(() -> "yielding resource lock"); + logger.trace(() -> "yielding resource lock"); break; } if (workQueue.isEmpty()) { - LOGGER.trace(() -> "no queue entries available"); + logger.trace(() -> "no queue entries available"); break; } processQueueEntries(); @@ -415,7 +415,7 @@ private WorkStealResult tryToStealWork(WorkQueue.Entry entry, BlockingMode block } var claimed = workQueue.remove(entry); if (claimed) { - LOGGER.trace(() -> "stole work: " + entry.task); + logger.trace(() -> "stole work: " + entry.task); var executed = executeStolenWork(entry, blockingMode); if (executed) { return WorkStealResult.EXECUTED_BY_THIS_WORKER; @@ -439,7 +439,7 @@ private void waitFor(Map> queueEntriesByR } else { runBlocking(future::isDone, () -> { - LOGGER.trace(() -> "blocking for forked children : %s".formatted(children)); + logger.trace(() -> "blocking for forked children : %s".formatted(children)); return future.join(); }); } @@ -457,7 +457,7 @@ private void executeAll(List children) { if (children.isEmpty()) { return; } - LOGGER.trace(() -> "running %d children directly".formatted(children.size())); + logger.trace(() -> "running %d children directly".formatted(children.size())); if (children.size() == 1) { executeTask(children.get(0)); return; @@ -509,17 +509,17 @@ private void executeTask(TestTask testTask) { if (!executed) { var resourceLock = testTask.getResourceLock(); try (var ignored = runBlocking(() -> false, () -> { - LOGGER.trace(() -> "blocking for resource lock: " + resourceLock); + logger.trace(() -> "blocking for resource lock: " + resourceLock); return resourceLock.acquire(); })) { - LOGGER.trace(() -> "acquired resource lock: " + resourceLock); + logger.trace(() -> "acquired resource lock: " + resourceLock); doExecute(testTask); } catch (InterruptedException ex) { Thread.currentThread().interrupt(); } finally { - LOGGER.trace(() -> "released resource lock: " + resourceLock); + logger.trace(() -> "released resource lock: " + resourceLock); } } } @@ -527,30 +527,30 @@ private void executeTask(TestTask testTask) { private boolean tryExecuteTask(TestTask testTask) { var resourceLock = testTask.getResourceLock(); if (resourceLock.tryAcquire()) { - LOGGER.trace(() -> "acquired resource lock: " + resourceLock); + logger.trace(() -> "acquired resource lock: " + resourceLock); try (resourceLock) { doExecute(testTask); return true; } finally { - LOGGER.trace(() -> "released resource lock: " + resourceLock); + logger.trace(() -> "released resource lock: " + resourceLock); } } else { - LOGGER.trace(() -> "failed to acquire resource lock: " + resourceLock); + logger.trace(() -> "failed to acquire resource lock: " + resourceLock); } return false; } private void doExecute(TestTask testTask) { - LOGGER.trace(() -> "executing: " + testTask); + logger.trace(() -> "executing: " + testTask); stateStack.push(new State()); try { testTask.execute(); } finally { stateStack.pop(); - LOGGER.trace(() -> "finished executing: " + testTask); + logger.trace(() -> "finished executing: " + testTask); } } @@ -650,7 +650,7 @@ private static class WorkStealingFuture extends BlockingAwareFuture<@Nullable Vo if (entry.future.isDone()) { return callable.call(); } - LOGGER.trace(() -> "blocking for child task: " + entry.task); + logger.trace(() -> "blocking for child task: " + entry.task); return workerThread.runBlocking(entry.future::isDone, () -> { try { return callable.call(); @@ -673,7 +673,7 @@ private static class WorkQueue implements Iterable { Entry add(TestTask task, int index) { Entry entry = new Entry(task, index); - LOGGER.trace(() -> "forking: " + entry.task); + logger.trace(() -> "forking: " + entry.task); return doAdd(entry); } @@ -682,7 +682,7 @@ void addAll(Collection entries) { } void reAdd(Entry entry) { - LOGGER.trace(() -> "re-enqueuing: " + entry.task); + logger.trace(() -> "re-enqueuing: " + entry.task); doAdd(entry); } @@ -726,10 +726,10 @@ private static final class Entry { this.future = new CompletableFuture<>(); this.future.whenComplete((__, t) -> { if (t == null) { - LOGGER.trace(() -> "completed normally: " + task); + logger.trace(() -> "completed normally: " + task); } else { - LOGGER.trace(t, () -> "completed exceptionally: " + task); + logger.trace(t, () -> "completed exceptionally: " + task); } }); this.task = task; @@ -828,7 +828,7 @@ static class WorkerLeaseManager { WorkerLease tryAcquire() { boolean acquired = semaphore.tryAcquire(); if (acquired) { - LOGGER.trace(() -> "acquired worker lease for new worker (available: %d)".formatted( + logger.trace(() -> "acquired worker lease for new worker (available: %d)".formatted( semaphore.availablePermits())); return new WorkerLease(this::release); } @@ -837,7 +837,7 @@ WorkerLease tryAcquire() { private ReacquisitionToken release(BooleanSupplier doneCondition) { semaphore.release(); - LOGGER.trace(() -> "release worker lease (available: %d)".formatted(semaphore.availablePermits())); + logger.trace(() -> "release worker lease (available: %d)".formatted(semaphore.availablePermits())); compensation.accept(doneCondition); return new ReacquisitionToken(); } @@ -850,7 +850,7 @@ void reacquire() throws InterruptedException { Preconditions.condition(!used, "Lease was already reacquired"); used = true; semaphore.acquire(); - LOGGER.trace(() -> "reacquired worker lease (available: %d)".formatted(semaphore.availablePermits())); + logger.trace(() -> "reacquired worker lease (available: %d)".formatted(semaphore.availablePermits())); } } diff --git a/junit-platform-launcher/src/main/java/org/junit/platform/launcher/core/EngineFilterer.java b/junit-platform-launcher/src/main/java/org/junit/platform/launcher/core/EngineFilterer.java index dc8237f091ee..c93608431dbe 100644 --- a/junit-platform-launcher/src/main/java/org/junit/platform/launcher/core/EngineFilterer.java +++ b/junit-platform-launcher/src/main/java/org/junit/platform/launcher/core/EngineFilterer.java @@ -32,7 +32,7 @@ class EngineFilterer { - private static final Logger LOGGER = LoggerFactory.getLogger(EngineFilterer.class); + private static final Logger logger = LoggerFactory.getLogger(EngineFilterer.class); private final List engineFilters; @@ -57,7 +57,7 @@ void performSanityChecks() { private void warnIfAllEnginesExcluded() { if (!checkedTestEngines.isEmpty() && checkedTestEngines.values().stream().allMatch(excluded -> excluded)) { - LOGGER.warn(() -> "All TestEngines were excluded by EngineFilters.\n" + getStateDescription()); + logger.warn(() -> "All TestEngines were excluded by EngineFilters.\n" + getStateDescription()); } } From 956b1296928c19159d622f9c558c4303e7101b3a Mon Sep 17 00:00:00 2001 From: "M.P. Korstanje" Date: Wed, 25 Mar 2026 16:13:03 +0100 Subject: [PATCH 2/2] Adopt naming conventions for constant fields --- CONTRIBUTING.md | 13 +++++++++++++ gradle/config/checkstyle/checkstyleMain.xml | 7 +++++-- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 365ab8fb1e0d..2862d52f0931 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -54,6 +54,8 @@ I hereby agree to the terms of the JUnit Contributor License Agreement. ### Naming Conventions +Acronym are words. + Whenever an acronym is included as part of a type name or method name, keep the first letter of the acronym uppercase and use lowercase for the rest of the acronym. Otherwise, it becomes _impossible_ to perform camel-cased searches in IDEs, and it becomes @@ -119,6 +121,17 @@ code -- class names, method names, variable names, etc. See [`ExtensionContext`](junit-jupiter-api/src/main/java/org/junit/jupiter/api/extension/ExtensionContext.java) and [`ParameterContext`](junit-jupiter-api/src/main/java/org/junit/jupiter/api/extension/ParameterContext.java) for example Javadoc. +### Constant fields + +A constant field is a `static final` field whose value is immutable. If a static final +field has a primitive type or an immutable reference type it is a constant field. + +- To minimize accessibility and mutability for all non-private `static final` fields + under `src/main` should be constant fields. +- Constant fields should be named using uppercase words separated by underscores. For + example `DEFAULT_HTTP_URL_PROVIDER`. + +Note: `org.junit.platform.commons.logging.Logger` is considered mutable. ### Nullability diff --git a/gradle/config/checkstyle/checkstyleMain.xml b/gradle/config/checkstyle/checkstyleMain.xml index 57264283b24d..2d5bf189b1c3 100644 --- a/gradle/config/checkstyle/checkstyleMain.xml +++ b/gradle/config/checkstyle/checkstyleMain.xml @@ -48,12 +48,15 @@ + - - + +