Skip to content

fix/correctness-silent-drops: close nine tier-1 silent-drop bugs on the path to v2.0 #31

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
vedanthvdev merged 1 commit into from
Apr 22, 2026
Merged

fix/correctness-silent-drops: close nine tier-1 silent-drop bugs on the path to v2.0 #31

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
129 changes: 129 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<FooService>` 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<FooService>` 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)
Expand Down
25 changes: 25 additions & 0 deletions affected-tests-core/src/main/java/io/affectedtests/core/AffectedTestsEngine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
104 changes: 76 additions & 28 deletions ...cted-tests-core/src/main/java/io/affectedtests/core/discovery/ImplementationStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -145,7 +147,7 @@ private Set<String> findImplementations(Set<String> 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
Expand All @@ -161,31 +163,42 @@ private Set<String> findImplementations(Set<String> changedClasses,
CompilationUnit cu = parseOrGet(sourceFile, index, fallbackParser);
if (cu == null) continue;

List<ClassOrInterfaceDeclaration> 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<T>) 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<TypeDeclaration<?>> declarations =
(List<TypeDeclaration<?>>) (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`
// is the loop's termination gate: once a class is
// 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;
}
}
Expand Down Expand Up @@ -216,15 +229,7 @@ private CompilationUnit parseOrGet(Path file, ProjectIndex index, JavaParser fal
if (index != null) {
return index.compilationUnit(file);
}
try {
ParseResult<CompilationUnit> 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");
}

/**
Expand All @@ -240,11 +245,54 @@ private Set<String> 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.
*
* <p>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<ClassOrInterfaceType> supertypesOf(TypeDeclaration<?> decl) {
List<ClassOrInterfaceType> 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;
}
}
Loading