Skip to content

testApplyMigrationZeroMigrationCheckedFirst never exercises the ordering its docstring asserts #41

Description

@thedavidmeister

testApplyMigrationZeroMigrationCheckedFirst asserts an ordering in its
docstring that it never exercises, and the mutation table never probed that
ordering either. Raised by CodeRabbit on
#39 and merged unresolved in
548604c.

What is on main

test/src/concrete/MigrationRegistryApplyMigration.t.sol, lines 418-430:

/// The zero id is refused BEFORE the already-applied read and before the
/// head, so it is always reported as `ZeroMigration` and never as anything
/// about where the namespace is.
function testApplyMigrationZeroMigrationCheckedFirst(address writer, bytes32 anyHead) external {
    vm.assume(writer != address(0));

    vm.expectRevert(abi.encodeWithSelector(IMigrationRegistryV1.ZeroMigration.selector));
    vm.prank(writer);
    sRegistry.applyMigration(anyHead, bytes32(0));

    vm.expectRevert(abi.encodeWithSelector(IMigrationRegistryV1.ZeroMigration.selector));
    vm.prank(writer);
    sRegistry.applyMigration(anyHead, bytes32(0));
}

The two blocks are byte-identical. Same caller, same head, same id, same
expected revert.

The defect

The docstring claims two orderings. The fuzzed anyHead covers one of them —
the zero id beats the head check, in every head state the fuzzer reaches. The
other, "the zero id is refused BEFORE the already-applied read", is never
exercised at all: neither block applies a migration first, so the namespace is
empty in both, and the already-applied read is never reached with a zero id in
either.

So the second block is not merely redundant. It is the place where the missing
coverage was meant to be, and duplicating the first block is what disguises the
gap as coverage — the test reads as though it makes two distinct assertions.

applyMigration's documented order is: zero id -> genesis id -> already
applied -> head -> zero timestamp -> write. Moving the zero-id refusal to sit
after the already-applied read would change what a caller is told — a
re-dispatched script passing a zero id would be answered
MigrationAlreadyApplied rather than ZeroMigration, pointing it at the
namespace instead of at the id it got wrong — and this suite would stay green
through that change.

Why the mutation pass missed it

PR #39's table carries M24, "record's zero-id refusal moves AFTER the head
check", killed by this test. That is the ordering the fuzzed head does cover.
There is no mutant for the zero-id refusal moving after the ALREADY-APPLIED
check, so nothing ever probed the half of the docstring that has no coverage.

This is the same shape as M25, which was a real find in the genesis sibling
during #39's own mutation pass: a test whose name and docstring claimed a
property broader than the calls it made. The zero-id sibling has the same gap
in a different dimension — M25's was the head, this one's is the already-applied
read.

Fix

Give the second block a namespace that has actually recorded something, so the
already-applied path is live when the zero id arrives:

function testApplyMigrationZeroMigrationCheckedFirst(address writer, bytes32 anyHead, bytes32 migration)
    external
{
    vm.assume(writer != address(0));
    assumeMigration(migration);

    vm.expectRevert(abi.encodeWithSelector(IMigrationRegistryV1.ZeroMigration.selector));
    vm.prank(writer);
    sRegistry.applyMigration(anyHead, bytes32(0));

    // A namespace that has moved on: the zero id is still reported as
    // `ZeroMigration` rather than as anything about the head or about what
    // has already been applied.
    vm.prank(writer);
    sRegistry.applyMigration(MIGRATION_HEAD_GENESIS, migration);

    vm.expectRevert(abi.encodeWithSelector(IMigrationRegistryV1.ZeroMigration.selector));
    vm.prank(writer);
    sRegistry.applyMigration(anyHead, bytes32(0));
}

assumeMigration is already defined in this file at line 25.

Adding the mutant that belongs with it — the zero-id refusal moved after the
already-applied check — and confirming the strengthened test kills it is part
of the fix, not a follow-up. Do not weaken the docstring to match the current
calls; the ordering it describes is the intended behaviour.

Check the siblings

The same claim-wider-than-the-calls shape should be swept across the rest of
the ordering tests rather than fixed only where a reviewer happened to look:

  • testApplyMigrationGenesisMigrationRevertsOnAnyHead
  • testAppliedZeroWriterCheckedFirst
  • testApplyMigrationAlreadyAppliedCheckedBeforeHead

For each, check every ordering its docstring asserts against the states its
calls actually construct, and confirm the mutation table has a mutant per
ordering.

Metadata

Metadata

Labels

No labels
No labels

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions