From 42d6a32c0792caf46ec46d4aa0abb96ad4638a37 Mon Sep 17 00:00:00 2001 From: Nicklas Wallgren Date: Tue, 8 Sep 2026 20:28:20 +0200 Subject: [PATCH] chore: close the analyzer log, widen the tests, drop dead build config - Close the file LogAndFileAppender writes to once the decorated mojo is done, rather than leaving one handle open per analyzed module. The listener reads the log back from the mojo, so modules building in parallel cannot close each other's file. - Name the deepest cause and the captured compiler output in the message of a failed compiler based step. A forked mojo buries the reason several wrappers down, which made a class version mismatch surface as a bare stack trace. - Add unit tests for ReactorCompletionTracker, including a concurrent claim, ViolationsFilterService, ExceptionUtils and the permissive and severity filtering of StepResults. 13 tests to 32. - Add a Checker Framework integration test. Its fixture is built outside this project's target directory, the step skips every file whose path contains /target/. - Remove the plexus-component-metadata execution, it generates nothing for a JSR-330 project. - Document the Java 21 requirement. --- README.md | 6 + pom.xml | 13 -- .../MojoLogDecoratorExecutionListener.java | 16 +- .../codequality/log/LogAndFileAppender.java | 17 +- .../step/CheckerFrameworkStep.java | 8 +- .../codequality/step/ErrorProneStep.java | 8 +- .../codequality/util/ExceptionUtils.java | 59 ++++++ .../codequality/CodeQualityReactorIT.java | 95 +++++++++- .../ViolationsFilterServiceUnitTest.java | 109 +++++++++++ .../codequality/step/StepResultsUnitTest.java | 53 +++++- .../ReactorCompletionTrackerUnitTest.java | 179 ++++++++++++++++++ .../util/ExceptionUtilsUnitTest.java | 59 ++++++ .../.mvn/maven.config | 1 + .../module-a/pom.xml | 16 ++ .../src/main/java/it/alpha/Alpha.java | 15 ++ .../module-b/pom.xml | 24 +++ .../module-b/src/main/java/it/beta/Beta.java | 17 ++ .../module-c/pom.xml | 16 ++ .../src/main/java/it/gamma/Gamma.java | 15 ++ .../it/checker-framework-reactor/pom.xml | 83 ++++++++ 20 files changed, 779 insertions(+), 30 deletions(-) create mode 100644 src/main/java/io/github/finoid/maven/plugins/codequality/util/ExceptionUtils.java create mode 100644 src/test/java/io/github/finoid/maven/plugins/codequality/filter/ViolationsFilterServiceUnitTest.java create mode 100644 src/test/java/io/github/finoid/maven/plugins/codequality/storage/ReactorCompletionTrackerUnitTest.java create mode 100644 src/test/java/io/github/finoid/maven/plugins/codequality/util/ExceptionUtilsUnitTest.java create mode 100644 src/test/resources/it/checker-framework-reactor/.mvn/maven.config create mode 100644 src/test/resources/it/checker-framework-reactor/module-a/pom.xml create mode 100644 src/test/resources/it/checker-framework-reactor/module-a/src/main/java/it/alpha/Alpha.java create mode 100644 src/test/resources/it/checker-framework-reactor/module-b/pom.xml create mode 100644 src/test/resources/it/checker-framework-reactor/module-b/src/main/java/it/beta/Beta.java create mode 100644 src/test/resources/it/checker-framework-reactor/module-c/pom.xml create mode 100644 src/test/resources/it/checker-framework-reactor/module-c/src/main/java/it/gamma/Gamma.java create mode 100644 src/test/resources/it/checker-framework-reactor/pom.xml diff --git a/README.md b/README.md index 9ee1ebc..01d8632 100644 --- a/README.md +++ b/README.md @@ -8,6 +8,12 @@ to detect style violations and potential bugs early in the development process. +## Requirements + +* **Java 21 or later.** The plugin itself is compiled for Java 21, and the Error Prone releases it defaults to require a + Java 21 compiler to run. +* Maven 3.9.6 or later. + ## Supported code quality tools * Checkstyle – Analyzes Java code for style guideline violations, helping enforce consistent formatting and naming diff --git a/pom.xml b/pom.xml index 7e2f0bd..10e036d 100644 --- a/pom.xml +++ b/pom.xml @@ -494,19 +494,6 @@ - - org.codehaus.plexus - plexus-component-metadata - 2.2.0 - - - process-annotations - - generate-metadata - - - - org.apache.maven.plugins maven-compiler-plugin diff --git a/src/main/java/io/github/finoid/maven/plugins/codequality/MojoLogDecoratorExecutionListener.java b/src/main/java/io/github/finoid/maven/plugins/codequality/MojoLogDecoratorExecutionListener.java index 1136775..1a36eb4 100644 --- a/src/main/java/io/github/finoid/maven/plugins/codequality/MojoLogDecoratorExecutionListener.java +++ b/src/main/java/io/github/finoid/maven/plugins/codequality/MojoLogDecoratorExecutionListener.java @@ -101,12 +101,24 @@ public void beforeMojoExecution(final MojoExecutionEvent event) { @Override public void afterMojoExecutionSuccess(final MojoExecutionEvent event) { - // No-op + closeDecoratedLog(event); } @Override public void afterExecutionFailure(final MojoExecutionEvent event) { - // No-op + closeDecoratedLog(event); + } + + /** + * Releases the file the decorated log of the given mojo writes to. + *

+ * The log is read back from the mojo rather than remembered, so that concurrently executing modules cannot close + * each other's file. Mojos this listener did not decorate are left alone. + */ + private static void closeDecoratedLog(final MojoExecutionEvent event) { + if (event.getMojo().getLog() instanceof LogAndFileAppender appender) { + appender.close(); + } } private static boolean isMojoOfType(final MojoExecutionEvent event, final String type) { diff --git a/src/main/java/io/github/finoid/maven/plugins/codequality/log/LogAndFileAppender.java b/src/main/java/io/github/finoid/maven/plugins/codequality/log/LogAndFileAppender.java index 57b250a..75ced92 100644 --- a/src/main/java/io/github/finoid/maven/plugins/codequality/log/LogAndFileAppender.java +++ b/src/main/java/io/github/finoid/maven/plugins/codequality/log/LogAndFileAppender.java @@ -5,6 +5,7 @@ import org.codehaus.plexus.logging.Logger; import org.jspecify.annotations.Nullable; +import java.io.Closeable; import java.io.File; import java.io.FileNotFoundException; import java.io.FileOutputStream; @@ -16,13 +17,17 @@ * This class wraps an existing {@link Logger} instance and appends log messages to a specified file, * making it useful for cases where logs need to be both written to the standard logging system * and persisted to a file. This implementation is inspired by {@link org.apache.maven.monitor.logging.DefaultLog}. + *

+ * Holds the file open until {@link #close()} is called, which + * {@link io.github.finoid.maven.plugins.codequality.MojoLogDecoratorExecutionListener} does once the decorated mojo has + * finished. A reactor of many modules would otherwise keep one file handle open per analyzed module. */ -public class LogAndFileAppender implements Log { +public class LogAndFileAppender implements Log, Closeable { private final Logger logger; private final LogLevel logLevel; private final PrintStream printStream; - @SuppressWarnings("required.method.not.called") // the FileOutputStream will be implicitly closed when the jvm exits + @SuppressWarnings("required.method.not.called") // ownership is transferred to the PrintStream, which close() closes public LogAndFileAppender(final Logger logger, final File file, final LogLevel logLevel) throws FileNotFoundException { this.logger = Precondition.nonNull(logger, "Logger shouldn't be null"); this.logLevel = Precondition.nonNull(logLevel, "LogLevel shouldn't be null"); @@ -131,6 +136,14 @@ public void error(final Throwable error) { printStream.println(error); } + /** + * Closes the underlying file. Subsequent writes are discarded by the {@link PrintStream} rather than throwing. + */ + @Override + public void close() { + printStream.close(); + } + @Override public boolean isDebugEnabled() { return logger.isDebugEnabled(); diff --git a/src/main/java/io/github/finoid/maven/plugins/codequality/step/CheckerFrameworkStep.java b/src/main/java/io/github/finoid/maven/plugins/codequality/step/CheckerFrameworkStep.java index 3eaa11b..d345276 100644 --- a/src/main/java/io/github/finoid/maven/plugins/codequality/step/CheckerFrameworkStep.java +++ b/src/main/java/io/github/finoid/maven/plugins/codequality/step/CheckerFrameworkStep.java @@ -8,6 +8,7 @@ import io.github.finoid.maven.plugins.codequality.report.CheckerFrameworkViolationLogParser; import io.github.finoid.maven.plugins.codequality.report.Violation; import io.github.finoid.maven.plugins.codequality.util.CollectorUtils; +import io.github.finoid.maven.plugins.codequality.util.ExceptionUtils; import io.github.finoid.maven.plugins.codequality.util.MojoUtils.ElementUtils; import io.github.finoid.maven.plugins.codequality.util.MojoUtils.PluginUtils; import io.github.finoid.maven.plugins.codequality.util.Precondition; @@ -142,7 +143,12 @@ private List executeStep( return parseViolations(context); } catch (final Exception e) { - throw new CodeQualityException("Error during execution of CheckerFramework step", e); + // The forked compiler reports through its own log, which the plugin redirects to a file, so the reason a + // step failed is regularly only in that file. Both the file and the deepest cause are named here, the + // wrapping exceptions of a forked mojo say little on their own. + throw new CodeQualityException(String.format( + "Error during execution of CheckerFramework step. Cause: %s. The output of the forked compiler was captured in %s", + ExceptionUtils.rootCauseMessage(e), checkerFrameworkOutputFilePath(currentProject)), e); } } diff --git a/src/main/java/io/github/finoid/maven/plugins/codequality/step/ErrorProneStep.java b/src/main/java/io/github/finoid/maven/plugins/codequality/step/ErrorProneStep.java index 2c686f1..b67a852 100644 --- a/src/main/java/io/github/finoid/maven/plugins/codequality/step/ErrorProneStep.java +++ b/src/main/java/io/github/finoid/maven/plugins/codequality/step/ErrorProneStep.java @@ -8,6 +8,7 @@ import io.github.finoid.maven.plugins.codequality.log.ErrorProneViolationLogParser; import io.github.finoid.maven.plugins.codequality.report.Violation; import io.github.finoid.maven.plugins.codequality.util.CollectorUtils; +import io.github.finoid.maven.plugins.codequality.util.ExceptionUtils; import io.github.finoid.maven.plugins.codequality.util.MojoUtils.ElementUtils; import io.github.finoid.maven.plugins.codequality.util.MojoUtils.PluginUtils; import io.github.finoid.maven.plugins.codequality.util.Precondition; @@ -134,7 +135,12 @@ private List executeStep(final CodeQualityConfiguration codeQualityCo return parseViolations(context); } catch (final Exception e) { - throw new CodeQualityException("Error during execution of ErrorProne step", e); + // The forked compiler reports through its own log, which the plugin redirects to a file, so the reason a + // step failed is regularly only in that file. Both the file and the deepest cause are named here, the + // wrapping exceptions of a forked mojo say little on their own. + throw new CodeQualityException(String.format( + "Error during execution of ErrorProne step. Cause: %s. The output of the forked compiler was captured in %s", + ExceptionUtils.rootCauseMessage(e), errorProneOutputFilePath(currentProject)), e); } } diff --git a/src/main/java/io/github/finoid/maven/plugins/codequality/util/ExceptionUtils.java b/src/main/java/io/github/finoid/maven/plugins/codequality/util/ExceptionUtils.java new file mode 100644 index 0000000..72e7c08 --- /dev/null +++ b/src/main/java/io/github/finoid/maven/plugins/codequality/util/ExceptionUtils.java @@ -0,0 +1,59 @@ +package io.github.finoid.maven.plugins.codequality.util; + +import lombok.experimental.UtilityClass; +import org.jspecify.annotations.Nullable; + +/** + * Utility class for extracting the interesting part of an exception chain. + */ +@UtilityClass +public class ExceptionUtils { + /** + * A cause chain is not guaranteed to be acyclic, so the traversal is bounded. + */ + private static final int MAX_DEPTH = 20; + + /** + * Resolves the message of the deepest cause which has one. + *

+ * The steps fork other mojos, so the reason a step failed regularly sits several wrappers down - a + * {@code MojoExecutionException} wrapping a {@code CompilationFailureException} wrapping the actual failure - while + * the wrappers themselves say little. Falls back to the simple name of the deepest cause when none of them carries a + * message. + * + * @param throwable the throwable to unwrap + * @return the message of the deepest cause which has one, never blank + */ + public static String rootCauseMessage(final Throwable throwable) { + Throwable deepest = throwable; + String message = messageOrNull(throwable); + + for (int depth = 0; depth < MAX_DEPTH; depth++) { + final Throwable cause = deepest.getCause(); + + if (cause == null || cause == deepest) { + break; + } + + deepest = cause; + + final String causeMessage = messageOrNull(cause); + if (causeMessage != null) { + message = causeMessage; + } + } + + return message != null ? message : deepest.getClass().getSimpleName(); + } + + @Nullable + private static String messageOrNull(final Throwable throwable) { + final String message = throwable.getMessage(); + + if (message == null || message.isBlank()) { + return null; + } + + return message.strip(); + } +} diff --git a/src/test/java/io/github/finoid/maven/plugins/codequality/CodeQualityReactorIT.java b/src/test/java/io/github/finoid/maven/plugins/codequality/CodeQualityReactorIT.java index e0cb341..fbfe946 100644 --- a/src/test/java/io/github/finoid/maven/plugins/codequality/CodeQualityReactorIT.java +++ b/src/test/java/io/github/finoid/maven/plugins/codequality/CodeQualityReactorIT.java @@ -10,6 +10,7 @@ import org.junit.jupiter.api.Test; import java.io.IOException; +import java.io.InputStream; import java.io.UncheckedIOException; import java.nio.charset.StandardCharsets; import java.nio.file.FileVisitResult; @@ -22,6 +23,7 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.Properties; import java.util.Set; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -46,6 +48,7 @@ class CodeQualityReactorIT { private static final String CHECKSTYLE_FIXTURE = "it/multi-module-reactor"; private static final String ERROR_PRONE_FIXTURE = "it/error-prone-reactor"; + private static final String CHECKER_FRAMEWORK_FIXTURE = "it/checker-framework-reactor"; private static final Map EXPECTED_CHECKSTYLE_VIOLATIONS_BY_PATH = Map.of( "module-a/src/main/java/it/alpha/Alpha.java", "Unused import - java.util.List.", @@ -53,12 +56,14 @@ class CodeQualityReactorIT { "module-c/src/main/java/it/gamma/Gamma.java", "Literal Strings should be compared using equals(), not '=='." ); - private static final Set EXPECTED_ERROR_PRONE_PATHS = Set.of( + private static final Set EXPECTED_COMPILER_STEP_PATHS = Set.of( "module-a/src/main/java/it/alpha/Alpha.java", "module-b/src/main/java/it/beta/Beta.java", "module-c/src/main/java/it/gamma/Gamma.java" ); + private static final List MODULES = List.of("module-a", "module-b", "module-c"); + private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper(); private static String pluginVersion; @@ -145,21 +150,79 @@ void givenErrorProneReactor_whenVerifyInParallel_thenViolationsOfEveryModuleAreR final List violations = aggregatedViolations(basedir); - Assertions.assertEquals(EXPECTED_ERROR_PRONE_PATHS, pathsOf(violations), + Assertions.assertEquals(EXPECTED_COMPILER_STEP_PATHS, pathsOf(violations), () -> "Every module is expected to contribute its own Error Prone violations, but got " + violations); // Every module has to have been analyzed by Error Prone, rather than a single module three times over violations.forEach(violation -> Assertions.assertTrue(violation.description().startsWith("ErrorProne: "), () -> "Unexpected non Error Prone violation: " + violation)); - // The log file of every module has to have been written next to that module, not next to a pinned one - for (final String module : List.of("module-a", "module-b", "module-c")) { - final Path log = basedir.resolve(module + "/target/errorprone-" + module + ".txt"); + assertPerModuleOutput(basedir, "errorprone"); + assertReportedOnce(basedir); + } - Assertions.assertTrue(Files.isRegularFile(log), () -> "Missing Error Prone output of " + module + ": " + log); - } + /** + * The Checker Framework step forks the compiler exactly like Error Prone does, and reads its findings back from the + * same kind of per module log file, so it is prone to the same cross module mix ups. + */ + @Test + @DisplayName("A parallel built reactor reports the Checker Framework violations of every module") + void givenCheckerFrameworkReactor_whenVerifyInParallel_thenViolationsOfEveryModuleAreReported() throws Exception { + // Not under this project's own target directory, see copyFixtureOutsideBuildDirectory + final Path basedir = copyFixtureOutsideBuildDirectory(CHECKER_FRAMEWORK_FIXTURE, "checker-framework-parallel"); + final Verifier verifier = verifier(basedir); + verifier.addCliOption("-T"); + verifier.addCliOption("4"); + // Keeps checker-qual, which the step requires on the class path, in step with the checker the plugin runs + verifier.addCliOption("-Dcq.it.checker.version=" + checkerFrameworkVersion()); + verifier.executeGoal("verify"); + verifier.resetStreams(); + + final List violations = aggregatedViolations(basedir); + + Assertions.assertEquals(EXPECTED_COMPILER_STEP_PATHS, pathsOf(violations), + () -> "Every module is expected to contribute its own Checker Framework violations, but got " + violations); + + violations.forEach(violation -> Assertions.assertTrue(violation.description().startsWith("CheckerFramework: "), + () -> "Unexpected non Checker Framework violation: " + violation)); + + assertPerModuleOutput(basedir, "checkerframework"); assertReportedOnce(basedir); + + // Only on success, a failed run is worth keeping around to look at + deleteRecursively(basedir.getParent()); + } + + /** + * Asserts that the output of the forked compiler of every module was written next to that module, rather than next + * to whichever module a pinned project happened to be. + */ + private void assertPerModuleOutput(final Path basedir, final String prefix) { + for (final String module : MODULES) { + final Path log = basedir.resolve(module + "/target/" + prefix + "-" + module + ".txt"); + + Assertions.assertTrue(Files.isRegularFile(log), () -> "Missing analyzer output of " + module + ": " + log); + } + } + + /** + * The Checker Framework version the plugin defaults to, read from the properties the build filters it into. + */ + private static String checkerFrameworkVersion() throws IOException { + final Properties properties = new Properties(); + + try (InputStream stream = CodeQualityReactorIT.class.getResourceAsStream("/checkerframework-versions.properties")) { + Assertions.assertNotNull(stream, "Missing checkerframework-versions.properties on the test class path"); + + properties.load(stream); + } + + final String version = properties.getProperty("checkerframework.version"); + + Assertions.assertNotNull(version, "Missing checkerframework.version property"); + + return version; } private Verifier verifier(final Path basedir) throws VerificationException { @@ -235,14 +298,26 @@ private String logOf(final Path basedir) throws IOException { } private Path copyFixture(final String fixture, final String name) throws IOException { + return copyFixtureTo(fixture, Paths.get(System.getProperty("basedir", ""), "target", "it", name).toAbsolutePath()); + } + + /** + * Copies a fixture to a working directory outside the build directory of this project. + *

+ * The Checker Framework step passes {@code -AskipFiles=/target/} to keep generated sources out of the analysis, and + * that pattern is matched against the whole path of every file. A fixture below this project's own {@code target} + * directory would therefore be skipped in its entirety and the analyzer would report nothing at all. + */ + private Path copyFixtureOutsideBuildDirectory(final String fixture, final String name) throws IOException { + return copyFixtureTo(fixture, Files.createTempDirectory("codequality-it-").resolve(name)); + } + + private Path copyFixtureTo(final String fixture, final Path target) throws IOException { final Path fixtureRoot = Paths.get(System.getProperty("basedir", ""), "target", "test-classes", fixture) .toAbsolutePath(); Assertions.assertTrue(Files.isDirectory(fixtureRoot), () -> "Missing integration test fixture: " + fixtureRoot); - final Path target = Paths.get(System.getProperty("basedir", ""), "target", "it", name) - .toAbsolutePath(); - deleteRecursively(target); Files.createDirectories(target); diff --git a/src/test/java/io/github/finoid/maven/plugins/codequality/filter/ViolationsFilterServiceUnitTest.java b/src/test/java/io/github/finoid/maven/plugins/codequality/filter/ViolationsFilterServiceUnitTest.java new file mode 100644 index 0000000..3c3ff32 --- /dev/null +++ b/src/test/java/io/github/finoid/maven/plugins/codequality/filter/ViolationsFilterServiceUnitTest.java @@ -0,0 +1,109 @@ +package io.github.finoid.maven.plugins.codequality.filter; + +import io.github.finoid.maven.plugins.codequality.fixtures.UnitTest; +import io.github.finoid.maven.plugins.codequality.fixtures.ViolationFaker; +import io.github.finoid.maven.plugins.codequality.report.Violation; +import org.apache.maven.plugin.logging.Log; +import org.apache.maven.plugin.logging.SystemStreamLog; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Set; + +class ViolationsFilterServiceUnitTest extends UnitTest { + private static final Log LOG = new SystemStreamLog(); + + @Test + void givenNoFilters_whenFilter_thenViolationsAreReturnedUnchanged() { + var violations = violations(); + var unit = new ViolationsFilterService(Collections.emptyList()); + + var result = unit.filter(violations, context("ANY")); + + Assertions.assertSame(violations, result); + } + + @Test + void givenFilterNotConfigured_whenFilter_thenItIsNotApplied() { + var applied = new ArrayList(); + var unit = new ViolationsFilterService(List.of(recordingFilter("NOT_CONFIGURED", applied))); + + var violations = violations(); + var result = unit.filter(violations, context("CONFIGURED")); + + Assertions.assertEquals(Collections.emptyList(), applied); + Assertions.assertSame(violations, result); + } + + @Test + void givenConfiguredFilters_whenFilter_thenOnlyThoseAreAppliedInOrder() { + var applied = new ArrayList(); + var unit = new ViolationsFilterService(List.of( + recordingFilter("FIRST", applied), + recordingFilter("SKIPPED", applied), + recordingFilter("SECOND", applied))); + + unit.filter(violations(), context("FIRST", "SECOND")); + + Assertions.assertEquals(List.of("FIRST", "SECOND"), applied); + } + + @Test + void givenChainedFilters_whenFilter_thenEachReceivesTheResultOfThePrevious() { + var unit = new ViolationsFilterService(List.of( + droppingFilter("DROP_PERMISSIVE", true), + droppingFilter("DROP_NON_PERMISSIVE", false))); + + var result = unit.filter(violations(), context("DROP_PERMISSIVE", "DROP_NON_PERMISSIVE")); + + Assertions.assertEquals(0, result.total()); + } + + private static Violations violations() { + final Violation violation = ViolationFaker.violation().create(); + + return new Violations(List.of(violation), List.of(violation)); + } + + private static ViolationsFilterService.Context context(final String... filterNames) { + return new ViolationsFilterService.Context(LOG, Set.of(filterNames)); + } + + private static ViolationFilter recordingFilter(final String name, final List applied) { + return new ViolationFilter() { + @Override + public Violations filter(final Violations violations, final Context context) { + applied.add(name); + + return violations; + } + + @Override + public String name() { + return name; + } + }; + } + + /** + * Empties one of the two buckets, so that chaining is observable in the result rather than only in a call order. + */ + private static ViolationFilter droppingFilter(final String name, final boolean permissive) { + return new ViolationFilter() { + @Override + public Violations filter(final Violations violations, final Context context) { + return permissive + ? new Violations(Collections.emptyList(), violations.getNonPermissiveViolations()) + : new Violations(violations.getPermissiveViolations(), Collections.emptyList()); + } + + @Override + public String name() { + return name; + } + }; + } +} diff --git a/src/test/java/io/github/finoid/maven/plugins/codequality/step/StepResultsUnitTest.java b/src/test/java/io/github/finoid/maven/plugins/codequality/step/StepResultsUnitTest.java index 12520ee..45f0271 100644 --- a/src/test/java/io/github/finoid/maven/plugins/codequality/step/StepResultsUnitTest.java +++ b/src/test/java/io/github/finoid/maven/plugins/codequality/step/StepResultsUnitTest.java @@ -5,6 +5,7 @@ import io.github.finoid.maven.plugins.codequality.fixtures.UnitTest; import io.github.finoid.maven.plugins.codequality.fixtures.ViolationFaker; import io.github.finoid.maven.plugins.codequality.report.Severity; +import io.github.finoid.maven.plugins.codequality.report.Violation; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; @@ -43,6 +44,50 @@ void givenPermissiveViolation_whenGetNonPermissiveViolations_thenReturnsNoViolat Assertions.assertTrue(result.isEmpty()); } + @Test + void givenBothPermissiveAndNonPermissiveResults_whenGetViolations_thenOnlyTheRequestedBucketIsReturned() { + var results = StepResults.ofResults(List.of( + ProjectStepResultsFaker.projectStepResults() + .withStepResult(StepResultFaker.stepResultFaker() + .withIsPermissive(true) + .withViolation(ViolationFaker.violation().withSeverity(Severity.MAJOR).create()) + .create()) + .create(), + ProjectStepResultsFaker.projectStepResults() + .withStepResult(StepResultFaker.stepResultFaker() + .withIsPermissive(false) + .withViolation(ViolationFaker.violation().withSeverity(Severity.CRITICAL).create()) + .create()) + .create())); + + Assertions.assertEquals(List.of(Severity.MAJOR), severitiesOf(results.getViolations(Severity.INFO, true))); + Assertions.assertEquals(List.of(Severity.CRITICAL), severitiesOf(results.getViolations(Severity.INFO, false))); + } + + @Test + void givenViolationsOfMixedSeverity_whenGetViolations_thenReturnedSortedBySeverity() { + var results = StepResults.ofResults(List.of(ProjectStepResultsFaker.projectStepResults() + .withStepResult(StepResult.create(StepType.CHECKSTYLE, false, List.of( + ViolationFaker.violation().withSeverity(Severity.BLOCKER).create(), + ViolationFaker.violation().withSeverity(Severity.MINOR).create(), + ViolationFaker.violation().withSeverity(Severity.MAJOR).create()))) + .create())); + + Assertions.assertEquals(List.of(Severity.MINOR, Severity.MAJOR, Severity.BLOCKER), + severitiesOf(results.getNonPermissiveViolations(Severity.INFO))); + } + + @Test + void givenViolationsBelowTheThreshold_whenGetViolations_thenTheyAreExcluded() { + var results = StepResults.ofResults(List.of(ProjectStepResultsFaker.projectStepResults() + .withStepResult(StepResult.create(StepType.CHECKSTYLE, false, List.of( + ViolationFaker.violation().withSeverity(Severity.INFO).create(), + ViolationFaker.violation().withSeverity(Severity.MAJOR).create()))) + .create())); + + Assertions.assertEquals(List.of(Severity.MAJOR), severitiesOf(results.getNonPermissiveViolations(Severity.MAJOR))); + } + @Test void givenNonPermissiveViolation_whenGetNonPermissiveViolationsAndHigherSeverity_thenReturnsNoViolation() { var nonPermissiveProjectStepResults = ProjectStepResultsFaker.projectStepResults() @@ -60,4 +105,10 @@ void givenNonPermissiveViolation_whenGetNonPermissiveViolationsAndHigherSeverity Assertions.assertTrue(result.isEmpty()); } -} \ No newline at end of file + + private static List severitiesOf(final List violations) { + return violations.stream() + .map(Violation::getSeverity) + .toList(); + } +} diff --git a/src/test/java/io/github/finoid/maven/plugins/codequality/storage/ReactorCompletionTrackerUnitTest.java b/src/test/java/io/github/finoid/maven/plugins/codequality/storage/ReactorCompletionTrackerUnitTest.java new file mode 100644 index 0000000..da64f67 --- /dev/null +++ b/src/test/java/io/github/finoid/maven/plugins/codequality/storage/ReactorCompletionTrackerUnitTest.java @@ -0,0 +1,179 @@ +package io.github.finoid.maven.plugins.codequality.storage; + +import io.github.finoid.maven.plugins.codequality.fixtures.UnitTest; +import org.apache.maven.execution.MavenSession; +import org.apache.maven.model.Build; +import org.apache.maven.model.Plugin; +import org.apache.maven.project.MavenProject; +import org.eclipse.aether.DefaultSessionData; +import org.eclipse.aether.RepositorySystemSession; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; +import org.mockito.Mockito; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.concurrent.Callable; +import java.util.concurrent.CyclicBarrier; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.stream.IntStream; + +class ReactorCompletionTrackerUnitTest extends UnitTest { + private static final String PLUGIN_KEY = "io.github.finoid:codequality-maven-plugin"; + + @Test + void givenModulesLeftToFinish_whenMarkCompleted_thenReportingIsNotClaimed() { + var projects = declaringProjects("module-a", "module-b", "module-c"); + var session = sessionOf(projects); + var unit = tracker(session); + + Assertions.assertFalse(unit.markCompletedAndClaimReporting(session, projects.get(0), PLUGIN_KEY)); + Assertions.assertFalse(unit.markCompletedAndClaimReporting(session, projects.get(1), PLUGIN_KEY)); + } + + @Test + void givenLastModuleToFinish_whenMarkCompleted_thenReportingIsClaimed() { + var projects = declaringProjects("module-a", "module-b"); + var session = sessionOf(projects); + var unit = tracker(session); + + unit.markCompletedAndClaimReporting(session, projects.get(0), PLUGIN_KEY); + + Assertions.assertTrue(unit.markCompletedAndClaimReporting(session, projects.get(1), PLUGIN_KEY)); + } + + @Test + void givenReportingAlreadyClaimed_whenMarkCompletedAgain_thenReportingIsNotClaimedTwice() { + var projects = declaringProjects("module-a"); + var session = sessionOf(projects); + var unit = tracker(session); + + Assertions.assertTrue(unit.markCompletedAndClaimReporting(session, projects.get(0), PLUGIN_KEY)); + Assertions.assertFalse(unit.markCompletedAndClaimReporting(session, projects.get(0), PLUGIN_KEY), + "A module which runs the goal twice is expected to claim the reporting only once"); + } + + @Test + void givenNoModuleDeclaresThePlugin_whenMarkCompleted_thenEveryModuleIsExpectedToFinish() { + // Covers the goal being invoked straight from the command line, where no module has to declare the plugin + var projects = List.of(project("module-a", false), project("module-b", false)); + var session = sessionOf(projects); + var unit = tracker(session); + + Assertions.assertFalse(unit.markCompletedAndClaimReporting(session, projects.get(0), PLUGIN_KEY)); + Assertions.assertTrue(unit.markCompletedAndClaimReporting(session, projects.get(1), PLUGIN_KEY)); + } + + @Test + void givenModulesNotDeclaringThePlugin_whenMarkCompleted_thenTheyAreNotWaitedFor() { + var declaring = declaringProjects("module-a", "module-b"); + var projects = new ArrayList<>(declaring); + projects.add(project("module-without-the-plugin", false)); + + var session = sessionOf(projects); + var unit = tracker(session); + + unit.markCompletedAndClaimReporting(session, declaring.get(0), PLUGIN_KEY); + + Assertions.assertTrue(unit.markCompletedAndClaimReporting(session, declaring.get(1), PLUGIN_KEY)); + } + + @Test + void givenModulesFinishingConcurrently_whenMarkCompleted_thenExactlyOneClaimsTheReporting() throws Exception { + final int modules = 8; + + // Repeated, a race is not guaranteed to show itself in a single run + for (int attempt = 0; attempt < 50; attempt++) { + var projects = declaringProjects(IntStream.range(0, modules) + .mapToObj(it -> "module-" + it) + .toArray(String[]::new)); + var session = sessionOf(projects); + var unit = tracker(session); + + var barrier = new CyclicBarrier(modules); + var executor = Executors.newFixedThreadPool(modules); + + try { + var claims = executor.invokeAll(projects.stream() + .map(project -> (Callable) () -> { + barrier.await(10, TimeUnit.SECONDS); + + return unit.markCompletedAndClaimReporting(session, project, PLUGIN_KEY); + }) + .toList()); + + Assertions.assertEquals(1, countClaims(claims), + "Exactly one of the concurrently finishing modules is expected to claim the reporting"); + } finally { + executor.shutdownNow(); + } + } + } + + private static long countClaims(final List> claims) throws Exception { + long claimed = 0; + + for (final Future claim : claims) { + if (Boolean.TRUE.equals(claim.get(10, TimeUnit.SECONDS))) { + claimed++; + } + } + + return claimed; + } + + private static ReactorCompletionTracker tracker(final MavenSession session) { + return new ReactorCompletionTracker(new SessionRepository(session)); + } + + /** + * A session backed by a real {@link DefaultSessionData}, so that the tracker exercises the concurrent data context + * it relies on rather than a stub of it. + */ + private static MavenSession sessionOf(final List projects) { + final MavenSession session = Mockito.mock(MavenSession.class); + + Mockito.lenient().when(session.getProjects()) + .thenReturn(projects); + final RepositorySystemSession repositorySession = Mockito.mock(RepositorySystemSession.class); + + Mockito.lenient().when(repositorySession.getData()) + .thenReturn(new DefaultSessionData()); + Mockito.lenient().when(session.getRepositorySession()) + .thenReturn(repositorySession); + + return session; + } + + private static List declaringProjects(final String... artifactIds) { + return Arrays.stream(artifactIds) + .map(it -> project(it, true)) + .toList(); + } + + private static MavenProject project(final String artifactId, final boolean declaresThePlugin) { + final MavenProject project = new MavenProject(); + + project.setGroupId("io.github.finoid.it"); + project.setArtifactId(artifactId); + project.setVersion("1.0"); + project.setPackaging("jar"); + + final Build build = new Build(); + + if (declaresThePlugin) { + final Plugin plugin = new Plugin(); + plugin.setGroupId("io.github.finoid"); + plugin.setArtifactId("codequality-maven-plugin"); + + build.addPlugin(plugin); + } + + project.getModel().setBuild(build); + + return project; + } +} diff --git a/src/test/java/io/github/finoid/maven/plugins/codequality/util/ExceptionUtilsUnitTest.java b/src/test/java/io/github/finoid/maven/plugins/codequality/util/ExceptionUtilsUnitTest.java new file mode 100644 index 0000000..f041dfd --- /dev/null +++ b/src/test/java/io/github/finoid/maven/plugins/codequality/util/ExceptionUtilsUnitTest.java @@ -0,0 +1,59 @@ +package io.github.finoid.maven.plugins.codequality.util; + +import io.github.finoid.maven.plugins.codequality.fixtures.UnitTest; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import java.time.Duration; +import java.util.List; + +class ExceptionUtilsUnitTest extends UnitTest { + @Test + void givenThrowableWithoutCause_whenRootCauseMessage_thenReturnsItsOwnMessage() { + Assertions.assertEquals("Boom", ExceptionUtils.rootCauseMessage(new IllegalStateException("Boom"))); + } + + @Test + void givenWrappedThrowable_whenRootCauseMessage_thenReturnsTheDeepestMessage() { + var cause = new IllegalArgumentException("Unsupported class file major version 65"); + var wrapped = new RuntimeException("Compilation failure", cause); + + Assertions.assertEquals("Unsupported class file major version 65", + ExceptionUtils.rootCauseMessage(new IllegalStateException("Unable to execute mojo", wrapped))); + } + + @Test + void givenDeepestCauseWithoutMessage_whenRootCauseMessage_thenReturnsTheDeepestAvailableMessage() { + var cause = new IllegalStateException((String) null); + var wrapped = new RuntimeException("Compilation failure", cause); + + Assertions.assertEquals("Compilation failure", ExceptionUtils.rootCauseMessage(wrapped)); + } + + @Test + void givenNoMessageAnywhere_whenRootCauseMessage_thenReturnsTheDeepestTypeName() { + // Note the explicit null message, RuntimeException(Throwable) derives one from the cause + var wrapped = new RuntimeException(null, new IllegalArgumentException()); + + Assertions.assertEquals("IllegalArgumentException", ExceptionUtils.rootCauseMessage(wrapped)); + } + + @Test + void givenBlankMessage_whenRootCauseMessage_thenTreatedAsAbsent() { + var wrapped = new RuntimeException("Compilation failure", new IllegalStateException(" ")); + + Assertions.assertEquals("Compilation failure", ExceptionUtils.rootCauseMessage(wrapped)); + } + + @Test + void givenCyclicCauseChain_whenRootCauseMessage_thenTerminates() { + var first = new IllegalStateException("First"); + var second = new IllegalStateException("Second", first); + first.initCause(second); + + // Which of the two the bounded traversal ends on is not interesting, that it ends at all is + var result = Assertions.assertTimeoutPreemptively(Duration.ofSeconds(5), () -> ExceptionUtils.rootCauseMessage(first)); + + Assertions.assertTrue(List.of("First", "Second").contains(result), () -> "Unexpected message: " + result); + } +} diff --git a/src/test/resources/it/checker-framework-reactor/.mvn/maven.config b/src/test/resources/it/checker-framework-reactor/.mvn/maven.config new file mode 100644 index 0000000..31c8930 --- /dev/null +++ b/src/test/resources/it/checker-framework-reactor/.mvn/maven.config @@ -0,0 +1 @@ +--no-transfer-progress \ No newline at end of file diff --git a/src/test/resources/it/checker-framework-reactor/module-a/pom.xml b/src/test/resources/it/checker-framework-reactor/module-a/pom.xml new file mode 100644 index 0000000..841fdc7 --- /dev/null +++ b/src/test/resources/it/checker-framework-reactor/module-a/pom.xml @@ -0,0 +1,16 @@ + + + 4.0.0 + + + io.github.finoid.it + checker-framework-reactor + 1.0 + + + module-a + jar + module-a + diff --git a/src/test/resources/it/checker-framework-reactor/module-a/src/main/java/it/alpha/Alpha.java b/src/test/resources/it/checker-framework-reactor/module-a/src/main/java/it/alpha/Alpha.java new file mode 100644 index 0000000..5346946 --- /dev/null +++ b/src/test/resources/it/checker-framework-reactor/module-a/src/main/java/it/alpha/Alpha.java @@ -0,0 +1,15 @@ +package it.alpha; + +import java.io.IOException; +import java.io.InputStream; +import java.nio.file.Files; +import java.nio.file.Path; + +public class Alpha { + // Checker Framework: required.method.not.called - the stream is never closed + public int size(final Path path) throws IOException { + final InputStream stream = Files.newInputStream(path); + + return stream.readAllBytes().length; + } +} diff --git a/src/test/resources/it/checker-framework-reactor/module-b/pom.xml b/src/test/resources/it/checker-framework-reactor/module-b/pom.xml new file mode 100644 index 0000000..60d9879 --- /dev/null +++ b/src/test/resources/it/checker-framework-reactor/module-b/pom.xml @@ -0,0 +1,24 @@ + + + 4.0.0 + + + io.github.finoid.it + checker-framework-reactor + 1.0 + + + module-b + jar + module-b + + + + io.github.finoid.it + module-a + 1.0 + + + diff --git a/src/test/resources/it/checker-framework-reactor/module-b/src/main/java/it/beta/Beta.java b/src/test/resources/it/checker-framework-reactor/module-b/src/main/java/it/beta/Beta.java new file mode 100644 index 0000000..95d29e2 --- /dev/null +++ b/src/test/resources/it/checker-framework-reactor/module-b/src/main/java/it/beta/Beta.java @@ -0,0 +1,17 @@ +package it.beta; + +import it.alpha.Alpha; + +import java.io.IOException; +import java.io.InputStream; +import java.nio.file.Files; +import java.nio.file.Path; + +public class Beta { + // Checker Framework: required.method.not.called - the stream is never closed + public int sizeOfBoth(final Path path) throws IOException { + final InputStream stream = Files.newInputStream(path); + + return stream.available() + new Alpha().size(path); + } +} diff --git a/src/test/resources/it/checker-framework-reactor/module-c/pom.xml b/src/test/resources/it/checker-framework-reactor/module-c/pom.xml new file mode 100644 index 0000000..8d13441 --- /dev/null +++ b/src/test/resources/it/checker-framework-reactor/module-c/pom.xml @@ -0,0 +1,16 @@ + + + 4.0.0 + + + io.github.finoid.it + checker-framework-reactor + 1.0 + + + module-c + jar + module-c + diff --git a/src/test/resources/it/checker-framework-reactor/module-c/src/main/java/it/gamma/Gamma.java b/src/test/resources/it/checker-framework-reactor/module-c/src/main/java/it/gamma/Gamma.java new file mode 100644 index 0000000..a5bf894 --- /dev/null +++ b/src/test/resources/it/checker-framework-reactor/module-c/src/main/java/it/gamma/Gamma.java @@ -0,0 +1,15 @@ +package it.gamma; + +import java.io.IOException; +import java.io.InputStream; +import java.nio.file.Files; +import java.nio.file.Path; + +public class Gamma { + // Checker Framework: required.method.not.called - the stream is never closed + public int firstByte(final Path path) throws IOException { + final InputStream stream = Files.newInputStream(path); + + return stream.read(); + } +} diff --git a/src/test/resources/it/checker-framework-reactor/pom.xml b/src/test/resources/it/checker-framework-reactor/pom.xml new file mode 100644 index 0000000..7a93b16 --- /dev/null +++ b/src/test/resources/it/checker-framework-reactor/pom.xml @@ -0,0 +1,83 @@ + + + 4.0.0 + + io.github.finoid.it + checker-framework-reactor + 1.0 + pom + checker-framework-reactor + + + 21 + 21 + 21 + 21 + UTF-8 + + + main + 4.2.3 + + + + + module-a + module-b + module-c + + + + + + org.checkerframework + checker-qual + ${cq.it.checker.version} + + + + + + + io.github.finoid + codequality-maven-plugin + ${cq.plugin.version} + + true + + + code-quality + verify + + code-quality + + + + + + + + false + + + false + + + true + + + org.checkerframework.checker.resourceleak.ResourceLeakChecker + + + + + + + +