chore(test-types): adds extra modifier validation; adds unit tests (#2710)

* chore: adds extra modifier validation; adds unit tests

* chore: add a blip about unit testing to the claude test-writing skill

* feat: add test that showcases the issue that `found_index` can still be `True` from an earlier call

* fix: fix state leak in modifiers; add tests to catch this

---------

Co-authored-by: Felix H <felix314159@users.noreply.github.com>

Author: fselmo

.claude/commands/write-test.md

@@ -61,6 +61,10 @@ Conventions and patterns for writing consensus tests. Run this skill before writ
 - `@pytest.mark.parametrize("name", [pytest.param(val, id="label"), ...])` with descriptive `id=` strings
 - Stack parametrize decorators for multiple dimensions
 
+## Unit Tests (execution_testing package)
+
+Plain pytest. Tests are co-located with each module under `packages/testing/src/execution_testing/` in a sibling `tests/` directory. When adding a guardrail or validation, verify the tests fail without the change and pass with it.
+
 ## After Writing Tests
 
 After writing or modifying tests, ask the user: "Would you like me to load the `/fill-tests` skill to verify the new tests fill correctly? (This loads an additional skill into context.)" If they agree, run `/fill-tests`, fill the new tests, then inspect the generated fixture JSON to verify the fixture contents match what the test intends.

packages/testing/src/execution_testing/test_types/block_access_list/modifiers.py

@@ -28,10 +28,9 @@ def _remove_field_from_accounts(
 ) -> Callable[[BlockAccessList], BlockAccessList]:
     """Abstracted helper to remove a field from specified accounts."""
     len_addresses = len(addresses)
-    found_addresses = set()
 
     def transform(bal: BlockAccessList) -> BlockAccessList:
-        nonlocal found_addresses
+        found_addresses: set[Address] = set()
         new_root = []
         for account_change in bal.root:
             if account_change.address in addresses:
@@ -70,10 +69,10 @@ def _modify_field_value(
     Abstracted helper to modify a field value for a specific account and
     transaction.
     """
-    found_address = False
 
     def transform(bal: BlockAccessList) -> BlockAccessList:
-        nonlocal found_address
+        found_address = False
+        found_index = False
         new_root = []
         for account_change in bal.root:
             if account_change.address == address:
@@ -84,15 +83,18 @@ def transform(bal: BlockAccessList) -> BlockAccessList:
                 if changes:
                     if nested and slot is not None:
                         # nested structure (storage)
+                        found_slot = False
                         for storage_slot in changes:
                             if storage_slot.slot == slot:
+                                found_slot = True
                                 for j, change in enumerate(
                                     storage_slot.slot_changes
                                 ):
                                     if (
                                         change.block_access_index
                                         == block_access_index
                                     ):
+                                        found_index = True
                                         kwargs = {
                                             "block_access_index": (
                                                 block_access_index
@@ -104,10 +106,16 @@ def transform(bal: BlockAccessList) -> BlockAccessList:
                                         )
                                         break
                                 break
+                        if not found_slot:
+                            raise ValueError(
+                                f"Storage slot {slot} not found in "
+                                f"storage_changes of account {address}"
+                            )
                     else:
                         # flat structure (nonce, balance, code)
                         for i, change in enumerate(changes):
                             if change.block_access_index == block_access_index:
+                                found_index = True
                                 kwargs = {
                                     "block_access_index": block_access_index,
                                     value_field: new_value,
@@ -120,10 +128,14 @@ def transform(bal: BlockAccessList) -> BlockAccessList:
                 new_root.append(account_change)
 
         if not found_address:
-            # sanity check that we actually found the address
             raise ValueError(
                 f"Address {address} not found in BAL to modify {field_name}"
             )
+        if not found_index:
+            raise ValueError(
+                f"Block access index {block_access_index} not found in "
+                f"{field_name} of account {address}"
+            )
 
         return BlockAccessList(root=new_root)
 
@@ -247,13 +259,12 @@ def swap_bal_indices(
     idx1: int, idx2: int
 ) -> Callable[[BlockAccessList], BlockAccessList]:
     """Swap block access indices throughout the BAL, modifying ordering."""
-    nonce_indices = {idx1: False, idx2: False}
-    balance_indices = nonce_indices.copy()
-    storage_indices = nonce_indices.copy()
-    code_indices = nonce_indices.copy()
 
     def transform(bal: BlockAccessList) -> BlockAccessList:
-        nonlocal nonce_indices, balance_indices, storage_indices, code_indices
+        nonce_indices = {idx1: False, idx2: False}
+        balance_indices = nonce_indices.copy()
+        storage_indices = nonce_indices.copy()
+        code_indices = nonce_indices.copy()
         new_root = []
         for account_change in bal.root:
             new_account = account_change.model_copy(deep=True)
@@ -385,10 +396,8 @@ def append_change(
     else:
         raise TypeError(f"Unsupported change type: {type(change)}")
 
-    found_address = False
-
     def transform(bal: BlockAccessList) -> BlockAccessList:
-        nonlocal found_address
+        found_address = False
         new_root = []
         for account_change in bal.root:
             if account_change.address == account:
@@ -427,10 +436,9 @@ def append_storage(
       slot_changes
     - If change provided and slot new: creates new BalStorageSlot
     """
-    found_address = False
 
     def transform(bal: BlockAccessList) -> BlockAccessList:
-        nonlocal found_address
+        found_address = False
         new_root = []
         for account_change in bal.root:
             if account_change.address == address:
@@ -477,10 +485,9 @@ def duplicate_account(
     address: Address,
 ) -> Callable[[BlockAccessList], BlockAccessList]:
     """Duplicate an account entry in the BAL."""
-    address_present = False
 
     def transform(bal: BlockAccessList) -> BlockAccessList:
-        nonlocal address_present
+        address_present = False
         new_root = []
         for account_change in bal.root:
             new_root.append(account_change)
@@ -514,15 +521,14 @@ def _duplicate_in_field(
     When sub_field and sub_match_fn are provided, find the parent entry
     via match_fn then duplicate within sub_field using sub_match_fn.
     """
-    found = False
 
     def _copy(entry: Any) -> Any:
         if hasattr(entry, "model_copy"):
             return entry.model_copy(deep=True)
         return ZeroPaddedHexNumber(entry)
 
     def transform(bal: BlockAccessList) -> BlockAccessList:
-        nonlocal found
+        found = False
         new_root = []
         for account_change in bal.root:
             if account_change.address == address:
@@ -663,10 +669,9 @@ def insert_storage_read(
     Useful for testing that a key must not appear in both
     storage_changes and storage_reads.
     """
-    found_address = False
 
     def transform(bal: BlockAccessList) -> BlockAccessList:
-        nonlocal found_address
+        found_address = False
         new_root = []
         for account_change in bal.root:
             if account_change.address == address:
@@ -721,8 +726,14 @@ def reorder_accounts(
     """Reorder accounts according to the provided index list."""
 
     def transform(bal: BlockAccessList) -> BlockAccessList:
-        if len(indices) != len(bal.root):
+        n = len(bal.root)
+        if len(indices) != n:
             raise ValueError("Index list length must match number of accounts")
+        if sorted(indices) != list(range(n)):
+            raise ValueError(
+                f"Indices must be a valid permutation of 0..{n - 1}, "
+                f"got {indices}"
+            )
         new_root = [bal.root[i] for i in indices]
         return BlockAccessList(root=new_root)
 

packages/testing/src/execution_testing/test_types/tests/test_block_access_list_modifiers.py

@@ -1,5 +1,7 @@
 """Unit tests for BAL modifier functions."""
 
+from typing import Callable
+
 import pytest
 
 from execution_testing.base_types import Address
@@ -13,6 +15,8 @@
     BlockAccessList,
 )
 from execution_testing.test_types.block_access_list.modifiers import (
+    append_change,
+    append_storage,
     duplicate_account,
     duplicate_balance_change,
     duplicate_code_change,
@@ -21,6 +25,13 @@
     duplicate_storage_read,
     duplicate_storage_slot,
     insert_storage_read,
+    modify_balance,
+    modify_code,
+    modify_nonce,
+    modify_storage,
+    remove_nonces,
+    reorder_accounts,
+    swap_bal_indices,
 )
 
 ALICE = Address(0xA)
@@ -217,3 +228,143 @@ def test_insert_storage_read_missing_address_raises() -> None:
     bal = BlockAccessList([BalAccountChange(address=ALICE, nonce_changes=[])])
     with pytest.raises(ValueError, match="not found"):
         insert_storage_read(CONTRACT, 1)(bal)
+
+
+def test_modify_nonce_missing_index_raises(
+    sample_bal: BlockAccessList,
+) -> None:
+    """Raise when the block_access_index is absent from nonce_changes."""
+    with pytest.raises(ValueError, match="not found"):
+        modify_nonce(ALICE, 99, 42)(sample_bal)
+
+
+def test_modify_balance_missing_index_raises(
+    sample_bal: BlockAccessList,
+) -> None:
+    """Raise when the block_access_index is absent from balance_changes."""
+    with pytest.raises(ValueError, match="not found"):
+        modify_balance(ALICE, 99, 9999)(sample_bal)
+
+
+def test_modify_code_missing_index_raises(sample_bal: BlockAccessList) -> None:
+    """Raise when the block_access_index is absent from code_changes."""
+    with pytest.raises(ValueError, match="not found"):
+        modify_code(ALICE, 99, b"\x00")(sample_bal)
+
+
+def test_modify_storage_missing_index_raises(
+    sample_bal: BlockAccessList,
+) -> None:
+    """Raise when block_access_index is absent within the storage slot."""
+    with pytest.raises(ValueError, match="not found"):
+        modify_storage(CONTRACT, 99, 1, 0xFF)(sample_bal)
+
+
+def test_modify_storage_missing_slot_raises(
+    sample_bal: BlockAccessList,
+) -> None:
+    """Raise when the storage slot itself is absent."""
+    with pytest.raises(ValueError, match="not found"):
+        modify_storage(CONTRACT, 1, 99, 0xFF)(sample_bal)
+
+
+def test_modify_nonce_reused_callable_missing_index_still_raises() -> None:
+    """Raise even when the same modifier callable is reused across BALs."""
+    modifier = modify_nonce(ALICE, 1, 42)
+    valid_bal = BlockAccessList(
+        [
+            BalAccountChange(
+                address=ALICE,
+                nonce_changes=[
+                    BalNonceChange(block_access_index=1, post_nonce=1),
+                ],
+            )
+        ]
+    )
+    missing_index_bal = BlockAccessList(
+        [
+            BalAccountChange(
+                address=ALICE,
+                nonce_changes=[],
+            )
+        ]
+    )
+
+    modifier(valid_bal)
+
+    with pytest.raises(ValueError, match="not found"):
+        modifier(missing_index_bal)
+
+
+def test_reorder_accounts_duplicate_index_raises(
+    sample_bal: BlockAccessList,
+) -> None:
+    """Raise when indices contain duplicates (not a valid permutation)."""
+    with pytest.raises(ValueError, match="valid permutation"):
+        reorder_accounts([0, 0])(sample_bal)
+
+
+def test_reorder_accounts_out_of_range_raises(
+    sample_bal: BlockAccessList,
+) -> None:
+    """Raise when indices are not a valid permutation (skipped index)."""
+    with pytest.raises(ValueError, match="valid permutation"):
+        reorder_accounts([0, 2])(sample_bal)
+
+
+_EMPTY_BAL = BlockAccessList([])
+_ALICE_ONLY_BAL = BlockAccessList(
+    [BalAccountChange(address=ALICE, nonce_changes=[])]
+)
+
+
+@pytest.mark.parametrize(
+    "modifier_factory, missing_bal",
+    [
+        pytest.param(
+            lambda: remove_nonces(ALICE), _EMPTY_BAL, id="remove_nonces"
+        ),
+        pytest.param(
+            lambda: swap_bal_indices(1, 1), _EMPTY_BAL, id="swap_bal_indices"
+        ),
+        pytest.param(
+            lambda: append_change(
+                ALICE, BalNonceChange(block_access_index=2, post_nonce=5)
+            ),
+            _EMPTY_BAL,
+            id="append_change",
+        ),
+        pytest.param(
+            lambda: append_storage(CONTRACT, slot=7, read=True),
+            _EMPTY_BAL,
+            id="append_storage",
+        ),
+        pytest.param(
+            lambda: duplicate_account(ALICE),
+            _EMPTY_BAL,
+            id="duplicate_account",
+        ),
+        pytest.param(
+            lambda: duplicate_nonce_change(ALICE, 1),
+            _ALICE_ONLY_BAL,
+            id="duplicate_nonce_change",
+        ),
+        pytest.param(
+            lambda: insert_storage_read(CONTRACT, 99),
+            _EMPTY_BAL,
+            id="insert_storage_read",
+        ),
+    ],
+)
+def test_reused_callable_does_not_carry_found_state(
+    sample_bal: BlockAccessList,
+    modifier_factory: Callable[
+        [], Callable[[BlockAccessList], BlockAccessList]
+    ],
+    missing_bal: BlockAccessList,
+) -> None:
+    """A modifier's found-state must not persist across calls."""
+    modifier = modifier_factory()
+    modifier(sample_bal)
+    with pytest.raises(ValueError, match="not found"):
+        modifier(missing_bal)