Migration cleanup: drop hand-rolled test DDL, swap to ensure_current/2#7
Merged
Conversation
Replace the inline DDL (uuid_generate_v7 fn, phoenix_kit_activities stub, phoenix_kit_settings stub) plus the `Ecto.Migrator.up(TestRepo, 0, PhoenixKitSync.Migration, ...)` call in `test/test_helper.exs` with `PhoenixKit.Migration.ensure_current/2` (core 1.7.105+ / phoenix_kit#515). Sync tables are owned by core (V37 creates them as `phoenix_kit_db_sync_*`; V44 renames to `phoenix_kit_sync_*`; V56/V58/V61/V73/V74 evolve them). phoenix_kit_settings (V03), phoenix_kit_activities (V90), and the uuid_generate_v7 function (V40) are also owned by core. Schema drift between test and prod is impossible by construction. Two pre-existing test failures surfaced — both committed in 7916940 ("PR BeamLabEU#5 follow-up bundle (F1–F4)") whose own message admits "DB tests will be exercised by the next full mix test run on a host with the test DB available". The old inline DDL had been masking the real LV behavior. Both skipped with `@tag :skip` and clear comments naming the bug + intended fix; out of migration-cleanup scope: - F1 Iron Law dead-render test — `handle_params/3`'s catch-all `handle_action` clause calls `load_connections/1` unconditionally, bypassing the `connected?` gate that F1 added to `mount/3`. Needs a one-line LV fix: swap `load_connections()` → `maybe_load_connections()` in `connections_live.ex:106`. - F4 revoke gettext test — looks for `[phx-click='revoke_connection']` in the list view, but the revoke button only exists in the connection detail view (`connections_live.ex:1940`). Test needs a navigate-then- click flow or a context-layer pin. Companion edits: - `AGENTS.md`: Database & Migrations / Testing sections updated. Depends on BeamLabEU/phoenix_kit#515 — CI will be red until core 1.7.105 publishes.
Bundle the two follow-ups from the migration-cleanup batch's documented skips into the same PR per workspace request to land quality fixes that are within reach. F1 Iron Law gate `handle_params/3`'s catch-all `handle_action` clause was calling `load_connections/1` unconditionally — the original F1 fix only gated `mount/3`. Swap to `maybe_load_connections/1` so the dead render path also seeds empty assigns and the doubled DB queries + async sender-status / receiver-verification HTTP calls only fire on the live phase. Pinned by the existing "dead render does not include connection names from the DB" test which is now unskipped. F4 revoke test rework The revoke button only renders in the connection detail view (connections_live.ex:1940), not in the list. The original test asserted on the list view and never passed. Mount directly into the detail view via the URL the `show_connection` push_patch would land on (`?action=show&id=<uuid>`), then click revoke. Now pins the gettext-wrapped reason as intended.
ddon
added a commit
that referenced
this pull request
May 5, 2026
handle_params/3 fires on both the dead render and the connected phase. The "show", "edit", and "sync" clauses each called Connections.get_connection/1 unconditionally, so a deep-linked URL (?action=show&id=<uuid>) ran the query in dead render — and on the nil branch, fanned out load_connections (the same DB+async-HTTP fan-out the F1 fix at maybe_load_connections/1 was specifically eliminating). Replace the three handle_action clauses for show/edit/sync with one connected?-gated dispatcher + a dispatch_resource_action/3 helper. Mount already seeds safe list-view assigns via maybe_load_connections/1, so the dead-render no-op is render-safe; the connected phase re-fires handle_params/3 and resolves the action. The nil-branch load_connections/1 calls in handle_connection_action/3 and handle_sync_action/2 are now implicitly gated since their parents only run on connected sockets. Pinning (connections_live_test.exs): - dead render of ?action=show&id=<uuid> does not include connection name (Phoenix.ConnTest.get/2; mirrors existing F1 dead-render test) - dead render of ?action=edit&id=<uuid> does not include connection name - live render of ?action=show&id=<uuid> *does* include connection name (pins the gate is connected?-conditional, not "never load") Surfaced by PR #7 review (dev_docs/pull_requests/2026/7-migration-cleanup /CLAUDE_REVIEW.md, finding 3). Pre-existing — not introduced by the migration-cleanup PR — but the F4 revoke test there now exercises the deep-link path explicitly, making the gap discoverable. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ddon
added a commit
that referenced
this pull request
May 5, 2026
Post-merge review covering the migration-cleanup PR: 4 files, +24/-73, 587/0/0 across 3 stable runs. Approve with one follow-up logged and fixed on-branch. Strengths called out: single-source-of-truth schema setup via core's ensure_current/2 (eliminates schema drift class entirely), correct F1/F4 fixes that resolve the rows sketched in PR #5's CLAUDE_REVIEW, the upstream ensure_current/2 design (microsecond-precision Migrator trick + table-comment short-circuit), good comment locator hygiene at test_helper.exs:41-56. Findings: - (1) merged-with-red-CI process pattern, post-mortem only since core 1.7.105 has since published and is pinned in mix.lock - (2) bundled-PR scope creep — nit - (3) latent Iron Law gap on deep-link entry to ?action=show|edit|sync (pre-existing; F4 test surfaced it). FIXED on-branch in the preceding commit. - (4) cosmetic schema_migrations row accumulation per mix test invocation — no action No outstanding follow-ups from this PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
2 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.
Summary
Replace the inline DDL (uuid_generate_v7 fn, phoenix_kit_activities stub, phoenix_kit_settings stub) plus the
Ecto.Migrator.up(TestRepo, 0, PhoenixKitSync.Migration, ...)call intest/test_helper.exswithPhoenixKit.Migration.ensure_current/2(core 1.7.105+ / phoenix_kit#515). Schema drift between test and prod is impossible by construction.Sync tables are owned by core (V37 creates them as
phoenix_kit_db_sync_*; V44 renames tophoenix_kit_sync_*; V56/V58/V61/V73/V74 evolve them). phoenix_kit_settings (V03), phoenix_kit_activities (V90), and the uuid_generate_v7 function (V40) are also owned by core.Should be merged after BeamLabEU/phoenix_kit#515 — CI will be red until core 1.7.105 publishes.
Quality follow-ups bundled (commit
e6127ac)The migration-cleanup commit surfaced two pre-existing test failures (committed in 7916940 with no DB-verified runs). Both fixed in this PR rather than punted:
handle_params/3's catch-allhandle_actionclause was callingload_connections/1unconditionally; the original F1 fix only gatedmount/3. Swap tomaybe_load_connections/1so the dead render seeds empty assigns and the doubled DB queries + async HTTP fan-out only fire on the live phase. The existing dead-render test now passes.connections_live.ex:1940), not the list. Updated the test to mount directly into the detail view via?action=show&id=<uuid>(where theshow_connectionpush_patch lands) before clicking revoke. Now pins the gettext-wrapped persisted reason as intended.Files touched
test/test_helper.exsEcto.Migrator.up(_, 0, PhoenixKitSync.Migration, _); replace withPhoenixKit.Migration.ensure_current/2lib/phoenix_kit_sync/web/connections_live.exhandle_actionwithmaybe_load_connections/1(F1 follow-up)test/phoenix_kit_sync/web/connections_live_test.exsAGENTS.mdVerification
Local test suite via
phoenix_kit_parentpath-dep override resolving to local core 1.7.104+:mix test— 587 tests, 0 failures, 0 skippedTest plan
mix test— 587 tests, 0 failures, 0 skippeddropdb && createdbbefore run