v2.2.1 — close the v2.2 code-review Medium + Low findings#38
Merged
vedanthvdev merged 1 commit intomasterfrom Apr 24, 2026
Merged
v2.2.1 — close the v2.2 code-review Medium + Low findings#38vedanthvdev merged 1 commit intomasterfrom
vedanthvdev merged 1 commit intomasterfrom
Conversation
80c130e to
0d4734f
Compare
… 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.
0d4734f to
4aad022
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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-servicepilot — but each represents areal failure mode for a second adopter who doesn't ship in exactly
the same posture (e.g. an adopter with
org.gradle.configuration-cache=trueingradle.properties, or aCI template that emits
-PaffectedTestsMode=with an unsetvariable).
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-cacheno longer crashes the pluginThe Bug A Callable captured
Project pto resolve each subproject'stestClassestask at task-graph time.Projectisnot 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 ownProperty<Boolean>forthe
--explaingate — both CC-serialisable.M3 —
Action.SKIPPEDhint no longer calls the skip "safe"v2.2's
DISCOVERY_INCOMPLETEhint was a two-way branch;Action.SKIPPED(an explicit operator opt-in viaonDiscoveryIncomplete = 'skipped') silently routed through theFULL_SUITEwording that said "the resolved action above is thesafe fallback".
SKIPPEDran zero tests on a partial-parse diff —the OPPOSITE of safe. Added a dedicated
SKIPPEDbranch that namesthe opt-in knob, recommends
'full_suite'as the escape, and nevercalls the current state safe.
L1 — empty
-PaffectedTestsMode=coerces to absentA CI template that unconditionally emits
-PaffectedTestsMode=$MODEwith an unset
$MODEsent the literal empty string through andcrashed
parseModewithUnknown affectedTests.mode ''. Theextension 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=truetripwire so the adopter can self-diagnose fromthe terminal.
L2 — module-block contract fails loudly on misuse
appendModulesBlocksilently renormalised map keys missing aleading colon, papering over any caller that forgot
AffectedTestTask#testTaskPath. Replaced the silent normalisationwith an
assertso the failure surface moves into the test suite.Test plan
testClassesDependencyCallableDoesNotCaptureProjectOrTask(M1)discoveryIncompleteHintOnSkippedNamesTheOptInAndOffersAnEscape(M3)emptyModePropertyCoercesToAbsentNotToError(L1)parseModeErrorNamesTheAutoFallbackOnUnknownValue(L4)modulesBlockRejectsKeysThatSkipTheTestTaskPathHelper(L2)07-v2.2.1-review-followups.featurewithfour scenarios, including a real Gradle
./gradlew affectedTest --configuration-cache --explainrun thatpins M1 end-to-end.
./gradlew :affected-tests-gradle:checkgreen (91 unit tests,62 functional scenarios, 0 failures).
Scope deliberately excluded (batched for v2.3)
testTaskPath()hard-codes:test. Fixing it properlyneeds a new DSL knob (
testTaskName) or a discovery mechanism forKMP
jvmTest/ AndroidtestDebugUnitTest/jvm-test-suitetask names. That's a feature, not a patch.
module FQN previews. Cosmetic polish, deferred.
review. Batched.
Release
.release-versionto2.2.1.[Unreleased]block (which documented items thatactually shipped in v2.2.0) into the
[v2.2.0]section, and adds anew
[v2.2.1]section describing the fixes in this PR.