Skip to content

refactor(executor): move inline unit tests to src/tests/ module#619

Open
diegokingston wants to merge 2 commits into
mainfrom
cleanup/executor-inline-tests
Open

refactor(executor): move inline unit tests to src/tests/ module#619
diegokingston wants to merge 2 commits into
mainfrom
cleanup/executor-inline-tests

Conversation

@diegokingston
Copy link
Copy Markdown
Collaborator

Summary

Relocates the three inline #[cfg(test)] mod tests blocks in executor/src into a dedicated executor/src/tests/ module, matching the convention already used by the prover and math crates.

From To Covers
flamegraph.rs tests/flamegraph_tests.rs demangle
vm/instruction/execution.rs tests/keccak_tests.rs keccak-f[1600] + Keccak syscall
vm/memory.rs tests/memory_tests.rs public-output commits, load bounds

demangle and Memory::set_bytes_aligned were module-private; bumped to pub(crate) so the relocated tests can reach them. No test logic changed — bodies moved verbatim, only use super::* rewritten to explicit crate:: paths.

Test plan

  • cargo clippy -p executor --all-targets -- -D warnings — clean
  • cargo test -p executor --lib — 12 passed (1 demangle + 4 keccak + 7 memory)

Relocates the three inline `#[cfg(test)] mod tests` blocks in executor/src
into a dedicated `executor/src/tests/` module, matching the convention
already used by the prover and math crates.

- flamegraph.rs                -> tests/flamegraph_tests.rs (demangle)
- vm/instruction/execution.rs  -> tests/keccak_tests.rs (keccak-f[1600] + syscall)
- vm/memory.rs                 -> tests/memory_tests.rs (public output, load bounds)

`demangle` and `Memory::set_bytes_aligned` were module-private; bumped to
pub(crate) so the relocated tests can reach them. No test logic changed.
@github-actions
Copy link
Copy Markdown

Codex Code Review

No security vulnerabilities, behavioral bugs, or significant performance issues found in the PR diff.

One minor cleanliness note: executor/src/lib.rs now exports pub mod tests; in normal builds, but the submodules are #[cfg(test)], so it becomes an empty public executor::tests module outside tests. That is likely harmless, but #[cfg(test)] mod tests; would avoid exposing test-only API surface.

I attempted cargo test -p executor --no-run, but the environment failed before compiling because rustup could not create a temp file under the read-only /home/runner/.rustup/tmp.

Comment thread executor/src/lib.rs
@@ -1,3 +1,4 @@
pub mod elf;
pub mod flamegraph;
pub mod tests;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug (Medium): Missing #[cfg(test)] gate — this is the exact convention the PR claims to match.

Both prover/src/lib.rs and crypto/math/src/lib.rs declare this as:

#[cfg(test)]
pub mod tests;

Without the gate, the tests module is compiled into every non-test build and becomes part of the crate's public API (visible as executor::tests). It will be an empty module in non-test builds (since all sub-modules in mod.rs are individually gated), but it still pollutes the public surface.

Suggested change
pub mod tests;
#[cfg(test)]
pub mod tests;

Comment thread executor/src/vm/memory.rs
/// Helper method to store a given input at an aligned address. It may also overwrite existing bytes with zero if inputs is not divisible by 4
/// Should only be used to write to public output and private input where these limitations are not a problem
fn set_bytes_aligned(&mut self, mut addr: u64, inputs: &[u8]) -> Result<(), MemoryError> {
pub(crate) fn set_bytes_aligned(
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: The doc comment on this function already warns it "Should only be used to write to public output and private input where these limitations are not a problem." Widening it to pub(crate) to serve only tests means any future code in the crate can accidentally call it outside those two intended sites. Consider keeping it pub(super) or using a test-only re-export instead.

@claude
Copy link
Copy Markdown

claude Bot commented May 22, 2026

Review

Clean refactor overall — test bodies are moved verbatim and the logic is unchanged. One bug to fix before merging, plus a minor nit.

Bug — missing #[cfg(test)] on the tests module (Medium)

executor/src/lib.rs declares pub mod tests; without a #[cfg(test)] gate. The two crates cited as the convention to match (prover, math) both use:

#[cfg(test)]
pub mod tests;

Without the gate the tests module ships into every non-test build and becomes part of the crate's public API (executor::tests). It will be empty in those builds (all sub-modules in mod.rs are individually gated), but it still pollutes the public surface and diverges from the stated goal.

Nit — pub(crate) on set_bytes_aligned (Low)

The function's own doc comment says it "Should only be used to write to public output and private input." pub(crate) makes it reachable from anywhere in the crate, widening the accidental-misuse surface. Worth keeping it narrower if possible (e.g. pub(super) or a test-only re-export).

@github-actions
Copy link
Copy Markdown

Benchmark Results for unmodified programs 🚀

Command Mean [ms] Min [ms] Max [ms] Relative
base binary_search 50.6 ± 2.5 48.8 57.4 1.00
head binary_search 52.0 ± 5.2 48.6 65.3 1.03 ± 0.12
Command Mean [ms] Min [ms] Max [ms] Relative
base bitwise_ops 50.1 ± 0.8 48.7 50.9 1.00 ± 0.03
head bitwise_ops 50.0 ± 1.4 48.5 52.2 1.00
Command Mean [ms] Min [ms] Max [ms] Relative
base fibonacci_26 55.2 ± 0.9 54.1 56.5 1.00
head fibonacci_26 56.5 ± 3.2 53.9 64.2 1.02 ± 0.06
Command Mean [ms] Min [ms] Max [ms] Relative
base hashmap 106.5 ± 1.7 104.6 109.5 1.00
head hashmap 107.7 ± 3.0 103.5 114.8 1.01 ± 0.03
Command Mean [ms] Min [ms] Max [ms] Relative
base keccak 108.5 ± 0.7 107.5 109.6 1.00 ± 0.02
head keccak 108.4 ± 1.7 105.6 110.7 1.00
Command Mean [ms] Min [ms] Max [ms] Relative
base matrix_multiply 61.9 ± 1.1 60.4 63.9 1.01 ± 0.03
head matrix_multiply 61.6 ± 1.6 59.9 65.4 1.00
Command Mean [ms] Min [ms] Max [ms] Relative
base modular_exp 50.5 ± 2.1 49.0 56.0 1.00
head modular_exp 50.6 ± 1.0 49.1 51.8 1.00 ± 0.05
Command Mean [ms] Min [ms] Max [ms] Relative
base quicksort 53.2 ± 0.7 51.7 54.0 1.01 ± 0.02
head quicksort 52.5 ± 0.8 51.6 53.9 1.00
Command Mean [ms] Min [ms] Max [ms] Relative
base sieve 55.9 ± 1.1 54.7 57.6 1.00
head sieve 55.9 ± 1.1 54.6 57.4 1.00 ± 0.03
Command Mean [ms] Min [ms] Max [ms] Relative
base sum_array 66.5 ± 1.0 65.0 67.9 1.00
head sum_array 67.0 ± 1.2 65.7 68.8 1.01 ± 0.02
Command Mean [ms] Min [ms] Max [ms] Relative
base syscall_commit 78.6 ± 2.8 76.0 85.2 1.00
head syscall_commit 82.8 ± 9.4 75.4 102.4 1.05 ± 0.12

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