From 758da157f6141070a02ae7d76a9a6c91d3567371 Mon Sep 17 00:00:00 2001 From: vedanthvasudev Date: Fri, 24 Apr 2026 15:10:15 +0100 Subject: [PATCH] feat/v2.2-adoption-feedback: polish the operator experience surfaced by the security-service pilot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The v2.1 pilot against Modulr's security-service (CAR-5190) surfaced five non-breaking rough edges that an adopter hits in their first hour with the plugin. v2.2 sharpens each one while keeping every DSL knob, default, and resolved behaviour from v2.1 bit-for-bit identical. Bug A: `--explain` no longer forces a full compile. The unconditional `testClasses` dependency turned a 3-second diagnostic run into a multi-minute compile on a real-world repo. The dependency is now wrapped in a `Callable` that re-evaluates after command-line parsing and returns an empty list when `--explain` is set, pruning `testClasses` from the task graph entirely. Dispatch runs keep the dependency as before. Bug B: situation-specific `Hint:` lines in the `--explain` trace. Pre-v2.2 every mapper-touching situation printed the same "outOfScopeTestDirs is configured" hint — correct for a subset of DISCOVERY_SUCCESS runs, actively misleading everywhere else. v2.2 splits the hint into three targeted branches: DISCOVERY_EMPTY names the realistic causes (testSuffixes, testDirs, no coverage yet), DISCOVERY_INCOMPLETE names the actual risk (parse failure → partial selection) plus the `onDiscoveryIncomplete` escalation knob, and DISCOVERY_SUCCESS keeps the v2.1 OOS wording in the one situation it was right for. The DISCOVERY_INCOMPLETE hint is action-aware: only the SELECTED branch claims the selection is partial; the FULL_SUITE branch (CI/STRICT default, or explicit override) drops that wording and the circular "escalate to what we already did" advice. Risk C: LOCAL mode + DISCOVERY_INCOMPLETE now emits a lifecycle WARN when it accepts a partial selection. LOCAL keeps `onDiscoveryIncomplete = SELECTED` on purpose (devs iterating on WIP want fast feedback), but without a visible signal a parse failure silently understates what actually ran. The new `WARN` marker "affectedTest: LOCAL mode accepted a partial selection" is grep-friendly, visible at Gradle's default log level, and gated so it never fires in CI/STRICT modes (which already escalate to FULL_SUITE on DISCOVERY_INCOMPLETE). The gate also short-circuits on `skipped=true` / empty FQN lists so an operator never sees a "0 test classes accepted" WARN immediately before a skipped run. The wording cross-references the engine's own parse-failure WARN rather than restating it. Feature D: `-PaffectedTestsMode=local|ci|strict|auto` runtime override. Mirrors the existing `-PaffectedTestsBaseRef` pattern so adopters can A/B modes without editing `build.gradle`. Implemented as a `convention()` on the mode Property so DSL-declared `mode = '...'` keeps precedence — a repo pinning its CI mode in the build script cannot be silently overridden by a stray `-P`. Polish E: `--explain` now shows the `:module:test` dispatch breakdown on a SELECTED run. Same grouping helper the dispatch path uses, same preview-truncation cap — so an operator comparing `--explain` against a real dispatch sees the exact same shape. Non-SELECTED runs suppress the block entirely rather than print a noisy "0 modules" line. The task-path normalisation is shared between explain and dispatch via a `testTaskPath` helper so the two operator-facing strings cannot drift. Every user-facing behaviour above is pinned by a Cucumber e2e scenario in `06-v2.2-adoption-feedback.feature`, plus unit tests on the Callable prune shape, the mode-precedence contract, the three hint variants (including the action-aware DISCOVERY_INCOMPLETE split), the Risk C WARN four-way gate (mode, situation, action, skipped/empty), and the Modules-block empty/multi-module/root/truncation edges. The two-module Bug A e2e scenario pins that `--explain` prunes `compileJava` on every subproject, not just the root. CHANGELOG bumped with the adoption-feedback narrative plus a follow-up Unreleased section capturing the v2.2 code-review polish; the release-version pin is set to 2.2.0 so the next merge to master tags and publishes v2.2.0. --- .release-version | 1 + CHANGELOG.md | 165 +++++- .../affectedtests/gradle/e2e/TestProject.java | 29 +- .../gradle/e2e/steps/CommonSteps.java | 61 ++- .../06-v2.2-adoption-feedback.feature | 281 ++++++++++ .../gradle/AffectedTestTask.java | 426 +++++++++++++-- .../gradle/AffectedTestsPlugin.java | 44 +- .../AffectedTestTaskExplainFormatTest.java | 500 +++++++++++++++++- .../gradle/AffectedTestsPluginTest.java | 110 ++++ 9 files changed, 1551 insertions(+), 66 deletions(-) create mode 100644 .release-version create mode 100644 affected-tests-gradle/src/functionalTest/resources/io/affectedtests/gradle/e2e/features/06-v2.2-adoption-feedback.feature diff --git a/.release-version b/.release-version new file mode 100644 index 0000000..ccbccc3 --- /dev/null +++ b/.release-version @@ -0,0 +1 @@ +2.2.0 diff --git a/CHANGELOG.md b/CHANGELOG.md index b8c435d..03f9c4f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,167 @@ adheres to [Semantic Versioning](https://semver.org/). ## [Unreleased] +### Changed — internal refinements from the v2.2 code review + +Follow-up polish on the v2.2 adoption-feedback release, all +non-breaking and none of them visible in build scripts. Captured here +rather than cut as a point release because the operator-facing +contract is identical to v2.2.0 on every green path. + +* **Risk C WARN no longer fires with "0 test classes".** The engine + rewrites `SELECTED` with nothing to dispatch into `skipped=true`; + the WARN now short-circuits on `result.skipped() || FQNs.isEmpty()` + so an operator never sees "LOCAL mode accepted a partial selection + of 0 test classes" immediately followed by a skipped run. +* **Risk C WARN wording is de-duplicated against the engine log.** + The message no longer restates "could not parse one or more Java + files" (the engine's own WARN a few lines up already says so) and + instead cross-references it — keeps the mode-specific postscript + without echoing the parse-failure fact twice. +* **`DISCOVERY_INCOMPLETE` hint is now action-aware.** `Action.SELECTED` + continues to say "selection is necessarily partial — set + `onDiscoveryIncomplete = 'full_suite'` to escalate". On + `Action.FULL_SUITE` (CI/STRICT default, or an explicit operator + override) the hint drops the partial-selection wording entirely and + only names the parse failure for next-run follow-up — avoids the + circular "escalate to the action we already took" advice. +* **Shared `testTaskPath` helper.** Both the `--explain` Modules-block + preview and the dispatch-path argv now route through + `AffectedTestTask#testTaskPath`, so the two operator-facing strings + can no longer drift. Root project is `":test"` with a leading colon + on both sides. +* **Unit tests for the Risk C WARN gate.** The four-way gate (mode, + situation, action, skipped/empty) is now pinned by six direct unit + tests on the pure `shouldWarnLocalDiscoveryIncomplete` / + `formatLocalDiscoveryIncompleteWarning` helpers, matching what the + Javadoc already promised. +* **Two-module Bug A e2e scenario.** Pins that `--explain` prunes + `compileJava` / `compileTestJava` on every subproject, not just the + root — catches a regression where someone pins the Callable wiring + to the root project instead of iterating via `allprojects { ... }`. + +## [v2.2.0] — adoption-feedback polish from the security-service pilot + +v2.2.0 is a **non-breaking** release driven entirely by what a second +real-world adopter (Modulr's `security-service`, CAR-5190) ran into +while plugging v2.1 in. Every DSL knob, every default, and every +resolved behaviour from v2.1 keeps working bit-for-bit; v2.2 only +sharpens the edges an operator touches when something goes wrong +or when they want to A/B a mode from CI. + +If you're on v2.1 today you can bump to v2.2 without touching +`build.gradle`. The only observable difference on a green path is a +faster `--explain` run (Bug A) and a per-module breakdown in that +trace (Polish E). The only observable difference on the unhappy path +is clearer hint wording (Bug B) and — in LOCAL mode specifically — a +loud `WARN` when discovery can't parse part of the diff (Risk C). + +### Fixed — `--explain` no longer forces a full compile (Bug A) + +Pre-v2.2 the `affectedTest` task eagerly depended on `testClasses` in +every subproject applying the `java` plugin. Great for the dispatch +path — the nested `./gradlew` needs class files to actually run tests +against — but pure overhead when the operator just wants to see the +decision trace. On a security-service-shaped repo this turned a +3-second diagnostic run into a multi-minute compile. + +v2.2 wires the dependency through a Gradle `Callable` that re-evaluates +after command-line parsing. When `--explain` is set the Callable +returns an empty list and `testClasses` is pruned from the task graph +entirely; when `--explain` is absent the dependency behaves exactly +as before. The fix is transparent to build scripts and is pinned by +both a unit test on the Callable return shape and a Cucumber e2e +scenario that asserts `:compileJava` is absent from the executed-task +list of a real `--explain` run. + +### Fixed — situation-specific Hint lines in the `--explain` trace (Bug B) + +Pre-v2.2 the `--explain` trace printed a single "Hint:" line regardless +of the resolved situation, and that hint always named +`outOfScopeTestDirs` / `outOfScopeSourceDirs`. Correct for a subset of +DISCOVERY_SUCCESS runs, actively misleading everywhere else — a +DISCOVERY_EMPTY run with no OOS configured, and a DISCOVERY_INCOMPLETE +run where the real risk is a parse failure, both got the same OOS +advice that had nothing to do with the actual cause. + +v2.2 splits that one line into three targeted branches: + +- **DISCOVERY_EMPTY** now leads with "discovery mapped 0 test classes" + and lists the three realistic causes — wrong `testSuffixes`, + `testDirs` misconfigured, or no test coverage yet. +- **DISCOVERY_INCOMPLETE** now names the actual risk: the mapper + couldn't parse one or more Java files in the diff, so the selection + is definitionally partial. It also points at + `onDiscoveryIncomplete = "full_suite"` as the escalation knob. +- **DISCOVERY_SUCCESS** with `outOfScopeTestDirs` / + `outOfScopeSourceDirs` configured-but-unmatched keeps the v2.1 OOS + advice — that was the one situation where the old hint was right. + +### Added — loud WARN when LOCAL mode accepts a partial selection (Risk C) + +LOCAL mode defaults `onDiscoveryIncomplete = SELECTED` on purpose — +developers iterating on WIP want fast feedback. The adoption risk: +when a Java parse failure drops files silently, the green "SELECTED" +summary overstates what actually ran, and there was no way to tell +from the non-`--explain` output that the selection was incomplete. + +v2.2 emits a lifecycle-level `WARN` in this exact combination +(`mode = LOCAL` + `DISCOVERY_INCOMPLETE` + `Action = SELECTED`) before +the dispatch fires. The marker string +`affectedTest: LOCAL mode accepted a partial selection` is grep-friendly +and visible at Gradle's default log level — operators don't need to +re-run with `--info` to see the safety signal. CI and STRICT modes +already escalate to FULL_SUITE on DISCOVERY_INCOMPLETE by default, so +the warning is mode-gated and does not fire there. + +### Added — `-PaffectedTestsMode` runtime override (Feature D) + +v2.2 mirrors the existing `-PaffectedTestsBaseRef` pattern: set +`-PaffectedTestsMode=local|ci|strict|auto` on the command line to flip +the plugin's mode without editing `build.gradle`. Useful for adoption +experiments ("what would STRICT mode pick on today's HEAD?") and for +CI jobs that want to A/B two modes from the same pipeline. + +DSL-declared `mode = '...'` still wins because Gradle Property +semantics apply explicit `set()` calls ahead of `convention()` — so +the `-P` is genuinely a fallback, not an override, and a repo pinning +its CI mode in `build.gradle` keeps that pin even if a stray `-P` +slips past review. + +### Added — `:module:test` dispatch breakdown in `--explain` (Polish E) + +The pre-v2.2 `--explain` trace named the total test-class count on a +SELECTED run but not the module distribution, so an operator asking +"which tasks will Gradle actually kick off?" still had to dry-run a +real dispatch to answer that. v2.2 threads the same grouping the +dispatch path uses into the trace: + +``` +Modules: 2 modules, 3 test classes to dispatch + :application:test (2 test classes) + com.example.FooTest + com.example.BarTest + :api:test (1 test class) + com.example.BazTest +``` + +Non-SELECTED runs (EMPTY_DIFF, docs-only, etc.) suppress the block +entirely rather than print a noisy "Modules: 0 modules" line. + +### Verified — regression coverage + +- New unit tests in `AffectedTestsPluginTest` pin the `--explain` + Callable's prune behaviour (present without `--explain`, absent with + it) and the mode-precedence contract (DSL beats convention). +- New unit tests in `AffectedTestTaskExplainFormatTest` pin the three + situation-specific hint variants and the empty-map / multi-module / + root-project / preview-truncation shapes of the Modules block. +- A new Cucumber feature + (`06-v2.2-adoption-feedback.feature`) pins every user-facing + behaviour above as a full TestKit e2e, so a regression on any of + the five fixes surfaces as a scenario failure rather than silent + drift in operator experience. + ## [v2.1.0] — DSL polish on top of the v2 breaking release v2.1.0 is the **first publicly tagged v2 release**. It bundles everything @@ -927,5 +1088,7 @@ broad strokes: safety hardening, multi-module scanning, axion-release versioning. See the Releases page for detail. -[Unreleased]: https://github.com/vedanthvdev/affected-tests/compare/v1.9.12...HEAD +[Unreleased]: https://github.com/vedanthvdev/affected-tests/compare/v2.2.0...HEAD +[v2.2.0]: https://github.com/vedanthvdev/affected-tests/releases/tag/v2.2.0 +[v2.1.0]: https://github.com/vedanthvdev/affected-tests/releases/tag/v2.1.0 [1.9.12]: https://github.com/vedanthvdev/affected-tests/releases/tag/v1.9.12 diff --git a/affected-tests-gradle/src/functionalTest/java/io/affectedtests/gradle/e2e/TestProject.java b/affected-tests-gradle/src/functionalTest/java/io/affectedtests/gradle/e2e/TestProject.java index f5b1bb1..21df31a 100644 --- a/affected-tests-gradle/src/functionalTest/java/io/affectedtests/gradle/e2e/TestProject.java +++ b/affected-tests-gradle/src/functionalTest/java/io/affectedtests/gradle/e2e/TestProject.java @@ -252,13 +252,34 @@ public void addGradleArgument(String argument) { * regression. */ public void runAffectedTests() { + runAffectedTests(true); + } + + /** + * Runs {@code affectedTest} without the {@code -x compileJava -x + * compileTestJava ...} safety net so the real task-graph + * dependency shape is exercised. Used by the v2.2 e2e scenario + * that pins the "--explain must not force a compile" fix: with + * the skip-flags present we'd short-circuit compile via CLI + * regardless of the plugin's dependency wiring, which would mask + * a regression in the fix. Every other scenario should stick + * with {@link #runAffectedTests()} so per-scenario wall time + * stays in the seconds-range. + */ + public void runAffectedTestsWithLiveDependencies() { + runAffectedTests(false); + } + + private void runAffectedTests(boolean skipCompileTasks) { List args = new ArrayList<>(); args.add("affectedTest"); args.add("--stacktrace"); - args.add("-x"); args.add("compileJava"); - args.add("-x"); args.add("compileTestJava"); - args.add("-x"); args.add("processResources"); - args.add("-x"); args.add("processTestResources"); + if (skipCompileTasks) { + args.add("-x"); args.add("compileJava"); + args.add("-x"); args.add("compileTestJava"); + args.add("-x"); args.add("processResources"); + args.add("-x"); args.add("processTestResources"); + } if (baselineCommit != null) { // Pass baseRef as a Gradle property instead of baking it // into build.gradle. See captureBaseline() for the full diff --git a/affected-tests-gradle/src/functionalTest/java/io/affectedtests/gradle/e2e/steps/CommonSteps.java b/affected-tests-gradle/src/functionalTest/java/io/affectedtests/gradle/e2e/steps/CommonSteps.java index be7d672..5020ac1 100644 --- a/affected-tests-gradle/src/functionalTest/java/io/affectedtests/gradle/e2e/steps/CommonSteps.java +++ b/affected-tests-gradle/src/functionalTest/java/io/affectedtests/gradle/e2e/steps/CommonSteps.java @@ -131,18 +131,34 @@ public void aCannedDiffThatProducesTheSituation(String situation) throws Excepti world.project().commit("diff: update foo"); } case "DISCOVERY_INCOMPLETE" -> { - // Malformed Java that JavaParser cannot parse. The - // engine reports a parse failure → DISCOVERY_INCOMPLETE. + // Malformed Java that JavaParser cannot parse, PAIRED + // with a well-formed prod/test mapping so discovery + // returns `DISCOVERY_INCOMPLETE + SELECTED` with a + // non-empty FQN set. This is the exact shape Risk C + // targets: the operator sees a selection succeed, but + // the parser silently dropped an input file so the + // selection is partial. Without the paired mapping + // the engine routes SELECTED-with-empty-FQNs to + // skipped=true and both the WARN and the explain + // hint are correctly suppressed — which would make + // the LOCAL-warn scenario silently untestable. + // // Shape matters: the `broken(` below is a truncated // parameter list that JavaParser treats as a hard // syntax error (vs missing semicolons which it tends // to recover from). + world.project().writeFile("src/main/java/com/example/FooService.java", + "package com.example;\npublic class FooService {}\n"); + world.project().writeFile("src/test/java/com/example/FooServiceTest.java", + "package com.example;\npublic class FooServiceTest {}\n"); world.project().writeFile("src/main/java/com/example/Broken.java", "package com.example;\npublic class Broken {\n public void broken(\n}\n"); world.project().captureBaseline(); + world.project().writeFile("src/main/java/com/example/FooService.java", + "package com.example;\npublic class FooService { /* tweak */ }\n"); world.project().writeFile("src/main/java/com/example/Broken.java", "package com.example;\npublic class Broken {\n public void broken(\n /* tweak */\n}\n"); - world.project().commit("diff: update broken"); + world.project().commit("diff: update foo + broken"); } default -> throw new IllegalArgumentException( "No canned setup for situation " + situation @@ -249,6 +265,26 @@ public void theAffectedTestsTaskRunsWith(String extraArg) throws Exception { world.project().runAffectedTests(); } + @Given("the Gradle command-line argument {string}") + public void theGradleCommandLineArgument(String arg) { + // Lets a scenario stack multiple CLI flags (e.g. + // `-PaffectedTestsMode=strict` and `--explain`) without + // inventing a per-combination `runs with "... ..."` step. + // Cleared automatically after the next runAffectedTests() + // call, so unrelated scenarios don't leak. + world.project().addGradleArgument(arg); + } + + @When("the affected-tests task runs with live task dependencies") + public void theAffectedTestsTaskRunsWithLiveTaskDependencies() throws Exception { + // Exercises the real dependency graph — no `-x compileJava` + // CLI escape hatch — so the v2.2 "--explain does not force + // compile" fix is provable via the resulting task list. If + // the fix regresses, `:compileJava` will reappear in the + // executed-tasks list and the paired assertion fails. + world.project().runAffectedTestsWithLiveDependencies(); + } + @When("any Gradle task is configured") public void anyGradleTaskIsConfigured() throws Exception { // Scenarios that assert on configuration-time failures run @@ -350,6 +386,25 @@ public void theSelectedTestsInclude(String testFqn) { + world.project().lastOutput()); } + @Then("the executed task list does not include {string}") + public void theExecutedTaskListDoesNotInclude(String taskPath) { + // TestKit's BuildResult.task() returns null when a task was + // never scheduled (filtered out, pruned, or its declaring + // subproject doesn't exist). That's exactly the "never + // scheduled" shape we want — a task that was scheduled and + // SKIPPED would still appear in the list with a SKIPPED + // outcome, which is a different (legitimate) behaviour we + // must not confuse with pruning. Fail loudly if the task + // was any non-null state so regressions — including the + // "explain quietly UP-TO-DATEs compile instead of pruning + // it" shape — surface as a named assertion. + org.gradle.testkit.runner.BuildTask task = world.project().lastBuildResult().task(taskPath); + assertTrue(task == null, + "Expected task " + taskPath + " to be absent from the executed task list, " + + "but it ran with outcome " + (task == null ? "" : task.getOutcome()) + + ". Output was:\n" + world.project().lastOutput()); + } + @Then("the outcome is {string}") public void theOutcomeIs(String outcome) { // The `Outcome:` line in the --explain trace is the canonical diff --git a/affected-tests-gradle/src/functionalTest/resources/io/affectedtests/gradle/e2e/features/06-v2.2-adoption-feedback.feature b/affected-tests-gradle/src/functionalTest/resources/io/affectedtests/gradle/e2e/features/06-v2.2-adoption-feedback.feature new file mode 100644 index 0000000..ab6e26a --- /dev/null +++ b/affected-tests-gradle/src/functionalTest/resources/io/affectedtests/gradle/e2e/features/06-v2.2-adoption-feedback.feature @@ -0,0 +1,281 @@ +Feature: v2.2 — adoption-feedback fixes surfaced on security-service + These scenarios pin the five user-facing fixes raised by the + security-service adoption in CAR-5190. Each scenario triggers the + exact shape the adopter ran into so a future regression on any of + the five resurfaces as a loud Cucumber failure rather than as + silent drift in operator-experience quality. + + Background: + Given a freshly initialised project with a committed baseline + + # ------------------------------------------------------------------ + # Bug A — `--explain` must not force a compile + # + # On a security-service-shaped repo the unconditional `testClasses` + # dependency turned a 3-second diagnostic into a 4-minute build, so + # operators stopped using --explain. The fix wires the dependency + # through a {@code Callable} that short-circuits when `--explain` is + # set, pruning compile from the task graph entirely. We prove it + # with a real dispatch (no `-x compileJava` CLI escape) and assert + # that `:compileJava` is absent from the executed-task list. + # ------------------------------------------------------------------ + Scenario: --explain runs without scheduling compileJava + Given a production class "com.example.FooService" with its matching test "com.example.FooServiceTest" + And the baseline commit is captured + And the diff modifies "src/main/java/com/example/FooService.java" + And the Gradle command-line argument "--explain" + When the affected-tests task runs with live task dependencies + Then the task succeeds + And the situation is DISCOVERY_SUCCESS + And the executed task list does not include ":compileJava" + And the executed task list does not include ":compileTestJava" + + # Paired negative case (compile dependency still required on + # non-explain dispatch) is covered by the unit test on the + # Callable return shape in AffectedTestsPluginTest — asserting + # "compile did run" via Cucumber would force a full nested + # ./gradlew dispatch in the test project, which is flaky in CI. + + Scenario: --explain prunes compileJava on every subproject, not just the root + # The Callable is registered per-java-plugin via `allprojects { ... }`, + # so a regression that pinned it to the root (or forgot to iterate) + # would pass the single-module scenario above but force compiles on + # every subproject. A security-service-shaped repo with 10 modules + # would take the 4-minute hit the fix was raised to prevent — this + # scenario catches that specific drift by running the same check on + # a two-subproject project graph. + Given the project is multi-module with sub-projects "api" and "application" + And a file at "application/src/main/java/com/example/UserService.java" with content: + """ + package com.example; + public class UserService {} + """ + And a file at "application/src/test/java/com/example/UserServiceTest.java" with content: + """ + package com.example; + public class UserServiceTest {} + """ + And the baseline commit is captured + And the diff modifies "application/src/main/java/com/example/UserService.java" + And the Gradle command-line argument "--explain" + When the affected-tests task runs with live task dependencies + Then the task succeeds + And the situation is DISCOVERY_SUCCESS + And the executed task list does not include ":compileJava" + And the executed task list does not include ":compileTestJava" + And the executed task list does not include ":api:compileJava" + And the executed task list does not include ":api:compileTestJava" + And the executed task list does not include ":application:compileJava" + And the executed task list does not include ":application:compileTestJava" + + # ------------------------------------------------------------------ + # Bug B — situation-specific hints + # + # Pre-v2.2, every mapper-touching situation printed the same + # "outOfScopeTestDirs is configured..." hint — actively misleading + # on runs where OOS was not the cause. v2.2 replaces the one-size + # hint with three targeted branches. Each scenario pins the + # situation-appropriate wording so hint drift fails a scenario + # rather than leaking into production. + # ------------------------------------------------------------------ + Scenario: DISCOVERY_EMPTY hint names testSuffixes and testDirs, not OOS knobs + # Production class changed, no matching test on disk. v2.1 + # printed "outOfScopeTestDirs configured" which was irrelevant. + # v2.2 leads with "mapped 0 test classes" and lists the three + # realistic causes. + Given a production class "com.example.Orphan" with no matching test on disk + And the baseline commit is captured + And the diff modifies "src/main/java/com/example/Orphan.java" + When the affected-tests task runs with "--explain" + Then the task succeeds + And the situation is DISCOVERY_EMPTY + And the output contains "Hint: discovery mapped 0 test classes" + And the output contains "testSuffixes" + And the output contains "testDirs" + And the output contains "no test coverage yet" + And the output does not contain "outOfScopeTestDirs is configured" + And the output does not contain "outOfScopeSourceDirs is configured" + + Scenario: DISCOVERY_INCOMPLETE hint flags partial-selection risk on SELECTED + # Parse failure means the mapper ran with missing inputs. On + # SELECTED (LOCAL mode's default, or an explicit operator opt-in) + # the selection is definitionally partial, so the hint must + # surface that risk plus the exact escalation knob. + # + # Mode is pinned explicitly — under AUTO the effective mode + # depends on CI-environment heuristics, and a CI runner would + # land FULL_SUITE and route through the paired scenario below. + # Pinning makes the assertion about hint wording, not about + # which mode the scenario picked. + Given the affected-tests DSL contains: + """ + mode = 'local' + """ + And a canned diff that produces the DISCOVERY_INCOMPLETE situation + When the affected-tests task runs with "--explain" + Then the task succeeds + And the situation is DISCOVERY_INCOMPLETE + And the output contains "failed to parse" + And the output contains "selection is necessarily partial" + And the output contains "onDiscoveryIncomplete" + + Scenario: DISCOVERY_INCOMPLETE hint on FULL_SUITE drops the partial-selection wording + # Paired scenario for the v2.2.1 H3 fix: CI/STRICT escalate + # DISCOVERY_INCOMPLETE to FULL_SUITE, so the old "selection is + # necessarily partial — set onDiscoveryIncomplete=full_suite to + # escalate" wording was factually wrong on two counts (the whole + # suite runs, and escalation already happened). The hint now + # names the parse failure as the root cause for next time and + # stops there — no circular escalation advice. + Given the affected-tests DSL contains: + """ + mode = 'ci' + """ + And a canned diff that produces the DISCOVERY_INCOMPLETE situation + When the affected-tests task runs with "--explain" + Then the task succeeds + And the situation is DISCOVERY_INCOMPLETE + And the output contains "failed to parse" + And the output does not contain "selection is necessarily partial" + And the output does not contain "onDiscoveryIncomplete = 'full_suite' to escalate" + + Scenario: DISCOVERY_SUCCESS keeps the v2.1 OOS-misconfig hint + # Regression guard: the one DISCOVERY_SUCCESS case where v2.1 was + # right — OOS configured, bucket empty, config probably silently + # broken — must still fire in v2.2. + Given the affected-tests DSL contains: + """ + outOfScopeTestDirs = ['api-test/**'] + """ + And a production class "com.example.FooService" with its matching test "com.example.FooServiceTest" + And the baseline commit is captured + And the diff modifies "src/main/java/com/example/FooService.java" + When the affected-tests task runs with "--explain" + Then the task succeeds + And the situation is DISCOVERY_SUCCESS + And the output contains "outOfScopeTestDirs is configured (1 entry) but no file in the diff matched" + + # ------------------------------------------------------------------ + # Risk C — LOCAL + DISCOVERY_INCOMPLETE silently trusts partial + # + # LOCAL mode keeps `onDiscoveryIncomplete = SELECTED` by design + # (devs iterating on WIP want fast feedback). The risk: an operator + # running locally doesn't realise the parser dropped a file, so the + # green "SELECTED" summary overstates what actually ran. v2.2 emits + # a WARN in this exact combination — visible at Gradle's default + # log level without --info. + # ------------------------------------------------------------------ + Scenario: LOCAL mode warns loudly when a partial selection is accepted + Given the affected-tests DSL contains: + """ + mode = 'local' + """ + And a canned diff that produces the DISCOVERY_INCOMPLETE situation + When the affected-tests task runs with "--explain" + Then the task succeeds + And the situation is DISCOVERY_INCOMPLETE + # WARN marker renders at lifecycle level; operators see it + # without --info. Asserting the exact marker (not just "WARN") + # keeps the assertion pinned to this specific safety check. + And the output contains "affectedTest: LOCAL mode accepted a partial selection" + And the output contains "Fix the parse error" + + Scenario: CI mode does not emit the LOCAL-only warning on DISCOVERY_INCOMPLETE + # CI's default is FULL_SUITE — no partial selection is accepted, + # so the WARN would be misinformation. Pins the mode-gate so + # someone doesn't accidentally move the warn outside the LOCAL + # branch. + Given the affected-tests DSL contains: + """ + mode = 'ci' + """ + And a canned diff that produces the DISCOVERY_INCOMPLETE situation + When the affected-tests task runs with "--explain" + Then the task succeeds + And the situation is DISCOVERY_INCOMPLETE + And the output does not contain "LOCAL mode accepted a partial selection" + + # ------------------------------------------------------------------ + # Feature D — -PaffectedTestsMode=... runtime override + # + # Mirrors the baseRef pattern: lets adopters A/B two modes without + # editing build.gradle. DSL-declared mode still wins (Gradle + # Property semantics: explicit set > convention > unset), which + # the "DSL beats -P" scenario pins. + # ------------------------------------------------------------------ + Scenario: -PaffectedTestsMode overrides the mode when the DSL leaves it unset + Given a production class "com.example.FooService" with its matching test "com.example.FooServiceTest" + And the baseline commit is captured + And the diff modifies "src/main/java/com/example/FooService.java" + And the Gradle command-line argument "-PaffectedTestsMode=strict" + When the affected-tests task runs with "--explain" + Then the task succeeds + # STRICT's matrix escalates EMPTY_DIFF to FULL_SUITE (unique to + # STRICT) — asserting that row proves -P actually threaded + # through to the mode. + And the output contains "EMPTY_DIFF FULL_SUITE" + And the output contains "Mode: STRICT" + + Scenario: DSL-declared mode beats -PaffectedTestsMode + # Gradle Property semantics: explicit set in the DSL is an + # "explicit value", which wins over a provider-supplied + # convention (the -P fallback). Pins this precedence so an + # adopter can reliably freeze their CI mode in build.gradle + # without a stray -P leaking past. + Given the affected-tests DSL contains: + """ + mode = 'ci' + """ + And a production class "com.example.FooService" with its matching test "com.example.FooServiceTest" + And the baseline commit is captured + And the diff modifies "src/main/java/com/example/FooService.java" + And the Gradle command-line argument "-PaffectedTestsMode=strict" + When the affected-tests task runs with "--explain" + Then the task succeeds + And the output contains "Mode: CI" + And the output does not contain "Mode: STRICT" + + # ------------------------------------------------------------------ + # Polish E — :module:test breakdown in --explain + # + # The pre-v2.2 trace named the total count but not the module + # distribution, forcing adopters to dry-run dispatch to answer + # "which tasks will Gradle actually kick off?". v2.2 pipes the + # real dispatch grouping (same helper as the non-explain path) into + # the trace so it answers itself. + # ------------------------------------------------------------------ + Scenario: --explain shows the :module:test breakdown for a SELECTED run + Given the project is multi-module with sub-projects "api" and "application" + And a file at "application/src/main/java/com/example/UserService.java" with content: + """ + package com.example; + public class UserService {} + """ + And a file at "application/src/test/java/com/example/UserServiceTest.java" with content: + """ + package com.example; + public class UserServiceTest {} + """ + And the baseline commit is captured + And the diff modifies "application/src/main/java/com/example/UserService.java" + When the affected-tests task runs with "--explain" + Then the task succeeds + And the situation is DISCOVERY_SUCCESS + And the output contains "Modules: 1 module, 1 test class to dispatch" + And the output contains ":application:test (1 test class)" + And the output contains " com.example.UserServiceTest" + + Scenario: --explain suppresses the Modules block when there is no selection + # Non-SELECTED runs (empty diff, docs-only, unmapped config file) + # have no dispatch grouping to show, and printing a + # "Modules: 0 modules" line is pure noise. Pins the empty-map + # fast path. We capture the baseline first so the empty diff is + # measured against an explicit ref rather than the plugin's + # default `origin/master`, which doesn't exist inside the + # TestKit scratch repo. + Given the baseline commit is captured + And the diff contains no committed changes on top of baseline + When the affected-tests task runs with "--explain" + Then the task succeeds + And the situation is EMPTY_DIFF + And the output does not contain "Modules:" 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 38d7198..7ec19a9 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 @@ -321,6 +321,20 @@ public void runAffectedTests() { AffectedTestsEngine engine = new AffectedTestsEngine(config, projectDir); AffectedTestsResult result = engine.run(); + // Loud warning before the explain/dispatch fork: LOCAL-mode's + // default on DISCOVERY_INCOMPLETE is SELECTED, which silently + // accepts a partial selection when the discovery parser hit an + // unparseable Java file. On a workstation that's a conscious + // trade-off (dev iterates on WIP tests, wants fast feedback), + // but unless the operator knows about it, a green "SELECTED N + // tests" summary reads as "we ran the affected tests" — not + // as "we ran what the parser could see, which may not be all + // of them". We emit WARN so it's visible in both the IDE and + // CI logs without extra flags, and we do it on the --explain + // path too so the diagnostic mode surfaces the same risk the + // dispatch mode does. + warnIfLocalDiscoveryIncompleteSelected(config, result); + boolean explain = getExplain().getOrElse(false); if (explain) { // {@code --explain} is a diagnostic mode: we print the trace and @@ -328,7 +342,34 @@ public void runAffectedTests() { // as many times as they need without waiting for the suite. // Every line goes through {@code lifecycle()} so the trace is // visible by default (no {@code --info} gymnastics). - for (String line : renderExplainTrace(config, result)) { + // + // Compute the module grouping here (not inside + // renderExplainTrace) because the grouping needs + // instance-level state — {@link #getSubprojectPaths()} and + // the {@code projectDir} Path — which the pure-function + // renderer deliberately doesn't see. Empty map when + // there's no selection to split by module, so the renderer + // can simply skip the "Modules:" block rather than print + // an empty one. + Map> moduleGroupsForExplain = Map.of(); + if (result.action() == Action.SELECTED && !result.testClassFqns().isEmpty()) { + // groupFqnsByModule returns keys shaped like the + // owning project path (``, `:api`, `:application`). + // The dispatch path then appends `:test` to produce + // the real `:module:test` task path before invoking + // the nested gradle. Mirror that transformation here + // so the --explain preview names the EXACT tasks + // that would run in a non-explain dispatch — anything + // less defeats the point of the diagnostic. + Map> rawGroups = groupFqnsByModule( + projectDir, result.testClassFqns(), result.testFqnToPath()); + Map> taskKeyed = new LinkedHashMap<>(rawGroups.size()); + for (Map.Entry> e : rawGroups.entrySet()) { + taskKeyed.put(testTaskPath(e.getKey()), e.getValue()); + } + moduleGroupsForExplain = taskKeyed; + } + for (String line : renderExplainTrace(config, result, moduleGroupsForExplain)) { getLogger().lifecycle(line); } return; @@ -491,8 +532,7 @@ private void executeTests(Path projectDir, Map> validatedGroups = new LinkedHashMap<>(grouped.size()); int totalValid = 0; for (Map.Entry> entry : grouped.entrySet()) { - String modulePath = entry.getKey(); - String taskPath = modulePath.isEmpty() ? "test" : modulePath + ":test"; + String taskPath = testTaskPath(entry.getKey()); List valid = new ArrayList<>(entry.getValue().size()); for (String fqn : entry.getValue()) { if (!isValidFqn(fqn)) { @@ -787,6 +827,25 @@ private void shutdownChild(Process process) { } } + /** + * Converts a Gradle project path (as returned by + * {@link #groupFqnsByModule}) into the test task path used for + * both dispatch argv and the {@code --explain} Modules-block + * preview. Package-private so the unit tests can pin the root / + * subproject shapes directly, and shared between the two sites + * so the two lines of output an operator diffs + * (explain preview vs dispatch log) cannot drift out of sync. + * + *

Empty key (= root project) resolves to {@code :test} with a + * leading colon. The nested Gradle invocation accepts both + * {@code test} and {@code :test} for the root case, so this + * normalisation is cosmetic on the dispatch side but keeps the + * preview unambiguous. + */ + static String testTaskPath(String modulePath) { + return modulePath.isEmpty() ? ":test" : modulePath + ":test"; + } + /** * Groups discovered test FQNs by the Gradle path of the subproject that * owns each test file. Tests under the root project fall under the @@ -933,6 +992,96 @@ public Object[] args() { } } + /** Marker substring used by both the emission and the tests to pin the WARN. */ + static final String LOCAL_DISCOVERY_INCOMPLETE_WARNING_MARKER = + "affectedTest: LOCAL mode accepted a partial selection"; + + /** + * Emits a lifecycle-level WARN when the resolved run is a LOCAL-mode + * {@link Situation#DISCOVERY_INCOMPLETE} + {@link Action#SELECTED} + * combination — i.e. the workstation default that silently trusts + * a partial discovery set after the Java parser dropped one or + * more files. Deliberately WARN (not INFO): Gradle renders WARN in + * the default log level, so operators don't need to opt in via + * {@code --info} to see the risk. + * + *

Called before the {@code --explain} / dispatch fork so both + * paths surface the same concern: a diagnostic run reading the + * same partial selection that a non-diagnostic run would actually + * dispatch must raise the same alarm. Fires at most once per + * task execution — there is only one resolved situation per run. + * + *

The gate logic lives in the pure {@link #shouldWarnLocalDiscoveryIncomplete} + * and message formatting in {@link #formatLocalDiscoveryIncompleteWarning}; + * this instance method only pipes the result through the task's + * Gradle logger. Unit tests exercise the pure pair directly so + * the four-way gate (mode, situation, action, skipped/empty) is + * locked in without a log-capture fixture. + */ + void warnIfLocalDiscoveryIncompleteSelected(AffectedTestsConfig config, + AffectedTestsResult result) { + if (!shouldWarnLocalDiscoveryIncomplete(config, result)) { + return; + } + // Intentionally does NOT restate the parse-failure fact — the + // engine's own WARN (rendered a few lines above, with file-level + // detail) already did that. This line's job is the mode-specific + // postscript the engine can't emit (it doesn't know the mode): + // LOCAL chose to honour a partial selection, here's how to + // escalate if you'd rather not. + getLogger().warn(formatLocalDiscoveryIncompleteWarning(result.testClassFqns().size())); + } + + /** + * Pure gate for {@link #warnIfLocalDiscoveryIncompleteSelected}. + * Returns {@code true} iff the WARN should fire. Package-private + * for direct unit-test coverage of each guard: + *

    + *
  • Non-LOCAL modes never warn (CI / STRICT escalate, no + * under-testing risk).
    • + *
  • Only DISCOVERY_INCOMPLETE ever warns (parse failure is + * the only "selection was quietly partial" shape).
    • + *
  • Only SELECTED ever warns (FULL_SUITE / SKIPPED either + * already escalated or already bailed, so there is no + * silent partial selection to surface).
    • + *
  • Skipped results or empty FQN lists short-circuit — the + * engine rewrites "SELECTED with nothing to select" into + * {@code skipped=true}, and a zero-count WARN immediately + * before the task bails would contradict itself.
    • + *
+ */ + static boolean shouldWarnLocalDiscoveryIncomplete(AffectedTestsConfig config, + AffectedTestsResult result) { + if (config.effectiveMode() != Mode.LOCAL) { + return false; + } + if (result.situation() != Situation.DISCOVERY_INCOMPLETE) { + return false; + } + if (result.action() != Action.SELECTED) { + return false; + } + if (result.skipped() || result.testClassFqns().isEmpty()) { + return false; + } + return true; + } + + /** + * Formats the Risk C WARN line. Package-private so tests can pin + * both the stable marker substring (for grep-based alerting + * assertions) and the singular/plural "test class"/"test classes" + * toggle on the FQN count without reaching into a log capture. + */ + static String formatLocalDiscoveryIncompleteWarning(int selectedCount) { + String classWord = selectedCount == 1 ? "test class" : "test classes"; + return LOCAL_DISCOVERY_INCOMPLETE_WARNING_MARKER + " of " + selectedCount + + " " + classWord + " — see the discovery WARN above for the files " + + "that failed to parse. Fix the parse error to recover a precise " + + "selection, or set onDiscoveryIncomplete = 'full_suite' to escalate " + + "(CI and STRICT already do)."; + } + /** * Renders an {@link EscalationReason} as a short human-readable phrase * suitable for a lifecycle log line. Package-private so the log-shape @@ -1034,8 +1183,31 @@ static List renderLifecycleDispatchPreview(String taskPath, List * *

Package-private so {@code AffectedTestTaskExplainFormatTest} * can assert the format without spinning up Gradle. + * + * @param moduleGroups ordered map of {@code :module:test} task + * path → list of FQNs dispatched to that + * module. Pass {@link Map#of()} when no + * per-module breakdown applies (every + * situation other than + * {@link Situation#DISCOVERY_SUCCESS + + * SELECTED}). The renderer skips the + * "Modules:" block when the map is empty, so + * the trace stays compact on non-selective + * runs. */ static List renderExplainTrace(AffectedTestsConfig config, AffectedTestsResult result) { + // 2-arg overload: preserves the signature every unit test + // was written against before v2.2 added the per-module + // breakdown. The module block is a DISCOVERY_SUCCESS-only + // diagnostic that every non-dispatch unit test can safely + // skip by threading an empty map through to the 3-arg + // renderer. + return renderExplainTrace(config, result, Map.of()); + } + + static List renderExplainTrace(AffectedTestsConfig config, + AffectedTestsResult result, + Map> moduleGroups) { List lines = new ArrayList<>(); lines.add("=== Affected Tests — decision trace (--explain) ==="); lines.add("Base ref: " + config.baseRef()); @@ -1081,15 +1253,28 @@ static List renderExplainTrace(AffectedTestsConfig config, AffectedTests } lines.add("Outcome: " + outcome); - // Diagnostic hint: when out-of-scope dirs are configured but the - // bucket is empty, the config is almost certainly silently - // broken (wrong path, wrong glob shape, trailing-slash typo). - // We call it out inline so the operator sees the misconfiguration - // on the same trace that shows the buckets, rather than finding - // out 30 minutes later when a full suite runs that should have - // been skipped. Suppressed on empty-diff runs — there's nothing - // for the config to have bitten. - appendOutOfScopeHint(lines, config, result); + // Diagnostic hint: pick the hint that actually matches the + // situation the operator is staring at. Earlier versions + // unconditionally printed the out-of-scope hint on every + // mapper-touching situation, which meant a DISCOVERY_EMPTY run + // (no test mapped to the changed prod class) or + // DISCOVERY_INCOMPLETE run (parse failure dropped a file from + // the mapper) would show OOS advice that had nothing to do + // with the actual problem. v2.2 splits the hint into three + // targeted branches — see {@link #appendSituationHint} for the + // full routing. + appendSituationHint(lines, config, result); + + // Per-module dispatch preview — populated only for + // SELECTED runs so the "what tasks will Gradle actually + // kick off?" question can be answered directly from the + // explain trace instead of a dry-run dispatch. On every + // other outcome the map is empty and we skip the block + // entirely to keep the trace compact. Shares the same + // {@link #groupFqnsByModule} as the real dispatch path so a + // SELECTED --explain line can never contradict what the + // next non-explain run will actually execute. + appendModulesBlock(lines, moduleGroups); // The full action matrix is cheap to print (five rows) and // invaluable for debugging "why did my explicit setting not @@ -1108,44 +1293,64 @@ static List renderExplainTrace(AffectedTestsConfig config, AffectedTests } /** - * Emits the "configured but matched nothing" hint for out-of-scope - * dirs when the signal points to a silent misconfiguration. Kept - * package-private so the explain-format tests can pin the exact + * Routes to the situation-specific hint block for the + * {@code --explain} trace. Kept package-private so + * {@code AffectedTestTaskExplainFormatTest} can pin the exact * conditions without spinning up Gradle. * - *

Gate: only {@link Situation#DISCOVERY_SUCCESS}, - * {@link Situation#DISCOVERY_EMPTY}, and {@link Situation#DISCOVERY_INCOMPLETE} - * can have been influenced by an out-of-scope pattern — all three - * run the mapper over the full diff and only then route post- - * discovery. The other four situations reach their - * outcome for reasons the out-of-scope bucket cannot change: - * {@link Situation#EMPTY_DIFF} (no files to match), - * {@link Situation#ALL_FILES_IGNORED} (ignore wins before - * out-of-scope ever looks at the file), - * {@link Situation#ALL_FILES_OUT_OF_SCOPE} (bucket is non-empty, so - * the "silent" precondition is false — also caught by the - * bucket-empty guard below), and {@link Situation#UNMAPPED_FILE} - * (by construction the file missed every pattern, including - * out-of-scope). Firing here trains reviewers to ignore the hint - * on routine docs-only / gradle-only MRs — that's the v1.9.17 - * sanity-test finding this gate exists to close. - * - *

Other preconditions: diff non-empty, at least one of - * {@code outOfScopeTestDirs} / {@code outOfScopeSourceDirs} - * configured, and the out-of-scope bucket empty. + *

Only the three mapper-touching situations produce hints in + * the v2.2+ trace: {@link Situation#DISCOVERY_SUCCESS} carries the + * original "out-of-scope configured but nothing matched" + * misconfiguration tell; {@link Situation#DISCOVERY_EMPTY} calls + * out the "mapped 0 tests" case with naming-suffix advice; and + * {@link Situation#DISCOVERY_INCOMPLETE} flags the parse-failure + * path so an operator doesn't silently accept a partial selection. + * The other four situations ({@link Situation#EMPTY_DIFF}, + * {@link Situation#ALL_FILES_IGNORED}, + * {@link Situation#ALL_FILES_OUT_OF_SCOPE}, + * {@link Situation#UNMAPPED_FILE}) reach their outcome for reasons + * none of these hints can usefully add to — so we stay silent and + * let the {@code Outcome:} line speak for itself. */ - static void appendOutOfScopeHint(List lines, - AffectedTestsConfig config, - AffectedTestsResult result) { - Situation situation = result.situation(); - if (situation != Situation.DISCOVERY_SUCCESS - && situation != Situation.DISCOVERY_EMPTY - && situation != Situation.DISCOVERY_INCOMPLETE) { - return; - } - if (result.changedFiles().isEmpty()) { - return; + static void appendSituationHint(List lines, + AffectedTestsConfig config, + AffectedTestsResult result) { + // Note on the removed `changedFiles().isEmpty()` guard that + // lived here pre-v2.2.1: the three situations the switch routes + // (DISCOVERY_SUCCESS / DISCOVERY_EMPTY / DISCOVERY_INCOMPLETE) + // all definitionally require at least one changed file to + // reach them — EMPTY_DIFF is the "no files" situation and it + // falls through the default branch. The guard was unreachable + // in production and gave a misleading impression that + // changed-file-count was part of the hint gate, so deleting + // it keeps the dispatch contract legible. + switch (result.situation()) { + case DISCOVERY_SUCCESS -> appendOutOfScopeMisconfigHint(lines, config, result); + case DISCOVERY_EMPTY -> appendDiscoveryEmptyHint(lines, config, result); + case DISCOVERY_INCOMPLETE -> appendDiscoveryIncompleteHint(lines, result); + default -> { + // No hint for EMPTY_DIFF / ALL_FILES_IGNORED / + // ALL_FILES_OUT_OF_SCOPE / UNMAPPED_FILE — see + // method-level javadoc for the per-situation rationale. + } } + } + + /** + * Fires on {@link Situation#DISCOVERY_SUCCESS} when + * {@code outOfScopeTestDirs} / {@code outOfScopeSourceDirs} were + * configured but no file in the diff matched any of them. The + * heuristic catches the silent-misconfig case that lands + * OOS-shaped MRs (docs-only, tooling-only) into + * DISCOVERY_SUCCESS-instead-of-SKIPPED: wrong path, wrong glob + * shape, trailing-slash typo. Suppressed on runs where the OOS + * bucket actually non-empty — the configuration clearly works on + * this diff, so firing would only train reviewers to ignore the + * hint. + */ + private static void appendOutOfScopeMisconfigHint(List lines, + AffectedTestsConfig config, + AffectedTestsResult result) { if (!result.buckets().outOfScopeFiles().isEmpty()) { return; } @@ -1173,6 +1378,135 @@ static void appendOutOfScopeHint(List lines, + "(e.g. 'api-test/src/test/java') or globs (e.g. 'api-test/**')."); } + /** + * Fires on {@link Situation#DISCOVERY_EMPTY} — the engine mapped + * production .java changes but no test class matched any strategy. + * v2.1 printed the OOS hint here which was actively misleading + * because the OOS bucket by definition never influenced the + * outcome; v2.2 swaps in the three things that actually produce + * an empty discovery set: test-file naming mismatches the + * configured suffix list, the test lives outside the configured + * {@code testDirs}, or the prod class genuinely has no test + * coverage yet. We enumerate the first two with the operator's + * actual config values so the hint is self-checking. + */ + private static void appendDiscoveryEmptyHint(List lines, + AffectedTestsConfig config, + AffectedTestsResult result) { + int prodFileCount = result.buckets().productionFiles().size(); + String fileWord = prodFileCount == 1 ? "file" : "files"; + lines.add("Hint: discovery mapped 0 test classes to the " + + prodFileCount + " changed production " + fileWord + "."); + lines.add(" Common causes:"); + lines.add(" * test name does not match testSuffixes " + + formatInlineList(config.testSuffixes()) + + " (e.g. Foo.java → FooTest.java / FooIT.java)"); + lines.add(" * test lives outside testDirs " + + formatInlineList(config.testDirs())); + lines.add(" * the production class has no test coverage yet"); + } + + /** + * Fires on {@link Situation#DISCOVERY_INCOMPLETE} — one or more + * Java files in the diff failed to parse, so the mapper ran with + * missing inputs. Two shapes reach this hint: + * + *

    + *
  • {@link Action#SELECTED} — LOCAL-mode default. The discovered + * selection is definitionally partial, and the hint names + * that risk explicitly plus the escalation knob an operator + * can flip to move off the partial-selection default. Pairs + * with the lifecycle WARN in + * {@link #warnIfLocalDiscoveryIncompleteSelected}.
    • + *
  • {@link Action#FULL_SUITE} — CI/STRICT default, or an + * explicit {@code onDiscoveryIncomplete='full_suite'} + * override. The whole suite runs, so "partial selection" + * wording is actively wrong and "let onDiscoveryIncomplete + * escalate" is circular — escalation already happened. We + * render a trimmed hint that just names the parse failure + * and the precise-selection follow-up.
    • + *
+ * + *

We don't count "parse failures" directly because the engine + * doesn't surface that number today; an operator who wants the + * exact file list reads the INFO-level engine log. The hint stays + * action-shape-agnostic on the file count so it can't drift into + * "0 files failed to parse" wording on edge cases. + */ + private static void appendDiscoveryIncompleteHint(List lines, + AffectedTestsResult result) { + if (result.action() == Action.SELECTED) { + lines.add("Hint: one or more Java files in the diff failed to parse, " + + "so discovery ran with missing inputs."); + lines.add(" The resolved selection is necessarily partial — fix the " + + "parse error to recover a precise selection, or set " + + "onDiscoveryIncomplete = 'full_suite' to escalate " + + "(CI and STRICT modes already do)."); + return; + } + // FULL_SUITE (or any non-SELECTED resolution): escalation has + // already handled the under-testing risk, so the hint's job + // is only to point the operator at the root cause for next + // time — NOT to repeat the escalation advice. + lines.add("Hint: one or more Java files in the diff failed to parse, " + + "so discovery ran with missing inputs."); + lines.add(" Fix the parse error to recover a precise selection on " + + "future runs (until then the resolved action above is the safe fallback)."); + } + + private static String formatInlineList(List items) { + if (items.isEmpty()) { + return "[]"; + } + return items.stream().collect(Collectors.joining(", ", "[", "]")); + } + + /** + * Renders the "Modules:" section of the {@code --explain} trace. + * No-op when the map is empty — every non-SELECTED run threads an + * empty map through so those traces stay compact. On SELECTED + * runs we list each {@code :module:test} target with its class + * count, then up to + * {@link #LIFECYCLE_FQN_PREVIEW_LIMIT} FQNs per module, matching + * the same preview cap the actual dispatch path uses — so an + * operator comparing an {@code --explain} trace against a + * subsequent non-explain run sees the same preview shape. + * + *

Package-private so + * {@code AffectedTestTaskExplainFormatTest} can pin both the + * empty-map fast path and the preview shape without spinning up + * Gradle. + */ + static void appendModulesBlock(List lines, Map> moduleGroups) { + if (moduleGroups == null || moduleGroups.isEmpty()) { + return; + } + int totalFqns = moduleGroups.values().stream().mapToInt(List::size).sum(); + String moduleWord = moduleGroups.size() == 1 ? "module" : "modules"; + String classWord = totalFqns == 1 ? "class" : "classes"; + lines.add("Modules: " + moduleGroups.size() + " " + moduleWord + ", " + + totalFqns + " test " + classWord + " to dispatch"); + for (Map.Entry> entry : moduleGroups.entrySet()) { + String taskPath = entry.getKey(); + List fqns = entry.getValue(); + // A Gradle task path always starts with ':'; the dispatch + // helper stores plain `test` for the root project though, + // so normalise here to keep the explain output consistent + // with a Gradle-CLI-shaped task label in both cases. + String normalised = taskPath.startsWith(":") ? taskPath : ":" + taskPath; + int size = fqns.size(); + String rowClassWord = size == 1 ? "class" : "classes"; + lines.add(" " + normalised + " (" + size + " test " + rowClassWord + ")"); + int preview = Math.min(size, LIFECYCLE_FQN_PREVIEW_LIMIT); + for (int i = 0; i < preview; i++) { + lines.add(" " + fqns.get(i)); + } + if (size > preview) { + lines.add(" … and " + (size - preview) + " more (use --info for full list)"); + } + } + } + private static void appendSample(List lines, String label, Set files) { if (files.isEmpty()) { return; diff --git a/affected-tests-gradle/src/main/java/io/affectedtests/gradle/AffectedTestsPlugin.java b/affected-tests-gradle/src/main/java/io/affectedtests/gradle/AffectedTestsPlugin.java index 48442cf..1c8156f 100644 --- a/affected-tests-gradle/src/main/java/io/affectedtests/gradle/AffectedTestsPlugin.java +++ b/affected-tests-gradle/src/main/java/io/affectedtests/gradle/AffectedTestsPlugin.java @@ -3,12 +3,14 @@ import org.gradle.api.GradleException; import org.gradle.api.Plugin; import org.gradle.api.Project; +import org.gradle.api.Task; import org.gradle.api.file.Directory; import java.io.File; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.concurrent.Callable; /** * Gradle plugin that registers the {@code affectedTest} task and the @@ -27,6 +29,19 @@ public void apply(Project project) { project.getProviders().gradleProperty("affectedTestsBaseRef") .orElse("origin/master") ); + // Mirror the baseRef pattern: let callers flip mode from the + // command line via `-PaffectedTestsMode=local|ci|strict|auto` + // without editing build.gradle. Useful for adoption experiments + // ("what would STRICT mode pick on today's HEAD?") and for CI + // jobs that want to A/B two modes from the same pipeline. + // DSL-declared `mode = '...'` still wins because Gradle's + // Property resolution applies explicit sets before conventions; + // when both the DSL and the -P are absent, no value is + // present and the core config falls through to AUTO — same as + // before this convention existed. + extension.getMode().convention( + project.getProviders().gradleProperty("affectedTestsMode") + ); // COMMITTED-ONLY by default: the plugin's whole job is "what // tests does this MR touch?", and the MR is the committed diff // — not the dev's WIP. Matching this default to the CI reality @@ -83,11 +98,30 @@ public void apply(Project project) { task.getRootDir().set(rootDir); task.getSubprojectPaths().set(project.provider(() -> collectSubprojectPaths(rootProject))); - // Ensure test classes are compiled before the nested gradle invocation - // runs. Without this, a fresh CI checkout would have nothing to test - // and Gradle would fail with "No tests found for given includes". - rootProject.allprojects(p -> p.getPluginManager().withPlugin("java", unused -> - task.dependsOn(p.getTasks().named("testClasses")))); + // Ensure test classes are compiled before the nested gradle + // invocation runs. Without this, a fresh CI checkout would + // have nothing to test and Gradle would fail with "No tests + // found for given includes". + // + // Wrapped in a {@link Callable} so the dependency is + // materialised at task-graph calculation time, which is + // after Gradle has parsed {@code --explain} into + // {@link AffectedTestTask#getExplain()}. When the operator + // asked for the decision trace we skip every + // {@code testClasses} dependency — {@code --explain} + // short-circuits before dispatch and never consumes a + // class file, so forcing a compile turns a 3-second + // diagnostic into a multi-minute full compile. The + // dispatch path still picks them up as before. + rootProject.allprojects(p -> p.getPluginManager().withPlugin("java", unused -> { + Callable> testClassesWhenDispatching = () -> { + if (Boolean.TRUE.equals(task.getExplain().getOrElse(false))) { + return List.of(); + } + return List.of(p.getTasks().named("testClasses").get()); + }; + task.dependsOn(testClassesWhenDispatching); + })); }); // Validate scalar-range constraints at configuration completion so diff --git a/affected-tests-gradle/src/test/java/io/affectedtests/gradle/AffectedTestTaskExplainFormatTest.java b/affected-tests-gradle/src/test/java/io/affectedtests/gradle/AffectedTestTaskExplainFormatTest.java index 3cce494..411a9bb 100644 --- a/affected-tests-gradle/src/test/java/io/affectedtests/gradle/AffectedTestTaskExplainFormatTest.java +++ b/affected-tests-gradle/src/test/java/io/affectedtests/gradle/AffectedTestTaskExplainFormatTest.java @@ -458,13 +458,12 @@ void hintDoesNotFireOnUnmappedFileEscalation() { void hintFiresOnDiscoveryEmptyFullSuiteEscalation() { // Positive case: when the diff changed real production code but // discovery found zero affected tests, the engine escalates to - // FULL_SUITE under the CI mode's safety net. The hint is STILL - // useful here — if the user had outOfScopeSourceDirs meant to - // exclude this production file from dispatching tests and the - // pattern silently didn't bite, the FULL_SUITE escalation is the - // unnecessary waste the hint exists to expose. Lock in that the - // situation-gate in appendOutOfScopeHint includes DISCOVERY_EMPTY - // alongside DISCOVERY_SUCCESS. + // FULL_SUITE under the CI mode's safety net. Pre-v2.2 the hint + // here was the OOS-misconfig text; v2.2 replaces it with the + // "discovery mapped 0 test classes" advice (naming suffixes, + // testDirs, no-coverage-yet). Either way the `Hint:` line + // must be present — its absence would silence the most + // expensive diagnostic signal the plugin emits. AffectedTestsConfig config = AffectedTestsConfig.builder() .outOfScopeSourceDirs(List.of("legacy-service/**")) .build(); @@ -540,4 +539,491 @@ void hintDoesNotFireOnEmptyDiff() { assertFalse(trace.contains("Hint:"), "Hint must stay silent when there are no changed files — nothing to diagnose"); } + + // ------------------------------------------------------------------ + // v2.2 — situation-specific Hint branches + // + // Pre-v2.2 every mapper-touching situation produced the same + // "outOfScopeTestDirs is configured but no file matched" hint. + // That was actively misleading on DISCOVERY_EMPTY runs (where the + // OOS knob was not the reason discovery mapped zero tests) and + // on DISCOVERY_INCOMPLETE runs (where a Java parse failure, not + // an OOS matcher, dropped files from the mapper). v2.2 splits + // the single hint into three targeted branches; these tests + // lock in each branch's content so regressions show up as test + // failures rather than as user-invisible wording drift. + // ------------------------------------------------------------------ + + @Test + void discoveryEmptyHintNamesNamingSuffixesAndTestDirs() { + // When the engine maps zero tests to a production change, the + // three realistic causes are: test-suffix mismatch, a test + // file outside the configured testDirs, or genuinely no test + // coverage yet. The hint must list all three in that order + // with the user's actual config values so they can self-check + // "does my testSuffixes list include 'IT'?" without leaving + // the trace. Verifying the content here (not just "Hint:" + // presence) would have caught the pre-v2.2 behaviour where + // this situation silently printed out-of-scope-dirs advice. + AffectedTestsConfig config = AffectedTestsConfig.builder() + .testSuffixes(List.of("Test", "IT")) + .testDirs(List.of("src/test/java")) + .build(); + AffectedTestsResult result = new AffectedTestsResult( + Set.of(), Map.of(), + Set.of("src/main/java/com/example/Foo.java"), + Set.of("com.example.Foo"), + Set.of(), + new Buckets(Set.of(), Set.of(), + Set.of("src/main/java/com/example/Foo.java"), + Set.of(), Set.of()), + true, false, + Situation.DISCOVERY_EMPTY, Action.FULL_SUITE, + EscalationReason.RUN_ALL_IF_NO_MATCHES); + + String trace = joined(AffectedTestTask.renderExplainTrace(config, result)); + + assertTrue(trace.contains("Hint: discovery mapped 0 test classes"), + "DISCOVERY_EMPTY hint must lead with 'mapped 0 test classes' — that names the " + + "actual shape of the problem, not an unrelated OOS misconfig claim. Got:\n" + trace); + assertTrue(trace.contains("testSuffixes [Test, IT]"), + "Hint must echo the user's configured testSuffixes so they can verify " + + "the naming convention matches their test files. Got:\n" + trace); + assertTrue(trace.contains("testDirs [src/test/java]"), + "Hint must echo testDirs so the operator can spot a test file placed " + + "outside the configured roots. Got:\n" + trace); + assertTrue(trace.contains("no test coverage yet"), + "Hint must explicitly call out the no-coverage case — otherwise users with " + + "genuinely untested classes will loop on naming suggestions. Got:\n" + trace); + assertFalse(trace.contains("outOfScopeTestDirs") || trace.contains("outOfScopeSourceDirs"), + "DISCOVERY_EMPTY hint must NOT mention out-of-scope knobs — OOS is orthogonal " + + "to why discovery mapped 0 tests, and surfacing it was the exact confusion " + + "v2.1 produced. Got:\n" + trace); + } + + @Test + void discoveryIncompleteHintCallsOutPartialSelectionRisk() { + // Parse failure means the mapper ran with missing inputs. + // The v2.1 trace printed "outOfScopeTestDirs is configured" + // here too, even when the user had no OOS entries — pure + // noise. v2.2 replaces that with the thing the operator + // actually needs to know: the selection is partial, and the + // fix is to either repair the parse error or escalate via + // onDiscoveryIncomplete. + AffectedTestsConfig config = AffectedTestsConfig.builder().build(); + AffectedTestsResult result = new AffectedTestsResult( + Set.of("com.example.FooTest"), Map.of(), + Set.of("src/main/java/com/example/Foo.java"), + Set.of("com.example.Foo"), + Set.of(), + new Buckets(Set.of(), Set.of(), + Set.of("src/main/java/com/example/Foo.java"), + Set.of(), Set.of()), + false, false, + Situation.DISCOVERY_INCOMPLETE, Action.SELECTED, + EscalationReason.NONE); + + String trace = joined(AffectedTestTask.renderExplainTrace(config, result)); + + assertTrue(trace.contains("failed to parse"), + "DISCOVERY_INCOMPLETE hint must name the parse-failure cause so the " + + "operator understands why selection is partial. Got:\n" + trace); + assertTrue(trace.contains("selection is necessarily partial"), + "Hint must explicitly warn that the selection is partial — that's the " + + "silent correctness risk v2.2 was raised to surface. Got:\n" + trace); + assertTrue(trace.contains("onDiscoveryIncomplete"), + "Hint must name the knob that toggles this behaviour so the operator " + + "has an actionable next step. Got:\n" + trace); + } + + @Test + void discoveryIncompleteHintOnFullSuiteDropsPartialSelectionWording() { + // v2.2.1 fix (H3 from CAR-5190 code review): CI/STRICT + // defaults — and explicit operator overrides — route + // DISCOVERY_INCOMPLETE to FULL_SUITE. The old shared hint + // claimed the "selection is necessarily partial" which is + // factually wrong (the full suite ran) and advised escalating + // via onDiscoveryIncomplete which is circular (escalation + // already happened). Pin the corrected wording: the hint + // names the parse failure as the root cause but does NOT + // claim a partial selection, and does NOT re-recommend the + // escalation that the current run already took. + AffectedTestsConfig config = AffectedTestsConfig.builder().mode(Mode.CI).build(); + AffectedTestsResult result = new AffectedTestsResult( + Set.of(), Map.of(), + Set.of("src/main/java/com/example/Foo.java"), + Set.of("com.example.Foo"), Set.of(), + new Buckets(Set.of(), Set.of(), + Set.of("src/main/java/com/example/Foo.java"), + Set.of(), Set.of()), + true, false, + Situation.DISCOVERY_INCOMPLETE, Action.FULL_SUITE, + EscalationReason.RUN_ALL_ON_NON_JAVA_CHANGE); + + String trace = joined(AffectedTestTask.renderExplainTrace(config, result)); + + assertTrue(trace.contains("failed to parse"), + "FULL_SUITE hint must still name the parse-failure cause so the operator " + + "can fix it next run. Got:\n" + trace); + assertFalse(trace.contains("selection is necessarily partial"), + "FULL_SUITE ran the whole suite — claiming the selection was partial " + + "contradicts the resolved action and actively misleads the operator. " + + "Got:\n" + trace); + assertFalse(trace.contains("onDiscoveryIncomplete = 'full_suite' to escalate"), + "The escalation has already happened on this run — repeating the 'set it " + + "to full_suite' advice is circular and trains operators to " + + "ignore the hint. Got:\n" + trace); + } + + @Test + void discoverySuccessHintKeepsV2dot1OutOfScopeMisconfigWording() { + // Regression: v2.2's hint refactor must preserve the single + // DISCOVERY_SUCCESS case v2.1 actually diagnosed correctly — + // "outOfScopeTestDirs is configured (N entries) but no file + // in the diff matched." Losing that would take the plugin + // backwards, since the silent-OOS-misconfig is exactly the + // failure mode the pre-v1.9.17 sanity-test caught. + AffectedTestsConfig config = AffectedTestsConfig.builder() + .outOfScopeTestDirs(List.of("api-test/**")) + .build(); + AffectedTestsResult result = new AffectedTestsResult( + Set.of("com.example.FooTest"), Map.of(), + Set.of("src/main/java/com/example/Foo.java"), + Set.of("com.example.Foo"), + Set.of("com.example.FooTest"), + new Buckets(Set.of(), Set.of(), + Set.of("src/main/java/com/example/Foo.java"), + Set.of(), Set.of()), + false, false, + Situation.DISCOVERY_SUCCESS, Action.SELECTED, + EscalationReason.NONE); + + String trace = joined(AffectedTestTask.renderExplainTrace(config, result)); + + assertTrue(trace.contains("outOfScopeTestDirs is configured (1 entry)"), + "DISCOVERY_SUCCESS must still surface the OOS-configured-but-nothing-matched " + + "misconfig — that's the v2.1 behaviour we are NOT breaking. Got:\n" + trace); + } + + // ------------------------------------------------------------------ + // v2.2 — Modules: per-task dispatch preview in --explain + // ------------------------------------------------------------------ + + @Test + void modulesBlockIsAbsentWhenNoGroupsPassed() { + // Every pre-v2.2 caller (and every non-SELECTED situation in + // v2.2) threads an empty map — the trace must stay compact + // in that case. Otherwise every EMPTY_DIFF / SKIPPED run + // would grow a useless "Modules: 0 modules, 0 test classes" + // line. + AffectedTestsConfig config = AffectedTestsConfig.builder().build(); + AffectedTestsResult result = new AffectedTestsResult( + Set.of(), Map.of(), + Set.of(), Set.of(), Set.of(), + Buckets.empty(), + false, true, + Situation.EMPTY_DIFF, Action.SKIPPED, + EscalationReason.NONE); + + String trace = joined(AffectedTestTask.renderExplainTrace(config, result, Map.of())); + + assertFalse(trace.contains("Modules:"), + "Modules block must be suppressed on empty map — SELECTED is the only " + + "situation where dispatch preview is meaningful. Got:\n" + trace); + } + + @Test + void modulesBlockRendersPerTaskBreakdownForMultiModuleSelection() { + // The real-world adoption scenario: production change in one + // module (`api/`) dispatches tests into another (`application/`). + // v2.1's --explain trace gave only a total test count and left + // the operator guessing which task Gradle would actually + // invoke. v2.2 prints the per-:module:test breakdown matching + // the real dispatch output so an --explain run answers the + // "what will Gradle kick off?" question directly. + AffectedTestsConfig config = AffectedTestsConfig.builder().build(); + AffectedTestsResult result = new AffectedTestsResult( + Set.of("com.example.FooTest", "com.example.BarTest", "com.example.BazTest"), + Map.of(), + Set.of("api/src/main/java/com/example/Foo.java"), + Set.of("com.example.Foo"), + Set.of("com.example.FooTest", "com.example.BarTest", "com.example.BazTest"), + new Buckets(Set.of(), Set.of(), + Set.of("api/src/main/java/com/example/Foo.java"), + Set.of(), Set.of()), + false, false, + Situation.DISCOVERY_SUCCESS, Action.SELECTED, + EscalationReason.NONE); + + Map> groups = new java.util.LinkedHashMap<>(); + groups.put(":application:test", List.of("com.example.FooTest", "com.example.BarTest")); + groups.put(":api:test", List.of("com.example.BazTest")); + + String trace = joined(AffectedTestTask.renderExplainTrace(config, result, groups)); + + assertTrue(trace.contains("Modules: 2 modules, 3 test classes to dispatch"), + "Modules summary must count modules and classes so the operator can eyeball " + + "the dispatch shape before running. Got:\n" + trace); + assertTrue(trace.contains(":application:test (2 test classes)"), + "Per-module row must name the task path and class count — exact same preview " + + "shape as the non-explain dispatch log, so switching between modes is " + + "a cognitive no-op. Got:\n" + trace); + assertTrue(trace.contains(":api:test (1 test class)"), + "Singular form 'test class' must render for 1-class modules — keeps the " + + "preview grammatical on small dispatches. Got:\n" + trace); + assertTrue(trace.contains(" com.example.FooTest") + && trace.contains(" com.example.BarTest"), + "Per-module FQN preview must indent the FQNs under their module row so the " + + "hierarchy reads naturally. Got:\n" + trace); + } + + @Test + void testTaskPathHelperNormalisesRootProjectToLeadingColon() { + // v2.2.1 (N1 from the code review): both the explain-block + // preview and the dispatch argv go through the shared + // AffectedTestTask#testTaskPath helper so the two operator- + // facing strings cannot drift. Pin the root-project case + // ("" → ":test") directly on the helper — the explain-side + // rendering just iterates this helper's output, so a + // per-renderer test would duplicate coverage. + assertTrue(":test".equals(AffectedTestTask.testTaskPath("")), + "Empty project path (root project) must render as ':test' with a leading " + + "colon so explain and dispatch name the task identically"); + assertTrue(":api:test".equals(AffectedTestTask.testTaskPath(":api")), + "Non-root project path must suffix ':test' — regression on this shape " + + "would make every non-root dispatch target the wrong task name"); + } + + @Test + void modulesBlockRendersHelperNormalisedRootTask() { + // Paired assertion: once the helper returns ':test', the + // renderer must pass it through verbatim. Prevents a drift + // where someone "fixes" the helper but leaves the renderer + // stripping the colon for root-module aesthetic reasons. + AffectedTestsConfig config = AffectedTestsConfig.builder().build(); + AffectedTestsResult result = new AffectedTestsResult( + Set.of("com.example.FooTest"), Map.of(), + Set.of("src/main/java/com/example/Foo.java"), + Set.of("com.example.Foo"), + Set.of("com.example.FooTest"), + new Buckets(Set.of(), Set.of(), + Set.of("src/main/java/com/example/Foo.java"), + Set.of(), Set.of()), + false, false, + Situation.DISCOVERY_SUCCESS, Action.SELECTED, + EscalationReason.NONE); + + String trace = joined(AffectedTestTask.renderExplainTrace(config, result, + Map.of(AffectedTestTask.testTaskPath(""), + List.of("com.example.FooTest")))); + + assertTrue(trace.contains(":test (1 test class)"), + "Root-project task must render as ':test' in the explain trace so every row " + + "reads as a Gradle task path. Got:\n" + trace); + } + + @Test + void modulesBlockTruncatesWithAndNMoreLineOverPreviewLimit() { + // Match the dispatch-path preview behaviour: print the first + // LIFECYCLE_FQN_PREVIEW_LIMIT FQNs, then a single "… and N + // more (use --info for full list)" trailer. Parity with the + // dispatch preview means an --explain run and a subsequent + // non-explain run show the same first N names — no cognitive + // load to map one to the other. + AffectedTestsConfig config = AffectedTestsConfig.builder().build(); + List fqns = new java.util.ArrayList<>(); + for (int i = 0; i < AffectedTestTask.LIFECYCLE_FQN_PREVIEW_LIMIT + 3; i++) { + fqns.add("com.example.Test" + i); + } + AffectedTestsResult result = new AffectedTestsResult( + Set.copyOf(fqns), Map.of(), + Set.of("src/main/java/com/example/Foo.java"), + Set.of("com.example.Foo"), + Set.copyOf(fqns), + new Buckets(Set.of(), Set.of(), + Set.of("src/main/java/com/example/Foo.java"), + Set.of(), Set.of()), + false, false, + Situation.DISCOVERY_SUCCESS, Action.SELECTED, + EscalationReason.NONE); + + String trace = joined(AffectedTestTask.renderExplainTrace(config, result, + Map.of(":application:test", fqns))); + + assertTrue(trace.contains("… and 3 more (use --info for full list)"), + "Trailer must name the remainder count and point at --info for the full set " + + "— any other phrasing drifts from the dispatch-path preview and trains " + + "operators to mistrust the trace. Got:\n" + trace); + } + + // ------------------------------------------------------------------ + // v2.2 — Risk C: LOCAL + DISCOVERY_INCOMPLETE + SELECTED WARN gate + // + // The instance method that actually emits the WARN is one-liner + // plumbing over the Gradle logger; all decision logic lives in + // shouldWarnLocalDiscoveryIncomplete (pure) and the message in + // formatLocalDiscoveryIncompleteWarning (pure). The tests exercise + // each guard on the pure gate so a regression on any of the four + // conditions (mode, situation, action, skipped/empty) surfaces in + // ms — without requiring a log-capture fixture — and mirrors what + // the Javadoc already promised. + // ------------------------------------------------------------------ + private static AffectedTestsConfig configWithMode(Mode mode) { + return AffectedTestsConfig.builder().mode(mode).build(); + } + + private static AffectedTestsResult makeResult(Situation situation, + Action action, + boolean skipped, + int fqnCount) { + Set fqns = new java.util.LinkedHashSet<>(); + for (int i = 0; i < fqnCount; i++) { + fqns.add("com.example.Test" + i); + } + return new AffectedTestsResult( + fqns, Map.of(), + Set.of("src/main/java/com/example/Foo.java"), + Set.of("com.example.Foo"), Set.copyOf(fqns), + new Buckets(Set.of(), Set.of(), + Set.of("src/main/java/com/example/Foo.java"), + Set.of(), Set.of()), + false, skipped, + situation, action, + EscalationReason.NONE); + } + + @Test + void riskCWarnFiresOnLocalDiscoveryIncompleteSelectedWithFqns() { + // The one combination the v2.2 fix was added for: LOCAL keeps + // `onDiscoveryIncomplete = SELECTED` by design, so the engine + // hands back a non-empty partial selection and the task must + // surface that silently-partial result to the operator. + AffectedTestsConfig config = configWithMode(Mode.LOCAL); + AffectedTestsResult result = makeResult( + Situation.DISCOVERY_INCOMPLETE, Action.SELECTED, false, 3); + + assertTrue(AffectedTestTask.shouldWarnLocalDiscoveryIncomplete(config, result), + "LOCAL + DISCOVERY_INCOMPLETE + SELECTED + non-empty FQNs is the exact " + + "v2.2 Risk C scenario — dropping the WARN here re-opens the silent " + + "partial-selection hole that CAR-5190 surfaced"); + } + + @Test + void riskCWarnSuppressedInCiModeOnIdenticalResult() { + // CI (and STRICT) default onDiscoveryIncomplete to FULL_SUITE + // so the engine never hands them a "SELECTED on incomplete + // discovery" result through the mode default. But a CI + // operator who explicitly overrode that to SELECTED should + // still NOT see a LOCAL-branded WARN — it would name the + // wrong mode and mis-guide the operator. Pin the gate on + // effectiveMode, not on the situation alone. + AffectedTestsConfig config = configWithMode(Mode.CI); + AffectedTestsResult result = makeResult( + Situation.DISCOVERY_INCOMPLETE, Action.SELECTED, false, 3); + + assertFalse(AffectedTestTask.shouldWarnLocalDiscoveryIncomplete(config, result), + "CI must never emit the LOCAL-specific WARN — wording names LOCAL's " + + "onDiscoveryIncomplete default and would be factually wrong in CI"); + } + + @Test + void riskCWarnSuppressedOnDiscoverySuccess() { + // DISCOVERY_SUCCESS with no parse failures is the happy path. + // The WARN is for parse-failure-induced partial selections + // specifically — any other situation triggering it would train + // operators that the signal means nothing. + AffectedTestsConfig config = configWithMode(Mode.LOCAL); + AffectedTestsResult result = makeResult( + Situation.DISCOVERY_SUCCESS, Action.SELECTED, false, 3); + + assertFalse(AffectedTestTask.shouldWarnLocalDiscoveryIncomplete(config, result), + "DISCOVERY_SUCCESS must never emit the WARN — there is no incomplete " + + "discovery to warn about"); + } + + @Test + void riskCWarnSuppressedOnFullSuiteEscalation() { + // An operator running LOCAL mode with an explicit + // `onDiscoveryIncomplete='full_suite'` override gets + // DISCOVERY_INCOMPLETE + FULL_SUITE. No silent partial + // selection exists (the full suite will run), so the WARN + // would contradict the explicit operator choice. + AffectedTestsConfig config = configWithMode(Mode.LOCAL); + AffectedTestsResult result = makeResult( + Situation.DISCOVERY_INCOMPLETE, Action.FULL_SUITE, false, 0); + + assertFalse(AffectedTestTask.shouldWarnLocalDiscoveryIncomplete(config, result), + "FULL_SUITE escalation removes the under-testing risk — the WARN must " + + "NOT fire or it would contradict the operator's explicit choice to " + + "escalate"); + } + + @Test + void riskCWarnSuppressedWhenEngineRewritesToSkipped() { + // The engine treats SELECTED with nothing to dispatch as + // skipped=true (`action == SELECTED && situation != DISCOVERY_SUCCESS` + // → skipped). Without this guard the WARN would fire + // "(0 test classes)" and then the task would bail before + // running anything — two contradictory log lines from the + // same code path. + AffectedTestsConfig config = configWithMode(Mode.LOCAL); + AffectedTestsResult result = makeResult( + Situation.DISCOVERY_INCOMPLETE, Action.SELECTED, true, 0); + + assertFalse(AffectedTestTask.shouldWarnLocalDiscoveryIncomplete(config, result), + "skipped=true means the engine already routed this run to nothing-to-dispatch " + + "— firing the WARN would claim a partial selection was accepted when " + + "nothing was actually dispatched"); + } + + @Test + void riskCWarnSuppressedWhenFqnListIsEmptyWithoutSkipped() { + // Defensive: should-never-happen but if a future code path + // lands SELECTED + empty FQNs without the skipped flag, the + // WARN must still suppress so an operator is never told a + // zero-count selection was "accepted". + AffectedTestsConfig config = configWithMode(Mode.LOCAL); + AffectedTestsResult result = makeResult( + Situation.DISCOVERY_INCOMPLETE, Action.SELECTED, false, 0); + + assertFalse(AffectedTestTask.shouldWarnLocalDiscoveryIncomplete(config, result), + "Empty FQN list means no partial selection was actually honoured — the " + + "WARN must suppress even if the engine forgot to mark the result skipped"); + } + + @Test + void riskCWarnMessageNamesMarkerAndEscalationKnob() { + // The marker substring doubles as a grep target for CI + // alerting (see LOCAL_DISCOVERY_INCOMPLETE_WARNING_MARKER + // javadoc) and the message must name the exact knob an + // operator can flip — anything vaguer sends the operator + // grepping through the CHANGELOG. + String message = AffectedTestTask.formatLocalDiscoveryIncompleteWarning(3); + + assertTrue(message.contains(AffectedTestTask.LOCAL_DISCOVERY_INCOMPLETE_WARNING_MARKER), + "Message must contain the stable marker substring so grep-based alerting " + + "keeps working across wording refreshes"); + assertTrue(message.contains("3 test classes"), + "Plural form must render the count — ambiguity on " + + "'how much did we actually accept' defeats the WARN's purpose. Got: " + + message); + assertTrue(message.contains("onDiscoveryIncomplete = 'full_suite'"), + "Message must name the exact knob so the fix is mechanical, not a " + + "CHANGELOG-grep exercise. Got: " + message); + } + + @Test + void riskCWarnMessageUsesSingularForExactlyOneFqn() { + // Plurality drift between "1 test class" and "1 test classes" + // is the sort of micro-bug that makes the message read like + // it was written by someone who doesn't use English, and + // operators lose trust fast. Pin it. + String message = AffectedTestTask.formatLocalDiscoveryIncompleteWarning(1); + + assertTrue(message.contains("1 test class "), + "Singular 1 must render as '1 test class' (no trailing s) — got: " + message); + assertFalse(message.contains("1 test classes"), + "Plural fallthrough on count=1 would look like a generated-from-template " + + "bug and cost the signal its credibility. Got: " + message); + } } diff --git a/affected-tests-gradle/src/test/java/io/affectedtests/gradle/AffectedTestsPluginTest.java b/affected-tests-gradle/src/test/java/io/affectedtests/gradle/AffectedTestsPluginTest.java index b9fd2be..df690c2 100644 --- a/affected-tests-gradle/src/test/java/io/affectedtests/gradle/AffectedTestsPluginTest.java +++ b/affected-tests-gradle/src/test/java/io/affectedtests/gradle/AffectedTestsPluginTest.java @@ -293,6 +293,116 @@ private static AffectedTestsExtension freshExtension() { return project.getExtensions().getByType(AffectedTestsExtension.class); } + @Test + void modeIsUnsetByDefault() { + // Critical invariant: with no DSL and no -P, the extension's + // mode Property must read as absent so the core config's + // two-tier resolver falls through to AUTO. Installing a + // literal convention (e.g. "auto") would silently pin the + // task input and break cacheability across environments + // where AUTO would legitimately resolve differently. + // + // Note: ProjectBuilder isolates this test from the home + // `gradle.properties` file, but NOT from JVM system properties. + // If a developer runs the test JVM with + // `-DaffectedTestsMode=strict` (unusual but legal), Gradle's + // providers API surfaces it as a gradleProperty and the + // convention fires, flipping this assertion. If you see a + // surprising failure here, check `-D` args on the test JVM + // first — the contract under test is "no property ≠ no + // convention fired", not "no property ≠ JVM system hygiene". + Project project = ProjectBuilder.builder().build(); + project.getPlugins().apply("io.github.vedanthvdev.affectedtests"); + + AffectedTestsExtension ext = project.getExtensions() + .getByType(AffectedTestsExtension.class); + assertFalse(ext.getMode().isPresent(), + "mode must have no value when neither DSL nor -PaffectedTestsMode is set — " + + "otherwise the core AUTO resolver never runs"); + } + + @Test + void dslDeclaredModeTakesPrecedenceOverAnyConvention() { + // Precedence contract: Gradle Property semantics mean + // explicit `set()` (DSL) always wins over `convention()` + // (our -PaffectedTestsMode fallback). Adopters need that so + // a stray CLI flag cannot silently override a repo's + // deliberately pinned mode. We can't exercise the real + // gradleProperty pipeline inside ProjectBuilder (it resolves + // against actual Gradle CLI state, not extra-properties), so + // the end-to-end `-P` wiring is pinned by the Cucumber e2e + // scenarios in 06-v2.2-adoption-feedback.feature. Here we + // guard the precedence half of the contract via an explicit + // convention() call — if someone inverts the resolution order + // or forgets to use convention(), this test catches it + // without depending on Gradle's CLI-property plumbing. + Project project = ProjectBuilder.builder().build(); + project.getPlugins().apply("io.github.vedanthvdev.affectedtests"); + + AffectedTestsExtension ext = project.getExtensions() + .getByType(AffectedTestsExtension.class); + // Simulate "the plugin installed a convention of 'strict'" + // (which is what `-PaffectedTestsMode=strict` ends up doing + // through the providers API in a real build). + ext.getMode().convention("strict"); + ext.getMode().set("ci"); + + assertEquals("ci", ext.getMode().get(), + "DSL-declared mode must win over any convention — swapping the precedence " + + "would let a stray -P clobber a repo's pinned mode policy"); + } + + @Test + void affectedTestDependsOnTestClassesWhenExplainUnset() { + // Bug A paired-negative: the dispatch path must keep the + // `testClasses` dependency so the nested ./gradlew has class + // files to run against. If the Callable returns empty in + // both branches (regressing the fix to "always prune"), this + // test breaks because the dep disappears from the task. + Project project = ProjectBuilder.builder().build(); + project.getPlugins().apply("java"); + project.getPlugins().apply("io.github.vedanthvdev.affectedtests"); + + var affectedTest = project.getTasks().getByName("affectedTest"); + // Force task configuration so afterEvaluate / the + // dependsOn Callable registration has executed. + ((ProjectInternal) project).evaluate(); + + java.util.Set deps = affectedTest.getTaskDependencies() + .getDependencies(affectedTest); + boolean pullsInTestClasses = deps.stream() + .anyMatch(t -> t instanceof org.gradle.api.Task + && ((org.gradle.api.Task) t).getName().equals("testClasses")); + assertTrue(pullsInTestClasses, + "Without --explain the dispatch path MUST pre-compile test classes so the " + + "nested gradle invocation finds something to run — pruning always would " + + "re-break the original reason the dependency existed"); + } + + @Test + void affectedTestPrunesTestClassesWhenExplainSet() { + // Bug A positive: flipping --explain on must short-circuit + // the testClasses dependency. Asserted at the Callable + // output level so a regression here surfaces independently + // of the Cucumber e2e scenario (which exercises it via the + // real Gradle runtime). + Project project = ProjectBuilder.builder().build(); + project.getPlugins().apply("java"); + project.getPlugins().apply("io.github.vedanthvdev.affectedtests"); + + AffectedTestTask task = (AffectedTestTask) project.getTasks().getByName("affectedTest"); + task.getExplain().set(true); + ((ProjectInternal) project).evaluate(); + + java.util.Set deps = task.getTaskDependencies().getDependencies(task); + boolean pullsInTestClasses = deps.stream() + .anyMatch(t -> t instanceof org.gradle.api.Task + && ((org.gradle.api.Task) t).getName().equals("testClasses")); + assertFalse(pullsInTestClasses, + "With --explain set the Callable MUST prune testClasses from the task " + + "dependency set — otherwise the diagnostic flag keeps paying compile cost"); + } + private static void assertAllAbsent(Class type, java.util.List forbiddenNames, String surfaceLabel) {