From 7b78354c3d0ba6067fbc648017b646e406528aef Mon Sep 17 00:00:00 2001 From: vedanthvasudev Date: Wed, 22 Apr 2026 21:34:48 +0100 Subject: [PATCH] fix/correctness-silent-drops: close nine tier-1 silent-drop bugs on the path to v2.0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A post-v1.9.19 multi-agent review found a cluster of silent-drop bugs that all shared the same failure mode: a strategy thought it had walked a construct but had actually missed one of its real-world shapes, so the consumer test for a changed class never entered the selected set — breaking the plugin's cardinal invariant (run more tests than needed, never fewer). This release ships a fix for each one with a regression test that fails on revert. - JavaParsers factory centralises `JavaParser` creation at `LanguageLevel.JAVA_25` (highest stable level in the bundled 3.28 build). Previously the default `JAVA_11` made every file using records, sealed types, or pattern matching produce `isSuccessful == false`, silently removing the file from every AST strategy. `parseOrWarn` replaces the four independent `parseOrGet` helpers and surfaces parse failures at WARN instead of DEBUG, closing the invisibility that hid this bug class. - TransitiveStrategy now preserves generic type arguments and walks method bodies via `findAll(ClassOrInterfaceType.class)`, so `List` consumers and helper-inside-method-body shapes no longer lose their reverse-dependency edge. - ImplementationStrategy iterates every `TypeDeclaration` subtype, so `record UsdMoney(long cents) implements Money` and `enum Currency implements HasCode` are now first-class implementers. The new `supertypesOf` helper covers class/record/enum/annotation declarations, with a defensive WARN-logged default branch to make the next unknown subtype loud. - GitChangeDetector diffs against the merge-base between `baseRef` and `HEAD` (falling back to `baseId` with a WARN log when no common ancestor exists), so post-divergence master commits no longer leak into a feature branch's diff and inflate the affected-tests set toward "everything". - UsageStrategy adds a Tier 3 AST pass that catches fully-qualified inline references (`new com.example.Foo()`, `Outer.Inner` inline qualification), and the Tier 1b wildcard tier now treats `import pkg.Outer.*` as a class-member wildcard rather than a package wildcard — the shape that made every Outer.java change silently drop all wildcard consumers. - PathToClassMapper routes `module-info.java` and `package-info.java` to the unmapped bucket instead of producing malformed FQNs, letting the UNMAPPED_FILE safety net escalate JPMS and package-annotation changes conservatively. - AffectedTestsEngine explicitly routes diffs that are a mix of ignored + out-of-scope files to `Situation.ALL_FILES_OUT_OF_SCOPE` (SKIPPED) instead of falling through to `DISCOVERY_EMPTY`, which under `mode = CI` would previously escalate a pure docs+api-test MR into a full CI run. All regression tests cover revert-mutation shapes and `./gradlew check` is green end-to-end. --- CHANGELOG.md | 129 ++++++++++++++++++ .../core/AffectedTestsEngine.java | 25 ++++ .../discovery/ImplementationStrategy.java | 104 ++++++++++---- .../core/discovery/JavaParsers.java | 107 +++++++++++++++ .../core/discovery/ProjectIndex.java | 15 +- .../core/discovery/TransitiveStrategy.java | 66 ++++----- .../core/discovery/UsageStrategy.java | 80 +++++++++-- .../core/git/GitChangeDetector.java | 49 ++++++- .../core/mapping/PathToClassMapper.java | 25 ++++ .../core/AffectedTestsEngineTest.java | 51 +++++++ .../discovery/ImplementationStrategyTest.java | 69 ++++++++++ .../discovery/TransitiveStrategyTest.java | 79 +++++++++++ .../core/discovery/UsageStrategyTest.java | 103 ++++++++++++++ .../core/git/GitChangeDetectorTest.java | 61 +++++++++ .../core/mapping/PathToClassMapperTest.java | 47 +++++++ 15 files changed, 916 insertions(+), 94 deletions(-) create mode 100644 affected-tests-core/src/main/java/io/affectedtests/core/discovery/JavaParsers.java diff --git a/CHANGELOG.md b/CHANGELOG.md index ab392d1..edc0f39 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,135 @@ adheres to [Semantic Versioning](https://semver.org/). ## [Unreleased] +### Fixed — post-v1.9.19 multi-reviewer correctness pass (v1.9.20) + +A second, deeper multi-agent review of the post-v1.9.19 tree surfaced a +cluster of tier-1 silent-drop bugs that all shared the same failure +mode: the strategy *thought* it had walked a construct but had actually +missed one of its real-world shapes, so the consumer test for a changed +class never entered the selected set. Every fix below ships with a +regression test that fails when the fix is reverted. + +- **JavaParser no longer silently drops every file using Java 14+ + language features.** `new JavaParser()` defaults to + `LanguageLevel.JAVA_11` in JavaParser 3.28, which means every + compilation unit containing a record, sealed type, or pattern-matching + shape produced `ParseResult.isSuccessful() == false`. The four call + sites (`ProjectIndex`, `TransitiveStrategy`, `ImplementationStrategy`, + `UsageStrategy`) then discarded the result and the file contributed + nothing to reverse-dependency, implementation, or usage discovery. + Any modern Spring/DDD codebase leaning on value-object records would + silently drop every consumer test that depended on a record-shaped + production class. All four sites now route through a new + `JavaParsers.newParser()` factory that sets the level to `JAVA_25` + (the highest stable, non-preview level bundled with JavaParser 3.28, + which closes the same failure mode for adopters shipping Java + 22–25-era syntax). `JavaParsers.parseOrWarn` also replaces the four + independent `parseOrGet` helpers so parse failures now log at `WARN` + with the first JavaParser diagnostic — previously the `isSuccessful + == false` branch logged at `DEBUG`, which is the exact invisibility + that hid this bug class for the last eight releases. Regression + lives in `ImplementationStrategyTest` + (`findsRecordImplementationOfChangedInterface`, + `findsEnumImplementationOfChangedInterface`). +- **`TransitiveStrategy` now preserves generic type arguments when + building the reverse-dependency graph.** The old pipeline parsed + field and method-signature types as plain strings, normalised + `List` down to `List`, looked the base name up against + the known FQN set (stdlib import, not a project class), and dropped + the edge. Every consumer that wrapped the changed type in a + collection, `Optional`, `Flux`, or any other container lost its + reverse edge — and with it, all of its tests. The new scan walks + every `ClassOrInterfaceType` node in the AST, so the inner `FooService` + of `List` is a first-class reference. Regression: + `TransitiveStrategyTest#preservesGenericArgumentsInReverseDependencyEdges`. +- **`TransitiveStrategy` now scans method bodies, not just signatures.** + The same string-based approach skipped everything inside method + bodies. A helper referenced only through `new PricingCalculator()`, + a cast, or an `instanceof` had no reverse edge and its test was + silently dropped whenever the helper changed — the single most + common refactor shape on a service layer. Switching to + `findAll(ClassOrInterfaceType.class)` covers field types, signatures, + local variables, instantiations, casts, and pattern checks in one + pass. Regression: + `TransitiveStrategyTest#discoversEdgesFromMethodBodyReferences`. +- **`ImplementationStrategy` now recognises records and enums as + implementers.** The AST pass only iterated + `ClassOrInterfaceDeclaration`, so + `record UsdMoney(long cents) implements Money` and + `enum Currency implements HasCode` were invisible to the supertype + walk. Naming-convention matching does not rescue records (records + are named after the value they hold, not after the interface they + implement), so a change to the interface silently dropped the + record's test. The pass now finds every `TypeDeclaration` subtype + and the new `supertypesOf` helper extracts the correct + extends/implements list for classes, records, and enums. An explicit + `AnnotationDeclaration` branch and a defensive `WARN`-logged default + branch close the door on the same bug class silently reappearing + when JavaParser introduces a future declaration kind. Regressions + live in `ImplementationStrategyTest` (the same two tests above). +- **`GitChangeDetector` now diffs against the merge-base, not the tip + of `baseRef`.** The old `committedChanges` path diffed HEAD directly + against `baseId`. If master moved on after the branch diverged — + which on any busy repo means "always" — every post-divergence + master-only commit showed up as a "change" on the feature branch, + inflating the affected-tests set toward "everything" and destroying + the whole point of selective testing. The new path computes the + merge-base via `RevFilter.MERGE_BASE` and diffs against that; on the + pathological case of no common ancestor it falls back to the + `baseId` tip so the previous behaviour remains available and now + surfaces as a `WARN` (was `DEBUG`) so operators can see the semantic + reversion on grafted or subtree-merged histories. Regression: + `GitChangeDetectorTest#diffsAgainstMergeBaseNotBaseRefTip`. +- **`UsageStrategy` now catches fully-qualified inline references.** A + test that skipped the import and wrote + `com.example.service.Thing t = new com.example.service.Thing();` + slid through every tier of the old matcher — no import, no + wildcard, different package, and the simple-name AST scan would + only fire if the test lived in the same package. A new "Tier 3" + walks every `ClassOrInterfaceType`, reads + `getNameWithScope()` via a `nameWithScopeOrNull` helper, and matches + the reconstituted dotted name against the changed-FQN set. The + same tier also catches inline references to inner classes of a + changed outer. Regressions: + `UsageStrategyTest#findsTestThatUsesChangedClassByFullyQualifiedInlineReference`, + `UsageStrategyTest#findsTestThatInnerClassQualifiesThroughChangedOuter`. +- **`UsageStrategy` now treats `import pkg.Outer.*` as a dependency on + `pkg.Outer`.** The old wildcard tier bucketed `pkg.Outer` as a + package wildcard (since it ended in `.*`) and then checked whether + the AST referenced the bare name `Outer` — which test code using the + wildcard almost never writes, because the whole point of the + wildcard is to skip qualification. Any `Outer.java` change that + shipped without touching its own imports therefore silently dropped + every consumer relying on the class-member wildcard. The matcher + now recognises class-member wildcards by direct FQN equality against + the changed set. Regression: + `UsageStrategyTest#findsTestThatWildcardsClassMembersOfChangedClass`. +- **`module-info.java` and `package-info.java` now route to the + unmapped bucket.** The old mapper produced `module-info` / + `com.example.package-info` as "production FQNs", poisoning every + downstream strategy (they would try to derive a simple class name + from the FQN and match it against real source classes, which never + matched and silently skipped all the tests the descriptor change + could actually affect). Both marker files carry project-wide + semantics — JPMS visibility, package annotations — so the correct + conservative routing is to hand them to the `UNMAPPED_FILE` safety + net, which in CI mode escalates to a full suite. Regressions: + `PathToClassMapperTest#moduleInfoRoutesToUnmappedNotProduction`, + `PathToClassMapperTest#packageInfoRoutesToUnmappedNotProduction`. +- **Mixed ignored + out-of-scope diffs now route to + `ALL_FILES_OUT_OF_SCOPE` instead of falling through to + `DISCOVERY_EMPTY`.** An MR that combined a markdown tweak (ignored + by default globs) with an `api-test/` change (out-of-scope under the + pilot config) previously landed nowhere. Neither bucket alone + matched the whole diff, so the engine dropped through to mapping, + produced empty production/test sets, and hit `DISCOVERY_EMPTY` — + which under CI mode defaults to `FULL_SUITE`. That is exactly the + "quietly escalate a no-op MR into a full CI run" shape v2 was built + to prevent. The engine now explicitly routes the union case to + `ALL_FILES_OUT_OF_SCOPE`, which defaults to `SKIPPED`. Regression: + `AffectedTestsEngineTest#mixedIgnoredAndOutOfScopeDiffRoutesToAllFilesOutOfScope`. + ### Fixed — post-v1.9.18 code-review pass A four-reviewer sweep (correctness, security, maintainability, simplicity) diff --git a/affected-tests-core/src/main/java/io/affectedtests/core/AffectedTestsEngine.java b/affected-tests-core/src/main/java/io/affectedtests/core/AffectedTestsEngine.java index 6b7afa2..8b212b4 100644 --- a/affected-tests-core/src/main/java/io/affectedtests/core/AffectedTestsEngine.java +++ b/affected-tests-core/src/main/java/io/affectedtests/core/AffectedTestsEngine.java @@ -223,6 +223,31 @@ public AffectedTestsResult run() { return resolveAmbiguous(Situation.ALL_FILES_OUT_OF_SCOPE, changedFiles, mapping.productionClasses(), mapping.testClasses(), buckets); } + // Mixed "nothing actionable" case: every file is either ignored + // or out-of-scope, but no single bucket owns the whole diff. The + // two pure-bucket checks above already short-circuited if any + // unmapped / production / test file existed, so reaching here + // with both ignored > 0 AND outOfScope > 0 means the diff + // contains exactly those two categories and nothing else. + // + // Pre-fix, this slid through to discovery with empty + // {@code productionClasses} and {@code testClasses}, every + // strategy returned empty, and the engine escalated via + // {@link Situation#DISCOVERY_EMPTY} — which under the default + // {@code runAllIfNoMatches=true} routed to {@code FULL_SUITE}. + // The result: a pure "docs + api-test" MR ran the whole unit + // suite despite the user having told the plugin "these dirs + // don't influence tests". Routing to {@link Situation#ALL_FILES_OUT_OF_SCOPE} + // instead lets {@code onAllFilesOutOfScope} (the stronger + // operator signal when both signals disagree) decide, which + // defaults to {@link Action#SKIPPED} in every built-in mode. + if (ignored + outOfScope == diffSize) { + log.info("All {} changed file(s) split between ignored ({}) and out-of-scope ({}); " + + "routing to ALL_FILES_OUT_OF_SCOPE.", + diffSize, ignored, outOfScope); + return resolveAmbiguous(Situation.ALL_FILES_OUT_OF_SCOPE, changedFiles, + mapping.productionClasses(), mapping.testClasses(), buckets); + } if (!mapping.unmappedChangedFiles().isEmpty()) { Action action = config.actionFor(Situation.UNMAPPED_FILE); // Filenames flow from the untrusted MR tree straight into the 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 44c8516..b562ba9 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 @@ -1,9 +1,11 @@ package io.affectedtests.core.discovery; import com.github.javaparser.JavaParser; -import com.github.javaparser.ParseResult; import com.github.javaparser.ast.CompilationUnit; import com.github.javaparser.ast.body.ClassOrInterfaceDeclaration; +import com.github.javaparser.ast.body.EnumDeclaration; +import com.github.javaparser.ast.body.RecordDeclaration; +import com.github.javaparser.ast.body.TypeDeclaration; import com.github.javaparser.ast.type.ClassOrInterfaceType; import io.affectedtests.core.config.AffectedTestsConfig; import org.slf4j.Logger; @@ -145,7 +147,7 @@ private Set findImplementations(Set changedClasses, // pass adds nothing new, or when depth hits the configured // transitiveDepth (reused as a sanity bound — in practice Java // hierarchies are 2-3 deep). - JavaParser fallbackParser = (index == null) ? new JavaParser() : null; + JavaParser fallbackParser = (index == null) ? JavaParsers.newParser() : null; // Deep-copy the inner sets so the fixpoint loop's subsequent // `.add(implFqn)` calls don't leak mutations into @@ -161,10 +163,29 @@ private Set findImplementations(Set changedClasses, CompilationUnit cu = parseOrGet(sourceFile, index, fallbackParser); if (cu == null) continue; - List declarations = - cu.findAll(ClassOrInterfaceDeclaration.class); + // Iterate every TypeDeclaration — not just + // ClassOrInterfaceDeclaration. Records (Java 16+) and + // enums (Java 5+) can both implement interfaces: + // + // record UsdMoney(long cents) implements Money { ... } + // enum Currency implements HasCode { USD, EUR, ... } + // + // The old loop only scanned classes/interfaces, so a + // service interface with a record-valued consumer + // (increasingly common in value-object-heavy codebases) + // saw its consumer silently dropped on interface + // changes — the one failure mode this strategy exists + // to prevent. + // JavaParser's findAll(Class) typing forces a raw + // TypeDeclaration here because TypeDeclaration is + // generic in its self-type. The supertypesOf helper + // below re-narrows each declaration via pattern- + // matching instanceof, so the raw iteration is safe. + @SuppressWarnings({"rawtypes", "unchecked"}) + List> declarations = + (List>) (List) cu.findAll(TypeDeclaration.class); - for (ClassOrInterfaceDeclaration decl : declarations) { + for (TypeDeclaration decl : declarations) { String implFqn = extractFqn(cu, decl); if (implFqn == null || implementations.contains(implFqn)) { // Already known — skip. `implementations.contains` @@ -172,20 +193,12 @@ private Set findImplementations(Set changedClasses, // in the set it stops seeding further passes. continue; } - for (ClassOrInterfaceType extended : decl.getExtendedTypes()) { - if (targetsBySimpleName.containsKey(extended.getNameAsString())) { + for (ClassOrInterfaceType supertype : supertypesOf(decl)) { + if (targetsBySimpleName.containsKey(supertype.getNameAsString())) { newImpls.add(implFqn); - log.debug("[impl] depth {}: {} extends {}", - depth + 1, implFqn, extended.getNameAsString()); - break; - } - } - if (newImpls.contains(implFqn)) continue; - for (ClassOrInterfaceType implemented : decl.getImplementedTypes()) { - if (targetsBySimpleName.containsKey(implemented.getNameAsString())) { - newImpls.add(implFqn); - log.debug("[impl] depth {}: {} implements {}", - depth + 1, implFqn, implemented.getNameAsString()); + log.debug("[impl] depth {}: {} ({}) extends/implements {}", + depth + 1, implFqn, decl.getClass().getSimpleName(), + supertype.getNameAsString()); break; } } @@ -216,15 +229,7 @@ private CompilationUnit parseOrGet(Path file, ProjectIndex index, JavaParser fal if (index != null) { return index.compilationUnit(file); } - try { - ParseResult result = fallbackParser.parse(file); - if (result.isSuccessful() && result.getResult().isPresent()) { - return result.getResult().get(); - } - } catch (Exception e) { - log.debug("Error parsing source file {}: {}", file, e.getMessage()); - } - return null; + return JavaParsers.parseOrWarn(fallbackParser, file, "impl"); } /** @@ -240,11 +245,54 @@ private Set collectSourceFqns(Path projectDir) { return fqns; } - private String extractFqn(CompilationUnit cu, ClassOrInterfaceDeclaration decl) { + private String extractFqn(CompilationUnit cu, TypeDeclaration decl) { String pkg = cu.getPackageDeclaration() .map(pd -> pd.getNameAsString()) .orElse(""); String name = decl.getNameAsString(); return pkg.isEmpty() ? name : pkg + "." + name; } + + /** + * Returns the combined extends + implements list for any + * {@link TypeDeclaration} shape we care about. + * + *

Records and annotations cannot {@code extends} another named + * type (records implicitly extend {@code java.lang.Record}, + * annotations implicitly extend {@code java.lang.annotation.Annotation}), + * and {@link EnumDeclaration} enumerates its constants rather than a + * superclass. For those shapes only {@code implements} contributes + * supertype edges. + */ + private static List supertypesOf(TypeDeclaration decl) { + List result = new ArrayList<>(); + if (decl instanceof ClassOrInterfaceDeclaration c) { + result.addAll(c.getExtendedTypes()); + result.addAll(c.getImplementedTypes()); + } else if (decl instanceof RecordDeclaration r) { + result.addAll(r.getImplementedTypes()); + } else if (decl instanceof EnumDeclaration e) { + result.addAll(e.getImplementedTypes()); + } else if (decl instanceof com.github.javaparser.ast.body.AnnotationDeclaration) { + // Annotations can't widen their implicit supertype + // (java.lang.annotation.Annotation). Empty list is the + // correct answer — not a drop. + return result; + } else { + // Defensive: when a future JavaParser release introduces a + // new TypeDeclaration subtype we want the new construct to + // be *loud* rather than silently dropping every consumer + // test that depended on it (the exact failure mode records + // caused before batch 4 fixed them). WARN surfaces at the + // default log level so operators can open a ticket; the + // strategy keeps running and treats the type as having no + // known supertypes, which is the correct conservative + // fallback (cannot become a false impl match; at worst we + // underselect for a single declaration). + log.warn("Affected Tests: [impl] unknown TypeDeclaration subtype {} for {} — " + + "supertype edges cannot be derived, skipping", + decl.getClass().getSimpleName(), decl.getNameAsString()); + } + return result; + } } diff --git a/affected-tests-core/src/main/java/io/affectedtests/core/discovery/JavaParsers.java b/affected-tests-core/src/main/java/io/affectedtests/core/discovery/JavaParsers.java new file mode 100644 index 0000000..43ba06c --- /dev/null +++ b/affected-tests-core/src/main/java/io/affectedtests/core/discovery/JavaParsers.java @@ -0,0 +1,107 @@ +package io.affectedtests.core.discovery; + +import com.github.javaparser.JavaParser; +import com.github.javaparser.ParseResult; +import com.github.javaparser.ParserConfiguration; +import com.github.javaparser.ParserConfiguration.LanguageLevel; +import com.github.javaparser.ast.CompilationUnit; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.nio.file.Path; + +/** + * Central factory for {@link JavaParser} instances used across the discovery + * strategies and {@link ProjectIndex}. + * + *

Motivating bug: JavaParser 3.28's default {@link LanguageLevel} is + * {@code JAVA_11}. Every strategy that instantiated {@code new JavaParser()} + * silently failed to parse any file using records, sealed types, or pattern + * matching — the parser reported {@code isSuccessful() == false} for the + * whole compilation unit, every call site then discarded the result, and the + * file contributed nothing to Transitive/Implementation/Usage discovery. Any + * consumer test whose only path to a changed class went through a record + * (e.g. {@code record UsdMoney(long cents) implements Money}) was silently + * dropped on every MR. + * + *

Setting the level to {@link LanguageLevel#JAVA_21} (or the highest + * stable level the parser understands) makes all stable Java language + * features parse cleanly. Language levels newer than the parser's build + * produce an {@code IllegalArgumentException}, which is preferable to the + * silent-drop failure mode we had before. + */ +final class JavaParsers { + + /** + * Highest stable (non-preview) language level supported by the bundled + * JavaParser build (3.28.0 ships {@code JAVA_1 … JAVA_25} + {@code + * BLEEDING_EDGE}; {@code JAVA_25} is the newest stable constant). Kept + * in one place so when the parser dependency is bumped we only move + * this constant up. + * + *

We deliberately avoid {@code BLEEDING_EDGE}: it lets preview + * syntax leak in, which is both slower to parse and more likely to + * behave inconsistently across JavaParser point releases. Every stable + * level up to {@link LanguageLevel#JAVA_25} is a strict syntactic + * superset of older levels, so setting the constant at the top still + * parses legacy projects cleanly. + */ + static final LanguageLevel LANGUAGE_LEVEL = LanguageLevel.JAVA_25; + + private static final Logger log = LoggerFactory.getLogger(JavaParsers.class); + + private JavaParsers() { + } + + /** + * Creates a new {@link JavaParser} configured with the plugin-wide + * language level. Use this in every place a parser is constructed; do + * not call {@code new JavaParser()} directly. + */ + static JavaParser newParser() { + ParserConfiguration config = new ParserConfiguration() + .setLanguageLevel(LANGUAGE_LEVEL); + return new JavaParser(config); + } + + /** + * Parses {@code file} with {@code parser} and returns the resulting + * {@link CompilationUnit}, or {@code null} if the file could not be + * parsed. Emits a single {@code WARN} line on unsuccessful parses so + * that the parse-failure silent-drop class of bug — a single file + * unparseable at {@link #LANGUAGE_LEVEL} silently removes itself from + * discovery, taking every test that depended on it along — is visible + * at the plugin's default log level instead of hiding under + * {@code DEBUG} the way the pre-v1.9.20 call sites did. + * + *

Label is prepended to the log line so the operator can see which + * discovery phase failed to parse the file (e.g. {@code transitive}, + * {@code impl}, {@code usage}, {@code index}). + */ + static CompilationUnit parseOrWarn(JavaParser parser, Path file, String label) { + try { + ParseResult result = parser.parse(file); + if (result.isSuccessful() && result.getResult().isPresent()) { + return result.getResult().get(); + } + // isSuccessful == false → JavaParser produced a result but + // flagged one or more problems. Most often on Java-version + // mismatches and partial source. Surface the first problem + // so operators can tell whether the file needs a dependency + // bump or is genuinely malformed. + String firstProblem = result.getProblems().isEmpty() + ? "no diagnostics" + : result.getProblems().get(0).getMessage(); + log.warn("Affected Tests: [{}] failed to parse {} at language level {}: {}", + label, file, LANGUAGE_LEVEL, firstProblem); + return null; + } catch (Exception e) { + // Preserve the pre-v1.9.20 behaviour of degrading the + // exception path to DEBUG (I/O races, file-deleted-under- + // JGit, etc. are noisy on CI) while still surfacing the + // much-more-common isSuccessful==false branch above. + log.debug("Affected Tests: [{}] error parsing {}: {}", label, file, e.getMessage()); + return null; + } + } +} 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 c15610b..4095514 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 @@ -1,7 +1,6 @@ package io.affectedtests.core.discovery; import com.github.javaparser.JavaParser; -import com.github.javaparser.ParseResult; import com.github.javaparser.ast.CompilationUnit; import io.affectedtests.core.config.AffectedTestsConfig; import io.affectedtests.core.mapping.OutOfScopeMatchers; @@ -32,7 +31,7 @@ public final class ProjectIndex { // Lazy AST cache. null entries mean "parsed but invalid/empty". private final Map cuCache = new HashMap<>(); - private final JavaParser parser = new JavaParser(); + private final JavaParser parser = JavaParsers.newParser(); private ProjectIndex(List sourceFiles, List testFiles, Map testFqnToPath, Set sourceFqns) { @@ -155,17 +154,7 @@ public CompilationUnit compilationUnit(Path file) { if (cuCache.containsKey(file)) { return cuCache.get(file); } - CompilationUnit cu = null; - try { - ParseResult result = parser.parse(file); - if (result.isSuccessful() && result.getResult().isPresent()) { - cu = result.getResult().get(); - } else { - log.debug("Failed to parse {}", file); - } - } catch (Exception e) { - log.debug("Error parsing {}: {}", file, e.getMessage()); - } + CompilationUnit cu = JavaParsers.parseOrWarn(parser, file, "index"); cuCache.put(file, cu); return cu; } 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 3f20c89..fb16dab 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 @@ -1,13 +1,9 @@ package io.affectedtests.core.discovery; import com.github.javaparser.JavaParser; -import com.github.javaparser.ParseResult; import com.github.javaparser.ast.CompilationUnit; import com.github.javaparser.ast.ImportDeclaration; -import com.github.javaparser.ast.body.FieldDeclaration; -import com.github.javaparser.ast.body.MethodDeclaration; -import com.github.javaparser.ast.body.Parameter; -import com.github.javaparser.ast.body.VariableDeclarator; +import com.github.javaparser.ast.type.ClassOrInterfaceType; import io.affectedtests.core.config.AffectedTestsConfig; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -129,7 +125,7 @@ private Map> buildReverseDependencyMap(List sourceFile ProjectIndex index, Set extraKnownFqns) { Map> reverseMap = new HashMap<>(); - JavaParser fallbackParser = (index == null) ? new JavaParser() : null; + JavaParser fallbackParser = (index == null) ? JavaParsers.newParser() : null; // First pass: collect all known FQNs so we can resolve simple names. // `extraKnownFqns` is unioned in so reverse edges to deleted classes @@ -169,18 +165,31 @@ private Map> buildReverseDependencyMap(List sourceFile .map(pd -> pd.getNameAsString()) .orElse(""); + // Walk every ClassOrInterfaceType node in the compilation unit. + // This is deliberately broader than the old "fields + method + // signatures" scan it replaces: + // + // * Generics: `List` parses as ClassOrInterfaceType(List) + // with a type-argument ClassOrInterfaceType(Foo). The old + // code normalised the outer type name and threw the + // argument away — any consumer that wrapped the changed + // class in a container lost its reverse edge. + // * Method bodies: local declarations (`PricingCalculator c + // = new PricingCalculator()`), ObjectCreationExpr + // (`new Foo()`), cast expressions, and `instanceof` checks + // all show up as ClassOrInterfaceType nodes inside the + // body subtree. The old code only looked at field types + // and method signatures, so a helper class instantiated + // inside a method body had no reverse edge and its + // consumer's tests were silently dropped on changes. + // * extends/implements: also surface as ClassOrInterfaceType, + // giving us a correct supertype-aware reverse edge for + // free. Set referencedTypes = new LinkedHashSet<>(); - - for (FieldDeclaration field : cu.findAll(FieldDeclaration.class)) { - for (VariableDeclarator var : field.getVariables()) { - referencedTypes.add(normalizeTypeName(var.getTypeAsString())); - } - } - - for (MethodDeclaration method : cu.findAll(MethodDeclaration.class)) { - referencedTypes.add(normalizeTypeName(method.getTypeAsString())); - for (Parameter param : method.getParameters()) { - referencedTypes.add(normalizeTypeName(param.getTypeAsString())); + for (ClassOrInterfaceType t : cu.findAll(ClassOrInterfaceType.class)) { + String simpleName = t.getNameAsString(); + if (!simpleName.isEmpty()) { + referencedTypes.add(simpleName); } } @@ -219,28 +228,7 @@ private CompilationUnit parseOrGet(Path file, ProjectIndex index, JavaParser fal if (index != null) { return index.compilationUnit(file); } - try { - ParseResult result = fallbackParser.parse(file); - if (result.isSuccessful() && result.getResult().isPresent()) { - return result.getResult().get(); - } - } catch (Exception e) { - log.debug("Error parsing {} for dependency map: {}", file, e.getMessage()); - } - return null; - } - - /** - * Strips generic parameters and array brackets from a type name so - * {@code List} and {@code Foo[]} both normalise to {@code Foo}. - */ - static String normalizeTypeName(String typeName) { - int idx = typeName.indexOf('<'); - String base = idx >= 0 ? typeName.substring(0, idx) : typeName; - while (base.endsWith("[]")) { - base = base.substring(0, base.length() - 2); - } - return base.trim(); + return JavaParsers.parseOrWarn(fallbackParser, file, "transitive"); } private String pathToFqn(Path file) { diff --git a/affected-tests-core/src/main/java/io/affectedtests/core/discovery/UsageStrategy.java b/affected-tests-core/src/main/java/io/affectedtests/core/discovery/UsageStrategy.java index f0d3172..ca0e36f 100644 --- a/affected-tests-core/src/main/java/io/affectedtests/core/discovery/UsageStrategy.java +++ b/affected-tests-core/src/main/java/io/affectedtests/core/discovery/UsageStrategy.java @@ -1,7 +1,6 @@ package io.affectedtests.core.discovery; import com.github.javaparser.JavaParser; -import com.github.javaparser.ParseResult; import com.github.javaparser.ast.CompilationUnit; import com.github.javaparser.ast.ImportDeclaration; import com.github.javaparser.ast.body.FieldDeclaration; @@ -80,7 +79,7 @@ private Set scanTestFiles(Set changedProductionClasses, simpleNames.add(simpleName); } - JavaParser fallbackParser = (index == null) ? new JavaParser() : null; + JavaParser fallbackParser = (index == null) ? JavaParsers.newParser() : null; for (Path testFile : testFiles) { CompilationUnit cu = parseOrGet(testFile, index, fallbackParser); @@ -106,15 +105,7 @@ private CompilationUnit parseOrGet(Path file, ProjectIndex index, JavaParser fal if (index != null) { return index.compilationUnit(file); } - try { - ParseResult result = fallbackParser.parse(file); - if (result.isSuccessful() && result.getResult().isPresent()) { - return result.getResult().get(); - } - } catch (Exception e) { - log.debug("Error parsing {}: {}", file, e.getMessage()); - } - return null; + return JavaParsers.parseOrWarn(fallbackParser, file, "usage"); } /** @@ -173,13 +164,35 @@ private boolean testReferencesChangedClass(CompilationUnit cu, } } - // Tier 1b: Wildcard import match + // Tier 1b: Wildcard import match. Two shapes have to be handled: + // + // * `import com.example.service.*;` — a package wildcard; + // the test may reference any simple type inside that + // package. We gate on the simple name actually appearing in + // the AST so we don't over-select. + // + // * `import com.example.Outer.*;` — a class-member + // wildcard. Every member of `Outer` (including its nested + // types and public static members) is visible in the test + // without further qualification, so a change to + // `Outer.java` — which PathToClassMapper reports as a + // change to `com.example.Outer` — must pull the test in + // unconditionally; the test doesn't have to mention + // `Outer` by name at all. Pre-fix this case was bucketed + // as a package wildcard (`wildcardPackages` held + // "com.example.Outer") and the subsequent + // `typeNameAppearsInAst("Outer")` check almost always + // failed, silently dropping the consumer's coverage. for (String changedFqn : changedFqns) { + if (wildcardPackages.contains(changedFqn)) { + log.debug(" Wildcard class-member import match: {}", changedFqn); + return true; + } String pkg = SourceFileScanner.packageOf(changedFqn); if (wildcardPackages.contains(pkg)) { String simpleName = SourceFileScanner.simpleClassName(changedFqn); if (typeNameAppearsInAst(cu, simpleName)) { - log.debug(" Wildcard import + type ref match: {}", changedFqn); + log.debug(" Wildcard package import + type ref match: {}", changedFqn); return true; } } @@ -200,9 +213,50 @@ private boolean testReferencesChangedClass(CompilationUnit cu, } } + // Tier 3: Fully-qualified inline references that never went + // through an import. Catches + // `com.example.other.Thing t = new com.example.other.Thing();` + // `(com.example.other.Thing) x` + // `com.example.other.Thing.Inner nested = ...;` + // i.e. anything where the test author typed the full dotted + // name of the changed class at a use site. Walks every + // {@link ClassOrInterfaceType} node and reads its + // {@code getNameWithScope()} (which reconstitutes the dotted + // chain from the parent scope references), so we don't depend + // on raw source text or comments. The bare-name case is + // already handled by Tier 1 / 1b / 2; skip dotless hits here + // to avoid double-counting. + for (ClassOrInterfaceType type : cu.findAll(ClassOrInterfaceType.class)) { + String scoped = nameWithScopeOrNull(type); + if (scoped == null || !scoped.contains(".")) continue; + for (String changedFqn : changedFqns) { + if (scoped.equals(changedFqn) || scoped.startsWith(changedFqn + ".")) { + log.debug(" Inline fully-qualified reference: {} -> {}", scoped, changedFqn); + return true; + } + } + } + return false; } + /** + * Returns the dotted name of {@code type} including its enclosing + * scope chain, or {@code null} when JavaParser throws while + * reconstructing it (e.g. a partially-resolved type node in an + * invalid source file). Isolating the guard here means the caller + * never has to defensively wrap the AST walk — a best-effort null + * is enough to skip a single type node while the rest of the file + * still contributes to discovery. + */ + private static String nameWithScopeOrNull(ClassOrInterfaceType type) { + try { + return type.getNameWithScope(); + } catch (RuntimeException e) { + return null; + } + } + /** * Checks whether the given simple type name appears in the AST as a type reference. */ 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 1ce40ba..13f7f76 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 @@ -11,6 +11,7 @@ import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevWalk; +import org.eclipse.jgit.revwalk.filter.RevFilter; import org.eclipse.jgit.storage.file.FileRepositoryBuilder; import org.eclipse.jgit.treewalk.AbstractTreeIterator; import org.eclipse.jgit.treewalk.CanonicalTreeParser; @@ -132,7 +133,19 @@ private Set committedChanges(Repository repository, Git git) throws IOEx + "Ensure the ref exists (e.g. run 'git fetch origin' in CI)."); } - AbstractTreeIterator baseTree = prepareTreeParser(repository, baseId); + // Diff against the merge-base of baseRef and HEAD, not the tip + // of baseRef. The difference matters any time master has moved + // forward since the branch diverged: a tip-of-base diff treats + // every post-divergence file added on master as "reverted" on + // the feature branch, inflating the diff with paths the MR + // never touched. In a large monorepo this routinely puts + // hundreds of unrelated files into the mapper and — because + // many of them are `.java` under the configured source dirs — + // silently drags dozens of unrelated test classes into every + // MR's dispatch. Merge-base matches how `git diff master...HEAD` + // (three-dot form, CI-standard) computes the MR scope. + ObjectId mergeBaseId = mergeBaseOrBase(repository, baseId, headId); + AbstractTreeIterator baseTree = prepareTreeParser(repository, mergeBaseId); AbstractTreeIterator headTree = prepareTreeParser(repository, headId); List diffs = git.diff() @@ -144,6 +157,40 @@ private Set committedChanges(Repository repository, Git git) throws IOEx return files; } + /** + * Returns the merge-base of {@code baseId} and {@code headId}, or + * {@code baseId} as a safe fallback when no common ancestor exists + * (unrelated histories, root-commit branches). The fallback keeps + * behaviour identical to pre-v1.9.20 for the one genuinely + * pathological case where there is nothing to merge-base against. + */ + private static ObjectId mergeBaseOrBase(Repository repository, + ObjectId baseId, ObjectId headId) throws IOException { + try (RevWalk walk = new RevWalk(repository)) { + RevCommit baseCommit = walk.parseCommit(baseId); + RevCommit headCommit = walk.parseCommit(headId); + walk.setRevFilter(RevFilter.MERGE_BASE); + walk.markStart(baseCommit); + walk.markStart(headCommit); + RevCommit mergeBase = walk.next(); + if (mergeBase != null) { + log.debug("Resolved merge-base {} between baseRef and HEAD", + mergeBase.getId().name()); + return mergeBase.getId(); + } + // WARN (not DEBUG) because this is a correctness-equivalent + // but semantically different code path: the diff shape is + // whatever `baseRef tip vs HEAD` produces, which on grafted + // or subtree-merged histories can dump the entire baseRef + // tree. Operators need to see the escalation, not hunt for + // it at DEBUG. + log.warn("Affected Tests: no merge-base between baseRef and HEAD — " + + "falling back to baseRef tip (diff may be inflated; see " + + "grafted/subtree-merge histories). This mirrors pre-v1.9.20 behaviour."); + return baseId; + } + } + private Set uncommittedChanges(Git git) throws GitAPIException { Set files = new LinkedHashSet<>(); collectPaths(git.diff().call(), files); 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 ba39054..34b18d0 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 @@ -164,6 +164,25 @@ public MappingResult mapChangedFiles(Set changedFiles) { continue; } + // module-info.java (JPMS descriptor) and package-info.java + // (package-level annotations / docs) are not production + // classes: they have no instantiable type and no FQN that + // tests can reference, so tryMapToClass would either + // produce a non-FQN ("module-info") or a dotted form like + // "com.example.package-info" that poisons every downstream + // strategy that treats it as a class name. Both shapes are + // genuine changes the operator cares about — a package + // annotation can affect every test in the package, and a + // module-info change alters JPMS visibility — so route + // them to the unmapped bucket and let the UNMAPPED_FILE + // safety net decide (FULL_SUITE in CI mode by default). + String fileName = extractFileName(filePath); + if ("module-info.java".equals(fileName) || "package-info.java".equals(fileName)) { + log.debug("Java marker file ({}) flagged as unmapped: {}", fileName, filePath); + unmappedChangedFiles.add(filePath); + continue; + } + String testFqn = tryMapToClass(filePath, config.testDirs()); if (testFqn != null) { testClasses.add(testFqn); @@ -238,6 +257,12 @@ private String tryMapToClass(String filePath, java.util.List sourceDirs) return null; } + private static String extractFileName(String filePath) { + String normalized = filePath.replace('\\', '/'); + int idx = normalized.lastIndexOf('/'); + return idx < 0 ? normalized : normalized.substring(idx + 1); + } + private static boolean hasTraversalSegment(String filePath) { if (filePath == null || filePath.isEmpty()) { return false; diff --git a/affected-tests-core/src/test/java/io/affectedtests/core/AffectedTestsEngineTest.java b/affected-tests-core/src/test/java/io/affectedtests/core/AffectedTestsEngineTest.java index 5320444..a0f2d3d 100644 --- a/affected-tests-core/src/test/java/io/affectedtests/core/AffectedTestsEngineTest.java +++ b/affected-tests-core/src/test/java/io/affectedtests/core/AffectedTestsEngineTest.java @@ -446,6 +446,57 @@ void markdownOnlyDiffRoutesToAllFilesIgnoredAndSkips() throws Exception { } } + @Test + void mixedIgnoredAndOutOfScopeDiffRoutesToAllFilesOutOfScope() throws Exception { + // Regression for batch-4 finding: an MR that combined a + // markdown change (ignored by default globs) with an api-test + // change (out-of-scope under the pilot config) previously + // landed nowhere. Neither bucket alone matched the whole + // diff, so the engine dropped through to mapping → empty + // production/test sets → DISCOVERY_EMPTY, which in CI mode + // defaults to FULL_SUITE. That is exactly the "quietly escalate + // a no-op MR into a full CI run" shape v2 was built to prevent. + // The fix: when ignored + out-of-scope together cover the + // whole diff, route straight to ALL_FILES_OUT_OF_SCOPE (skip) + // just as either bucket would individually. + try (Git git = initRepoWithInitialCommit()) { + String base = git.log().call().iterator().next().getName(); + + // One ignored file (markdown) + one out-of-scope file + // (api-test). Neither bucket on its own matches the diff + // — only the union does. + Files.writeString(tempDir.resolve("docs.md"), "# docs"); + + Path apiTestDir = tempDir.resolve("api-test/src/test/java/com/example/api"); + Files.createDirectories(apiTestDir); + Files.writeString(apiTestDir.resolve("FooSteps.java"), + "package com.example.api;\npublic class FooSteps {}"); + + git.add().addFilepattern(".").call(); + git.commit().setMessage("mixed ignored+out-of-scope").call(); + + AffectedTestsConfig config = AffectedTestsConfig.builder() + .baseRef(base) + .includeUncommitted(false) + .includeStaged(false) + .outOfScopeTestDirs(java.util.List.of("api-test/src/test/java")) + .build(); + + AffectedTestsEngine engine = new AffectedTestsEngine(config, tempDir); + AffectedTestsEngine.AffectedTestsResult result = engine.run(); + + assertEquals(Situation.ALL_FILES_OUT_OF_SCOPE, result.situation(), + "Mixed ignored+out-of-scope must route to ALL_FILES_OUT_OF_SCOPE, " + + "not fall through to DISCOVERY_EMPTY"); + assertEquals(Action.SKIPPED, result.action(), + "Mixed ignored+out-of-scope must default to SKIPPED — otherwise " + + "markdown+api-test MRs silently kick off full CI runs"); + assertTrue(result.skipped()); + assertFalse(result.runAll()); + assertTrue(result.testClassFqns().isEmpty()); + } + } + @Test void modeCiEscalatesDiscoveryEmpty() throws Exception { // Mode.CI's DISCOVERY_EMPTY default is FULL_SUITE — a CI user who 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 fdaa8c4..28d21e4 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 @@ -132,6 +132,75 @@ void returnsEmptyWhenNoImplementations() throws IOException { assertTrue(result.isEmpty()); } + @Test + void findsRecordImplementationOfChangedInterface() throws IOException { + // Regression: records can implement interfaces, and modern + // value-object code leans heavily on them + // (`record UsdMoney(long cents) implements Money`). The old + // strategy only iterated ClassOrInterfaceDeclaration so + // records were invisible to the AST supertype pass — and + // since naming-convention matches like "MoneyImpl" don't fit + // record idioms either (records typically get named for the + // value they hold, not for the interface), the record's test + // was silently dropped on every change to the interface it + // implements. + Path prodDir = tempDir.resolve("src/main/java/com/example"); + Files.createDirectories(prodDir); + Files.writeString(prodDir.resolve("Money.java"), + "package com.example;\npublic interface Money { long cents(); }"); + Files.writeString(prodDir.resolve("UsdMoney.java"), """ + package com.example; + + public record UsdMoney(long cents) implements Money {} + """); + + Path testDir = tempDir.resolve("src/test/java/com/example"); + Files.createDirectories(testDir); + Files.writeString(testDir.resolve("UsdMoneyTest.java"), + "package com.example;\npublic class UsdMoneyTest {}"); + + Set result = strategy.discoverTests( + Set.of("com.example.Money"), tempDir); + + assertTrue(result.contains("com.example.UsdMoneyTest"), + "Record implementation's test must be discovered when the " + + "interface it implements changes"); + } + + @Test + void findsEnumImplementationOfChangedInterface() throws IOException { + // Same failure mode as the record case: enums frequently + // implement behaviour-carrying interfaces + // (`enum Currency implements HasCode`), and the old strategy + // skipped them entirely. An interface-level change that tweaks + // the contract for every enum constant should re-run the + // enum's test; pre-fix it quietly did not. + Path prodDir = tempDir.resolve("src/main/java/com/example"); + Files.createDirectories(prodDir); + Files.writeString(prodDir.resolve("HasCode.java"), + "package com.example;\npublic interface HasCode { String code(); }"); + Files.writeString(prodDir.resolve("Currency.java"), """ + package com.example; + + public enum Currency implements HasCode { + USD { public String code() { return "USD"; } }, + EUR { public String code() { return "EUR"; } }; + } + """); + + Path testDir = tempDir.resolve("src/test/java/com/example"); + Files.createDirectories(testDir); + Files.writeString(testDir.resolve("CurrencyTest.java"), + "package com.example;\npublic class CurrencyTest {}"); + + Set result = strategy.discoverTests( + Set.of("com.example.HasCode"), tempDir); + + assertTrue(result.contains("com.example.CurrencyTest"), + "Enum implementation's test must be discovered when the " + + "interface it implements changes"); + } + @Test void findsGrandchildImplementationThroughMultiLevelHierarchy() throws IOException { // Hierarchy: interface A <-- abstract class B implements A diff --git a/affected-tests-core/src/test/java/io/affectedtests/core/discovery/TransitiveStrategyTest.java b/affected-tests-core/src/test/java/io/affectedtests/core/discovery/TransitiveStrategyTest.java index 3dd3fdc..2a1be01 100644 --- a/affected-tests-core/src/test/java/io/affectedtests/core/discovery/TransitiveStrategyTest.java +++ b/affected-tests-core/src/test/java/io/affectedtests/core/discovery/TransitiveStrategyTest.java @@ -131,6 +131,85 @@ public class C { "Depth 2 should reach CTest"); } + @Test + void preservesGenericArgumentsInReverseDependencyEdges() throws IOException { + // Regression: the old normalize-then-match pipeline stripped + // `List` down to `List`, looked up `List` against + // the known FQN set (stdlib import, not a project class), and + // threw the edge away. Any consumer that wrapped the changed + // type in a container, Optional, Flux, Map, or any other + // generic lost its reverse-dependency edge — and with it, all + // of its tests. Collection-of-service is the single most + // common shape in Spring service layers, so this silently + // dropped a very large fraction of downstream coverage. + Path prodDir = tempDir.resolve("src/main/java/com/example"); + Files.createDirectories(prodDir); + Files.writeString(prodDir.resolve("FooService.java"), + "package com.example;\npublic class FooService {}"); + Files.writeString(prodDir.resolve("BarService.java"), """ + package com.example; + + import java.util.List; + + public class BarService { + private List fooServices; + } + """); + + Path testDir = tempDir.resolve("src/test/java/com/example"); + Files.createDirectories(testDir); + Files.writeString(testDir.resolve("BarServiceTest.java"), + "package com.example;\npublic class BarServiceTest {}"); + + Set result = strategy.discoverTests( + Set.of("com.example.FooService"), tempDir); + + assertTrue(result.contains("com.example.BarServiceTest"), + "Must treat `List` as a reverse edge to FooService — " + + "stripping generics silently drops every `List` " + + "consumer's tests"); + } + + @Test + void discoversEdgesFromMethodBodyReferences() throws IOException { + // Regression: the old scan looked at field types and method + // signatures only. A helper class referenced solely inside a + // method body — `new PricingCalculator()`, `(PricingCalculator) + // svc`, `svc instanceof PricingCalculator` — had no reverse + // edge, so its tests were silently dropped whenever the + // helper changed. This fixture models the simplest such + // shape: OrderService instantiates a changed PricingCalculator + // inside a method, and OrderServiceTest is the only test that + // exercises that path. Pre-fix, OrderServiceTest was invisible + // to the transitive walk. + Path prodDir = tempDir.resolve("src/main/java/com/example"); + Files.createDirectories(prodDir); + Files.writeString(prodDir.resolve("PricingCalculator.java"), + "package com.example;\npublic class PricingCalculator {}"); + Files.writeString(prodDir.resolve("OrderService.java"), """ + package com.example; + + public class OrderService { + public void charge() { + PricingCalculator calc = new PricingCalculator(); + } + } + """); + + Path testDir = tempDir.resolve("src/test/java/com/example"); + Files.createDirectories(testDir); + Files.writeString(testDir.resolve("OrderServiceTest.java"), + "package com.example;\npublic class OrderServiceTest {}"); + + Set result = strategy.discoverTests( + Set.of("com.example.PricingCalculator"), tempDir); + + assertTrue(result.contains("com.example.OrderServiceTest"), + "Must discover the consumer's test when the changed class is " + + "only referenced inside a method body — otherwise the " + + "most common helper-refactor MR silently drops coverage"); + } + @Test void discoversConsumerTestsForDeletedProductionClass() throws IOException { // The fixture mirrors a `git rm FooService.java` MR: FooService is in diff --git a/affected-tests-core/src/test/java/io/affectedtests/core/discovery/UsageStrategyTest.java b/affected-tests-core/src/test/java/io/affectedtests/core/discovery/UsageStrategyTest.java index 7653fe8..9024537 100644 --- a/affected-tests-core/src/test/java/io/affectedtests/core/discovery/UsageStrategyTest.java +++ b/affected-tests-core/src/test/java/io/affectedtests/core/discovery/UsageStrategyTest.java @@ -362,6 +362,109 @@ public class ConstantsUserTest { + "backing class changes"); } + @Test + void findsTestThatUsesChangedClassByFullyQualifiedInlineReference() throws IOException { + // Regression: a test that never imports the changed class but + // refers to it inline by its full dotted name + // com.example.service.Thing t = new com.example.service.Thing(); + // slid through every tier pre-fix (no matching import, no + // wildcard, different package, and the simple-name AST scan + // would only fire if the test lived in the same package). The + // new Tier 3 walks ClassOrInterfaceType nodes and reads + // getNameWithScope() so the inline fully-qualified shape is + // caught deterministically, without depending on raw source + // text or on comment stripping. + Path testDir = tempDir.resolve("src/test/java/com/example/other"); + Files.createDirectories(testDir); + Files.writeString(testDir.resolve("FullyQualifiedInlineTest.java"), """ + package com.example.other; + + public class FullyQualifiedInlineTest { + public void t() { + com.example.service.Thing x = + new com.example.service.Thing(); + } + } + """); + + Set result = strategy.discoverTests( + Set.of("com.example.service.Thing"), tempDir); + + assertTrue(result.contains("com.example.other.FullyQualifiedInlineTest"), + "Tests that inline-qualify the changed class must still be " + + "selected — otherwise any test in a sibling package " + + "that avoids imports (common in Cucumber-style steps) " + + "silently drops coverage"); + } + + @Test + void findsTestThatInnerClassQualifiesThroughChangedOuter() throws IOException { + // Follow-up to findsTestThatImportsInnerClassOfChangedOuter: + // same semantic (Outer.java change surfaces only the outer + // FQN, but the test actually uses Outer.Inner) but expressed + // inline rather than via an import. The Tier 3 startsWith + // check must treat `com.example.Outer.Inner` as a dependency + // of `com.example.Outer` so a test that never imports Outer + // still gets selected. + Path testDir = tempDir.resolve("src/test/java/com/example/other"); + Files.createDirectories(testDir); + Files.writeString(testDir.resolve("InlineInnerTest.java"), """ + package com.example.other; + + public class InlineInnerTest { + public void t() { + com.example.Outer.Inner x = new com.example.Outer.Inner(); + } + } + """); + + Set result = strategy.discoverTests( + Set.of("com.example.Outer"), tempDir); + + assertTrue(result.contains("com.example.other.InlineInnerTest"), + "Inline reference to an inner class of the changed outer must " + + "still pull the test in — the inner is part of the " + + "outer's file and shares its change signature"); + } + + @Test + void findsTestThatWildcardsClassMembersOfChangedClass() throws IOException { + // Regression: `import com.example.Outer.*;` imports every + // member of Outer — nested types, public static fields, + // nested classes. PathToClassMapper reports changes at the + // outer-FQN granularity, so when Outer.java changes the + // strategy sees `changedFqns = { "com.example.Outer" }`. The + // old wildcard tier bucketed "com.example.Outer" as a package + // wildcard and then checked whether the AST referenced the + // simple name "Outer" — which test code using the wildcard + // almost never writes (the whole point of the wildcard is to + // skip qualification). Result: the consumer's tests were + // silently dropped on every change to Outer.java. The new + // class-member wildcard tier returns true as soon as the + // wildcard target equals a changed FQN. + Path testDir = tempDir.resolve("src/test/java/com/example/consumers"); + Files.createDirectories(testDir); + Files.writeString(testDir.resolve("WildcardMemberTest.java"), """ + package com.example.consumers; + + import com.example.Outer.*; + + public class WildcardMemberTest { + public void t() { + Inner i = new Inner(); + } + } + """); + + Set result = strategy.discoverTests( + Set.of("com.example.Outer"), tempDir); + + assertTrue(result.contains("com.example.consumers.WildcardMemberTest"), + "`import pkg.Outer.*` must be treated as a dependency on " + + "pkg.Outer — every member the wildcard brings in lives " + + "inside Outer.java and shares its change signature"); + } + @Test void findsTestThatUsesStaticWildcardImportFromChangedClass() throws IOException { // Same as above but with the wildcard static form: 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 1987b33..d71b054 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 @@ -294,6 +294,67 @@ void explainsShallowCloneWhenBaseObjectMissing() throws Exception { } } + @Test + void diffsAgainstMergeBaseNotBaseRefTip() throws Exception { + // Regression: the old path diffed HEAD directly against the + // tip of baseRef. If master moved on after the branch + // diverged, every file that landed on master post-divergence + // showed up as a "change" on the feature branch — even though + // the feature branch never touched those files. Modulr + // typically ships 20-100 merges/day on master, so a week-old + // feature branch would see the whole backlog of unrelated + // production changes dumped into its diff, inflating the + // affected-tests set to essentially "everything" and + // destroying the whole point of selective testing. The fix + // computes the merge-base between baseRef and HEAD first, so + // the diff only reflects what the feature branch actually + // changed relative to where it diverged. + try (Git git = initRepoWithInitialCommit()) { + // --- Shared history: create foo on master --- + Path foo = tempDir.resolve("src/main/java/com/example/Foo.java"); + Files.createDirectories(foo.getParent()); + Files.writeString(foo, "package com.example;\npublic class Foo {}"); + git.add().addFilepattern(".").call(); + String divergencePoint = git.commit().setMessage("add Foo").call().getName(); + + // --- Branch off and commit a feature change --- + git.branchCreate().setName("feature").call(); + git.checkout().setName("feature").call(); + Path bar = tempDir.resolve("src/main/java/com/example/Bar.java"); + Files.writeString(bar, "package com.example;\npublic class Bar {}"); + git.add().addFilepattern(".").call(); + git.commit().setMessage("feature: add Bar").call(); + + // --- Master advances with an unrelated change --- + // The feature branch must NOT see this in its diff. + git.checkout().setName("master").call(); + Path unrelated = tempDir.resolve("src/main/java/com/example/Unrelated.java"); + Files.writeString(unrelated, "package com.example;\npublic class Unrelated {}"); + git.add().addFilepattern(".").call(); + git.commit().setMessage("master: unrelated change").call(); + + // --- Detect changes from feature branch against master --- + git.checkout().setName("feature").call(); + AffectedTestsConfig config = AffectedTestsConfig.builder() + .baseRef("master") + .includeUncommitted(false) + .includeStaged(false) + .build(); + + GitChangeDetector detector = new GitChangeDetector(tempDir, config); + Set changed = detector.detectChangedFiles(); + + assertTrue(changed.contains("src/main/java/com/example/Bar.java"), + "Feature branch's own change must surface, got: " + changed); + assertFalse(changed.contains("src/main/java/com/example/Unrelated.java"), + "Unrelated master-only change must NOT appear in the " + + "feature branch's diff — merge-base is the boundary, " + + "got: " + changed); + assertFalse(divergencePoint.isEmpty(), + "sanity: divergence point must exist"); + } + } + @Test void failsLoudlyOnNonGitDirectory() { Path nonGitDir = tempDir.resolve("not-a-repo"); diff --git a/affected-tests-core/src/test/java/io/affectedtests/core/mapping/PathToClassMapperTest.java b/affected-tests-core/src/test/java/io/affectedtests/core/mapping/PathToClassMapperTest.java index fc0e607..1945e07 100644 --- a/affected-tests-core/src/test/java/io/affectedtests/core/mapping/PathToClassMapperTest.java +++ b/affected-tests-core/src/test/java/io/affectedtests/core/mapping/PathToClassMapperTest.java @@ -388,6 +388,53 @@ void rejectsPathsWithTraversalSegmentAsUnmapped() { "Clean path in the same batch must still be mapped normally"); } + @Test + void moduleInfoRoutesToUnmappedNotProduction() { + // Regression: `module-info.java` is the JPMS descriptor, not a + // production type. The old path handed it to tryMapToClass, + // which produced the non-FQN "module-info" and classified it + // as a production class — poisoning every downstream strategy + // (naming/usage/impl all try to derive a simple class name + // from the FQN and match `module-info` against real source + // classes, which never matches and silently skips all the + // tests that the module-descriptor change could actually + // affect). Routing it to unmapped hands control to the + // UNMAPPED_FILE safety net, which defaults to FULL_SUITE in + // CI mode — the correct conservative choice because a + // module-info change alters visibility for every consumer. + Set changed = Set.of("src/main/java/module-info.java"); + MappingResult result = mapper.mapChangedFiles(changed); + + assertTrue(result.productionClasses().isEmpty(), + "module-info must never become a production FQN"); + assertTrue(result.unmappedChangedFiles().contains("src/main/java/module-info.java"), + "module-info must land in the unmapped bucket so the safety " + + "net can escalate, got: " + result.unmappedChangedFiles()); + } + + @Test + void packageInfoRoutesToUnmappedNotProduction() { + // Same failure mode as module-info: `package-info.java` is a + // marker file that carries package-level annotations/docs, not + // a production type. Pre-fix it became the FQN + // `com.example.package-info`, which is syntactically illegal + // as a Java identifier but which tryMapToClass happily + // produced. Every strategy then silently failed to resolve + // the dotted form to a real class. A change to + // package-info.java can legitimately affect every test in + // that package (e.g. adding a package-wide `@Nullable` + // default), so the UNMAPPED_FILE escalation is the correct + // conservative routing. + Set changed = Set.of("src/main/java/com/example/package-info.java"); + MappingResult result = mapper.mapChangedFiles(changed); + + assertTrue(result.productionClasses().isEmpty(), + "package-info must never become a production FQN"); + assertTrue(result.unmappedChangedFiles() + .contains("src/main/java/com/example/package-info.java"), + "package-info must land in the unmapped bucket"); + } + @Test void allowsLegitimateFilenamesThatContainDotDotSubstring() { // Negative: a file literally named `foo..bar.java` is legal on