acc: run dashboard tests under the READPLAN variant - #6162
Merged
Conversation
Deploying a saved plan (bundle deploy --plan) takes a different code path than an in-memory plan: everything the deploy applies comes from the plan file. Only one dashboard test exercised that path, so a regression in what a saved plan carries for dashboards would not have been caught. Add EnvMatrix.READPLAN = ["", "1"] to the dashboard tests that deploy, and route their deploys through readplanarg so each runs both ways. Since all matrix variants must produce identical output, any divergence between the two paths now fails the test. Covered: change-serialized-dashboard (content edit -> update), nested-folders (reads serialized_dashboard back from the API), publish-failure-cleans-up-dashboard (failure path and cleanup DELETE), and deployment/bind/dashboard plus its recreation/ subdir (bind writes state without going through DoCreate/DoUpdate). The deploys are no longer traced because the command line itself differs between variants (--plan vs none); the goldens they feed are shared and unchanged, which is what shows both paths send the same requests. migrate/dashboards is left alone: it asserts on GET requests to //dashboards to confirm the direct engine ran, and a saved-plan deploy skips the read phase, so that assertion has nothing to match. Co-authored-by: Isaac
Collaborator
Integration test reportCommit: 8df533e
14 interesting tests: 7 flaky, 4 SKIP, 3 RECOVERED
Top 50 slowest tests (at least 2 minutes):
|
Sankalp-Mittal
marked this pull request as ready for review
August 5, 2026 07:59
Sankalp-Mittal
changed the title
denik
approved these changes
Aug 5, 2026
The repo convention for an untraced readplanarg deploy is no comment: of the ~20 pre-existing readplanarg tests, only two comment it, and both explain something the code cannot convey (the bug being guarded in deploy/readplan/grants-remove-principal, the local-vs-cloud plan JSON in select/basic). Restating "not traced because the command line differs by READPLAN" in five scripts is what CLAUDE.md rejects. Kept the two comments that carry a real why: bind writing state outside DoCreate/DoUpdate, and the re-plan needed after unbind. Restored the pre-existing comments in change-serialized-dashboard and publish-failure verbatim. Co-authored-by: Isaac
Sankalp-Mittal
enabled auto-merge
August 5, 2026 11:01
…board-tests-readplan
Sankalp-Mittal
disabled auto-merge
August 5, 2026 11:17
Sankalp-Mittal
enabled auto-merge
August 5, 2026 11:17
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
3 participants
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.
Changes
Add
EnvMatrix.READPLAN = ["", "1"]to the dashboard acceptance tests thatdeploy, and route their
bundle deploycalls through thereadplanarghelper soeach test runs twice: once with an in-memory plan, once applying a plan saved to
a file with
--plan.Because all
EnvMatrixvariants must produce identical output files, anydivergence between the two deploy paths now fails the test automatically.
Tests converted:
resources/dashboards/change-serialized-dashboardserialized_dashboard→ update; asserts the PATCH/POST request bodiesresources/dashboards/nested-foldersserialized_dashboardback from the API after deployresources/dashboards/publish-failure-cleans-up-dashboarddeployment/bind/dashboard(+recreation/)DoCreate/DoUpdateWhy
bundle deploy --plantakes a different code path than a plain deploy:everything applied comes from the serialized plan file rather than from a plan
computed in memory. Only one dashboard test exercised that path, so a regression
in what a saved plan carries for dashboards would have gone uncaught.
This lands the coverage on its own, ahead of the
serialized_dashboardcontent-hash change (#6105), so that any later failure is unambiguously
attributable to that change rather than to a pre-existing gap.
Result
No request or state golden changed. The only golden edits are removed
>>> [CLI] bundle deploytrace lines — the deploys are no longer traced becausethe command line itself differs between variants (
--planvs none).That the shared goldens are byte-identical across both variants is the point:
out.patch.requests.direct.txt,out.post.requests.txtandout.state_after_bind.*.jsonconfirm both paths issue the same API requests andpersist the same state on
maintoday.Not converted
migrate/dashboardsis deliberately left alone. It asserts on GET requests to//dashboardsto confirm the direct engine ran, but a saved-plan deploy skipsthe read phase, so no GETs are recorded and that assertion has nothing to match.
Converting it would mean weakening a real assertion. Migration therefore remains
uncovered for the saved-plan path — worth noting, since like bind it writes state
without going through
DoCreate/DoUpdate.Tests
Acceptance-only; no production code changes. Verified green on unmodified
origin/mainbefore converting, so failures were attributable.task fmt,task wsandtask lint-qare clean.This pull request and its description were written by Isaac.