Skip to content

test(migrations): every migration function is registered, and every registered function is one (#3395) - #3972

Merged
vybe merged 5 commits into
devfrom
AndriiPasternak31/issue-3395-s3
Oct 27, 2026
Merged

vybe merged 5 commits into
devfrom
AndriiPasternak31/issue-3395-s3

Conversation

@AndriiPasternak31

@AndriiPasternak31 AndriiPasternak31 commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Part of #3395. This is one of three independent slices; the upgrade-parity slice closes the issue. The three share no file and merge in any order.

What changed and why

run_all_migrations runs only what MIGRATIONS lists. A _migrate* function that is written but never added to that list is never run on an existing install, and nothing fails. The one guard against it today is test_fan_out_id_migration_present, which checks a single name, the one that caused the original outage.

This PR adds TestMigrationsConstant::test_every_migrate_function_is_registered_and_vice_versa to tests/unit/test_migrations.py. It checks every name, in both directions:

  • every _migrate* function defined in src/backend/db/migrations.py is registered in MIGRATIONS;
  • every registered function is a _migrate* function of that module;
  • no function is registered under two entries.

How it finds the functions:

  • It reads every top-level def or async def named _migrate* (decorated or not) from the file's source, resolves each name to the object the loaded module binds, and compares those by identity with the MIGRATIONS list captured at import. Other tests in the file patch the module attribute, which is why the captured list is used.
  • Non-vacuity check: the names read from the source must be non-empty and equal the _migrate* callables the loaded module binds; a mismatch names the difference. An empty or partial discovery cannot pass, for example after migrations.py is split into a package. The function set comes from the module, never from a hand-written list.
  • An async def migration is refused even when registered: run_all_migrations calls it without awaiting it, so it would be recorded as applied without running.
  • The failure message names each offending function or entry.
  • The docstring states the convention this creates: a helper in migrations.py that is deliberately not a migration must not use the _migrate prefix.

tests/registry.json gains an entry for unit/test_migrations.py, which had none. The entry sits next to the other migration-runner entry, not at the end of the array, so it does not collide with the upgrade-parity slice, which adds its own entry.

What this slice alone catches

An unregistered function. This includes a data-only migration (an UPDATE backfill), which no schema comparison can see. Neither the fresh-install reverse-parity slice nor the merge-base upgrade-parity slice detects one. The reverse also holds: this slice cannot see a column that schema.py declares when no migration function exists at all. The upgrade-parity slice covers that case.

Other shape: none of the six standard cases (fresh install, empty data, another origin, flag off, clock 30 days ahead, restricted container) applies to a test that introspects a module. The test's second case is the converse direction: a registered function that is not a _migrate* function of the module.

No docs: test-only change (bug-fix tier).

Journey Impact: none: a test-only change. No runtime code, endpoint, UI or migration behaviour changes, so no user or operator journey is affected.

Validation: pytest tests/unit/test_migrations.py tests/unit/test_schema_parity.py tests/unit/test_alembic_parity_guard.py → 46 passed under pytest-randomly seeds 11, 222 and 3333 (Python 3.12). tests/lint_sys_modules.py and tests/lint_root_test_placement.py pass. test_2338_journey_catalog.py → 51 passed, and the JOURNEYS.md --check is clean. ruff passes. gitleaks over the branch commits found no leaks. On today's dev, 170 functions are defined and 170 are registered. After the review fix 1d8cca27a: tests/unit/test_migrations.py → 21 passed; the wider migration guard set → 100 passed under three pytest-randomly seeds and with ordering off (Python 3.14); ruff, the JOURNEYS.md --check and both lint scripts pass.

Mutation: each mutation was applied to db/migrations.py from a copy, then restored.

  • M1, deleting ("a2a_caller_identity", _migrate_a2a_caller_identity) from MIGRATIONS → red: _migrate* functions defined in db/migrations.py but not registered in MIGRATIONS (existing installs never run them): ['_migrate_a2a_caller_identity']
  • D1, adding an unregistered data-only _migrate_probe_backfill → red, naming ['_migrate_probe_backfill']
  • V1, registering probe_backfill, which is not a _migrate* function → red: MIGRATIONS entries whose function is not a _migrate* function defined in db/migrations.py: ['probe_backfill -> migrations.probe_backfill']
  • Registering _migrate_a2a_caller_identity a second time → red: functions registered under more than one MIGRATIONS entry
  • Parsing an empty source instead of the file → red: no _migrate* functions defined; parsing only the first 100 top-level nodes → red, naming the missing functions.
  • Adding an async def _migrate_probe_async → red as unregistered; registering it → red: async _migrate* functions: ... ['_migrate_probe_async']. Adding a registered @functools.cache _migrate* → green.

The existing suite stays green on M1 (518 tests over the 11 migration-touching files, measured at planning).

Unverified:

  • Not run under CI's Python 3.13 image; the test only introspects the module and does not depend on the SQLite version.
  • The full tests/unit tier was not run on this branch alone; it runs once on the integration branch for the batch of slices.
  • .claude/agents/test-runner.md was not updated. It lives in the private submodule, and this PR leaves gitlinks untouched.

Sibling PRs: #3972 (slice C, registry bijection), #3973 (slice A, reverse parity), #3974 (slice B, upgrade guard; carries the closing keyword for #3395). Follow-ups filed: F1=#3975 F2=#3976 F3=#3977 F4=#3978 F5=#3979 F6=#3980.

…egistered function is one (#3395)

run_all_migrations runs only what MIGRATIONS lists, so a migration function
that is written but never registered ships green today and never runs on an
existing install. A data-only migration (an UPDATE backfill) is invisible to
any schema diff. test_fan_out_id_migration_present pinned this class for one
name; this test pins it for every name, in both directions.

The functions are discovered from the loaded module (inspect, filtered to
_migrate* defined in db/migrations.py) and compared by identity with the
MIGRATIONS captured at import. Non-vacuity: the discovered set must be
non-empty and equal the top-level `def _migrate*` names parsed from the
file's source, so an empty or partial discovery cannot pass.

Mutations, each applied to db/migrations.py from a cp snapshot and restored:
- M1, delete ("a2a_caller_identity", _migrate_a2a_caller_identity) from MIGRATIONS:
  AssertionError: _migrate* functions defined in db/migrations.py but not registered in MIGRATIONS (existing installs never run them): ['_migrate_a2a_caller_identity']
- D1, add an unregistered data-only _migrate_probe_backfill (UPDATE, no DDL):
  AssertionError: _migrate* functions defined in db/migrations.py but not registered in MIGRATIONS (existing installs never run them): ['_migrate_probe_backfill']
- V1, register probe_backfill, which is not a _migrate* function:
  AssertionError: MIGRATIONS entries whose function is not a _migrate* function defined in db/migrations.py: ['probe_backfill -> migrations.probe_backfill']
- Vacuity, point the test's file filter at a non-existent split.py:
  AssertionError: no _migrate* functions discovered in .../src/backend/db/migrations.py
M0 (no mutation): green, 170 defined = 170 registered.

The plan's size floor (discovered >= len(MIGRATIONS)) was replaced: under V1
it fired first and reported "discovery is broken" instead of naming the
foreign entry. The source-parse check is the floor instead.

Part of #3395.
…ction

tests/registry.json had no entry for unit/test_migrations.py. The entry sits
beside the other migration-runner entry (#1160) rather than at the end of the
array, so it does not collide with another slice of #3395 that appends there.

Part of #3395.
…der two entries (#3395)

/review finding: the set comparison is a bijection on functions only, so one
_migrate* function registered under two MIGRATIONS names passed while running
twice on every fresh database. The test now names every function with more
than one entry.

Mutation, register _migrate_a2a_caller_identity a second time as
"a2a_caller_identity_again":
  AssertionError: functions registered under more than one MIGRATIONS entry: ["_migrate_a2a_caller_identity <- ['a2a_caller_identity', 'a2a_caller_identity_again']"]
M0, M1, D1 and V1 re-run after this change: green, then red with the same
messages as in the first commit.

Part of #3395.
…function rule (#3395)

Follows the /review fix that refuses one function under two MIGRATIONS
entries.

Part of #3395.

@dolho dolho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/validate-pr — Lane A (tests + registry only, additive)

Check Result
Scope tests/unit/test_migrations.py + tests/registry.json; no runtime code, no deletions in tests/ ✅
Commits conventional, descriptive ✅
Issue Part of #3395, closing keyword deliberately on the sibling slice #3974 ✅
Secrets / host paths none ✅
Run on the PR head test_migrations.py: 18 passed; registry.json parses ✅
Mutation, reproduced here deleted ("a2a_caller_identity", _migrate_a2a_caller_identity) from MIGRATIONS → the new test goes red, the other 17 stay green ✅
Non-vacuity discovery is cross-checked against the parsed source, so an empty or split module cannot pass ✅
CI all checks green ✅

Execution coverage (§5.4)

The test introspects the live db.migrations module and compares function objects by identity against the MIGRATIONS captured at import, so it executes the real registry, not its source text. The ast.parse is only the non-vacuity cross-check. Correctly classified as a guard whose consumer is run_all_migrations itself.

Non-blocking: an async def or decorated _migrate* would fail the non-vacuity check with a source/introspection mismatch rather than the registry message. You already note this; fine as is, since none exists.

APPROVE.

🤖 Reviewed with Claude Code

#3395)

The bijection discovered functions by introspection and cross-checked
them against the parsed source. A `_migrate*` that is `async def`, or
decorated by a wrapper from another module, dropped out of one
derivation but not the other, so it failed the non-vacuity check with
"discovered N, source defines M" (equal counts when there is one of
each) instead of being named as unregistered. A registered decorated
migration failed the same way, although it runs.

Discovery now reads the source: every top-level `def` or `async def`
named `_migrate*`, resolved to the object the module binds, which is
the object MIGRATIONS holds. The non-vacuity cross-check compares those
names with the `_migrate*` callables the loaded module binds and names
the difference.

An `async def` migration is refused even when registered:
run_all_migrations calls it without awaiting it, so it is recorded as
applied and its body never runs. The old mismatch caught that only by
accident.
@AndriiPasternak31

Copy link
Copy Markdown
Contributor Author

Fixed in 1d8cca2 rather than left as a note.

The finding holds, and the message was worse than a mismatch: with one async def and one decorated _migrate* in the same file, it read discovered 2, source defines 2. A registered migration with a decorator from another module failed the same way, although it runs.

Discovery now reads the source: every top-level def or async def _migrate*, decorated or not. Each name resolves to the object the module binds, which is what MIGRATIONS holds. The non-vacuity cross-check compares those names with the _migrate* callables the loaded module binds, and lists the difference instead of counting it. So both cases now get the "not registered" message, by name.

One thing the old mismatch was doing by accident: it was the only thing stopping a registered async def migration. run_all_migrations calls it without awaiting it, so the name is written to schema_migrations and the body never runs. I ran that case: no table, row recorded, only a "never awaited" warning. It now has its own message, which fires even when the function is registered.

Three new tests run against a synthetic module: an unregistered decorated and an unregistered async function are named as unregistered, a registered decorated one passes, and a registered async one is refused. All three fail on e2fecdb. Your mutation (dropping a2a_caller_identity) and the others in the description give the same messages as before. test_migrations.py: 21 passed.

@vybe
vybe merged commit 94eb129 into dev Oct 27, 2026
22 checks 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.

3 participants