test: pin hash and compact regression values - #265
Conversation
Signed-off-by: Nikhil Sharma <nikhilsharma230303@gmail.com>
📝 WalkthroughWalkthroughAdds hardcoded snapshot regression tests pinning ChangesRegression snapshot tests
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (6)
src/transaction/txtype.rs (1)
196-197: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert against protocol constants, not raw literals.
Line 196 and Line 197 should assert against named constants (e.g., compact extended identifier flag and
POL_TX_TYPE) instead of3usize/0x7e.As per coding guidelines,
src/**/*.rs: Extract magic numbers into documented constants.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/transaction/txtype.rs` around lines 196 - 197, Replace the magic number literals in the assert_eq calls with named constants. Extract 3usize (the compact extended identifier flag) and 0x7e (POL_TX_TYPE) into properly documented constant definitions, then use these constants in place of the raw literals in the assertion statements within the test function. This aligns with the coding guideline to extract magic numbers into documented constants for improved code clarity and maintainability.Source: Coding guidelines
src/transaction/pol.rs (2)
260-263: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove inline snapshot comments in this Rust source file.
Line 260 through Line 263 introduces inline comments that conflict with the repository’s Rust-file comment rule.
As per coding guidelines,
src/**/*.rs: No comments in code unless explicitly requested.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/transaction/pol.rs` around lines 260 - 263, Remove the entire multi-line comment block that describes the self-generated snapshot and references compress_to_buf output and test_pol_transaction_hash_regression. This comment violates the repository's coding guideline that prohibits comments in Rust source files (src/**/*.rs) unless explicitly requested. Delete all lines containing the snapshot explanation and cargo test instruction.Source: Coding guidelines
242-242: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared test inputs into named constants.
Line 242 and Line 271 repeat raw values (
10,1000) for the same regression fixture path; promote them to constants to avoid drift between tests.As per coding guidelines,
src/**/*.rs: Extract magic numbers into documented constants.Also applies to: 271-271
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/transaction/pol.rs` at line 242, The magic numbers `10` and `1000` are hardcoded in multiple test calls to `create_pol_transaction` on lines 242 and 271, which violates the coding guideline to extract magic numbers into documented constants. Define named constants at the module level with descriptive names that explain what these values represent (e.g., their role in the test fixture), then replace all hardcoded occurrences of `10` and `1000` in both `create_pol_transaction` invocations with these constants to ensure consistency and maintainability across the test suite.Source: Coding guidelines
src/primitives/header.rs (2)
985-988: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace bitflag literals with named constants.
Line 986 and Line 987 hardcode
4and1; use named constants so the test contract stays explicit and resilient to future encoding changes.As per coding guidelines,
src/**/*.rs: Extract magic numbers into documented constants.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/primitives/header.rs` around lines 985 - 988, The test function compact_bitflag_layout contains hardcoded magic numbers 4 and 1 in the assertions for CompactBerachainHeader::bitflag_encoded_bytes() and BerachainHeaderExt::bitflag_encoded_bytes() respectively. Extract these magic numbers into named constants with descriptive names that document their purpose (for example, constants like COMPACT_BERACHAIN_HEADER_BITFLAG_SIZE and BERACHAIN_HEADER_EXT_BITFLAG_SIZE), define them at the module level or in an appropriate scope, and then replace the hardcoded values 4 and 1 with these named constants in the respective assert_eq! calls to improve code maintainability and resilience.Source: Coding guidelines
958-980: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove inline comments from the new regression block.
Line 958 through Line 980 introduces new inline comments in Rust code, which conflicts with the repo rule for
src/**/*.rs.As per coding guidelines,
src/**/*.rs: No comments in code unless explicitly requested.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/primitives/header.rs` around lines 958 - 980, Remove all inline comments from the regression test block containing the constants HOLESKY_COMPACT, HOLESKY_HASH, and WITH_PROPOSER_COMPACT. Delete all comment lines that begin with "//" in this test section while keeping the const declarations and hex value assignments intact. This aligns with the repository rule that prohibits inline comments in Rust source files under src/**/*.rs.Source: Coding guidelines
src/transaction/mod.rs (1)
1237-1250: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the new inline explanatory comments in this test module.
Line 1237 through Line 1250 adds inline comments that violate the Rust path rule for this repository.
As per coding guidelines,
src/**/*.rs: No comments in code unless explicitly requested.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/transaction/mod.rs` around lines 1237 - 1250, Remove all the inline explanatory comments in the test module around the constant definitions. Specifically, delete the comment lines preceding the MINIMAL_POL_HASH constant definition, the MINIMAL_POL_COMPACT constant definition, and the BerachainTxEnvelope compress_to_buf() output comment section. Keep only the actual constant declarations and their assignments, as the repository coding guidelines prohibit explanatory comments in src/**/*.rs files unless explicitly requested.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/primitives/header.rs`:
- Around line 985-988: The test function compact_bitflag_layout contains
hardcoded magic numbers 4 and 1 in the assertions for
CompactBerachainHeader::bitflag_encoded_bytes() and
BerachainHeaderExt::bitflag_encoded_bytes() respectively. Extract these magic
numbers into named constants with descriptive names that document their purpose
(for example, constants like COMPACT_BERACHAIN_HEADER_BITFLAG_SIZE and
BERACHAIN_HEADER_EXT_BITFLAG_SIZE), define them at the module level or in an
appropriate scope, and then replace the hardcoded values 4 and 1 with these
named constants in the respective assert_eq! calls to improve code
maintainability and resilience.
- Around line 958-980: Remove all inline comments from the regression test block
containing the constants HOLESKY_COMPACT, HOLESKY_HASH, and
WITH_PROPOSER_COMPACT. Delete all comment lines that begin with "//" in this
test section while keeping the const declarations and hex value assignments
intact. This aligns with the repository rule that prohibits inline comments in
Rust source files under src/**/*.rs.
In `@src/transaction/mod.rs`:
- Around line 1237-1250: Remove all the inline explanatory comments in the test
module around the constant definitions. Specifically, delete the comment lines
preceding the MINIMAL_POL_HASH constant definition, the MINIMAL_POL_COMPACT
constant definition, and the BerachainTxEnvelope compress_to_buf() output
comment section. Keep only the actual constant declarations and their
assignments, as the repository coding guidelines prohibit explanatory comments
in src/**/*.rs files unless explicitly requested.
In `@src/transaction/pol.rs`:
- Around line 260-263: Remove the entire multi-line comment block that describes
the self-generated snapshot and references compress_to_buf output and
test_pol_transaction_hash_regression. This comment violates the repository's
coding guideline that prohibits comments in Rust source files (src/**/*.rs)
unless explicitly requested. Delete all lines containing the snapshot
explanation and cargo test instruction.
- Line 242: The magic numbers `10` and `1000` are hardcoded in multiple test
calls to `create_pol_transaction` on lines 242 and 271, which violates the
coding guideline to extract magic numbers into documented constants. Define
named constants at the module level with descriptive names that explain what
these values represent (e.g., their role in the test fixture), then replace all
hardcoded occurrences of `10` and `1000` in both `create_pol_transaction`
invocations with these constants to ensure consistency and maintainability
across the test suite.
In `@src/transaction/txtype.rs`:
- Around line 196-197: Replace the magic number literals in the assert_eq calls
with named constants. Extract 3usize (the compact extended identifier flag) and
0x7e (POL_TX_TYPE) into properly documented constant definitions, then use these
constants in place of the raw literals in the assertion statements within the
test function. This aligns with the coding guideline to extract magic numbers
into documented constants for improved code clarity and maintainability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b64ba9c0-2d3b-4bf9-910c-076871c1019a
📒 Files selected for processing (7)
src/chainspec/mod.rssrc/primitives/header.rssrc/test_utils.rssrc/test_utils/regression_fixtures.rssrc/transaction/mod.rssrc/transaction/pol.rssrc/transaction/txtype.rs
There was a problem hiding this comment.
Pull request overview
This PR adds deterministic regression tests to pin expected hash and compact-encoding outputs for Berachain-specific primitives (headers, PoL transactions, tx type encoding, and genesis hashes), so any upstream/internal encoding or hashing changes will surface as test failures instead of silently changing compatibility.
Changes:
- Introduces shared deterministic regression fixtures plus an ignored “snapshot generator” test for regenerating pinned values.
- Adds regression tests for header compact/hash (including Berachain extension fields) and PoL transaction hash/compact/envelope-compact.
- Adds regression tests for Bepolia and mainnet genesis hash values.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/transaction/txtype.rs | Adds a regression test pinning Berachain tx type compact encoding output. |
| src/transaction/pol.rs | Adds PoL transaction hash and envelope compact encoding regression tests. |
| src/transaction/mod.rs | Adds regression tests for minimal PoL tx hash/compact and envelope compact encoding. |
| src/test_utils/regression_fixtures.rs | Adds deterministic fixtures and an ignored generator for printing/pinning regression values. |
| src/test_utils.rs | Exposes the regression fixtures module for tests. |
| src/primitives/header.rs | Adds header compact/hash regression tests, including Berachain extension fields, and roundtrip checks. |
| src/chainspec/mod.rs | Adds regression tests pinning Bepolia and mainnet genesis hashes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Nikhil Sharma <nikhilsharma230303@gmail.com>
|
@calbera PTAL, thanks |
Closes #89
Summary
This adds regression coverage for hash and compact encoding paths across Berachain-specific primitives, pinning fixed expected values so upstream or internal encoding changes fail tests instead of silently breaking compatibility.
src/test_utils/regression_fixtures.rswith shared deterministic fixtures and an ignored generator for regenerating snapshotsTest plan
cargo test regressionmake pr(fmt, dprint, clippy, deny, machete, docs, nextest)holesky_compact_matches_reth_v1_11_4holesky_hash_regressionprev_proposer_pubkey_compact_regressionprev_proposer_pubkey_hash_regressionminimal_pol_tx_hash_regressionminimal_pol_tx_compact_regressionminimal_pol_envelope_compact_regressiontest_berachain_tx_type_compact_regressiontest_pol_transaction_hash_regressiontest_pol_transaction_compact_regressiontest_bepolia_genesis_hash_regressiontest_mainnet_genesis_hash_regressionSummary by CodeRabbit