From 4aad0228bc2792a1d7d830c3af2b20a3aa37f833 Mon Sep 17 00:00:00 2001 From: vedanthvasudev Date: Fri, 24 Apr 2026 21:28:16 +0100 Subject: [PATCH] fix/v2.2.1-review-followups: close the Medium + Low findings from the v2.2 code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit v2.2.1 is a non-breaking patch that closes the four Medium/Low findings a structured code review raised against the v2.2 changeset, plus the one Low that pairs cleanly with the Medium. None were adopter-reported regressions — v2.2 is green end-to-end on the security-service pilot — but each represents a real failure mode for a second adopter who doesn't ship in exactly the same posture. M1 (Configuration Cache compatibility). The Bug A Callable captured Project p to resolve each subproject's testClasses task at task-graph time, which fails to serialise under org.gradle.configuration-cache. Replaced with eager TaskProvider capture plus the task's own Property for the --explain gate — both CC-serialisable. M3 (Action.SKIPPED hint). The DISCOVERY_INCOMPLETE hint was a two-way branch; SKIPPED silently routed through the FULL_SUITE wording and was labelled "the safe fallback". SKIPPED ran zero tests, which is the OPPOSITE of safe. Added a dedicated SKIPPED branch that names the opt-in knob and recommends 'full_suite' without ever calling the skip safe. L1 (empty -PaffectedTestsMode). CI templates that unconditionally emit -PaffectedTestsMode=$MODE with an unset variable sent the literal empty string through and crashed parseMode. The extension convention now filters empty/whitespace so the empty case is identical to omitting the flag. L4 (unknown-mode error). The v2.2 error listed only the four legal values. Added the AUTO fallback hint and named the actual runner trigger set (CI=true, GITHUB_ACTIONS, GITLAB_CI, JENKINS_HOME, CIRCLECI, TRAVIS, BUILDKITE, TF_BUILD) so a Jenkins or Azure Pipelines operator reading the error on their own runner doesn't wrongly conclude AUTO falls back to LOCAL. L2 (module-block contract). appendModulesBlock silently renormalised map keys missing a leading colon, papering over any caller that forgot testTaskPath. Replaced with an assert so the failure surface moves into tests. Coverage: four new unit tests (one per finding) plus a new Cucumber feature file 07-v2.2.1-review-followups.feature with four scenarios including a real Gradle --configuration-cache run that pins M1. Review-pass nits folded in after the first push: the L4 error string now lists the full runner-trigger set (not just CI=true); the SKIPPED hint reads 'meant no tests ran' rather than 'accepted no test run'; and the CHANGELOG's empty '### Added' subsection in [v2.2.1] is reframed as a housekeeping note on the v2.2.0 backfill. CI visibility: the PR workflow used to run a single `./gradlew check` step, which did execute the Cucumber e2e suite (check dependsOn functionalTest) but buried the result inside an opaque status line. Split into four named stages — Compile / Unit tests / E2E (Cucumber + Gradle TestKit) tests / Full build — and added a failure-only test-report artifact upload. A red "E2E" step now points straight at the adopter-facing failure mode without mixing unit-test noise in. Release workflow is unchanged: `./gradlew clean build` still gates every publish because `build` transitively depends on `check`. All 91 unit tests and 62 functional scenarios pass locally. --- .github/workflows/ci.yml | 53 ++++++- .release-version | 1 + CHANGELOG.md | 149 +++++++++++++++--- .../gradle/e2e/steps/CommonSteps.java | 13 ++ .../07-v2.2.1-review-followups.feature | 134 ++++++++++++++++ .../gradle/AffectedTestTask.java | 77 ++++++--- .../gradle/AffectedTestsPlugin.java | 28 +++- .../AffectedTestTaskExplainFormatTest.java | 96 +++++++++++ .../gradle/AffectedTestTaskLocaleTest.java | 47 ++++++ .../gradle/AffectedTestsPluginTest.java | 94 +++++++++++ 10 files changed, 643 insertions(+), 49 deletions(-) create mode 100644 .release-version create mode 100644 affected-tests-gradle/src/functionalTest/resources/io/affectedtests/gradle/e2e/features/07-v2.2.1-review-followups.feature diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 98a7d47..5b00d04 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -20,5 +20,54 @@ jobs: - uses: gradle/actions/setup-gradle@v6 - - name: Build and test - run: ./gradlew check + # Compile once up-front so the two test stages below don't each + # pay for a cold classpath. Splitting test execution from + # compilation also makes the failure signal in the GitHub UI + # unambiguous — a red "Unit tests" or "E2E (Cucumber) tests" + # step means a test actually failed, not a compile error. + - name: Compile + run: ./gradlew classes testClasses functionalTestClasses + + # Unit tests: ProjectBuilder-level coverage of the decision + # engine, extension wiring, --explain rendering, and locale + # edge cases. Cheap, fast, and the first gate adopters hit + # when the plugin regresses on a pure-logic change. + - name: Unit tests + run: ./gradlew :affected-tests-core:test :affected-tests-gradle:test + + # E2E (Cucumber + Gradle TestKit): drives real Gradle builds + # through the full plugin surface that security-service and + # other adopters consume. Each scenario in + # src/functionalTest/resources/.../features runs a fresh + # Gradle daemon, so these tests catch build-time wiring + # issues (Shadow relocations, plugin-publish variants, + # Configuration Cache compatibility) that the unit suite + # cannot. Surfaced as a distinct step so a red status here + # points straight at the adopter-facing failure mode. + - name: E2E (Cucumber + Gradle TestKit) tests + run: ./gradlew :affected-tests-gradle:functionalTest + + # Assemble the shadow JAR and exercise any remaining `check` + # hooks that neither test task above owns. Re-excluding test + # and functionalTest here keeps the step honest: if it fails, + # the cause is packaging, not test results. + - name: Full build (shadow JAR + remaining check tasks) + run: ./gradlew build -x test -x functionalTest + + # Upload test reports on any failure above so a failing run + # is debuggable without rerunning with --info locally. The + # Cucumber HTML report in particular is the fastest way to + # read an e2e failure — the console log for a multi-scenario + # failure is dense. + - name: Upload test reports + if: failure() + uses: actions/upload-artifact@v4 + with: + name: test-reports + path: | + affected-tests-gradle/build/reports/tests/ + affected-tests-gradle/build/test-results/ + affected-tests-core/build/reports/tests/ + affected-tests-core/build/test-results/ + if-no-files-found: ignore + retention-days: 14 diff --git a/.release-version b/.release-version new file mode 100644 index 0000000..c043eea --- /dev/null +++ b/.release-version @@ -0,0 +1 @@ +2.2.1 diff --git a/CHANGELOG.md b/CHANGELOG.md index 03f9c4f..23bfb20 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,14 +4,137 @@ All notable changes to this plugin are documented here. The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/). -## [Unreleased] +## [v2.2.1] — code-review follow-ups after the v2.2 ship + +v2.2.1 is a **non-breaking patch** that closes the Medium + Low +findings a structured code review raised against the v2.2 +changeset. None of them were adopter-reported regressions — v2.2 is +green end-to-end on the `security-service` pilot — but each one +represents a real failure mode for a second adopter who doesn't +ship in exactly the same posture. Point-releasing them now keeps +the v2.3 tech-plan window open for the larger feature work +(non-conventional test task names, explain-trace truncation +unification) without delaying these fixes behind it. + +If you're on v2.2.0 today you can bump to v2.2.1 without touching +`build.gradle`. The only observable change on a green path is that +enabling `org.gradle.configuration-cache` now works. The only +observable changes on the unhappy path are sharper error messages +and a new hint branch for the `onDiscoveryIncomplete = 'skipped'` +opt-in. + +### Fixed — `org.gradle.configuration-cache` no longer crashes the plugin (M1) + +The v2.2 dispatch-dependency Callable closed over a `Project` +reference to resolve each subproject's `testClasses` task. `Project` +is explicitly documented as **not** configuration-cache-serialisable, +so a repo with `org.gradle.configuration-cache=true` in +`gradle.properties` would fail to serialise the task graph with: + +``` +Task `:affectedTest` of type `...AffectedTestTask`: cannot serialize +object of type Project +``` + +The fix resolves each subproject's `testClasses` task once, eagerly, +into a `TaskProvider` (which IS CC-serialisable) and closes the +Callable over that plus the task's own `Property` for the +`--explain` gate. Cache-hit rate and task-graph shape are identical +to v2.2 — only the serialisation surface changed. A new functional +scenario runs `./gradlew affectedTest --configuration-cache +--explain` on a SELECTED-shaped diff and pins "Configuration cache +entry …" in the output so the next edit that accidentally re-captures +`Project` breaks a test instead of only an adopter's CI. + +Adopters who don't enable CC see no change. + +### Fixed — `Action.SKIPPED` hint no longer calls the skip "safe" (M3) + +v2.2's `DISCOVERY_INCOMPLETE` hint was a two-way branch: SELECTED +warned about the partial-selection risk; everything else printed "the +resolved action above is the safe fallback". That "everything else" +bucket silently absorbed `Action.SKIPPED` — which an operator can +opt into explicitly via `onDiscoveryIncomplete = 'skipped'`. Calling +SKIPPED "safe" is precisely inverted advice: the run executed zero +tests on a partial-parse diff. + +v2.2.1 gives SKIPPED its own hint branch: + +``` +Hint: one or more Java files in the diff failed to parse, + so discovery ran with missing inputs. + onDiscoveryIncomplete = 'skipped' meant no tests ran + for this diff — fix the parse error to restore + coverage, or set onDiscoveryIncomplete = 'full_suite' + if silently skipping a partial-parse diff is not the + intended policy. +``` + +The hint names the exact knob that was set (so a second reader of the +trace can locate the override) and names the escape (`'full_suite'`) +without ever calling the current state "safe". SELECTED and +FULL_SUITE wording are unchanged. + +### Fixed — `-PaffectedTestsMode=` (empty) no longer crashes (L1) + +CI templates that unconditionally emit `-PaffectedTestsMode=$MODE` +with an unset `$MODE` passed the literal empty string into the +extension's convention, and v2.2 rejected it with +`Unknown affectedTests.mode ''`. v2.2.1 filters empty/whitespace in +the convention chain so the empty case is identical to omitting the +flag — the AUTO default keeps working, and the "unknown mode" error +stays reserved for actual typos. + +### Fixed — unknown-mode error names the AUTO fallback (L4) + +v2.2's `parseMode` error listed the four legal values only, which +led adopters to ask "so which one is the default?". v2.2.1 adds the +AUTO-fallback hint plus the `CI=true` tripwire that AUTO keys off: + +``` +Unknown affectedTests.mode 'xyzzy'. Expected one of: auto, local, +ci, strict (omit the value or leave -PaffectedTestsMode unset to +keep the AUTO default, which picks CI when CI=true is exported). +``` + +### Changed — explain-trace module-block contract now fails loudly on misuse (L2) + +`AffectedTestTask#appendModulesBlock` used to silently re-normalise +map keys that were missing a leading colon, papering over any caller +that forgot to route their module paths through +`AffectedTestTask#testTaskPath`. A future edit that fed raw +`"api:test"` strings into the map produced a split trace (`":api:test"` +next to `"core:test"`). The helper now asserts the contract up-front, +so the failure surface moves into the test suite. + +The assertion is plain `assert`, active on every Gradle test JVM by +default and a no-op on production runs — the public operator-facing +trace output is unchanged. + +_Changelog housekeeping: the v2.2.0 section below has been backfilled +with items that were in the v2.2.0 ship but were still sitting under +`[Unreleased]` in the CHANGELOG when the tag was cut._ + +## [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). ### 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. +Follow-up polish on the v2.2 adoption-feedback changeset, all +non-breaking and none of them visible in build scripts. * **Risk C WARN no longer fires with "0 test classes".** The engine rewrites `SELECTED` with nothing to dispatch into `skipped=true`; @@ -45,22 +168,6 @@ contract is identical to v2.2.0 on every green path. 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 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 5020ac1..6765b8f 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 @@ -313,6 +313,19 @@ public void theTaskFailsAtConfigurationTime() { "Failure must come from configuration / evaluation phase, got:\n" + world.project().lastOutput()); } + @Then("the task fails") + public void theTaskFails() { + // Generic failure assertion for scenarios where the failure + // phase is uninteresting — e.g. an unknown `-P` value that + // escapes the extension convention and is only rejected at + // task-action time via {@code parseMode}. Asserting on the + // lifecycle phase would tie the test to an implementation + // detail (exactly where the enum lookup fires), so this + // step only pins "the build did not turn green". + assertTrue(world.project().lastBuildFailed(), + "Expected a failing build, got green:\n" + world.project().lastOutput()); + } + @Then("the situation is {word}") public void theSituationIs(String situation) { // The --explain trace prints the ACTUAL situation on the diff --git a/affected-tests-gradle/src/functionalTest/resources/io/affectedtests/gradle/e2e/features/07-v2.2.1-review-followups.feature b/affected-tests-gradle/src/functionalTest/resources/io/affectedtests/gradle/e2e/features/07-v2.2.1-review-followups.feature new file mode 100644 index 0000000..ed19de7 --- /dev/null +++ b/affected-tests-gradle/src/functionalTest/resources/io/affectedtests/gradle/e2e/features/07-v2.2.1-review-followups.feature @@ -0,0 +1,134 @@ +Feature: v2.2.1 — code-review follow-ups from the v2.2 ship + Scenarios for the Medium + Low findings raised by the v2.2 code + review: Configuration Cache compatibility (M1), Action.SKIPPED + hint wording (M3), empty-string `-PaffectedTestsMode` coercion + (L1), and the AUTO-fallback error wording on an unknown `-P` + value (L4). Every scenario here pins an operator-facing contract + the v2.2 release shipped slightly short on — if any of these + scenarios regress, a real adopter notices. + + Background: + Given a freshly initialised project with a committed baseline + + # ------------------------------------------------------------------ + # M1 — Configuration Cache compatibility + # + # The v2.2 Callable for the Bug A `testClasses` dependency captured + # `Project p` so a CC-enabled adopter would fail to serialise the + # task graph ("cannot serialize object of type Project"). v2.2.1 + # fixed it by capturing `TaskProvider` and `Property` + # eagerly — both CC-serialisable. Pin it with a live + # `--configuration-cache` run on the same SELECTED shape adopters + # use daily: if the Callable ever regresses to capturing Project + # again, this scenario turns red. + # ------------------------------------------------------------------ + Scenario: --configuration-cache stores and reuses the task graph without Project capture + 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 "--configuration-cache" + When the affected-tests task runs with "--explain" + Then the task succeeds + # Gradle logs the CC-store event on the first run — no store line + # means CC never engaged (e.g. because a problem was detected and + # CC quietly degraded). The exact phrase changed between 8.x and + # 9.x; match the stable prefix "Configuration cache entry" which + # both minor lines emit. + And the output contains "Configuration cache entry" + # A Project-capture regression surfaces as this specific message + # — pin the negative so a future edit that re-introduces it can't + # hide behind a green "entry stored" line. + And the output does not contain "cannot be serialized" + And the output does not contain "cannot serialize" + + # ------------------------------------------------------------------ + # M3 — Action.SKIPPED hint no longer calls the skip "safe" + # + # v2.2 routed Action.SKIPPED through the FULL_SUITE hint branch, + # which said "the resolved action above is the safe fallback". + # Calling SKIPPED "safe" is precisely inverted advice: SKIPPED ran + # zero tests on a partial-parse diff. v2.2.1 gives SKIPPED its own + # hint branch that names the opt-in knob and recommends + # `'full_suite'` if the skip wasn't intentional. + # ------------------------------------------------------------------ + Scenario: DISCOVERY_INCOMPLETE on SKIPPED names the opt-in and offers an escape + # Requires the opt-in: LOCAL mode with + # `onDiscoveryIncomplete = 'skipped'` is the only path that lands + # Action.SKIPPED on a parse-failure diff. CI/STRICT escalate + # (their paired scenario is in feature 06). + Given the affected-tests DSL contains: + """ + mode = 'local' + onDiscoveryIncomplete = 'skipped' + """ + 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 action is SKIPPED + And the output contains "failed to parse" + # Hint must name both the user-set knob AND the escape hatch so + # an operator reading the trace can tell whether the skip was + # deliberate and, if not, exactly which value to flip. + And the output contains "onDiscoveryIncomplete = 'skipped'" + And the output contains "onDiscoveryIncomplete = 'full_suite'" + # The FULL_SUITE branch's "safe fallback" wording must NOT leak + # into the SKIPPED branch — that claim is factually wrong when + # zero tests ran. + And the output does not contain "safe fallback" + # Partial-selection wording belongs to the SELECTED branch only; + # SKIPPED ran nothing, so the selection is not partial — it's + # non-existent. + And the output does not contain "selection is necessarily partial" + + # ------------------------------------------------------------------ + # L1 — empty `-PaffectedTestsMode` coerces to absent + # + # A CI template that unconditionally emits + # `-PaffectedTestsMode=$MODE` with an unset $MODE would send the + # literal empty string through. v2.2 crashed parseMode with + # "Unknown affectedTests.mode ''". v2.2.1 filters empty/whitespace + # in the extension convention so the empty case is identical to + # omitting the flag. The scenario below pins "no crash" rather than + # a specific resolved mode, because the AUTO default depends on + # whether a CI=true env var is present on the test JVM — asserting + # on a specific mode would make the scenario environment-sensitive. + # ------------------------------------------------------------------ + Scenario: An empty -PaffectedTestsMode behaves like the flag is 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=" + When the affected-tests task runs with "--explain" + Then the task succeeds + And the situation is DISCOVERY_SUCCESS + # The regression we're guarding: v2.2 would surface this exact + # error message for the empty-string case. v2.2.1 must not. + And the output does not contain "Unknown affectedTests.mode ''" + And the output does not contain "Unknown affectedTests.mode" + + # ------------------------------------------------------------------ + # L4 — parseMode error names the AUTO fallback + # + # v2.2's error listed the four legal values only, which led + # adopters to ask "so which one is the default?". v2.2.1 adds the + # AUTO-fallback hint plus the CI=true tripwire to the message so + # the fix is self-service from the operator's terminal. + # ------------------------------------------------------------------ + Scenario: An unknown -PaffectedTestsMode names AUTO and CI=true in the error + 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=xyzzy" + When the affected-tests task runs with "--explain" + Then the task fails + # Echo-back of the bad value keeps typo diagnosis local — no need + # to re-read the CI log to find what the template substituted. + And the output contains "Unknown affectedTests.mode 'xyzzy'" + # Legal-values list must stay (the v2.2 wording kept this right). + And the output contains "auto, local, ci, strict" + # The v2.2.1 additions: name the fallback AND the environment + # signal so an adopter understands what "just leave -P off" will + # actually do on each machine. + And the output contains "AUTO" + And the output contains "CI=true" 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 7ec19a9..bf0fc57 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 @@ -479,8 +479,26 @@ static Mode parseMode(String raw) { // even though the spelling is literally in the valid list. return Mode.valueOf(raw.trim().toUpperCase(Locale.ROOT)); } catch (IllegalArgumentException e) { + // Mention the AUTO fallback explicitly: the most common + // cause of a bad value here is a CI template that forgot + // to scrub an unset variable, and the operator needs to + // know they can just leave the -P off (or pass an empty + // value — see the plugin's convention wiring) to get the + // environment-aware default. + // + // We list the actual trigger set (mirror + // AffectedTestsConfig.Builder.detectMode) rather than + // just "CI=true" because a Jenkins or Azure Pipelines + // operator reading a "CI=true is exported" hint on + // their own runner would wrongly conclude AUTO falls + // back to LOCAL — JENKINS_HOME / TF_BUILD flip it to CI + // without CI=true ever being set. throw new GradleException("Unknown affectedTests.mode '" + raw - + "'. Expected one of: auto, local, ci, strict.", e); + + "'. Expected one of: auto, local, ci, strict " + + "(omit the value or leave -PaffectedTestsMode unset to keep " + + "the AUTO default, which picks CI on recognised runners — " + + "CI=true, GITHUB_ACTIONS, GITLAB_CI, JENKINS_HOME, CIRCLECI, " + + "TRAVIS, BUILDKITE, TF_BUILD).", e); } } @@ -1435,23 +1453,34 @@ private static void appendDiscoveryEmptyHint(List lines, */ 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. + // Every branch opens with the same root-cause line — the parse + // failure is the operator-actionable fact regardless of what + // the mode chose to do about it. Only the follow-on guidance + // differs per resolved Action. 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)."); + switch (result.action()) { + case SELECTED -> 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)."); + case FULL_SUITE -> lines.add(" Fix the parse error to " + + "recover a precise selection on future runs (until then the " + + "resolved action above is the safe fallback)."); + // Reachable only via an explicit `onDiscoveryIncomplete = + // 'skipped'` DSL override — unusual but legal. SKIPPED is + // the OPPOSITE of safe here (we neither narrowed nor + // escalated; we just didn't run tests), so the hint must + // not call it "the safe fallback" the way FULL_SUITE does. + // We name the opt-in knob and offer the two sane exits + // (fix the parse error, or flip the knob to 'full_suite') + // without implying the current state is acceptable. + case SKIPPED -> lines.add(" onDiscoveryIncomplete = " + + "'skipped' meant no tests ran for this diff — fix the parse " + + "error to restore coverage, or set onDiscoveryIncomplete = " + + "'full_suite' if silently skipping a partial-parse diff is " + + "not the intended policy."); + } } private static String formatInlineList(List items) { @@ -1487,16 +1516,20 @@ static void appendModulesBlock(List lines, Map> mod lines.add("Modules: " + moduleGroups.size() + " " + moduleWord + ", " + totalFqns + " test " + classWord + " to dispatch"); for (Map.Entry> entry : moduleGroups.entrySet()) { + // Contract: callers pass the key through + // {@link #testTaskPath(String)}, which guarantees a + // canonical ":module:test" / ":test" shape with a leading + // colon. We assert rather than silently re-normalise so + // any future caller that forgets the helper breaks loudly + // in tests instead of producing a ":x" / "x" mixed trace. 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; + assert taskPath.startsWith(":") : "appendModulesBlock expects testTaskPath-shaped " + + "keys (leading ':'); got '" + taskPath + "' — route through " + + "AffectedTestTask#testTaskPath before inserting into the map"; int size = fqns.size(); String rowClassWord = size == 1 ? "class" : "classes"; - lines.add(" " + normalised + " (" + size + " test " + rowClassWord + ")"); + lines.add(" " + taskPath + " (" + size + " test " + rowClassWord + ")"); int preview = Math.min(size, LIFECYCLE_FQN_PREVIEW_LIMIT); for (int i = 0; i < preview; i++) { lines.add(" " + fqns.get(i)); 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 1c8156f..2f8e6c1 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,8 +3,8 @@ 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 org.gradle.api.tasks.TaskProvider; import java.io.File; import java.util.LinkedHashMap; @@ -39,8 +39,17 @@ public void apply(Project project) { // 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. + // + // Empty/whitespace-only values are coerced to absent: a CI + // template that always emits `-PaffectedTestsMode=$MODE` with + // an unset $MODE would otherwise land "" on the convention and + // fail parseMode with "Unknown affectedTests.mode ''". Trimming + // + filtering here keeps the happy path working ("no value == + // no override") and the error wording targeted at actual typos. extension.getMode().convention( project.getProviders().gradleProperty("affectedTestsMode") + .map(String::trim) + .filter(s -> !s.isEmpty()) ); // COMMITTED-ONLY by default: the plugin's whole job is "what // tests does this MR touch?", and the MR is the committed diff @@ -113,12 +122,23 @@ public void apply(Project project) { // 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. + // + // CC-safe capture: we resolve the {@code TaskProvider} + // for each subproject's {@code testClasses} task eagerly + // (providers are configuration-cache-serialisable) and + // close the Callable over that Provider plus the task's + // own {@code Property}. We deliberately do NOT + // capture {@code Project p} or {@code task} — either would + // make the lambda unserialisable when an adopter enables + // {@code org.gradle.configuration-cache=true}. + var explainFlag = task.getExplain(); rootProject.allprojects(p -> p.getPluginManager().withPlugin("java", unused -> { - Callable> testClassesWhenDispatching = () -> { - if (Boolean.TRUE.equals(task.getExplain().getOrElse(false))) { + TaskProvider testClasses = p.getTasks().named("testClasses"); + Callable>> testClassesWhenDispatching = () -> { + if (Boolean.TRUE.equals(explainFlag.getOrElse(false))) { return List.of(); } - return List.of(p.getTasks().named("testClasses").get()); + return List.of(testClasses); }; task.dependsOn(testClassesWhenDispatching); })); 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 411a9bb..bcd9545 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 @@ -675,6 +675,50 @@ void discoveryIncompleteHintOnFullSuiteDropsPartialSelectionWording() { + "ignore the hint. Got:\n" + trace); } + @Test + void discoveryIncompleteHintOnSkippedNamesTheOptInAndOffersAnEscape() { + // v2.2.1 fix (M3 from v2.2 code review): when the operator + // has explicitly set `onDiscoveryIncomplete = 'skipped'`, the + // plugin runs zero tests on a partial-parse diff. The v2.2 + // hint lumped SKIPPED into the FULL_SUITE branch and printed + // "the resolved action above is the safe fallback" — which + // is precisely wrong: SKIPPED is the OPPOSITE of safe here + // (neither selection narrowed nor suite escalated). Pin the + // corrected wording: the hint names the opt-in knob, names + // the parse failure, and points at `'full_suite'` as the + // fix — without ever calling the skip "safe". + AffectedTestsConfig config = AffectedTestsConfig.builder().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()), + false, true, // runAll=false, skipped=true + Situation.DISCOVERY_INCOMPLETE, Action.SKIPPED, + EscalationReason.NONE); + + String trace = joined(AffectedTestTask.renderExplainTrace(config, result)); + + assertTrue(trace.contains("failed to parse"), + "SKIPPED hint must still name the parse-failure cause — the opt-in " + + "doesn't change why discovery was incomplete. Got:\n" + trace); + assertTrue(trace.contains("onDiscoveryIncomplete = 'skipped'"), + "Hint must name the exact knob the operator set so a second reader of " + + "the trace can locate the override. Got:\n" + trace); + assertTrue(trace.contains("onDiscoveryIncomplete = 'full_suite'"), + "Hint must offer 'full_suite' as the escape hatch — SKIPPED is the only " + + "branch where escalation advice is not circular. Got:\n" + trace); + assertFalse(trace.contains("safe fallback"), + "SKIPPED ran no tests — calling that the 'safe fallback' directly " + + "contradicts the correctness posture the plugin is supposed to " + + "defend. Got:\n" + trace); + assertFalse(trace.contains("selection is necessarily partial"), + "Nothing was selected — the partial-selection wording belongs to the " + + "SELECTED branch only. Got:\n" + trace); + } + @Test void discoverySuccessHintKeepsV2dot1OutOfScopeMisconfigWording() { // Regression: v2.2's hint refactor must preserve the single @@ -822,6 +866,58 @@ void modulesBlockRendersHelperNormalisedRootTask() { + "reads as a Gradle task path. Got:\n" + trace); } + @Test + void modulesBlockRejectsKeysThatSkipTheTestTaskPathHelper() { + // v2.2.1 fix (L2 from v2.2 code review): appendModulesBlock + // used to silently re-normalise keys missing a leading colon, + // masking bugs where a future caller forgot to route through + // testTaskPath(). The renderer now asserts the contract so + // the failure surface moves into tests instead of into a + // split operator trace (":api:test" next to "core:test"). + // + // Asserted with `assert`, so this test only trips when + // assertions are on — which is how every Gradle test JVM is + // configured. Pins both the positive signal (helper output + // flows through) and the negative (raw module path does not). + 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); + + // A key without a leading colon violates the helper contract. + // Canonical Java idiom: the assign-inside-assert executes + // only when assertions are enabled, so the flag stays false + // on a JVM running with `-da` and we skip the negative half. + // Gradle's test JVM runs with assertions on by default. + boolean assertionsOn = false; + assert assertionsOn = true; + if (!assertionsOn) { + return; + } + + Map> bad = Map.of("api:test", + List.of("com.example.FooTest")); + try { + AffectedTestTask.renderExplainTrace(config, result, bad); + throw new IllegalStateException("appendModulesBlock must reject keys missing " + + "the leading colon so a future caller that forgets testTaskPath() " + + "fails loudly instead of silently emitting split traces"); + } catch (AssertionError expected) { + assertTrue(expected.getMessage() != null + && expected.getMessage().contains("testTaskPath"), + "Assertion message must name the helper so the fix is obvious. Got: " + + expected.getMessage()); + } + } + @Test void modulesBlockTruncatesWithAndNMoreLineOverPreviewLimit() { // Match the dispatch-path preview behaviour: print the first diff --git a/affected-tests-gradle/src/test/java/io/affectedtests/gradle/AffectedTestTaskLocaleTest.java b/affected-tests-gradle/src/test/java/io/affectedtests/gradle/AffectedTestTaskLocaleTest.java index 03e1906..81d4db5 100644 --- a/affected-tests-gradle/src/test/java/io/affectedtests/gradle/AffectedTestTaskLocaleTest.java +++ b/affected-tests-gradle/src/test/java/io/affectedtests/gradle/AffectedTestTaskLocaleTest.java @@ -2,6 +2,7 @@ import io.affectedtests.core.config.Action; import io.affectedtests.core.config.Mode; +import org.gradle.api.GradleException; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -53,4 +54,50 @@ void parseActionIsLocaleIndependent() { assertEquals(Action.FULL_SUITE, AffectedTestTask.parseAction("full_suite", "onEmptyDiff")); assertEquals(Action.SKIPPED, AffectedTestTask.parseAction("skipped", "onEmptyDiff")); } + + @Test + void parseModeErrorNamesTheAutoFallbackOnUnknownValue() { + // v2.2.1 fix (L4 from v2.2 code review): the v2.2 error + // message for `-PaffectedTestsMode=xyzzy` only listed the + // four legal values. Adopters flipped `-P` on in CI and + // didn't realise that OMITTING the flag was the right + // escape — leading to tickets asking "which mode is + // 'default'?". The fix: the error names `auto` explicitly, + // explains it's picked when the flag is absent, and + // mentions the `CI=true` tripwire so the operator knows + // what AUTO will actually do on a CI worker. + GradleException ex = assertThrows(GradleException.class, + () -> AffectedTestTask.parseMode("xyzzy"), + "parseMode must reject unknown values — bare enum lookups silently " + + "throwing IllegalArgumentException would lose the adopter-facing hint"); + + String msg = ex.getMessage(); + assertTrue(msg.contains("'xyzzy'"), + "Error must echo the bad value back so the operator can spot typos in " + + "multi-variable CI templates. Got: " + msg); + assertTrue(msg.contains("auto, local, ci, strict"), + "Error must list the legal values — regressing this message would make " + + "the typo case harder to self-diagnose. Got: " + msg); + assertTrue(msg.contains("AUTO"), + "Error must name the AUTO fallback explicitly — otherwise adopters " + + "reading the message don't know they can just drop the -P. Got: " + msg); + assertTrue(msg.contains("CI=true"), + "Error must still name the baseline CI=true trigger — regressing this " + + "specific variable out of the list breaks the most common " + + "adopter mental model. Got: " + msg); + // The trigger set must cover runners that don't export CI=true + // (Jenkins, Azure Pipelines). Sample-check one non-CI=true + // runner so a future edit that over-simplifies the message + // back to "CI=true is exported" — the v2.2.1 L-B regression — + // breaks this test. We use GITHUB_ACTIONS as the assertion + // anchor because it's the runner with the widest adopter + // footprint that also ships CI=true, so a passing test + // guarantees the message is at least TRYING to list runners + // rather than hardcoding a single env-var name. + assertTrue(msg.contains("JENKINS_HOME") || msg.contains("TF_BUILD"), + "Error must name at least one runner that does NOT export CI=true " + + "(Jenkins or Azure Pipelines) — otherwise an operator on those " + + "runners would wrongly conclude AUTO falls back to LOCAL on " + + "their box. Got: " + msg); + } } 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 df690c2..59dc0b4 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 @@ -403,6 +403,100 @@ void affectedTestPrunesTestClassesWhenExplainSet() { + "dependency set — otherwise the diagnostic flag keeps paying compile cost"); } + @Test + void testClassesDependencyCallableDoesNotCaptureProjectOrTask() { + // v2.2.1 fix (M1 from v2.2 code review): the Bug A Callable + // used to capture `Project p` and resolve testClasses via + // `p.getTasks().named("testClasses").get()` at task-graph + // time. Project references are NOT + // configuration-cache-serialisable, so enabling Gradle CC on + // an adopter repo would fail with "cannot serialize object + // of type Project" the moment the lambda tried to cross the + // CC boundary. + // + // The fix: capture the `TaskProvider` eagerly (providers + // ARE CC-serialisable) plus the task's own + // `Property` for --explain, and close over THOSE + // instead. Pin that contract here by asserting the + // dependencies of a fully-configured task are TaskProvider- + // shaped (or the task instances Gradle resolves them into) + // rather than raw Project references leaking through. + // + // The Gradle CC runtime doesn't expose a "would-this- + // serialise" hook from ProjectBuilder, so a full end-to-end + // CC compatibility check lives in the Cucumber e2e scenario + // that runs the real Gradle daemon with + // `--configuration-cache`. This unit test pins the + // Callable's OUTPUT SHAPE so a future edit that regresses + // to capturing Project breaks a fast test instead of only + // the slow functional run. + Project project = ProjectBuilder.builder().build(); + project.getPlugins().apply("java"); + project.getPlugins().apply("io.github.vedanthvdev.affectedtests"); + + var affectedTest = project.getTasks().getByName("affectedTest"); + ((ProjectInternal) project).evaluate(); + + java.util.Set deps = + affectedTest.getTaskDependencies().getDependencies(affectedTest); + // Every dep must be a concrete Task (Gradle unwraps providers + // for us at getDependencies() time); the contract under test + // is that we get here at all without Gradle throwing on a + // non-serialisable Callable capture. A regression to + // capturing `Project p` would produce identical runtime + // behaviour here (it only bites under --configuration-cache + // in the e2e suite) — so also pin the SOURCE of the + // testClasses dep lives on the root project's TaskContainer, + // not on a foreign reference that could carry a Project + // with it. + var testClassesDep = deps.stream() + .filter(t -> "testClasses".equals(t.getName())) + .findFirst(); + assertTrue(testClassesDep.isPresent(), + "Sanity: the dispatch path must still pull in testClasses after the " + + "CC-safe refactor — otherwise M1 accidentally regressed Bug A"); + assertEquals(project, testClassesDep.get().getProject(), + "testClasses dep must resolve to the same Project the plugin was applied to — " + + "if it's routed via a stale reference the CC-safe capture has drifted"); + } + + @Test + void emptyModePropertyCoercesToAbsentNotToError() { + // v2.2.1 fix (L1 from v2.2 code review): some CI templates + // unconditionally emit `-PaffectedTestsMode=$MODE` where + // $MODE may be unset, landing the literal string "" on the + // mode convention. Pre-fix this crashed parseMode with + // "Unknown affectedTests.mode ''". The correct semantics: + // an empty or whitespace-only value means "no override" — + // identical to omitting the flag — so the extension must + // filter it out BEFORE the convention applies. We can't + // drive `gradleProperty` inside ProjectBuilder, so simulate + // the final convention state the same way + // `dslDeclaredModeTakesPrecedenceOverAnyConvention` does: + // by calling .convention() on the extension directly. + Project project = ProjectBuilder.builder().build(); + project.getPlugins().apply("io.github.vedanthvdev.affectedtests"); + + AffectedTestsExtension ext = project.getExtensions() + .getByType(AffectedTestsExtension.class); + + // Gradle's map+filter chain on the real -P pipeline does this: + // providers.gradleProperty(...).map(trim).filter(!empty). If + // the raw property is "" or " ", the chain yields an empty + // provider, and re-applying it as a convention leaves the + // extension's mode property absent. Pin that exact shape. + org.gradle.api.provider.Provider emptyishProperty = + project.getProviders().provider(() -> " ") + .map(String::trim) + .filter(s -> !s.isEmpty()); + ext.getMode().convention(emptyishProperty); + + assertFalse(ext.getMode().isPresent(), + "An empty/whitespace -PaffectedTestsMode must behave like 'flag not set' — " + + "otherwise CI templates that always emit the flag with an unset " + + "variable crash parseMode with 'Unknown mode \\'\\''"); + } + private static void assertAllAbsent(Class type, java.util.List forbiddenNames, String surfaceLabel) {