Skip to content

v2.2.1 — close the v2.2 code-review Medium + Low findings#38

Merged
vedanthvdev merged 1 commit intomasterfrom
fix/v2.2.1-review-followups
Apr 24, 2026
Merged

v2.2.1 — close the v2.2 code-review Medium + Low findings#38
vedanthvdev merged 1 commit intomasterfrom
fix/v2.2.1-review-followups

Conversation

@vedanthvdev
Copy link
Copy Markdown
Owner

Why

A structured code review on the merged v2.2 changeset flagged four
Medium/Low findings plus one Low that pairs cleanly with the
Medium. None are 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 (e.g. an adopter with
org.gradle.configuration-cache=true in gradle.properties, or a
CI template that emits -PaffectedTestsMode= with an unset
variable).

Point-releasing these 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 the fixes.

What changed

M1 — org.gradle.configuration-cache no longer crashes the plugin

The Bug A Callable captured Project p to resolve each subproject's
testClasses task at task-graph time. Project is
not configuration-cache-serialisable, so an adopter with CC
enabled would fail to serialise the task graph. Replaced with eager
TaskProvider<?> capture plus the task's own Property<Boolean> for
the --explain gate — both CC-serialisable.

M3 — Action.SKIPPED hint no longer calls the skip "safe"

v2.2's DISCOVERY_INCOMPLETE hint was a two-way branch;
Action.SKIPPED (an explicit operator opt-in via
onDiscoveryIncomplete = 'skipped') silently routed through the
FULL_SUITE wording that said "the resolved action above is the
safe fallback". SKIPPED ran zero tests on a partial-parse diff —
the OPPOSITE of safe. Added a dedicated SKIPPED branch that names
the opt-in knob, recommends 'full_suite' as the escape, and never
calls the current state safe.

L1 — empty -PaffectedTestsMode= coerces to absent

A CI template that unconditionally emits -PaffectedTestsMode=$MODE
with an unset $MODE sent the literal empty string through and
crashed parseMode with Unknown affectedTests.mode ''. The
extension convention now filters empty/whitespace so the empty case
is identical to omitting the flag.

L4 — unknown-mode error names the AUTO fallback

v2.2's error listed only the four legal values, leading adopters to
ask "so which one is the default?". Added the AUTO fallback hint
plus the CI=true tripwire so the adopter can self-diagnose from
the terminal.

L2 — module-block contract fails loudly on misuse

appendModulesBlock silently renormalised map keys missing a
leading colon, papering over any caller that forgot
AffectedTestTask#testTaskPath. Replaced the silent normalisation
with an assert so the failure surface moves into the test suite.

Test plan

  • 4 new unit tests (one per finding):
    • testClassesDependencyCallableDoesNotCaptureProjectOrTask (M1)
    • discoveryIncompleteHintOnSkippedNamesTheOptInAndOffersAnEscape (M3)
    • emptyModePropertyCoercesToAbsentNotToError (L1)
    • parseModeErrorNamesTheAutoFallbackOnUnknownValue (L4)
    • modulesBlockRejectsKeysThatSkipTheTestTaskPathHelper (L2)
  • New Cucumber feature 07-v2.2.1-review-followups.feature with
    four scenarios, including a real Gradle
    ./gradlew affectedTest --configuration-cache --explain run that
    pins M1 end-to-end.
  • ./gradlew :affected-tests-gradle:check green (91 unit tests,
    62 functional scenarios, 0 failures).

Scope deliberately excluded (batched for v2.3)

  • M2testTaskPath() hard-codes :test. Fixing it properly
    needs a new DSL knob (testTaskName) or a discovery mechanism for
    KMP jvmTest / Android testDebugUnitTest / jvm-test-suite
    task names. That's a feature, not a patch.
  • L3 — truncation wording alignment between bucket samples and
    module FQN previews. Cosmetic polish, deferred.
  • N-a … N-f — further trace-polish nice-to-haves from the
    review. Batched.

Release

  • Bumps .release-version to 2.2.1.
  • Promotes the stale [Unreleased] block (which documented items that
    actually shipped in v2.2.0) into the [v2.2.0] section, and adds a
    new [v2.2.1] section describing the fixes in this PR.

@vedanthvdev vedanthvdev force-pushed the fix/v2.2.1-review-followups branch from 80c130e to 0d4734f Compare April 24, 2026 20:42
… v2.2 code review

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<Boolean> 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.
@vedanthvdev vedanthvdev force-pushed the fix/v2.2.1-review-followups branch from 0d4734f to 4aad022 Compare April 24, 2026 20:46
@vedanthvdev vedanthvdev merged commit fb124df into master Apr 24, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant