Skip to content

refactor(crypto): move inline merkle-backend tests to tests/#615

Open
diegokingston wants to merge 1 commit into
mainfrom
refactor/move-crypto-inline-tests
Open

refactor(crypto): move inline merkle-backend tests to tests/#615
diegokingston wants to merge 1 commit into
mainfrom
refactor/move-crypto-inline-tests

Conversation

@diegokingston
Copy link
Copy Markdown
Collaborator

Relocate #[cfg(test)] mod tests blocks out of crypto/crypto source files into crypto/crypto/src/tests/.

  • backends/field_element.rs tests -> tests/field_element_tests.rs
  • backends/field_element_vector.rs tests -> tests/field_element_vector_tests.rs
  • merkle.rs disk_spill_serde_tests -> appended to tests/merkle_tests.rs
  • MerkleTree::mmap_backing bumped to pub(crate) so the relocated disk-spill serde test can reach it

Relocate #[cfg(test)] mod tests blocks out of crypto/crypto source
files into crypto/crypto/src/tests/.

- backends/field_element.rs tests -> tests/field_element_tests.rs
- backends/field_element_vector.rs tests -> tests/field_element_vector_tests.rs
- merkle.rs disk_spill_serde_tests -> appended to tests/merkle_tests.rs
- MerkleTree::mmap_backing bumped to pub(crate) so the relocated
  disk-spill serde test can reach it
@diegokingston diegokingston marked this pull request as ready for review May 22, 2026 19:14
#[cfg(feature = "disk-spill")]
#[cfg_attr(feature = "serde", serde(skip))]
mmap_backing: Option<MmapNodeBacking>,
pub(crate) mmap_backing: Option<MmapNodeBacking>,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Medium — unnecessary visibility widening

mmap_backing is promoted to pub(crate) solely so the relocated test can write restored.mmap_backing.is_none(). This leaks a disk-spill implementation detail to every crate-internal caller, not just test code.

Preferred alternatives (in order of preference):

  1. Add a #[cfg(test)] accessor on MerkleTree so the visibility stays private outside tests:
Suggested change
pub(crate) mmap_backing: Option<MmapNodeBacking>,
mmap_backing: Option<MmapNodeBacking>,
#[cfg(test)]
impl<B: IsMerkleTreeBackend> MerkleTree<B> {
    pub(crate) fn has_mmap_backing(&self) -> bool {
        self.mmap_backing.is_some()
    }
}
  1. Test a weaker but equivalent invariant that doesn't require field access, e.g. check that restored.nodes is non-empty instead.

assert_eq!(batch_proof.path.len(), 2);
}

#[cfg(all(test, feature = "serde", feature = "disk-spill"))]
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 test in cfg predicate

This file is only compiled because lib.rs declares #[cfg(test)] pub mod tests;, so the test condition here is already implied. The inner module will only ever exist during a test build.

Suggested change
#[cfg(all(test, feature = "serde", feature = "disk-spill"))]
#[cfg(all(feature = "serde", feature = "disk-spill"))]

@github-actions
Copy link
Copy Markdown

Codex Code Review

No issues found in the PR diff.

I could not complete test verification in this sandbox: cargo test was blocked by rustup/cargo trying to write under read-only /home/runner/.rustup / /home/runner/.cargo, and offline mode still required the workspace git dependency checkout.

@github-actions
Copy link
Copy Markdown

Codex Code Review

Findings

  • Low Security / Invariant Risk: crypto/crypto/src/merkle_tree/merkle.rs widens mmap_backing from private to pub(crate) only so the relocated test can inspect it. This field participates in the unsafe mmap-backed node access invariant; after spill_nodes_to_disk, nodes is empty and correctness depends on mmap_backing not being mutated externally. Prefer keeping the test inside merkle.rs, or add a narrow #[cfg(test)] accessor like has_mmap_backing() instead of exposing the field crate-wide.

No Critical/High/Medium issues found in the PR diff.

I attempted to run tests, but cargo test was blocked because rustup tried to write under /home/runner/.rustup/tmp, which is read-only in this environment.

#[cfg(all(test, feature = "serde", feature = "disk-spill"))]
mod disk_spill_serde_tests {
use crate::merkle_tree::backends::field_element::FieldElementBackend;
use crate::merkle_tree::merkle::*;
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 — wildcard import pulls in more than needed

Only MerkleTree is used from this module; the glob makes the dependency surface opaque.

Suggested change
use crate::merkle_tree::merkle::*;
use crate::merkle_tree::merkle::MerkleTree;

@claude
Copy link
Copy Markdown

claude Bot commented May 22, 2026

Review: refactor(crypto): move inline merkle-backend tests to tests/

Clean refactor — the test logic is identical to what was removed from the source files, and the module structure is correct (the tests parent is already behind #[cfg(test)] in lib.rs, so all the pub mod declarations in tests/mod.rs are safe).

Three minor issues flagged inline:

Severity File Issue
Medium merkle.rs:60 mmap_backing bumped to pub(crate) just to serve one test assertion — leaks an internal field to all crate code when disk-spill is active. A #[cfg(test)] accessor keeps the field private.
Low merkle_tests.rs:140 #[cfg(all(test, ...))test is redundant; the whole tests module is already #[cfg(test)].
Low merkle_tests.rs:143 use crate::merkle_tree::merkle::* — only MerkleTree is used; prefer the explicit import.

The pub(crate) concern is the only one worth addressing before merge; the other two are cosmetic.

@@ -0,0 +1,118 @@
//! Tests for the field-element Merkle tree backend.

use alloc::vec::Vec;
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 — unnecessary alloc::vec::Vec import

This was needed in the original field_element.rs where it was compiled in no_std configurations. This tests/ module is #[cfg(test)]-gated and always compiled with std, so Vec is already in the prelude. The sibling field_element_vector_tests.rs doesn't import it.

Suggested change
use alloc::vec::Vec;

assert_eq!(batch_proof.path.len(), 2);
}

#[cfg(all(test, feature = "serde", feature = "disk-spill"))]
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 — dead test predicate in cfg guard

This file lives inside a module that is already declared #[cfg(test)] in lib.rs, so the test predicate here is always true and does nothing. It was correct in merkle.rs (where it was copy-pasted from) because that file is compiled unconditionally, but here it's misleading.

Suggested change
#[cfg(all(test, feature = "serde", feature = "disk-spill"))]
#[cfg(all(feature = "serde", feature = "disk-spill"))]

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