From ed0c1709ea839bccdd9b463b7cfb2b11b675a91c Mon Sep 17 00:00:00 2001 From: vedanthvasudev Date: Wed, 22 Apr 2026 14:46:00 +0100 Subject: [PATCH] fix/review-findings-batch-1: Batch of correctness, reliability, and DX fixes from the post-v1.9.15 review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Consolidates the tier-1 findings from the four parallel review passes (correctness, security, reliability, adversarial) run against the v2 codebase. Every fix below landed with a targeted test so the regression cannot re-enter silently. Correctness — the two bugs that could silently reduce coverage: - out-of-scope dir globs now align on both sides of the pipeline. The diff-side classifier in PathToClassMapper honoured "api-test/**" but the on-disk classifier in ProjectIndex treated the same string as a literal prefix, so a mixed diff (one production file + a refactor under api-test/) bucketed the api-test file correctly yet still dispatched tests discovered under api-test/src/test/java. Both sides now delegate to a shared OutOfScopeMatchers utility, with a regression test that exercises literal and glob shapes against identical on-disk layouts. - `git rm`-only MRs no longer skip all tests. DiffEntry.DELETE entries now surface through their old path so ignore/out-of-scope rules apply normally; deleted production classes reach the transitive strategy instead of routing the whole MR through EMPTY_DIFF → SKIPPED under local/ci mode. The existing engine filter still drops FQNs whose backing file is gone, so surfacing deletions never asks Gradle to run a missing test. - ImplementationStrategy now recognises the DefaultFooService prefix shape, not only the FooServiceImpl suffix. The plugin ships with implementationNaming = ["Impl", "Default"] and the Javadoc documents both shapes, but the naming-convention loop used to append both tokens as suffixes (FooServiceDefault), matching nothing real. Reliability — hardening against inputs the v2 engine didn't handle: - InvalidPathException (NUL bytes, Linux-committed colon paths on Windows, reserved device names) is now caught in both PathToClassMapper.isIgnored and the out-of-scope glob matcher. Before, the unhandled exception killed the whole affectedTest task with a stack trace; now the offending file falls through to the unmapped bucket and the safety net escalates normally. - GitChangeDetector translates JGit's MissingObjectException into a targeted message naming the likely cause (shallow clone in CI that doesn't know the base ref). Before, users saw the raw JGit exception and had to guess whether the problem was the ref, the clone depth, or a corrupt repo. - Malformed globs in outOfScope*Dirs and ignorePaths now fail at config-time with an IllegalStateException naming the config key, list index, and offending pattern, instead of surfacing a raw PatternSyntaxException from several engine frames deep. - parseMode / parseAction / isWindows pinned to Locale.ROOT so Turkish-locale JVMs don't turn `mode = "ci"` into `Unknown affectedTests.mode 'ci'`. Matches the Locale.ROOT pin AffectedTestsConfig already applied to legacy parsing. DX and defense-in-depth: - FQN shape validated before passing to Gradle's --tests argv. Discovered strings that aren't Java-shaped identifiers are logged as warnings and skipped — a last line of defence against a buggy custom strategy or parser anomaly injecting a shell metacharacter into the test-filter flag. - Per-FQN dispatch lines demoted from lifecycle to info. Each MR now gets one lifecycle summary per Gradle task (":api:test (3 test classes)"); individual FQNs print only under --info. Before, a 200-class MR buried the outcome/situation line the user actually cared about under 200 dispatch lines. Documentation: - Mode Javadoc re-aligned with the mode-defaults table in the design doc (LOCAL is the pre-v2 baseline; CI adds the DISCOVERY_EMPTY = FULL_SUITE safety net on top of that). - Situation Javadoc cross-reference for ALL_FILES_OUT_OF_SCOPE fixed — the old reference pointed at a since-renamed constant. - TransitiveStrategy and AffectedTestsExtension now document the actual transitiveDepth default of 4, not the pre-v2 value of 2. --- CHANGELOG.md | 85 +++++++++++ .../io/affectedtests/core/config/Mode.java | 9 +- .../affectedtests/core/config/Situation.java | 2 +- .../discovery/ImplementationStrategy.java | 25 +++- .../core/discovery/ProjectIndex.java | 68 +++++---- .../core/discovery/TransitiveStrategy.java | 2 +- .../core/git/GitChangeDetector.java | 49 +++++- .../core/mapping/OutOfScopeMatchers.java | 136 +++++++++++++++++ .../core/mapping/PathToClassMapper.java | 141 ++++++------------ .../discovery/ImplementationStrategyTest.java | 33 ++++ .../core/discovery/ProjectIndexTest.java | 91 +++++++++++ .../core/git/GitChangeDetectorTest.java | 38 +++++ .../core/mapping/OutOfScopeMatchersTest.java | 110 ++++++++++++++ .../gradle/AffectedTestTask.java | 68 ++++++++- .../gradle/AffectedTestsExtension.java | 4 +- .../AffectedTestTaskFqnValidationTest.java | 54 +++++++ .../gradle/AffectedTestTaskLocaleTest.java | 56 +++++++ 17 files changed, 822 insertions(+), 149 deletions(-) create mode 100644 affected-tests-core/src/main/java/io/affectedtests/core/mapping/OutOfScopeMatchers.java create mode 100644 affected-tests-core/src/test/java/io/affectedtests/core/discovery/ProjectIndexTest.java create mode 100644 affected-tests-core/src/test/java/io/affectedtests/core/mapping/OutOfScopeMatchersTest.java create mode 100644 affected-tests-gradle/src/test/java/io/affectedtests/gradle/AffectedTestTaskFqnValidationTest.java create mode 100644 affected-tests-gradle/src/test/java/io/affectedtests/gradle/AffectedTestTaskLocaleTest.java diff --git a/CHANGELOG.md b/CHANGELOG.md index 3d870cc..df21953 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,6 +51,91 @@ adheres to [Semantic Versioning](https://semver.org/). CI run. The hint is suppressed on empty diffs, on runs where the bucket is non-empty, and on zero-config installs — so its rarity is itself a signal. +- `OutOfScopeMatchers` — an internal utility shared between + `PathToClassMapper` and `ProjectIndex`. Not a public API, but + called out here because it is the structural fix behind the + glob-alignment bug in the Fixed section below, and because its + malformed-glob error path is now the single source of truth for + `Affected Tests: invalid glob at outOfScope*Dirs[N]` messages. + +### Fixed — post-v1.9.15 review batch + +- `outOfScopeTestDirs` / `outOfScopeSourceDirs` glob entries now work + on both sides of the pipeline. Before this fix the diff-side + classifier in `PathToClassMapper` honoured `"api-test/**"` but the + on-disk classifier in `ProjectIndex` treated the same string as a + literal prefix, so a mixed diff (one production file + a refactor + under `api-test/`) bucketed the api-test file correctly yet still + dispatched tests discovered under `api-test/src/test/java`. Both + sides now delegate to a shared compiler, with a regression test that + exercises the literal and glob shapes against identical on-disk + layouts. +- `git rm`-only MRs no longer silently skip all tests. `DiffEntry.DELETE` + entries now surface through their old path, so ignore/out-of-scope + rules apply normally and deleted production classes reach the + transitive strategy instead of routing the whole MR through + `EMPTY_DIFF → SKIPPED` under `local`/`ci` mode. The existing engine + filter still drops FQNs whose backing file is gone, so surfacing + deletions never asks Gradle to run a missing test. +- `ImplementationStrategy` now recognises the `DefaultFooService` + prefix shape, not only the `FooServiceImpl` suffix. The plugin has + always shipped `implementationNaming = ["Impl", "Default"]` and the + Javadoc documents both shapes, but the naming-convention loop used + to append both tokens as suffixes (`FooServiceDefault`), matching + nothing real. The AST-scan branch rescued explicit + `implements FooService` cases; generics-only declarations and files + that JavaParser couldn't parse silently missed the Default-prefixed + impl. +- Turkish (and any other locale whose case-folding tables differ from + US English) no longer turn `mode = "ci"` into `Unknown + affectedTests.mode 'ci'`. `parseMode` and `parseAction` now force + `Locale.ROOT`, matching `AffectedTestsConfig`'s own parsing. The + Windows detection in the Gradle-command resolver was pinned to + `Locale.ROOT` for the same reason. +- `PathToClassMapper.isIgnored` and the `OutOfScopeMatchers` glob + matchers now fail closed on `InvalidPathException` (NUL bytes, + Linux-committed filenames like `foo:bar.md` arriving on a Windows + CI runner, Windows reserved names). Before this fix the unhandled + exception killed the whole `affectedTest` task with a stack trace; + now the offending file falls through to the unmapped bucket and the + safety net escalates normally. +- `GitChangeDetector` now translates JGit's `MissingObjectException` + into a targeted message naming the likely cause: a shallow clone in + CI that doesn't know the base ref. Before, users saw the raw JGit + exception and had to guess whether the problem was the ref, the + clone depth, or a corrupt repo. +- Malformed globs in `outOfScopeTestDirs` / `outOfScopeSourceDirs` / + `ignorePaths` now fail at config-time with an + `IllegalStateException` naming the config key, list index, and + offending pattern. The raw `PatternSyntaxException` the JVM throws + is useless on its own because it doesn't say which config entry + caused the regex error. +- The `--tests` argv assembly now skips and warns on any discovered + FQN that isn't shaped like a Java identifier. Defense-in-depth + against a buggy custom strategy or parser anomaly injecting a shell + metacharacter, whitespace, or a hyphen into Gradle's test filter — + such strings either crashed the test runner with an obscure glob- + expansion error or silently matched zero tests. +- Per-FQN dispatch lines in the task output demoted from `lifecycle` + to `info`. Each MR now gets one `lifecycle` summary per Gradle task + (`:api:test (3 test classes)`); the individual FQNs print only when + the user opts in with `--info`. Before, a 200-class MR produced 200 + console lines and drowned the single line the user actually cared + about — the outcome/situation summary. + +### Documentation + +- `Mode` Javadoc now matches the mode-defaults table in the design + doc. The post-v2 table said zero-config CI users get + `DISCOVERY_EMPTY = FULL_SUITE` on top of `LOCAL`, but the class- + level doc still described `LOCAL` as the pre-v2 baseline "minus" + the safety net. +- `Situation` Javadoc cross-reference for `ALL_FILES_OUT_OF_SCOPE` + updated — the old reference pointed at a since-renamed constant. +- `TransitiveStrategy` and `AffectedTestsExtension` now document the + actual `transitiveDepth` default of `4`, not the pre-v2 value of + `2`. Consumers reading Javadoc were being told they needed to set + `transitiveDepth = 4` explicitly; they do not. ## [1.9.12] — 2026-04-22 diff --git a/affected-tests-core/src/main/java/io/affectedtests/core/config/Mode.java b/affected-tests-core/src/main/java/io/affectedtests/core/config/Mode.java index cef9123..e66d79c 100644 --- a/affected-tests-core/src/main/java/io/affectedtests/core/config/Mode.java +++ b/affected-tests-core/src/main/java/io/affectedtests/core/config/Mode.java @@ -21,10 +21,11 @@ * * *

The pre-v2 zero-config baseline was - * {@code runAllIfNoMatches=false}, {@code runAllOnNonJavaChange=true} — - * which translates to the {@link #LOCAL} column above minus the - * {@code DISCOVERY_EMPTY=FULL_SUITE} CI safety net. Zero-config users - * running in CI now get the safer default without having to opt in. + * {@code runAllIfNoMatches=false}, {@code runAllOnNonJavaChange=true} + * — which lines up exactly with the {@link #LOCAL} column above. The + * {@link #CI} profile adds the {@code DISCOVERY_EMPTY=FULL_SUITE} + * safety net on top of that so zero-config users who land in a CI + * environment get the safer default without having to opt in. */ public enum Mode { /** Detect CI vs. local at build() time based on common CI env vars. */ diff --git a/affected-tests-core/src/main/java/io/affectedtests/core/config/Situation.java b/affected-tests-core/src/main/java/io/affectedtests/core/config/Situation.java index 322d904..b839e8b 100644 --- a/affected-tests-core/src/main/java/io/affectedtests/core/config/Situation.java +++ b/affected-tests-core/src/main/java/io/affectedtests/core/config/Situation.java @@ -36,7 +36,7 @@ public enum Situation { /** * Every file in the diff matched {@link AffectedTestsConfig#ignorePaths()} * (or the legacy {@code excludePaths} shim). Distinct from - * {@link #ALL_OUT_OF_SCOPE} so users can treat "purely docs changes" + * {@link #ALL_FILES_OUT_OF_SCOPE} so users can treat "purely docs changes" * differently from "purely api-test changes". */ ALL_FILES_IGNORED, diff --git a/affected-tests-core/src/main/java/io/affectedtests/core/discovery/ImplementationStrategy.java b/affected-tests-core/src/main/java/io/affectedtests/core/discovery/ImplementationStrategy.java index a7e7041..21befb6 100644 --- a/affected-tests-core/src/main/java/io/affectedtests/core/discovery/ImplementationStrategy.java +++ b/affected-tests-core/src/main/java/io/affectedtests/core/discovery/ImplementationStrategy.java @@ -105,12 +105,27 @@ private Set findImplementations(Set changedClasses, simpleNameToFqns.computeIfAbsent(SourceFileScanner.simpleClassName(fqn), k -> new HashSet<>()).add(fqn); } - // 1. Naming convention: look for *Impl classes - for (String suffix : config.implementationNaming()) { - for (String fqn : changedClasses) { - String implSimpleName = SourceFileScanner.simpleClassName(fqn) + suffix; + // 1. Naming convention: look for both suffix (*Impl) and prefix + // (Default*) derivatives of the changed interface name. The + // config list ships with {"Impl", "Default"} and the Builder + // javadoc promises both shapes; before this loop checked both + // sides, "Default" was appended as a suffix (FooServiceDefault), + // which matches nothing real — Spring/Guice code writes + // DefaultFooService. The AST branch below rescues the clean + // "implements FooService" case, but impls that declare the + // super-type via generics only, or files JavaParser could not + // parse, were silently missed. + Set changedSimpleNames = new HashSet<>(); + for (String fqn : changedClasses) { + changedSimpleNames.add(SourceFileScanner.simpleClassName(fqn)); + } + for (String token : config.implementationNaming()) { + for (String changedSimple : changedSimpleNames) { + String suffixShape = changedSimple + token; + String prefixShape = token + changedSimple; for (String sourceFqn : allSourceFqns) { - if (SourceFileScanner.simpleClassName(sourceFqn).equals(implSimpleName)) { + String sourceSimple = SourceFileScanner.simpleClassName(sourceFqn); + if (sourceSimple.equals(suffixShape) || sourceSimple.equals(prefixShape)) { implementations.add(sourceFqn); } } diff --git a/affected-tests-core/src/main/java/io/affectedtests/core/discovery/ProjectIndex.java b/affected-tests-core/src/main/java/io/affectedtests/core/discovery/ProjectIndex.java index ab24619..c15610b 100644 --- a/affected-tests-core/src/main/java/io/affectedtests/core/discovery/ProjectIndex.java +++ b/affected-tests-core/src/main/java/io/affectedtests/core/discovery/ProjectIndex.java @@ -4,11 +4,13 @@ import com.github.javaparser.ParseResult; import com.github.javaparser.ast.CompilationUnit; import io.affectedtests.core.config.AffectedTestsConfig; +import io.affectedtests.core.mapping.OutOfScopeMatchers; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import java.nio.file.Path; import java.util.*; +import java.util.function.Predicate; /** * Pre-scanned index of source and test files for a project directory. @@ -43,32 +45,43 @@ private ProjectIndex(List sourceFiles, List testFiles, public static ProjectIndex build(Path projectDir, AffectedTestsConfig config) { log.info("Building project index for {}", projectDir); - List oosSource = config.outOfScopeSourceDirs(); - List oosTest = config.outOfScopeTestDirs(); + // Share the exact matcher compilation PathToClassMapper uses so + // diff-side bucketing and indexed-file-side filtering agree on + // every entry — glob form, literal form, or mixed. Before this + // shared source of truth the two sides silently disagreed on + // glob entries: the mapper compiled "api-test/**" into a + // PathMatcher while this class treated it as a literal prefix + // and matched nothing, so mixed diffs still dispatched tests + // under "api-test/". + List> oosSourceMatchers = OutOfScopeMatchers.compile( + config.outOfScopeSourceDirs(), "outOfScopeSourceDirs"); + List> oosTestMatchers = OutOfScopeMatchers.compile( + config.outOfScopeTestDirs(), "outOfScopeTestDirs"); + boolean hasOutOfScope = !oosSourceMatchers.isEmpty() || !oosTestMatchers.isEmpty(); List sourceFiles = filterOutOfScope( SourceFileScanner.collectSourceFiles(projectDir, config.sourceDirs()), - projectDir, oosSource, oosTest); + projectDir, oosSourceMatchers, oosTestMatchers, hasOutOfScope); List testFiles = filterOutOfScope( SourceFileScanner.collectTestFiles(projectDir, config.testDirs()), - projectDir, oosSource, oosTest); + projectDir, oosSourceMatchers, oosTestMatchers, hasOutOfScope); LinkedHashMap testFqnToPath = SourceFileScanner.scanTestFqnsWithFiles( projectDir, config.testDirs()); - if (!oosSource.isEmpty() || !oosTest.isEmpty()) { + if (hasOutOfScope) { // Drop out-of-scope test FQNs from the dispatch map. Without // this, discovery strategies could still return FQNs living // under {@code api-test/src/test/java} and the task would then // try to run them — the entire point of the out-of-scope knob // is that those tests never reach the affected-test dispatch. testFqnToPath.entrySet().removeIf(entry -> isUnderAny( - entry.getValue(), projectDir, oosSource, oosTest)); + entry.getValue(), projectDir, oosSourceMatchers, oosTestMatchers)); } Set sourceFqns = new LinkedHashSet<>(); for (String sourceDir : config.sourceDirs()) { for (Path resolved : SourceFileScanner.findAllMatchingDirs(projectDir, sourceDir)) { - if (isUnderAny(resolved, projectDir, oosSource, oosTest)) continue; + if (hasOutOfScope && isUnderAny(resolved, projectDir, oosSourceMatchers, oosTestMatchers)) continue; sourceFqns.addAll(SourceFileScanner.fqnsUnder(resolved)); } } @@ -76,7 +89,7 @@ public static ProjectIndex build(Path projectDir, AffectedTestsConfig config) { log.info("Project index: {} source files, {} test files, {} source FQNs, {} test FQNs" + " (out-of-scope source dirs: {}, out-of-scope test dirs: {})", sourceFiles.size(), testFiles.size(), sourceFqns.size(), testFqnToPath.size(), - oosSource.size(), oosTest.size()); + config.outOfScopeSourceDirs().size(), config.outOfScopeTestDirs().size()); return new ProjectIndex( Collections.unmodifiableList(sourceFiles), @@ -87,13 +100,15 @@ public static ProjectIndex build(Path projectDir, AffectedTestsConfig config) { } private static List filterOutOfScope(List files, Path projectDir, - List oosSource, List oosTest) { - if (oosSource.isEmpty() && oosTest.isEmpty()) { + List> oosSourceMatchers, + List> oosTestMatchers, + boolean hasOutOfScope) { + if (!hasOutOfScope) { return files; } List filtered = new ArrayList<>(files.size()); for (Path file : files) { - if (!isUnderAny(file, projectDir, oosSource, oosTest)) { + if (!isUnderAny(file, projectDir, oosSourceMatchers, oosTestMatchers)) { filtered.add(file); } } @@ -101,14 +116,17 @@ private static List filterOutOfScope(List files, Path projectDir, } /** - * Normalised, boundary-aware "does this absolute path sit under any of - * the given project-relative dirs?" check. Mirrors - * {@link io.affectedtests.core.mapping.PathToClassMapper} semantics so - * a diff file and an indexed file that point to the same location are - * routed the same way. + * Normalised "does this absolute path sit under any of the compiled + * out-of-scope matchers?" check. Evaluates exactly the matchers + * {@link io.affectedtests.core.mapping.PathToClassMapper} uses on + * the diff side, so a file and an indexed file pointing to the + * same location route the same way whether the entry was written + * as {@code api-test}, {@code api-test/**}, or {@code **/api-test/**}. */ - static boolean isUnderAny(Path file, Path projectDir, List oosSource, List oosTest) { - if (oosSource.isEmpty() && oosTest.isEmpty()) return false; + static boolean isUnderAny(Path file, Path projectDir, + List> oosSourceMatchers, + List> oosTestMatchers) { + if (oosSourceMatchers.isEmpty() && oosTestMatchers.isEmpty()) return false; String rel; try { rel = projectDir.toAbsolutePath().relativize(file.toAbsolutePath()).toString(); @@ -116,18 +134,8 @@ static boolean isUnderAny(Path file, Path projectDir, List oosSource, Li return false; } String normalized = rel.replace(java.io.File.separatorChar, '/'); - return startsWithAny(normalized, oosSource) || startsWithAny(normalized, oosTest); - } - - private static boolean startsWithAny(String normalized, List dirs) { - for (String dir : dirs) { - if (dir == null || dir.isBlank()) continue; - String d = dir.replace('\\', '/'); - if (!d.endsWith("/")) d += "/"; - if (normalized.startsWith(d)) return true; - if (normalized.contains("/" + d)) return true; - } - return false; + return OutOfScopeMatchers.matchesAny(normalized, oosSourceMatchers) + || OutOfScopeMatchers.matchesAny(normalized, oosTestMatchers); } public List sourceFiles() { return sourceFiles; } diff --git a/affected-tests-core/src/main/java/io/affectedtests/core/discovery/TransitiveStrategy.java b/affected-tests-core/src/main/java/io/affectedtests/core/discovery/TransitiveStrategy.java index 7f4fcd1..9e96896 100644 --- a/affected-tests-core/src/main/java/io/affectedtests/core/discovery/TransitiveStrategy.java +++ b/affected-tests-core/src/main/java/io/affectedtests/core/discovery/TransitiveStrategy.java @@ -23,7 +23,7 @@ * changes, walks this "used-by" graph N levels deep to find consumers, then * discovers tests for those consumers via the naming and usage strategies. *

- * Depth is configurable via {@code transitiveDepth} (default 2, max 5). + * Depth is configurable via {@code transitiveDepth} (default 4, max 5). */ public final class TransitiveStrategy implements TestDiscoveryStrategy { diff --git a/affected-tests-core/src/main/java/io/affectedtests/core/git/GitChangeDetector.java b/affected-tests-core/src/main/java/io/affectedtests/core/git/GitChangeDetector.java index a7c7864..8170e76 100644 --- a/affected-tests-core/src/main/java/io/affectedtests/core/git/GitChangeDetector.java +++ b/affected-tests-core/src/main/java/io/affectedtests/core/git/GitChangeDetector.java @@ -4,6 +4,7 @@ import org.eclipse.jgit.api.Git; import org.eclipse.jgit.api.errors.GitAPIException; import org.eclipse.jgit.diff.DiffEntry; +import org.eclipse.jgit.errors.MissingObjectException; import org.eclipse.jgit.errors.NoWorkTreeException; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectReader; @@ -26,10 +27,20 @@ * Detects changed files using JGit by comparing the current HEAD (and optionally * uncommitted changes) against a configurable base ref. * - *

Paths for {@code DELETE} changes and the old path of a {@code RENAME} are - * intentionally omitted from the result. Including them would lead the engine to - * try and re-run tests that no longer exist on disk, which Gradle rejects with - * {@code No tests found for given includes}. + *

{@code DELETE} changes surface through the old path so a pure-deletion + * MR is NOT bucketed as an empty diff. The engine's dispatch filter at + * {@link io.affectedtests.core.AffectedTestsEngine} drops any FQN whose + * test file no longer exists on disk, so emitting the deleted path is safe + * — it routes the diff through the normal ignore / out-of-scope / unmapped + * logic (so a deleted {@code *.md} still gets ignored, a deleted + * {@code api-test/**} still counts as out-of-scope, and a deleted production + * class still pushes discovery through the transitive strategy for callers + * whose tests may have started failing). + * + *

The old path of a {@code RENAME} is still dropped in favour of the + * new path alone; the new path is the file currently on disk and the + * file the diff actually modified, so adding the old path would only + * double-count. */ public final class GitChangeDetector { @@ -75,6 +86,21 @@ public Set detectChangedFiles() { // is not inside a git work tree at all. throw new IllegalStateException( "Affected Tests: directory '" + projectDir + "' is not a git work tree.", e); + } catch (MissingObjectException e) { + // Surfaces separately from generic IOException because this is + // overwhelmingly the "shallow clone" failure mode — JGit resolved + // the ref to an ObjectId but the commit object isn't in the local + // pack. The actions/checkout default is fetch-depth=1, so the + // first CI run on a new pipeline hits this and the generic + // "I/O error while reading git metadata" message sends operators + // hunting for disk or JGit problems instead of the real fix. + throw new IllegalStateException( + "Affected Tests: base commit " + e.getObjectId().name() + + " for ref '" + config.baseRef() + "' is not available locally. " + + "This usually means the repository is a shallow clone. " + + "Run 'git fetch --unshallow' locally, or set fetch-depth: 0 on " + + "actions/checkout (GitHub) / GIT_DEPTH: 0 (GitLab).", + e); } catch (IOException e) { throw new IllegalStateException( "Affected Tests: I/O error while reading git metadata at '" + projectDir + "': " @@ -118,9 +144,18 @@ private Set committedChanges(Repository repository, Git git) throws IOEx switch (entry.getChangeType()) { case ADD, COPY, MODIFY -> files.add(entry.getNewPath()); case DELETE -> { - // Deleted files have no matching class on disk; adding them would - // cause downstream "No tests found" failures for deleted tests - // and is pointless for deleted production classes. + // Before we surfaced DELETEs, a pure 'git rm' MR routed + // through EMPTY_DIFF and — in LOCAL/CI modes — silently + // skipped all tests. Now the old path flows through the + // normal bucketing so ignore/out-of-scope rules still + // apply (deleted '*.md' stays ignored, deleted + // 'api-test/**' stays out-of-scope, deleted production + // classes reach the transitive strategy). The engine's + // existing filter at AffectedTestsEngine drops any + // discovered FQN whose test file no longer exists, so + // surfacing the deleted path never asks Gradle to run a + // missing test. + files.add(entry.getOldPath()); } case RENAME -> files.add(entry.getNewPath()); } diff --git a/affected-tests-core/src/main/java/io/affectedtests/core/mapping/OutOfScopeMatchers.java b/affected-tests-core/src/main/java/io/affectedtests/core/mapping/OutOfScopeMatchers.java new file mode 100644 index 0000000..c595352 --- /dev/null +++ b/affected-tests-core/src/main/java/io/affectedtests/core/mapping/OutOfScopeMatchers.java @@ -0,0 +1,136 @@ +package io.affectedtests.core.mapping; + +import java.nio.file.FileSystems; +import java.nio.file.InvalidPathException; +import java.nio.file.PathMatcher; +import java.util.ArrayList; +import java.util.List; +import java.util.function.Predicate; + +/** + * Compiles the raw {@code outOfScope*Dirs} strings a user writes in + * {@code build.gradle} into {@link Predicate}s over normalised path + * strings. + * + *

Kept in its own class so both {@link PathToClassMapper} (which + * classifies diff-side paths) and + * {@link io.affectedtests.core.discovery.ProjectIndex} (which filters + * indexed files on disk) evaluate the same shape of entry with the + * same semantics. Before this extraction the two sides disagreed on + * glob-form entries: the mapper honoured + * {@code outOfScopeTestDirs = ["api-test/**"]} while the index + * treated the same string as a literal prefix, so a mixed diff (one + * production file + a refactor under {@code api-test/}) would bucket + * the api-test file correctly but still dispatch its test because the + * index never filtered it out. That divergence is what this utility + * closes — there is exactly one source of truth for "is this path + * out of scope" now. + * + *

Each raw entry is classified into one of two semantics based on + * whether it contains any glob metacharacter ({@code *}, {@code ?}, + * {@code [}, {): + * + *

+ * + *

Mixing both shapes in the same list is supported — this is what + * lets existing adopters migrate at their own pace without the plugin + * ever silently losing coverage. + * + *

{@code null} and blank entries are dropped quietly: a + * mis-concatenated list literal on the Gradle side is not worth + * failing a build over, and the {@code --explain} hint already + * surfaces the more likely "configured but nothing matched" failure. + * + *

Malformed glob entries fail the build at config-time with a + * targeted {@link IllegalStateException} naming the offending key, + * index, and pattern — the raw + * {@link java.util.regex.PatternSyntaxException} the JVM throws is + * useless on its own because it does not mention which config entry + * caused the regex error, and by the time it surfaces the user is + * several stack-trace frames deep in the engine. + */ +public final class OutOfScopeMatchers { + + private OutOfScopeMatchers() {} + + /** + * Compile the raw {@code dirs} strings into matchers, labelling + * any malformed glob errors with {@code configKey}. + * + * @param dirs raw entries from {@code build.gradle}; may be empty + * @param configKey the user-facing name of the list being compiled + * ({@code "outOfScopeTestDirs"} etc.) so error + * messages point at the right knob + * @return immutable list of predicates; empty when {@code dirs} is + * empty or contains only null/blank entries + */ + public static List> compile(List dirs, String configKey) { + if (dirs == null || dirs.isEmpty()) return List.of(); + List> matchers = new ArrayList<>(dirs.size()); + for (int i = 0; i < dirs.size(); i++) { + String dir = dirs.get(i); + if (dir == null || dir.isBlank()) continue; + String normalizedDir = dir.replace('\\', '/'); + if (hasGlobMetachar(normalizedDir)) { + PathMatcher pm; + try { + pm = FileSystems.getDefault().getPathMatcher("glob:" + normalizedDir); + } catch (IllegalArgumentException e) { + // Covers both PatternSyntaxException and raw IAE shapes + // from the default glob compiler; both inherit from IAE. + throw new IllegalStateException( + "Affected Tests: invalid glob at " + configKey + "[" + i + + "]: '" + dir + "' — " + e.getMessage(), e); + } + matchers.add(path -> { + try { + return pm.matches(java.nio.file.Path.of(path)); + } catch (InvalidPathException e) { + // A changed file whose path contains characters the + // platform refuses to parse (mostly a Windows-in-git + // edge case) can't match any glob — fail closed so + // the engine routes it through the unmapped bucket + // instead of pretending the glob bit. + return false; + } + }); + } else { + String prefix = normalizedDir.endsWith("/") ? normalizedDir : normalizedDir + "/"; + matchers.add(path -> + path.startsWith(prefix) || path.contains("/" + prefix)); + } + } + return List.copyOf(matchers); + } + + /** + * Fast-path "does any compiled matcher claim this path?" check. + * Works on an already-normalised path string so every caller + * agrees on the slash character. + */ + public static boolean matchesAny(String normalizedPath, List> matchers) { + if (matchers.isEmpty()) return false; + for (Predicate matcher : matchers) { + if (matcher.test(normalizedPath)) return true; + } + return false; + } + + private static boolean hasGlobMetachar(String s) { + for (int i = 0; i < s.length(); i++) { + char c = s.charAt(i); + if (c == '*' || c == '?' || c == '[' || c == '{') return true; + } + return false; + } +} diff --git a/affected-tests-core/src/main/java/io/affectedtests/core/mapping/PathToClassMapper.java b/affected-tests-core/src/main/java/io/affectedtests/core/mapping/PathToClassMapper.java index 3790fa6..6918561 100644 --- a/affected-tests-core/src/main/java/io/affectedtests/core/mapping/PathToClassMapper.java +++ b/affected-tests-core/src/main/java/io/affectedtests/core/mapping/PathToClassMapper.java @@ -5,6 +5,7 @@ import org.slf4j.LoggerFactory; import java.nio.file.FileSystems; +import java.nio.file.InvalidPathException; import java.nio.file.PathMatcher; import java.util.ArrayList; import java.util.LinkedHashSet; @@ -36,16 +37,38 @@ public final class PathToClassMapper { public PathToClassMapper(AffectedTestsConfig config) { this.config = config; - this.ignoreMatchers = config.ignorePaths().stream() - .map(p -> FileSystems.getDefault().getPathMatcher("glob:" + p)) - .toList(); + this.ignoreMatchers = compileIgnoreMatchers(config.ignorePaths()); // Out-of-scope dirs are compiled once, up-front, so every diff // pays only the matcher cost instead of re-parsing the config // strings per-file. The pre-compile step is also where we decide // between glob and literal-prefix semantics, so users can mix - // both shapes in the same list without surprise. - this.outOfScopeTestMatchers = compileOutOfScopeMatchers(config.outOfScopeTestDirs()); - this.outOfScopeSourceMatchers = compileOutOfScopeMatchers(config.outOfScopeSourceDirs()); + // both shapes in the same list without surprise. The compiled + // matcher list is shared with ProjectIndex so diff-side and + // indexed-file-side agree on what "out of scope" means for a + // given entry — see OutOfScopeMatchers for the rationale. + this.outOfScopeTestMatchers = OutOfScopeMatchers.compile( + config.outOfScopeTestDirs(), "outOfScopeTestDirs"); + this.outOfScopeSourceMatchers = OutOfScopeMatchers.compile( + config.outOfScopeSourceDirs(), "outOfScopeSourceDirs"); + } + + private static List compileIgnoreMatchers(List ignorePaths) { + List matchers = new ArrayList<>(ignorePaths.size()); + for (int i = 0; i < ignorePaths.size(); i++) { + String p = ignorePaths.get(i); + try { + matchers.add(FileSystems.getDefault().getPathMatcher("glob:" + p)); + } catch (IllegalArgumentException e) { + // Catches both PatternSyntaxException and the raw + // IllegalArgumentException the JVM throws for some + // shapes like "glob:[" — both inherit from IAE and are + // equally unhelpful on their own. + throw new IllegalStateException( + "Affected Tests: invalid glob at ignorePaths[" + i + "]: '" + p + + "' — " + e.getMessage(), e); + } + } + return List.copyOf(matchers); } /** @@ -228,7 +251,20 @@ private String extractModuleFromDirs(String normalizedPath, java.util.List> matchers) { if (matchers.isEmpty()) return false; - String normalized = filePath.replace('\\', '/'); - for (Predicate matcher : matchers) { - if (matcher.test(normalized)) return true; - } - return false; - } - - /** - * Compiles each raw out-of-scope dir string into a {@link Predicate} - * that answers "does this (normalised) file path sit under this - * entry?". - * - *

Each entry is classified into one of two semantics based on - * whether it contains any glob metacharacter ({@code *}, {@code ?}, - * {@code [}, {@code \{}): - * - *

- * - *

Mixing both shapes in the same list is supported — this is - * what lets existing adopters migrate at their own pace without the - * plugin ever silently losing coverage. - * - *

{@code null} and blank entries are dropped quietly: a - * mis-concatenated list literal on the Gradle side is not worth - * failing a build over, and the {@code --explain} hint already - * surfaces the more likely "configured but nothing matched" failure. - */ - private static List> compileOutOfScopeMatchers(List dirs) { - if (dirs == null || dirs.isEmpty()) return List.of(); - List> matchers = new ArrayList<>(dirs.size()); - for (String dir : dirs) { - if (dir == null || dir.isBlank()) continue; - String normalizedDir = dir.replace('\\', '/'); - if (hasGlobMetachar(normalizedDir)) { - PathMatcher pm = FileSystems.getDefault() - .getPathMatcher("glob:" + normalizedDir); - matchers.add(path -> { - try { - return pm.matches(java.nio.file.Path.of(path)); - } catch (java.nio.file.InvalidPathException e) { - // A changed file whose path contains characters the - // platform refuses to parse (mostly a Windows-in-git - // edge case) can't match any glob — fail closed so - // the engine routes it through the unmapped bucket - // instead of pretending the glob bit. - return false; - } - }); - } else { - // Literal-prefix branch preserves the pre-v2 "boundary-aware - // prefix" semantics verbatim: leading-edge or /-bounded. - String prefix = normalizedDir.endsWith("/") ? normalizedDir : normalizedDir + "/"; - matchers.add(path -> - path.startsWith(prefix) || path.contains("/" + prefix)); - } - } - return List.copyOf(matchers); - } - - /** - * True if the string contains any character the JVM's - * {@code glob:} syntax treats as a metacharacter. Kept deliberately - * small: these four cover every pattern the README, Gradle docs, - * and user bug reports mention. Anything more exotic still routes - * through the literal-prefix branch and will fail closed (i.e. - * match nothing), which is safer than inferring glob intent from a - * stray character. - */ - private static boolean hasGlobMetachar(String s) { - for (int i = 0; i < s.length(); i++) { - char c = s.charAt(i); - if (c == '*' || c == '?' || c == '[' || c == '{') return true; - } - return false; + return OutOfScopeMatchers.matchesAny(filePath.replace('\\', '/'), matchers); } } diff --git a/affected-tests-core/src/test/java/io/affectedtests/core/discovery/ImplementationStrategyTest.java b/affected-tests-core/src/test/java/io/affectedtests/core/discovery/ImplementationStrategyTest.java index 7ab8d2b..63cdc58 100644 --- a/affected-tests-core/src/test/java/io/affectedtests/core/discovery/ImplementationStrategyTest.java +++ b/affected-tests-core/src/test/java/io/affectedtests/core/discovery/ImplementationStrategyTest.java @@ -71,6 +71,39 @@ void findsTestViaAstExtendsScanning() throws IOException { "Should find test for class that extends the changed class"); } + @Test + void findsTestForDefaultPrefixedImplementation() throws IOException { + // Regression: the config ships with implementationNaming = + // {"Impl", "Default"}, but the naming-convention loop used + // to append both tokens as suffixes — FooServiceImpl matched, + // FooServiceDefault did not because nobody writes + // FooServiceDefault; real Spring/Guice code writes + // DefaultFooService. The AST branch rescues the explicit + // "implements FooService" case, but diffs where the super + // type is inferred (generics-only declarations, or files + // JavaParser couldn't parse) silently missed the Default- + // prefixed impl. Lock in the prefix shape explicitly. + Path prodDir = tempDir.resolve("src/main/java/com/example"); + Files.createDirectories(prodDir); + Files.writeString(prodDir.resolve("FooService.java"), + "package com.example;\npublic interface FooService {}"); + // No "implements FooService" written — force the naming- + // convention branch to do the work alone. + Files.writeString(prodDir.resolve("DefaultFooService.java"), + "package com.example;\npublic class DefaultFooService {}"); + + Path testDir = tempDir.resolve("src/test/java/com/example"); + Files.createDirectories(testDir); + Files.writeString(testDir.resolve("DefaultFooServiceTest.java"), + "package com.example;\npublic class DefaultFooServiceTest {}"); + + Set result = strategy.discoverTests( + Set.of("com.example.FooService"), tempDir); + + assertTrue(result.contains("com.example.DefaultFooServiceTest"), + "Should find the DefaultFooServiceTest via the prefix-shape naming convention"); + } + @Test void returnsEmptyWhenDisabled() throws IOException { AffectedTestsConfig config = AffectedTestsConfig.builder() diff --git a/affected-tests-core/src/test/java/io/affectedtests/core/discovery/ProjectIndexTest.java b/affected-tests-core/src/test/java/io/affectedtests/core/discovery/ProjectIndexTest.java new file mode 100644 index 0000000..d3617ed --- /dev/null +++ b/affected-tests-core/src/test/java/io/affectedtests/core/discovery/ProjectIndexTest.java @@ -0,0 +1,91 @@ +package io.affectedtests.core.discovery; + +import io.affectedtests.core.config.AffectedTestsConfig; +import io.affectedtests.core.config.Mode; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; + +import static org.junit.jupiter.api.Assertions.*; + +/** + * The regression this suite locks in is the v1.9.14 bug where + * {@code outOfScopeTestDirs = ["api-test/**"]} filtered the diff-side + * view in {@link io.affectedtests.core.mapping.PathToClassMapper} but + * silently did nothing on the on-disk view in {@link ProjectIndex}. + * Net effect for the user: an MR that only touched + * {@code api-test/...} was correctly classified as out-of-scope, yet + * tests discovered under {@code api-test/src/test/java} still got + * dispatched because this class never dropped them. The fix is + * covered in production code by delegating to {@link + * io.affectedtests.core.mapping.OutOfScopeMatchers}; this suite + * verifies the dispatch map and file lists agree with that delegation. + */ +class ProjectIndexTest { + + @TempDir + Path projectDir; + + @Test + void globOutOfScopeTestDirDropsTestFqn() throws Exception { + // Two test classes: one under api-test/** (out of scope) and + // one under src/test/java (normal). The dispatch map must + // contain exactly the normal one. + writeJava(projectDir.resolve("api-test/src/test/java/com/example/ApiFoo.java"), + "package com.example; public class ApiFoo {}"); + writeJava(projectDir.resolve("src/test/java/com/example/FooTest.java"), + "package com.example; public class FooTest {}"); + + AffectedTestsConfig config = AffectedTestsConfig.builder() + .mode(Mode.CI) + .testDirs(List.of("src/test/java", "api-test/src/test/java")) + .outOfScopeTestDirs(List.of("api-test/**")) + .build(); + + ProjectIndex index = ProjectIndex.build(projectDir, config); + + assertTrue(index.testFqns().contains("com.example.FooTest"), + "Normal test FQN must survive the filter"); + assertFalse(index.testFqns().contains("com.example.ApiFoo"), + "Glob out-of-scope test dir must drop the api-test FQN on the on-disk side, " + + "not just the diff side — this is the bug the v1.9.14 --explain Hint " + + "was introduced to warn about."); + } + + @Test + void literalAndGlobFormsAgreeOnTheSameOutcome() throws Exception { + // Given an identical on-disk layout, writing the config as a + // literal prefix ("api-test") and as a glob ("api-test/**") + // must drop the same FQN. If these ever diverge again the + // user's only visible signal is a mysterious full test run + // after a pure api-test MR, so the lock-in matters. + writeJava(projectDir.resolve("api-test/src/test/java/com/example/ApiFoo.java"), + "package com.example; public class ApiFoo {}"); + writeJava(projectDir.resolve("src/test/java/com/example/FooTest.java"), + "package com.example; public class FooTest {}"); + + ProjectIndex literalIndex = ProjectIndex.build(projectDir, + AffectedTestsConfig.builder() + .mode(Mode.CI) + .testDirs(List.of("src/test/java", "api-test/src/test/java")) + .outOfScopeTestDirs(List.of("api-test")) + .build()); + ProjectIndex globIndex = ProjectIndex.build(projectDir, + AffectedTestsConfig.builder() + .mode(Mode.CI) + .testDirs(List.of("src/test/java", "api-test/src/test/java")) + .outOfScopeTestDirs(List.of("api-test/**")) + .build()); + + assertEquals(literalIndex.testFqns(), globIndex.testFqns(), + "Literal and glob forms of the same logical rule must produce the same index"); + } + + private static void writeJava(Path target, String body) throws Exception { + Files.createDirectories(target.getParent()); + Files.writeString(target, body); + } +} diff --git a/affected-tests-core/src/test/java/io/affectedtests/core/git/GitChangeDetectorTest.java b/affected-tests-core/src/test/java/io/affectedtests/core/git/GitChangeDetectorTest.java index e189f04..04cc53c 100644 --- a/affected-tests-core/src/test/java/io/affectedtests/core/git/GitChangeDetectorTest.java +++ b/affected-tests-core/src/test/java/io/affectedtests/core/git/GitChangeDetectorTest.java @@ -145,6 +145,44 @@ void detectsUncommittedChanges() throws Exception { } } + @Test + void detectsDeletedFilesByOldPath() throws Exception { + // Regression: before this fix, a pure `git rm` MR produced zero + // entries in the diff (because DELETE's newPath is /dev/null and + // the detector only recorded newPath). That routed the whole MR + // through EMPTY_DIFF, which in LOCAL/CI modes silently skips all + // tests — the exact "run more, never less" violation the plugin + // is designed to prevent. Surfacing the old path lets the engine + // bucket the deletion correctly: ignored globs still ignore it, + // out-of-scope dirs still ignore it, and production-code deletes + // reach the transitive strategy like any other change. + try (Git git = initRepoWithInitialCommit()) { + File doomed = tempDir.resolve("src/main/java/com/example/Doomed.java").toFile(); + doomed.getParentFile().mkdirs(); + Files.writeString(doomed.toPath(), "package com.example;\npublic class Doomed {}"); + git.add().addFilepattern(".").call(); + git.commit().setMessage("add Doomed").call(); + + String baseCommit = git.log().call().iterator().next().getName(); + + // Delete the file on a follow-up commit + git.rm().addFilepattern("src/main/java/com/example/Doomed.java").call(); + git.commit().setMessage("remove Doomed").call(); + + AffectedTestsConfig config = AffectedTestsConfig.builder() + .baseRef(baseCommit) + .includeUncommitted(false) + .includeStaged(false) + .build(); + + GitChangeDetector detector = new GitChangeDetector(tempDir, config); + Set changed = detector.detectChangedFiles(); + + assertTrue(changed.contains("src/main/java/com/example/Doomed.java"), + "Deleted file must surface through its old path so the engine can bucket it"); + } + } + @Test void failsLoudlyOnNonGitDirectory() { Path nonGitDir = tempDir.resolve("not-a-repo"); diff --git a/affected-tests-core/src/test/java/io/affectedtests/core/mapping/OutOfScopeMatchersTest.java b/affected-tests-core/src/test/java/io/affectedtests/core/mapping/OutOfScopeMatchersTest.java new file mode 100644 index 0000000..be916eb --- /dev/null +++ b/affected-tests-core/src/test/java/io/affectedtests/core/mapping/OutOfScopeMatchersTest.java @@ -0,0 +1,110 @@ +package io.affectedtests.core.mapping; + +import org.junit.jupiter.api.Test; + +import java.util.List; +import java.util.function.Predicate; + +import static org.junit.jupiter.api.Assertions.*; + +/** + * Locks in the shared out-of-scope matcher compilation so + * {@link PathToClassMapper} and + * {@link io.affectedtests.core.discovery.ProjectIndex} can never drift + * again. Before this extraction the two classes disagreed on glob + * entries ({@code api-test/**}) and the user-visible outcome was that + * a mixed diff still dispatched api-test tests — the exact hole the + * v1.9.14 {@code --explain} Hint was introduced to warn about. + */ +class OutOfScopeMatchersTest { + + @Test + void literalEntryUsesBoundaryAwarePrefix() { + List> matchers = OutOfScopeMatchers.compile( + List.of("api-test"), "outOfScopeTestDirs"); + + assertTrue(OutOfScopeMatchers.matchesAny("api-test/src/test/java/Foo.java", matchers)); + assertTrue(OutOfScopeMatchers.matchesAny("modules/api-test/src/test/java/Foo.java", matchers)); + assertFalse(OutOfScopeMatchers.matchesAny("api-test-utils/src/test/java/Foo.java", matchers), + "Literal 'api-test' must not claim 'api-test-utils' — boundary semantics are the " + + "whole reason we don't just call String.contains."); + } + + @Test + void globEntryMatchesNestedPaths() { + List> matchers = OutOfScopeMatchers.compile( + List.of("api-test/**"), "outOfScopeTestDirs"); + + assertTrue(OutOfScopeMatchers.matchesAny("api-test/src/test/java/Foo.java", matchers), + "api-test/** must match a file several segments deep"); + assertFalse(OutOfScopeMatchers.matchesAny("api-test-utils/src/test/java/Foo.java", matchers)); + } + + @Test + void mixedEntriesCoexistInOneList() { + List> matchers = OutOfScopeMatchers.compile( + List.of("api-test", "performance-test/**"), "outOfScopeTestDirs"); + + assertTrue(OutOfScopeMatchers.matchesAny("api-test/src/test/java/Foo.java", matchers)); + assertTrue(OutOfScopeMatchers.matchesAny("performance-test/src/test/java/Foo.java", matchers)); + assertFalse(OutOfScopeMatchers.matchesAny("src/test/java/Foo.java", matchers)); + } + + @Test + void blankAndNullEntriesAreDroppedQuietly() { + // A mis-concatenated list literal in build.gradle is not worth + // failing a build over — worst case the user sees zero matches + // and --explain's Hint kicks in. + List> matchers = OutOfScopeMatchers.compile( + java.util.Arrays.asList("api-test/**", "", null, " "), + "outOfScopeTestDirs"); + + assertEquals(1, matchers.size(), "Blank and null entries must not yield a no-op matcher"); + assertTrue(OutOfScopeMatchers.matchesAny("api-test/src/test/java/Foo.java", matchers)); + } + + @Test + void malformedGlobFailsAtConfigTimeWithConfigKeyAndIndex() { + // The JVM's raw PatternSyntaxException ("Unclosed character + // class near index 4") doesn't say which config key or which + // index is wrong — by the time the user sees it they're deep + // in the engine stack trace. Surface the context here so they + // can fix build.gradle without grepping. + List badDirs = List.of("api-test/**", "**[unclosed"); + + IllegalStateException ise = assertThrows( + IllegalStateException.class, + () -> OutOfScopeMatchers.compile(badDirs, "outOfScopeTestDirs")); + + String msg = ise.getMessage(); + assertTrue(msg.contains("outOfScopeTestDirs"), + "Error must name the config key so users know where to look. Got: " + msg); + assertTrue(msg.contains("[1]"), + "Error must name the index so users know which list entry failed. Got: " + msg); + assertTrue(msg.contains("**[unclosed"), + "Error must include the offending pattern. Got: " + msg); + } + + @Test + void globMatcherFailsClosedOnInvalidPathInput() { + // A Linux-committed filename arriving on a Windows CI runner + // can fail Path.of with InvalidPathException (colon, pipe, + // null byte, reserved device names). The glob branch must + // not bubble that exception up and kill the whole task — + // it must fail closed so the file routes through the + // unmapped bucket and the safety net takes over. + List> matchers = OutOfScopeMatchers.compile( + List.of("api-test/**"), "outOfScopeTestDirs"); + + String badPath = "api-test/foo\0bar.java"; // NUL is illegal on every JVM + + assertFalse(OutOfScopeMatchers.matchesAny(badPath, matchers), + "InvalidPathException must be swallowed so the engine keeps running"); + } + + @Test + void emptyAndNullListsYieldEmptyMatchers() { + assertTrue(OutOfScopeMatchers.compile(List.of(), "outOfScopeTestDirs").isEmpty()); + assertTrue(OutOfScopeMatchers.compile(null, "outOfScopeTestDirs").isEmpty()); + } +} diff --git a/affected-tests-gradle/src/main/java/io/affectedtests/gradle/AffectedTestTask.java b/affected-tests-gradle/src/main/java/io/affectedtests/gradle/AffectedTestTask.java index 4b4e59d..4cedf42 100644 --- a/affected-tests-gradle/src/main/java/io/affectedtests/gradle/AffectedTestTask.java +++ b/affected-tests-gradle/src/main/java/io/affectedtests/gradle/AffectedTestTask.java @@ -33,6 +33,7 @@ import java.util.Map; import java.util.Objects; import java.util.Set; +import java.util.regex.Pattern; import java.util.stream.Collectors; /** @@ -446,18 +447,26 @@ private AffectedTestsConfig buildConfig() { return builder.build(); } - private static Mode parseMode(String raw) { + // Package-private so AffectedTestTaskLocaleTest can lock in the + // Locale.ROOT contract — the Turkish-locale failure this guards + // against is not reachable from any Gradle-level test shape, so + // direct unit coverage is the only line of defence. + static Mode parseMode(String raw) { try { - return Mode.valueOf(raw.trim().toUpperCase()); + // Locale.ROOT is mandatory here: Turkish-locale JVMs turn + // "ci".toUpperCase() into "Cİ" (U+0130) and the enum lookup + // then fails with a misleading "Unknown mode 'ci'" error + // even though the spelling is literally in the valid list. + return Mode.valueOf(raw.trim().toUpperCase(Locale.ROOT)); } catch (IllegalArgumentException e) { throw new GradleException("Unknown affectedTests.mode '" + raw + "'. Expected one of: auto, local, ci, strict.", e); } } - private static Action parseAction(String raw, String property) { + static Action parseAction(String raw, String property) { try { - return Action.valueOf(raw.trim().toUpperCase()); + return Action.valueOf(raw.trim().toUpperCase(Locale.ROOT)); } catch (IllegalArgumentException e) { throw new GradleException("Unknown " + property + " action '" + raw + "'. Expected one of: selected, full_suite, skipped.", e); @@ -499,10 +508,35 @@ private void executeTests(Path projectDir, String taskPath = modulePath.isEmpty() ? "test" : modulePath + ":test"; args.add(taskPath); + // One module-level lifecycle line; the per-FQN entries + // are info-level because they can legitimately run into + // the thousands on a widely-used utility change and + // spamming lifecycle can blow past CI log-size caps + // (GitHub Actions truncates at 4 MiB/step) before the + // nested test output even starts. + getLogger().lifecycle(" {} ({} test class{})", + taskPath, fqnsForModule.size(), fqnsForModule.size() == 1 ? "" : "es"); for (String fqn : fqnsForModule) { + if (!isValidFqn(fqn)) { + // Defence in depth against a compromised source tree + // sneaking shell-like tokens into a --tests argument. + // Gradle's CLI parser currently treats the next argv + // element as the value of --tests regardless of + // content, but that's an undocumented contract and a + // future parser change would turn this into real + // argument injection. Non-Java-shaped FQNs cannot + // correspond to a real JVM test class anyway, so + // dropping them costs nothing and forces the bad + // input to surface visibly. + getLogger().warn( + "Affected Tests: skipping malformed test FQN '{}' for task {} — " + + "not a Java-shaped identifier, cannot correspond to a " + + "real test class.", fqn, taskPath); + continue; + } args.add("--tests"); args.add(fqn); - getLogger().lifecycle(" {} -> {}", taskPath, fqn); + getLogger().info(" {} -> {}", taskPath, fqn); } } } @@ -598,8 +632,30 @@ private String resolveGradleCommand(Path projectDir) { return isWindows() ? "gradle.bat" : "gradle"; } + /** + * Java-shaped identifier regex with {@code $} allowed inside names + * for inner classes ({@code com.example.Outer$Inner}) and a + * {@code .methodName} tail for Gradle's + * {@code --tests com.example.Foo.someMethod} syntax. Matches a + * strict superset of what a sane Java source file can declare. + * Anything outside this shape cannot correspond to a real test + * class and is dropped with a warning before reaching the nested + * Gradle invocation. + */ + private static final Pattern JAVA_FQN = + Pattern.compile("[A-Za-z_$][A-Za-z0-9_$]*(\\.[A-Za-z_$][A-Za-z0-9_$]*)*"); + + static boolean isValidFqn(String fqn) { + return fqn != null && !fqn.isEmpty() && JAVA_FQN.matcher(fqn).matches(); + } + private static boolean isWindows() { - return System.getProperty("os.name", "").toLowerCase().contains("win"); + // Locale.ROOT keeps "Windows" → "windows" on Turkish-locale JVMs; + // without it the dotted-i rules turn it into "wındows" and the + // contains("win") check misses, routing Windows runners down the + // non-Windows branch and picking 'gradlew' / 'gradle' where + // 'gradlew.bat' / 'gradle.bat' is required. + return System.getProperty("os.name", "").toLowerCase(Locale.ROOT).contains("win"); } /** diff --git a/affected-tests-gradle/src/main/java/io/affectedtests/gradle/AffectedTestsExtension.java b/affected-tests-gradle/src/main/java/io/affectedtests/gradle/AffectedTestsExtension.java index 0153bfc..bf17120 100644 --- a/affected-tests-gradle/src/main/java/io/affectedtests/gradle/AffectedTestsExtension.java +++ b/affected-tests-gradle/src/main/java/io/affectedtests/gradle/AffectedTestsExtension.java @@ -100,7 +100,9 @@ public abstract class AffectedTestsExtension { /** * How many levels of transitive dependencies to follow when the * {@code transitive} strategy is enabled. - * Default: {@code 2}. Range: 0–5. + * Default: {@code 4} — matches the depth most Java controller → + * service → repository chains actually reach in Modulr-shaped + * codebases while leaving one level of margin. Range: 0–5. * * @return the transitive depth property */ diff --git a/affected-tests-gradle/src/test/java/io/affectedtests/gradle/AffectedTestTaskFqnValidationTest.java b/affected-tests-gradle/src/test/java/io/affectedtests/gradle/AffectedTestTaskFqnValidationTest.java new file mode 100644 index 0000000..a6c1360 --- /dev/null +++ b/affected-tests-gradle/src/test/java/io/affectedtests/gradle/AffectedTestTaskFqnValidationTest.java @@ -0,0 +1,54 @@ +package io.affectedtests.gradle; + +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.*; + +/** + * Defense-in-depth for the path between discovery and + * {@code test --tests }. Discovery should only ever produce + * Java-shaped identifiers, but a mis-configured custom strategy or a + * parser bug could in principle yield a string with a shell + * metacharacter or whitespace. Before this guard, such a string would + * be appended verbatim to the {@code test} task's argv and either + * crash the runner with an obscure glob-expansion error or — on the + * bad-luck path — match no test and silently reduce coverage. + */ +class AffectedTestTaskFqnValidationTest { + + @Test + void acceptsStandardJavaFqns() { + assertTrue(AffectedTestTask.isValidFqn("com.example.FooTest")); + assertTrue(AffectedTestTask.isValidFqn("com.example.inner.Foo$BarTest")); + assertTrue(AffectedTestTask.isValidFqn("Simple")); + assertTrue(AffectedTestTask.isValidFqn("_weird.$Names")); + } + + @Test + void rejectsNullAndEmpty() { + assertFalse(AffectedTestTask.isValidFqn(null)); + assertFalse(AffectedTestTask.isValidFqn("")); + } + + @Test + void rejectsShellMetacharsAndWhitespace() { + // Each of these would be a real problem if passed to + // Gradle's --tests arg: the shell-glob ones would either + // expand on some OSes or silently match nothing on others; + // whitespace in an FQN cannot correspond to a real Java + // class; the hyphen reaches the JUnit filter and quietly + // matches zero classes. + assertFalse(AffectedTestTask.isValidFqn("com.example.*")); + assertFalse(AffectedTestTask.isValidFqn("com.example.Foo Test")); + assertFalse(AffectedTestTask.isValidFqn("com/example/Foo")); + assertFalse(AffectedTestTask.isValidFqn("com.example;DROP TABLE")); + assertFalse(AffectedTestTask.isValidFqn("com.example.foo-bar")); + } + + @Test + void rejectsLeadingDotOrTrailingDot() { + assertFalse(AffectedTestTask.isValidFqn(".LeadingDot")); + assertFalse(AffectedTestTask.isValidFqn("trailing.Dot.")); + assertFalse(AffectedTestTask.isValidFqn("double..dot")); + } +} diff --git a/affected-tests-gradle/src/test/java/io/affectedtests/gradle/AffectedTestTaskLocaleTest.java b/affected-tests-gradle/src/test/java/io/affectedtests/gradle/AffectedTestTaskLocaleTest.java new file mode 100644 index 0000000..03e1906 --- /dev/null +++ b/affected-tests-gradle/src/test/java/io/affectedtests/gradle/AffectedTestTaskLocaleTest.java @@ -0,0 +1,56 @@ +package io.affectedtests.gradle; + +import io.affectedtests.core.config.Action; +import io.affectedtests.core.config.Mode; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import java.util.Locale; + +import static org.junit.jupiter.api.Assertions.*; + +/** + * Turkish-locale regression. Before {@link Locale#ROOT} was pinned on + * {@code toUpperCase()} inside the config parsers, {@code "ci"} turned + * into {@code "Cİ"} (U+0130) on a Turkish-locale JVM, enum lookup + * failed, and the user got a misleading "Unknown affectedTests.mode" + * error on config they had copy-pasted verbatim from the README. + * + *

Reproducing the original JDK bug requires actually flipping the + * default locale — a plain string comparison won't catch the regression + * because the production code is already calling {@code toUpperCase()}; + * the difference is only which collation table it consults. + */ +class AffectedTestTaskLocaleTest { + + private Locale originalLocale; + + @BeforeEach + void saveLocale() { + originalLocale = Locale.getDefault(); + } + + @AfterEach + void restoreLocale() { + Locale.setDefault(originalLocale); + } + + private static final Locale TURKISH = Locale.forLanguageTag("tr-TR"); + + @Test + void parseModeIsLocaleIndependent() { + Locale.setDefault(TURKISH); + assertEquals(Mode.CI, AffectedTestTask.parseMode("ci")); + assertEquals(Mode.LOCAL, AffectedTestTask.parseMode("local")); + assertEquals(Mode.STRICT, AffectedTestTask.parseMode("strict")); + } + + @Test + void parseActionIsLocaleIndependent() { + Locale.setDefault(TURKISH); + assertEquals(Action.SELECTED, AffectedTestTask.parseAction("selected", "onEmptyDiff")); + assertEquals(Action.FULL_SUITE, AffectedTestTask.parseAction("full_suite", "onEmptyDiff")); + assertEquals(Action.SKIPPED, AffectedTestTask.parseAction("skipped", "onEmptyDiff")); + } +}