fix(test): JSON snapshot tests for actor params (partial) #6371
Conversation
WalkthroughThis PR refactors HasLotusJson implementations for actor parameters into per-version modules, relaxes snapshot testing trait bounds to require Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
hanabi1224
requested review from
akaladarshi and
sudo-shashank
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lotus_json/actors/params/datacap_params.rs (1)
685-693: Snapshot JSON keys correctly use PascalCase.The snapshot JSON uses PascalCase (e.g., "Multiaddrs") consistent with the
#[serde(rename_all = "PascalCase")]attribute on the struct. However, the snapshot at line 693 hasmulti_addresses: vec![]while the JSON shows"Multiaddrs": ["Ag==", "Aw=="]- this mismatch will cause the test to fail.🔎 Suggested fix
The Rust struct value should match the JSON expectation:
- multi_addresses: vec![], + multi_addresses: vec![vec![2], vec![3]],
🧹 Nitpick comments (3)
src/lotus_json/signature_type.rs (1)
52-68: Consider adding the same derives to versioned types for consistency.
SignatureTypeV2LotusJson,SignatureTypeV3LotusJson, andSignatureTypeV4LotusJsonlack theDebug,Clone,Eq,PartialEqderives that were added toSignatureTypeLotusJson. If these versioned types may need snapshot testing in the future, adding the derives now would maintain consistency.🔎 Proposed fix
-#[derive(Deserialize, Serialize, JsonSchema)] +#[derive(Debug, Clone, Eq, PartialEq, Deserialize, Serialize, JsonSchema)] #[serde(untagged)] pub enum SignatureTypeV2LotusJson { Integer(#[schemars(with = "u8")] fvm_shared2::crypto::signature::SignatureType), } -#[derive(Deserialize, Serialize, JsonSchema)] +#[derive(Debug, Clone, Eq, PartialEq, Deserialize, Serialize, JsonSchema)] #[serde(untagged)] pub enum SignatureTypeV3LotusJson { Integer(#[schemars(with = "u8")] fvm_shared3::crypto::signature::SignatureType), } -#[derive(Deserialize, Serialize, JsonSchema)] +#[derive(Debug, Clone, Eq, PartialEq, Deserialize, Serialize, JsonSchema)] #[serde(untagged)] pub enum SignatureTypeV4LotusJson { Integer(#[schemars(with = "u8")] fvm_shared4::crypto::signature::SignatureType), }src/lotus_json/actors/params/datacap_params.rs (1)
128-128: Minor: Inconsistent indentation (tabs vs spaces).Line 128 uses a tab character for indentation while the rest of the file uses spaces. Same issue appears on line 191.
src/lotus_json/actors/params/miner_params.rs (1)
610-610: Minor: Inconsistent indentation (tabs vs spaces).Lines 610, 666, and 767 use tab characters while the rest of the file uses spaces. Consider normalizing to spaces for consistency.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/lotus_json/actors/params/account_params.rs(2 hunks)src/lotus_json/actors/params/datacap_params.rs(21 hunks)src/lotus_json/actors/params/miner_params.rs(3 hunks)src/lotus_json/beneficiary_term.rs(1 hunks)src/lotus_json/election_proof.rs(1 hunks)src/lotus_json/extended_sector_info.rs(1 hunks)src/lotus_json/gossip_block.rs(1 hunks)src/lotus_json/ipld.rs(1 hunks)src/lotus_json/mod.rs(3 hunks)src/lotus_json/pending_beneficiary_change.rs(1 hunks)src/lotus_json/po_st_proof.rs(1 hunks)src/lotus_json/power_claim.rs(1 hunks)src/lotus_json/sector_info.rs(1 hunks)src/lotus_json/sector_size.rs(1 hunks)src/lotus_json/signature_type.rs(1 hunks)src/lotus_json/ticket.rs(1 hunks)src/shim/sector.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 5996
File: src/lotus_json/actor_states/methods/paych_params.rs:269-277
Timestamp: 2025-09-02T08:46:04.937Z
Learning: In payment channel actor Lotus JSON serialization, empty merges should be serialized as `null` (not an empty array `[]`) to maintain compatibility with Lotus behavior.
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.
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/ticket.rssrc/lotus_json/pending_beneficiary_change.rssrc/lotus_json/sector_info.rssrc/lotus_json/po_st_proof.rssrc/lotus_json/sector_size.rssrc/lotus_json/beneficiary_term.rssrc/lotus_json/extended_sector_info.rssrc/lotus_json/ipld.rssrc/lotus_json/actors/params/miner_params.rssrc/lotus_json/election_proof.rssrc/lotus_json/power_claim.rssrc/lotus_json/actors/params/account_params.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/po_st_proof.rssrc/lotus_json/election_proof.rs
📚 Learning: 2025-09-02T10:05:34.350Z
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 5923
File: src/rpc/registry/actors/miner.rs:221-223
Timestamp: 2025-09-02T10:05:34.350Z
Learning: For miner actor ChangeOwnerAddress and ChangeOwnerAddressExported methods: versions 8-10 use bare Address as parameter type, while versions 11+ use ChangeOwnerAddressParams. This reflects the evolution of the Filecoin miner actor parameter structures across versions.
Applied to files:
src/lotus_json/actors/params/miner_params.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/actors/params/miner_params.rssrc/lotus_json/actors/params/datacap_params.rssrc/lotus_json/actors/params/account_params.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/actors/params/miner_params.rssrc/lotus_json/actors/params/account_params.rs
🧬 Code graph analysis (3)
src/lotus_json/actors/params/miner_params.rs (1)
src/lotus_json/mod.rs (13)
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)
src/lotus_json/actors/params/datacap_params.rs (2)
src/lotus_json/mod.rs (6)
snapshots(147-147)snapshots(581-583)snapshots(595-597)snapshots(612-614)snapshots(636-638)assert_all_snapshots(286-296)src/shim/address.rs (1)
new_id(142-144)
src/lotus_json/actors/params/account_params.rs (2)
src/lotus_json/mod.rs (8)
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)src/shim/address.rs (1)
new_id(142-144)
⏰ 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: tests-release
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: Coverage
- GitHub Check: Build MacOS
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build Ubuntu
- GitHub Check: All lint checks
🔇 Additional comments (21)
src/lotus_json/sector_size.rs (1)
7-10: LGTM!The addition of
Debug,Clone,Eq, andPartialEqderives toSectorSizeLotusJsonis appropriate for enabling snapshot testing. The newtype wrapper pattern ensures these traits derive correctly from the innerSectorSizetype.src/lotus_json/signature_type.rs (1)
14-23: LGTM!The trait derives enable snapshot testing for
SignatureTypeLotusJson.src/lotus_json/pending_beneficiary_change.rs (1)
9-22: LGTM!The addition of
Debug,Eq, andPartialEqderives (alongside existingClone) appropriately enables snapshot testing forPendingBeneficiaryChangeLotusJson. All field types (Address,TokenAmount,ChainEpoch,bool) support these trait derivations.src/lotus_json/sector_info.rs (1)
8-19: LGTM!The trait derives are appropriate for enabling snapshot testing. All field types (
RegisteredSealProof,u64,Cid) supportDebug,Clone,Eq, andPartialEqderivations.src/lotus_json/ipld.rs (1)
33-39: Eq derivation works correctly; NaN edge case already handled in tests.Adding
EqtoIpldLotusJsonis fine sinceipld_core::IpldimplementsEq. Note that the codebase already handles the float edge case: the quickcheck test explicitly convertsNaNto0.0viafixup_floats()(lines 283–302) becauseNaN != NaNbreaks equality comparisons. This mitigation ensures round-trip tests pass correctly.src/shim/sector.rs (1)
229-230: LGTM!Adding
Eqderive toRegisteredPoStProofis consistent with other similar shim types in this file (e.g.,RegisteredSealProof,SectorInfo,ExtendedSectorInfo) that already deriveEq. This enables stricter equality semantics needed for snapshot testing.src/lotus_json/gossip_block.rs (1)
9-22: LGTM!Adding
Debug,Clone,Eq, andPartialEqderives toGossipBlockLotusJsonenables the snapshot testing infrastructure to compare and debug serialized representations.src/lotus_json/beneficiary_term.rs (1)
8-22: LGTM!Adding
Debug,Eq, andPartialEqderives toBeneficiaryTermLotusJsonsupports the snapshot testing infrastructure. The test at the bottom of the file validates the implementation.src/lotus_json/ticket.rs (1)
8-15: LGTM!Adding
Debug,Clone,Eq, andPartialEqderives toTicketLotusJsonaligns with the PR-wide pattern for snapshot testing support.src/lotus_json/po_st_proof.rs (1)
9-19: LGTM!Adding
Debug,Clone,Eq, andPartialEqderives toPoStProofLotusJsonenables snapshot testing. TheRegisteredPoStProoffield now supportsEqvia the derives insrc/shim/sector.rs.src/lotus_json/power_claim.rs (1)
8-8: LGTM!Adding
EqandPartialEqderives aligns with the updated snapshot testing infrastructure that now requiresT::LotusJson: PartialEq + Debug.src/lotus_json/election_proof.rs (1)
8-8: LGTM!The additional trait derives (
Debug,Clone,Eq,PartialEq) enable this type to work with the updated snapshot testing infrastructure.src/lotus_json/extended_sector_info.rs (1)
8-8: LGTM!The trait derives are consistent with the broader pattern in this PR, enabling snapshot testing comparisons.
src/lotus_json/actors/params/datacap_params.rs (1)
25-62: Well-structured per-version module pattern.The macro-based approach cleanly generates per-version modules with embedded tests, type aliases, and
HasLotusJsonimplementations. This pattern provides good test coverage and organization.src/lotus_json/actors/params/miner_params.rs (2)
667-671: Good practice: Using#[ignore]with issue reference.Marking tests as ignored with a reference to the tracking issue (#6369) is a good approach for documenting known issues while maintaining test scaffolding.
766-770: Consistent use of#[ignore]with issue tracking.This follows the same pattern as the constructor params test, properly documenting the pending work via issue #6370.
src/lotus_json/mod.rs (3)
248-258: Good addition: Simplified single-type snapshot test macro.This new macro arm provides a cleaner way to generate snapshot tests for individual types without requiring the version and module path complexity.
286-295: Well-designed trait bound relaxation.Moving the
PartialEq + Debugrequirements fromTtoT::LotusJsonis a smart design decision. This allows testing types that don't implement these traits themselves (common with external crate types), while still enabling meaningful equality comparisons through the LotusJson representation.
299-324: Correct approach to avoid Clone requirement on T.The updated flow:
- Convert
valtoval_lotus_json(consumes val, no Clone needed)- Compare serialized form against expected snapshot
- Deserialize to
T::LotusJson, convert toT, then back toT::LotusJson- Compare the round-tripped
LotusJsonagainst the originalThis avoids requiring
CloneorEqonTwhile still validating the round-trip correctly.src/lotus_json/actors/params/account_params.rs (2)
30-78: Well-structured per-version module pattern.The refactored macro generates clean per-version modules with embedded tests and type aliases. The pattern is consistent with other param files in this PR.
120-123: Clear handling of API evolution across actor versions.The distinction between
ConstructorParams(v11-14) andtypes::ConstructorParams(v15-17) correctly handles the module structure changes across actor state versions.
| macro_rules! impl_lotus_json_for_miner_declare_faults_recovered_params { | ||
| ($($version:literal),+) => { | ||
| $( | ||
| paste! { | ||
| impl HasLotusJson for fil_actor_miner_state::[<v $version>]::DeclareFaultsRecoveredParams { | ||
| type LotusJson = DeclareFaultsRecoveredParamsLotusJson; | ||
| paste! { | ||
| impl HasLotusJson for fil_actor_miner_state::[<v $version>]::DeclareFaultsRecoveredParams { | ||
| type LotusJson = DeclareFaultsRecoveredParamsLotusJson; | ||
|
|
||
| #[cfg(test)] | ||
| fn snapshots() -> Vec<(serde_json::Value, Self)> { | ||
| vec![] | ||
| } | ||
| #[cfg(test)] | ||
| fn snapshots() -> Vec<(serde_json::Value, Self)> { | ||
| vec![] | ||
| } | ||
|
|
||
| fn into_lotus_json(self) -> Self::LotusJson { | ||
| DeclareFaultsRecoveredParamsLotusJson { | ||
| recoveries: self.recoveries.into_iter().map(|r| r.into_lotus_json()).collect(), | ||
| fn into_lotus_json(self) -> Self::LotusJson { | ||
| DeclareFaultsRecoveredParamsLotusJson { | ||
| recoveries: self.recoveries.into_iter().map(|r| r.into_lotus_json()).collect(), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn from_lotus_json(lotus_json: Self::LotusJson) -> Self { | ||
| Self { | ||
| recoveries: lotus_json.recoveries.into_iter().map(|r| fil_actor_miner_state::[<v $version>]::RecoveryDeclaration::from_lotus_json(r)).collect(), | ||
| fn from_lotus_json(lotus_json: Self::LotusJson) -> Self { | ||
| Self { | ||
| recoveries: lotus_json.recoveries.into_iter().map(|r| fil_actor_miner_state::[<v $version>]::RecoveryDeclaration::from_lotus_json(r)).collect(), | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| )+ | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find assert_all_snapshots function definition and usage
rg -n "assert_all_snapshots" --type rust -B 3 -A 5 | head -100Repository: ChainSafe/forest
Length of output: 5271
🏁 Script executed:
#!/bin/bash
# Find the definition of assert_all_snapshots function
rg -n "fn assert_all_snapshots" --type rust -A 10Repository: ChainSafe/forest
Length of output: 639
🏁 Script executed:
#!/bin/bash
# Check miner_params.rs for DeclareFaultsRecoveredParams tests and snapshots
rg -n "DeclareFaultsRecoveredParams" src/lotus_json/actors/params/miner_params.rs -A 5 -B 5 | head -150Repository: ChainSafe/forest
Length of output: 1374
🏁 Script executed:
#!/bin/bash
# Find where the macro impl_lotus_json_for_miner_declare_faults_recovered_params is invoked
rg -n "impl_lotus_json_for_miner_declare_faults_recovered_params" src/lotus_json/actors/params/miner_params.rs -B 2 -A 2Repository: ChainSafe/forest
Length of output: 702
🏁 Script executed:
#!/bin/bash
# Search for tests that call assert_all_snapshots on DeclareFaultsRecoveredParams
rg -n "DeclareFaultsRecoveredParams" src/lotus_json/ --type rust | grep -i testRepository: ChainSafe/forest
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check if there are any module-level tests in miner_params.rs that would call assert_all_snapshots
rg -n "#\[test\]" src/lotus_json/actors/params/miner_params.rs | head -20Repository: ChainSafe/forest
Length of output: 110
🏁 Script executed:
#!/bin/bash
# Check the tests around line 766 and nearby to see if any relate to DeclareFaultsRecoveredParams
sed -n '755,780p' src/lotus_json/actors/params/miner_params.rsRepository: ChainSafe/forest
Length of output: 1089
🏁 Script executed:
#!/bin/bash
# Check if the impl_lotus_json_for_miner_declare_faults_recovered_params macro generates tests
# Look at the full macro definition
sed -n '730,760p' src/lotus_json/actors/params/miner_params.rsRepository: ChainSafe/forest
Length of output: 1318
🏁 Script executed:
#!/bin/bash
# Check if there are any module tests that might call assert_all_snapshots on the versions of DeclareFaultsRecoveredParams
rg -n "mod tests\|#\[cfg\(test\)\]" src/lotus_json/actors/params/miner_params.rsRepository: ChainSafe/forest
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check how many lines the file has and look at the end for test organization
wc -l src/lotus_json/actors/params/miner_params.rs
# Also check if there's a tests module at the end
tail -50 src/lotus_json/actors/params/miner_params.rsRepository: ChainSafe/forest
Length of output: 4182
🏁 Script executed:
#!/bin/bash
# Compare with another macro that DOES generate tests - check impl_lotus_json_for_recover_declaration_params_v9_and_above
sed -n '761,795p' src/lotus_json/actors/params/miner_params.rsRepository: ChainSafe/forest
Length of output: 1626
🏁 Script executed:
#!/bin/bash
# Check how many similar macros generate tests vs don't
# Let's look at a few other impl macros to understand the pattern
rg -n "macro_rules! impl_lotus_json_for_miner" src/lotus_json/actors/params/miner_params.rs | head -15Repository: ChainSafe/forest
Length of output: 1177
🏁 Script executed:
#!/bin/bash
# Check impl_lotus_json_for_miner_constructor_params to see if it generates a test module
sed -n '660,720p' src/lotus_json/actors/params/miner_params.rsRepository: ChainSafe/forest
Length of output: 3553
🏁 Script executed:
#!/bin/bash
# Check if other macros without test modules also have empty snapshots
# Look at impl_lotus_json_for_miner_change_owner_address_params
sed -n '850,890p' src/lotus_json/actors/params/miner_params.rsRepository: ChainSafe/forest
Length of output: 1505
🏁 Script executed:
#!/bin/bash
# Verify: are there any tests that would call assert_all_snapshots for DeclareFaultsRecoveredParams?
# Search for any reference to snapshots test related to this type
rg -n "DeclareFaultsRecoveredParams" src/ --type rust | grep -E "test|snapshots|assert"Repository: ChainSafe/forest
Length of output: 205
🏁 Script executed:
#!/bin/bash
# Check how many similar types have empty snapshots but generate tests
rg -B 10 "vec!\[\]" src/lotus_json/actors/params/miner_params.rs | grep -A 10 "fn snapshots()" | head -40Repository: ChainSafe/forest
Length of output: 479
Add snapshot test data to impl_lotus_json_for_miner_declare_faults_recovered_params or mark as ignored.
The snapshots() method returns an empty vector, which would cause assert_all_snapshots to panic at the assert!(!snapshots.is_empty()) check. While this macro doesn't currently have a test calling assert_all_snapshots, the pattern in the codebase (e.g., impl_lotus_json_for_miner_constructor_params) shows similar types generate test modules. To maintain consistency and prevent future failures when tests are added, either provide meaningful snapshot test data or add an ignored test with a linked issue.
🤖 Prompt for AI Agents
In src/lotus_json/actors/params/miner_params.rs around lines 730–756 the
snapshots() test hook for
impl_lotus_json_for_miner_declare_faults_recovered_params currently returns an
empty Vec which will break assert_all_snapshots; either populate snapshots()
with at least one realistic (serde_json::Value, Self) tuple representing a
typical DeclareFaultsRecoveredParams instance (construct a sample
RecoveryDeclaration value, convert it into the actor type and its lotus JSON to
form the tuple) or replace the snapshots() implementation with a non-empty Vec
by adding a single placeholder sample, otherwise add a short #[cfg(test)]
ignored test (with linked issue number in the ignore comment) to prevent
assert_all_snapshots from running until proper snapshots are added.
There was a problem hiding this comment.
This PR adds unit tests for a subset of JSON types. Will track missing tests with separate issues later.
There was a problem hiding this comment.
@hanabi1224, thank you for clarifying! I understand this PR focuses on adding tests for a subset of JSON types, with the plan to track remaining test coverage through separate issues. That's a sensible incremental approach.
When you create the tracking issues for the missing tests, you might want to include DeclareFaultsRecoveredParams (and any similar types with empty snapshots() methods) to ensure they get proper test data in a future PR.
🧠 Learnings used
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.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5886
File: src/daemon/db_util.rs:236-259
Timestamp: 2025-08-11T14:00:47.060Z
Learning: In Forest's snapshot import (`src/daemon/db_util.rs`), when F3 CID is present in snapshot metadata but the actual F3 data is missing, this should cause a hard error as it indicates snapshot corruption. Only errors during the F3 import process itself (after data is successfully retrieved) should be non-fatal and logged.
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.
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
What's the fix? |
@LesnyRumcajs JSON snapshot tests need to be declared explicitly, like this. And you can see coverage of |
|
Ok, so they weren't run because they were in the impl block? |
@hanabi1224 Does this work as well,I think this was added for the tests only. |
@akaladarshi this also works, so |
@LesnyRumcajs They did not run because there were no declarations of the test functions. The trait function is just a helper by itself. |
|
Oh yeah, now I see it. Thanks! |
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
Summary by CodeRabbit
Tests
Refactor
Style
✏️ Tip: You can customize this high-level summary in your review settings.