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")); + } +}