Skip to content

refactor(prover): move inline table tests to tests/#614

Open
diegokingston wants to merge 2 commits into
mainfrom
refactor/move-inline-table-tests
Open

refactor(prover): move inline table tests to tests/#614
diegokingston wants to merge 2 commits into
mainfrom
refactor/move-inline-table-tests

Conversation

@diegokingston
Copy link
Copy Markdown
Collaborator

Relocate the #[cfg(test)] mod tests blocks out of the table source files in prover/src/tables/ into prover/src/tests/, alongside the existing per-table test files.

  • 7 new files: page/register/load/memw/memw_aligned/memw_register/ keccak_rnd _tests.rs
  • decode and trace_builder inline tests appended to their existing decode_tests.rs / trace_builder_tests.rs
  • is_register_op and collect_bitwise_from_keccak bumped to pub(crate) so the relocated trace_builder tests can reach them

bitwise.rs has no #[test] functions (only a #[cfg(test)] test helper) and is left unchanged; trace_builder's #[cfg(test)] impl helper methods also stay in place.

Relocate the #[cfg(test)] mod tests blocks out of the table source files
in prover/src/tables/ into prover/src/tests/, alongside the existing
per-table test files.

- 7 new files: page/register/load/memw/memw_aligned/memw_register/
  keccak_rnd _tests.rs
- decode and trace_builder inline tests appended to their existing
  decode_tests.rs / trace_builder_tests.rs
- is_register_op and collect_bitwise_from_keccak bumped to pub(crate)
  so the relocated trace_builder tests can reach them

bitwise.rs has no #[test] functions (only a #[cfg(test)] test helper)
and is left unchanged; trace_builder's #[cfg(test)] impl helper methods
also stay in place.
@github-actions
Copy link
Copy Markdown

Codex Code Review

No issues found in the PR diff.

The changes are primarily test relocation into prover/src/tests/*, plus two helpers widened to pub(crate) for those tests. I didn’t see security, correctness, or significant performance regressions in the changed code.

Verification note: I attempted cargo test -p prover --tests, but the sandbox could not run it because rustup tried to write to read-only /home/runner/.rustup/tmp; redirecting to /tmp then failed due blocked network/toolchain sync.

@claude
Copy link
Copy Markdown

claude Bot commented May 22, 2026

Review: refactor(prover): move inline table tests to tests/

Clean refactoring — no security issues, no logic changes, no bugs introduced. Tests are mechanically moved, not modified.

Two minor points:

[Low] Redundant #[cfg(test)] on inner submodules in trace_builder_tests.rs
The keccak_tests and routing_tests submodules at lines 566/776 are wrapped in #[cfg(test)], but the entire prover/src/tests module is already gated by #[cfg(test)] in lib.rs (line 22). The inner wrappers are no-ops. Likely inherited from the original inline location where the guard was needed — safe to drop.

[Low] pub(crate) visibility bumps for is_register_op / collect_bitwise_from_keccak
These two functions are promoted from private to pub(crate) solely to support the relocated tests. The change is minimal and correct, but it means they're now callable from non-test code anywhere in the crate. If they're purely test-support helpers, consider leaving their signatures annotated with a comment to that effect — or alternatively, keep them private and use a #[cfg(test)] impl block (as is already done for other helpers in trace_builder.rs) to avoid widening their visibility at all.

);
}

#[cfg(test)]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[Low] Redundant #[cfg(test)] — this file is already only compiled under #[cfg(test)] (see lib.rs:22), so this inner guard is a no-op. Safe to remove.

Suggested change
#[cfg(test)]
mod keccak_tests {

}
}

#[cfg(test)]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[Low] Redundant #[cfg(test)] — same as above, already in a test-only module.

Suggested change
#[cfg(test)]
mod routing_tests {

Continue relocating #[cfg(test)] mod tests blocks out of prover source
files into prover/src/tests/.

- auto_storage.rs tests -> tests/auto_storage_tests.rs, gated on the
  disk-spill feature to match the auto_storage module
- constraints/templates.rs test -> tests/templates_tests.rs
- select_storage_mode and the GOLDILOCKS_BYTES / CUBIC_EXT_BYTES /
  KECCAK_NODE_BYTES consts bumped to pub(crate) for the relocated tests
@diegokingston diegokingston marked this pull request as ready for review May 22, 2026 18:57
@github-actions
Copy link
Copy Markdown

Codex Code Review

No issues found in the PR diff.

The production changes appear limited to moving tests out of implementation modules and widening a few helpers/constants to pub(crate) so crate-level tests can access them. I didn’t see security, VM correctness, significant performance, or simplicity problems.

I could not run cargo test because the sandbox has read-only /home/runner/.rustup/.cargo paths and restricted network access, so Cargo could not prepare dependencies.

);
}

#[cfg(test)]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Low – redundant #[cfg(test)]

This file is already only compiled in test mode (declared as #[cfg(test)] pub mod trace_builder_tests; in mod.rs), so #[cfg(test)] on the nested mod keccak_tests and mod routing_tests is a no-op. Same pattern repeats at line 776. Harmless, but slightly misleading.

@claude
Copy link
Copy Markdown

claude Bot commented May 22, 2026

Review: refactor(prover): move inline table tests to tests/

Clean, low-risk refactoring. All tests are faithfully relocated with no logic changes or drops. A few minor observations:

Low – redundant #[cfg(test)] on nested modules

In trace_builder_tests.rs the new mod keccak_tests and mod routing_tests (lines 566 and 776) carry #[cfg(test)], but the enclosing file is already only compiled in test mode via #[cfg(test)] pub mod trace_builder_tests; in mod.rs. The attribute is a no-op. (Inline comment filed on line 566.)

Low – pub(crate) visibility bumps widen the internal API

Six items are now crate-visible that were previously module-private:

  • select_storage_mode, GOLDILOCKS_BYTES, CUBIC_EXT_BYTES, KECCAK_NODE_BYTES in auto_storage.rs
  • is_register_op, collect_bitwise_from_keccak in trace_builder.rs

These are implementation details needed only by the relocated tests. All stay behind pub(crate) (not pub), and auto_storage is already feature-gated, so this is an acceptable trade-off for the simpler refactor. Worth knowing if someone later reaches for these thinking they're part of an intentional internal interface.

Nothing else to flag

  • The #[cfg(all(test, feature = "disk-spill"))] gate on auto_storage_tests is correct — auto_storage is itself behind #[cfg(feature = "disk-spill")] in lib.rs, so no test regression.
  • INV_SHIFT_32 / SHIFT_32 are already pub const in templates.rs; templates_tests.rs compiles without further visibility changes.
  • All five test_is_register_op_* routing tests are present in mod routing_tests at the end of trace_builder_tests.rs.

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.

1 participant