test(health): add the plugin-manifest check beside the merged one - #1137
test(health): add the plugin-manifest check beside the merged one#1137simonoppowa wants to merge 2 commits into
Conversation
The release-branch half of the same pairing. #1131 landed a plugin-manifest check on develop while #1132 landed a merged-manifest one here; this brings the file to the same canonical version on both branches, carrying both checks. The plugin check is the one that matters most in CI: it reads each plugin's own manifest from .flutter-plugins-dependencies and needs no Android build, so it runs in linux-checks. The merged-manifest check needs a build and covers what the plugin list cannot enumerate. Also takes #1131's permission pattern, which tolerates attribute order and quote style. The pattern this branch shipped required android:name first and would have stayed green on `<uses-permission android:maxSdkVersion="34" android:name="…" />`.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
🟢 Approval recommended
The change is confined to test/guard logic and documentation, with no production behavior impact and no correctness issues found in the updated implementation.
Pull request overview
This PR updates the Health Connect permission guard test to also inspect Android manifests from Flutter plugins (using .flutter-plugins-dependencies) and aligns the merged-manifest check to use the same robust permission-matching pattern, ensuring Health Connect permissions cannot return via plugin upgrades or manifest-merger inputs without being detected.
Changes:
- Adds a plugin-manifest inspection pass (no Android build required) so
linux-checkscan catch health permissions introduced by dependencies. - Switches the merged-manifest scan to reuse the shared
healthPermissionregex so it tolerates XML attribute order and quote style. - Updates inline documentation/comments to describe the three overlapping checks and the expected skip behavior when no build artifacts exist.
File summaries
| File | Description |
|---|---|
| test/unit_test/health_connect_permissions_test.dart | Extends the permission guard to scan plugin manifests and standardizes permission matching across app and merged-manifest checks. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The guard's merged-manifest check moved onto the shared element-wise matcher on release (f0fe0b0) while this branch still extracted with the older `healthPermission` pattern. Kept release's side: that pattern required `android:name` first and double-quoted, and a merged manifest carrying `android:maxSdkVersion="34" android:name='…READ_STEPS'` walked straight past it. This branch's restructuring of the header and the three-check explanation are unaffected. # Conflicts: # test/unit_test/health_connect_permissions_test.dart
There was a problem hiding this comment.
🟢 Approval recommended
The change is limited to test documentation and messaging, and the only noted issue is a minor improvement to a failure message’s accuracy.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Superseded by #1144. Its content — the three-check header restructuring and the two improved messages — is included there verbatim, and lands on This PR was also pointed at the wrong base for most of its life: the branch sits one commit off Since Closing as superseded, not as wrong. |
develop's copy had fallen well behind. main now carries three overlapping
checks; develop had two, and the two it had were the older implementation.
Missing here: reading the app's own release overlays, honouring
`tools:node="remove"`, and the merged-manifest check that inspects what
Gradle actually produced.
Taken wholesale from the shipped version rather than ported hunk by hand.
Piecemeal porting is what let the two branches drift in the first place —
each fix landed on the release branch as review found it, and the pairing
PRs kept chasing a file that had already moved again.
Nothing develop-only is lost. Every test it had is still here and one is
added; the only lines that disappear are the older single-manifest read and
the regex the shared matcher replaces.
Verified on develop by making each capability fail and pass:
* a merged manifest carrying READ_STEPS with the attributes reversed and
single-quoted fails, naming the full set;
* READ_BODY_FAT in android/app/src/release fails the refused-permissions
test — a source set develop's copy never read;
* a plugin declaring READ_DISTANCE alongside a tools:node="remove" for it
in the app manifest passes, so the standard remedy is not blocked.
Supersedes the pairing PRs: simonoppowa#1136 ports an older version of this same group,
and simonoppowa#1137's content is included here.
The
release/2.3.0half of the pair; #1136 is thedevelophalf. After both,health_connect_permissions_test.dartis byte-identical on the two branches.Why
#1131 (→
develop) and #1132 (→ this branch) fixed the same Copilot finding independently and differently. This branch had only the merged-manifest check;develophad only the plugin one. They are complementary:linux-checksThe plugin check is the one worth having on the release branch: it runs on every PR without an Android build, which the merged-manifest check cannot claim.
Also adopts #1131's permission pattern
The pattern this branch shipped required
android:nameto be the first attribute. #1131's tolerates attribute order and quote style. Injectinginto a merged manifest fails the guard now; under the old pattern it stayed green — a legal spelling of one of the three permissions Play rejected, slipping past the guard written to catch it.
Verified on this base
flutter test— 2217 passflutter analyze— cleanNot blocking the release
This is test coverage, not behaviour. The shipped permission set is unchanged and already correct on this branch. If you would rather ship 2.3.0 first, this can wait — but it costs nothing to take now.