Skip to content

fix(test): more JSON snapshot tests#6384

Merged
hanabi1224 merged 1 commit intomainfrom
hm/lotus-json-tests-3
Jan 6, 2026
Merged

fix(test): more JSON snapshot tests#6384
hanabi1224 merged 1 commit intomainfrom
hm/lotus-json-tests-3

Conversation

@hanabi1224
Copy link
Contributor

@hanabi1224 hanabi1224 commented Jan 6, 2026

Summary of changes

Part of #6365

Changes introduced in this pull request:

Reference issue to close (if applicable)

Closes

Other information and links

Change checklist

  • I have performed a self-review of my own code,
  • I have made corresponding changes to the documentation. All new code adheres to the team's documentation standards,
  • I have added tests that prove my fix is effective or that my feature works (if possible),
  • I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

Summary by CodeRabbit

  • Tests

    • Added snapshot testing infrastructure for JSON serialization validation across multiple versions, enabling automated verification of data transformation consistency and preventing unintended serialization changes.
  • Refactor

    • Reorganized type implementations into modular, version-specific structures for improved maintainability and clarity; enhanced equality comparison support for core data structures.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 6, 2026

Walkthrough

This pull request systematically refactors lotus_json serialization implementations to add equality semantics (Eq/PartialEq derives), reorganize versioned trait implementations into per-version modules, and enhance test coverage via macro invocations. The test_snapshots macro signature is updated to accept type paths rather than identifiers.

Changes

Cohort / File(s) Summary
Equality Traits Addition
src/lotus_json/entry.rs, src/lotus_json/filter_estimate.rs, src/lotus_json/token_state.rs
Added Eq and PartialEq derives to EntryLotusJson, FilterEstimateLotusJson, and TokenStateLotusJson structs.
Macro Infrastructure Update
src/lotus_json/mod.rs
Updated test_snapshots! macro signature from ($ty:ident) to ($ty:ty) to accept type paths instead of simple identifiers.
Entry Serialization & Tests
src/lotus_json/entry.rs
Added serde(with = "crate::lotus_json") attribute on receiver field for custom deserialization; added crate::test_snapshots!(Entry) invocation.
MinerPower Test Implementation
src/lotus_json/miner_power.rs
Implemented concrete snapshots() function with test JSON data including nested MinerPower/TotalPower objects; replaced commented assertion with crate::test_snapshots!(MinerPower) macro.
Per-Version FilterEstimate Modules
src/lotus_json/filter_estimate.rs
Refactored HasLotusJson implementations into per-version modules (impl_filter_estimate_lotus_json_X) with local type aliases and module-scoped snapshot tests.
Per-Version Signature Modules
src/lotus_json/signature.rs
Introduced three new modules (signature2, signature3, signature4), each providing HasLotusJson for respective fvm_sharedN::crypto::signature::Signature types with version-specific snapshot data (BLS for v2, Secp256k1 for v3/v4).
Per-Version SignatureType Modules
src/lotus_json/signature_type.rs
Reorganized HasLotusJson implementations for SignatureType into per-version modules (signature2, signature3, signature4) with local type imports and module-scoped snapshot tests.
TokenState Refactoring
src/lotus_json/token_state.rs
Added TokenState import from public type path; changed HasLotusJson impl target from qualified path to TokenState; updated JSON field names to PascalCase via struct-level rename_all; added crate::test_snapshots!(TokenState) invocation.
Per-Version TransientData Modules
src/lotus_json/transient_data.rs
Reorganized HasLotusJson implementations into per-version modules with local type aliases and per-version snapshot tests; preserved into/from conversion logic within module scope.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

RPC

Suggested reviewers

  • akaladarshi
  • LesnyRumcajs

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 32.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix(test): more JSON snapshot tests' directly and accurately describes the primary purpose of the changes, which is adding JSON snapshot tests across multiple files.
✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3823a8a and 386932a.

📒 Files selected for processing (8)
  • src/lotus_json/entry.rs
  • src/lotus_json/filter_estimate.rs
  • src/lotus_json/miner_power.rs
  • src/lotus_json/mod.rs
  • src/lotus_json/signature.rs
  • src/lotus_json/signature_type.rs
  • src/lotus_json/token_state.rs
  • src/lotus_json/transient_data.rs
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6381
File: src/lotus_json/actors/states/cron_state.rs:8-8
Timestamp: 2026-01-05T12:54:51.873Z
Learning: In the Forest codebase, CronStateLotusJson in src/lotus_json/actors/states/cron_state.rs derives only PartialEq (not Eq) because it contains Entry types that wrap external dependencies from fil_actor_cron_state, which don't implement Eq. This constraint from external dependencies prevents deriving Eq.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5930
File: build.rs:64-77
Timestamp: 2025-08-13T09:43:20.301Z
Learning: hanabi1224 prefers hard compile-time errors in build scripts rather than runtime safeguards or collision detection, believing it's better to fail fast and fix root causes of issues like malformed snapshot names.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6057
File: src/cli/subcommands/f3_cmd.rs:0-0
Timestamp: 2025-09-09T10:37:17.947Z
Learning: hanabi1224 prefers having default timeouts (like 10m for --no-progress-timeout) to prevent commands from hanging indefinitely, even when the timeout flag isn't explicitly provided by users. This fail-fast approach is preferred over requiring explicit flag usage.
📚 Learning: 2025-08-18T12:25:29.183Z
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 5896
File: src/lotus_json/actor_states/methods/verified_reg_actor.rs:133-137
Timestamp: 2025-08-18T12:25:29.183Z
Learning: In Lotus JSON for verified registry actor, the SectorAllocationClaimsLotusJson struct uses "SectorExpiry" as the field name for the expiry field, not "Expiry". The current code with #[serde(rename = "SectorExpiry")] is correct.

Applied to files:

  • src/lotus_json/miner_power.rs
  • src/lotus_json/filter_estimate.rs
  • src/lotus_json/entry.rs
  • src/lotus_json/transient_data.rs
  • src/lotus_json/token_state.rs
📚 Learning: 2026-01-05T12:54:40.850Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6381
File: src/lotus_json/actors/states/cron_state.rs:8-8
Timestamp: 2026-01-05T12:54:40.850Z
Learning: In Rust code reviews, do not derive Eq for a struct if any field does not implement Eq (e.g., types from external dependencies). If a type like CronStateLotusJson includes fields wrapping external dependencies that lack Eq, derive PartialEq (or implement PartialEq manually) but avoid deriving Eq. This ensures comparisons compile and reflect actual equivalence semantics. When needed, consider implementing custom PartialEq (and possibly Eq) only after ensuring all fields (or wrappers) implement Eq, or keep PartialEq-only if full equality semantics cannot be expressed.

Applied to files:

  • src/lotus_json/miner_power.rs
  • src/lotus_json/mod.rs
  • src/lotus_json/filter_estimate.rs
  • src/lotus_json/entry.rs
  • src/lotus_json/transient_data.rs
  • src/lotus_json/signature_type.rs
  • src/lotus_json/signature.rs
  • src/lotus_json/token_state.rs
📚 Learning: 2026-01-05T12:56:13.802Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6381
File: src/lotus_json/actors/states/evm_state.rs:41-44
Timestamp: 2026-01-05T12:56:13.802Z
Learning: In Rust codebases (e.g., Forest), do not add #[cfg(test)] to functions already annotated with #[test]. The #[test] attribute ensures the function is compiled only for tests, so a separate #[cfg(test)] is redundant and can be removed if present. Apply this check to all Rust files that contain #[test] functions.

Applied to files:

  • src/lotus_json/miner_power.rs
  • src/lotus_json/mod.rs
  • src/lotus_json/filter_estimate.rs
  • src/lotus_json/entry.rs
  • src/lotus_json/transient_data.rs
  • src/lotus_json/signature_type.rs
  • src/lotus_json/signature.rs
  • src/lotus_json/token_state.rs
📚 Learning: 2026-01-05T13:02:14.604Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6381
File: src/shim/actors/builtin/cron/mod.rs:47-47
Timestamp: 2026-01-05T13:02:14.604Z
Learning: The Entry enum in src/shim/actors/builtin/cron/mod.rs cannot derive Eq because it wraps external fil_actor_cron_state::vX::Entry types (v8-v17) that don't implement Eq. Only PartialEq can be derived for this enum due to this external dependency constraint.

Applied to files:

  • src/lotus_json/entry.rs
📚 Learning: 2025-09-10T12:07:10.578Z
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 6000
File: src/lotus_json/actors/params/init_params.rs:34-47
Timestamp: 2025-09-10T12:07:10.578Z
Learning: In Lotus JSON for init actor Exec4Params, the InitExec4ParamsLotusJson struct uses "sub_address" as the field name which serializes to "SubAddress" in JSON via PascalCase conversion. This correctly maps to the internal fil_actor_init_state subaddress field through the conversion methods, maintaining Lotus API compatibility.

Applied to files:

  • src/lotus_json/entry.rs
📚 Learning: 2025-09-02T08:44:08.346Z
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 5996
File: src/lotus_json/actor_states/methods/paych_params.rs:56-58
Timestamp: 2025-09-02T08:44:08.346Z
Learning: In Lotus JSON for payment channel SignedVoucher, the secret_pre_image field should use serde rename to "SecretHash" (not "SecretPreImage") for Lotus API compatibility, even though the internal struct field is named SecretPreimage. This matches the actual JSON format used by Lotus.

Applied to files:

  • src/lotus_json/signature.rs
  • src/lotus_json/token_state.rs
📚 Learning: 2025-09-11T16:03:14.328Z
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 6000
File: src/tool/subcommands/api_cmd/state_decode_params_tests/miner.rs:4-9
Timestamp: 2025-09-11T16:03:14.328Z
Learning: In the Forest codebase's state_decode_params_tests module, common imports like `to_vec`, `TokenAmount`, `Cid`, etc. are centralized in the mod.rs file and made available to submodules through `use super::*;`, eliminating the need for duplicate imports in individual test files.

Applied to files:

  • src/lotus_json/token_state.rs
🧬 Code graph analysis (4)
src/lotus_json/miner_power.rs (2)
src/tool/subcommands/snapshot_cmd.rs (1)
  • json (536-547)
src/shim/actors/builtin/power/mod.rs (2)
  • miner_power (224-241)
  • total_power (162-205)
src/lotus_json/filter_estimate.rs (4)
src/lotus_json/entry.rs (3)
  • snapshots (21-29)
  • into_lotus_json (31-46)
  • from_lotus_json (48-50)
src/lotus_json/miner_power.rs (3)
  • snapshots (24-49)
  • into_lotus_json (50-56)
  • from_lotus_json (57-63)
src/lotus_json/mod.rs (17)
  • snapshots (147-147)
  • snapshots (581-583)
  • snapshots (595-597)
  • snapshots (612-614)
  • snapshots (636-638)
  • serde_json (316-316)
  • serde_json (340-340)
  • into_lotus_json (148-148)
  • into_lotus_json (584-586)
  • into_lotus_json (598-600)
  • into_lotus_json (615-621)
  • into_lotus_json (639-646)
  • from_lotus_json (149-149)
  • from_lotus_json (587-589)
  • from_lotus_json (601-606)
  • from_lotus_json (622-628)
  • from_lotus_json (647-654)
src/lotus_json/signature.rs (12)
  • snapshots (59-67)
  • snapshots (94-102)
  • snapshots (128-136)
  • snapshots (162-170)
  • into_lotus_json (69-75)
  • into_lotus_json (104-109)
  • into_lotus_json (138-143)
  • into_lotus_json (172-177)
  • from_lotus_json (77-83)
  • from_lotus_json (111-116)
  • from_lotus_json (145-150)
  • from_lotus_json (179-184)
src/lotus_json/transient_data.rs (1)
src/lotus_json/mod.rs (18)
  • snapshots (147-147)
  • snapshots (581-583)
  • snapshots (595-597)
  • snapshots (612-614)
  • snapshots (636-638)
  • assert_all_snapshots (286-296)
  • serde_json (316-316)
  • serde_json (340-340)
  • into_lotus_json (148-148)
  • into_lotus_json (584-586)
  • into_lotus_json (598-600)
  • into_lotus_json (615-621)
  • into_lotus_json (639-646)
  • from_lotus_json (149-149)
  • from_lotus_json (587-589)
  • from_lotus_json (601-606)
  • from_lotus_json (622-628)
  • from_lotus_json (647-654)
src/lotus_json/signature.rs (2)
src/lotus_json/signature_type.rs (8)
  • into_lotus_json (33-35)
  • into_lotus_json (87-89)
  • into_lotus_json (117-119)
  • into_lotus_json (147-149)
  • from_lotus_json (37-41)
  • from_lotus_json (91-95)
  • from_lotus_json (121-125)
  • from_lotus_json (151-155)
src/lotus_json/key_info.rs (2)
  • into_lotus_json (36-42)
  • from_lotus_json (44-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
  • GitHub Check: Build forest binaries on Linux AMD64
  • GitHub Check: tests-release
  • GitHub Check: Coverage
  • GitHub Check: Build MacOS
  • GitHub Check: cargo-publish-dry-run
  • GitHub Check: Build Ubuntu
  • GitHub Check: All lint checks
🔇 Additional comments (23)
src/lotus_json/entry.rs (2)

52-52: LGTM: Snapshot test invocation added.

This wires up automatic snapshot testing for the Entry type using the updated macro.


8-15: LGTM: Added Eq and PartialEq derives for equality comparisons.

The EntryLotusJson struct contains Address and u64 fields, both of which implement Eq. While the underlying Entry enum wraps external types that don't implement Eq, this LotusJson representation is safe to derive Eq.

src/lotus_json/miner_power.rs (2)

25-48: LGTM: Concrete snapshot test data implemented.

The snapshot correctly maps MinerPower fields to their PascalCase JSON representation. The test data includes meaningful values for power claims and the has_min_power flag.


65-65: LGTM: Snapshot test macro invocation added.

This properly wires up the snapshot testing for MinerPower.

src/lotus_json/transient_data.rs (3)

31-81: LGTM: Per-version module structure for TransientData.

The modular approach with a local type alias T and dedicated test function is well-organized. The #[cfg(test)] on the trait's snapshots() method is appropriate here since it's a trait requirement (not a test function), while the actual #[test] fn snapshots() drives the test execution.


83-121: LGTM: Per-version module structure for TransientDataLifespan.

Follows the same consistent pattern as TransientData modules with proper test scaffolding.


127-127: LGTM: Macro invocation for versions 16 and 17.

This generates the implementations and tests for both supported EVM state versions.

src/lotus_json/filter_estimate.rs (3)

24-59: LGTM: Per-version module structure for FilterEstimate.

The modular approach with type alias T, dedicated test function, and HasLotusJson implementation follows the established pattern. The snapshot correctly tests default values with "0" string representation for BigInt fields.


65-66: LGTM: Macro invocation for versions 14-17.

This generates implementations and tests for all supported actor versions.


8-17: LGTM: Added Eq and PartialEq derives for equality comparisons.

The struct contains BigInt fields which implement Eq. This enables direct comparison of FilterEstimateLotusJson instances in tests and other code requiring equality.

src/lotus_json/token_state.rs (4)

7-7: LGTM: Simplified import.

The local import use token::state::TokenState; makes the code cleaner compared to using the fully qualified path.


9-26: LGTM: Added Eq and PartialEq derives.

The struct contains TokenAmount, Cid, and u32 fields, which should all support Eq. This enables equality comparisons needed for snapshot testing.


35-46: LGTM: Updated snapshot with PascalCase field names.

The JSON keys now correctly reflect the #[serde(rename_all = "PascalCase")] attribute on the struct.


67-67: LGTM: Snapshot test macro invocation added.

This wires up automatic snapshot testing for TokenState.

src/lotus_json/signature_type.rs (3)

70-98: LGTM: Per-version module for fvm_shared2 SignatureType.

The module correctly scopes the HasLotusJson implementation to fvm_shared2::crypto::signature::SignatureType with appropriate snapshots testing both Secp256k1 and BLS variants.


100-128: LGTM: Per-version module for fvm_shared3 SignatureType.

Follows the same consistent pattern as signature2 module.


130-158: LGTM: Per-version module for fvm_shared4 SignatureType.

Completes the per-version coverage with the same consistent pattern.

src/lotus_json/mod.rs (1)

248-258: LGTM: Macro signature updated to accept type paths.

Changing $ty:ident to $ty:ty allows the macro to accept full type paths (not just simple identifiers), enabling usage like crate::test_snapshots!(module::Type). The pastey::paste! macro's :snake modifier correctly handles type paths, so this pattern is fully supported.

src/lotus_json/signature.rs (5)

19-53: LGTM!

The versioned struct definitions are well-structured. Appropriately derives PartialEq without Eq to accommodate external dependencies that may not implement Eq. Based on learnings, this is the correct approach.


85-86: LGTM!

Proper addition of snapshot test registration for the shim Signature type.


87-119: LGTM!

The signature2 module correctly implements HasLotusJson for fvm_shared2::crypto::signature::Signature. The snapshot test data is consistent: Type 2 corresponds to BLS, and the base64 data decodes correctly to "hello world!".


121-153: LGTM!

The signature3 module correctly implements HasLotusJson for fvm_shared3::crypto::signature::Signature. The snapshot uses Type 1 (Secp256k1), which differs from v2's BLS type—this appears intentional for testing variety across versions.


155-187: LGTM!

The signature4 module correctly implements HasLotusJson for fvm_shared4::crypto::signature::Signature, mirroring the pattern established in signature3.

The structural duplication across signature2, signature3, and signature4 modules is acceptable given the multi-version external dependency support requirement.


Comment @coderabbitai help to get the list of available commands and usage tips.

@hanabi1224 hanabi1224 marked this pull request as ready for review January 6, 2026 12:19
@hanabi1224 hanabi1224 requested a review from a team as a code owner January 6, 2026 12:19
@hanabi1224 hanabi1224 requested review from akaladarshi and sudo-shashank and removed request for a team January 6, 2026 12:19
@hanabi1224 hanabi1224 enabled auto-merge January 6, 2026 12:43
@hanabi1224 hanabi1224 added this pull request to the merge queue Jan 6, 2026
@codecov
Copy link

codecov bot commented Jan 6, 2026

Codecov Report

❌ Patch coverage is 75.59809% with 51 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.08%. Comparing base (3823a8a) to head (386932a).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/lotus_json/signature.rs 52.63% 27 Missing ⚠️
src/lotus_json/signature_type.rs 42.85% 24 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
src/lotus_json/entry.rs 100.00% <ø> (+60.00%) ⬆️
src/lotus_json/filter_estimate.rs 100.00% <100.00%> (+42.85%) ⬆️
src/lotus_json/miner_power.rs 100.00% <100.00%> (+56.25%) ⬆️
src/lotus_json/mod.rs 73.84% <ø> (ø)
src/lotus_json/token_state.rs 100.00% <100.00%> (+50.00%) ⬆️
src/lotus_json/transient_data.rs 100.00% <100.00%> (+100.00%) ⬆️
src/lotus_json/signature_type.rs 57.62% <42.85%> (ø)
src/lotus_json/signature.rs 68.60% <52.63%> (ø)

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3823a8a...386932a. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Merged via the queue into main with commit 402d261 Jan 6, 2026
48 checks passed
@hanabi1224 hanabi1224 deleted the hm/lotus-json-tests-3 branch January 6, 2026 13:19
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.

2 participants