Skip to content

Conversation

@AlexeyKrasnoperov
Copy link
Member

@AlexeyKrasnoperov AlexeyKrasnoperov commented Dec 16, 2025

Summary of changes

The conformance tests policy name "validate" is confusing. The name "custom" fits it more, and is self-explanatory.

Changes introduced in this pull request:

  • Rename conformance tests policy from "validate" to "custom".

Other information and links

Good to have self-explanatory policies names that will match the names used in the compatibility report.

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

  • Refactor
    • Internal test utility methods were refactored for improved clarity and consistency.

Note: This release contains no user-facing changes.

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

@AlexeyKrasnoperov AlexeyKrasnoperov requested a review from a team as a code owner December 16, 2025 12:04
@AlexeyKrasnoperov AlexeyKrasnoperov requested review from LesnyRumcajs and hanabi1224 and removed request for a team December 16, 2025 12:04
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 16, 2025

Walkthrough

This change renames test helper methods in RpcTest from validate/validate_raw to custom/custom_raw to better reflect their purpose. All internal wiring and test suites are updated to use the new method names consistently.

Changes

Cohort / File(s) Summary
RpcTest method renaming
src/tool/subcommands/api_cmd/api_compare_tests.rs
Renamed public test helper methods: validatecustom, validate_rawcustom_raw. Updated identity() to delegate to Self::custom(). Updated internal method calls and documentation. Updated all test suites (common_tests, eth_tests, gas_tests_with_tipset, validate_message_lookup, validate_tagged_tipset_v2) to use renamed methods.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Verify all validate and validate_raw usages have been replaced with custom and custom_raw respectively
  • Confirm internal delegation chains (e.g., identity()Self::custom()) are correct
  • Check that test suite updates are consistent and no call sites were missed

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 63.64% 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 clearly and concisely summarizes the main change: renaming a public test helper method from 'validate' to 'custom' in conformance tests.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch ak/rename-conformance-tests-policy-validate-to-custom

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e2fc9d2 and 881cd50.

📒 Files selected for processing (1)
  • src/tool/subcommands/api_cmd/api_compare_tests.rs (7 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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/tool/subcommands/api_cmd/api_compare_tests.rs
📚 Learning: 2025-08-28T12:52:46.927Z
Learnt from: LesnyRumcajs
Repo: ChainSafe/forest PR: 6011
File: src/cli/main.rs:18-25
Timestamp: 2025-08-28T12:52:46.927Z
Learning: In Forest CLI (src/cli/main.rs), the early RPC network check before Cli::parse_from() does not block help/version commands because clap processes these internally before reaching the RPC call. LesnyRumcajs confirmed this implementation works correctly and that RPC call failures are acceptable in this context.

Applied to files:

  • src/tool/subcommands/api_cmd/api_compare_tests.rs
📚 Learning: 2025-08-04T13:36:22.993Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5835
File: src/chain/tests.rs:58-76
Timestamp: 2025-08-04T13:36:22.993Z
Learning: In the Forest codebase, `Vec<u8>` can be used as an `AsyncWrite` implementation in test contexts. The user confirmed that tests using `&mut Vec<u8>` with `export` and `export_v2` functions compile and pass both locally and on CI.

Applied to files:

  • src/tool/subcommands/api_cmd/api_compare_tests.rs
🧬 Code graph analysis (1)
src/tool/subcommands/api_cmd/api_compare_tests.rs (6)
src/rpc/methods/chain.rs (1)
  • from_lotus_json (1494-1499)
src/lotus_json/signed_message.rs (1)
  • from_lotus_json (81-88)
src/rpc/methods/eth.rs (1)
  • from_lotus_json (651-674)
src/blocks/tipset.rs (1)
  • from_lotus_json (666-668)
src/lotus_json/mod.rs (5)
  • from_lotus_json (149-149)
  • from_lotus_json (573-575)
  • from_lotus_json (587-592)
  • from_lotus_json (608-614)
  • from_lotus_json (633-640)
src/rpc/registry/methods_reg.rs (1)
  • from_lotus_json (184-186)
⏰ 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). (8)
  • GitHub Check: Build MacOS
  • GitHub Check: Coverage
  • GitHub Check: All lint checks
  • GitHub Check: Build Ubuntu
  • GitHub Check: cargo-publish-dry-run
  • GitHub Check: Build forest binaries on Linux AMD64
  • GitHub Check: tests
  • GitHub Check: tests-release
🔇 Additional comments (5)
src/tool/subcommands/api_cmd/api_compare_tests.rs (5)

322-365: LGTM! Clean rename from validate to custom.

The method rename improves clarity—"custom" better conveys that these methods accept custom validation logic. The implementation remains unchanged, and the documentation is appropriate for the new name.


368-370: LGTM! Correct delegation to renamed method.

The identity method now correctly delegates to Self::custom instead of the old Self::validate method name.


457-459: LGTM! Test functions updated to use renamed method.

All test functions that previously called RpcTest::validate now correctly use RpcTest::custom. The validation logic in the closures remains unchanged.

Also applies to: 1384-1387, 2182-2214


2589-2596: LGTM! Helper functions updated consistently.

The helper functions validate_message_lookup and validate_tagged_tipset_v2 now correctly use RpcTest::custom. Note that the function names themselves still contain "validate", which is fine—they describe what they validate, not the policy name.

Also applies to: 2598-2610


322-322: Clean refactoring with consistent updates across the codebase.

This PR successfully renames the test policy from "validate" to "custom" throughout the file. All method definitions and call sites are updated consistently with no logic changes. The new name better reflects that these methods accept custom validation logic, aligning with the PR's goal of improving clarity.

Also applies to: 331-331, 369-369, 457-457, 1384-1384, 2182-2182, 2590-2590, 2599-2599


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.

❤️ Share

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

@codecov
Copy link

codecov bot commented Dec 16, 2025

Codecov Report

❌ Patch coverage is 0% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 40.40%. Comparing base (e2fc9d2) to head (881cd50).

Files with missing lines Patch % Lines
src/tool/subcommands/api_cmd/api_compare_tests.rs 0.00% 9 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
src/tool/subcommands/api_cmd/api_compare_tests.rs 0.00% <0.00%> (ø)

... and 3 files 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 e2fc9d2...881cd50. Read the comment docs.

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

Copy link
Member

@LesnyRumcajs LesnyRumcajs left a comment

Choose a reason for hiding this comment

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

As discussed, my recommendation is to have a change across the board, i.e., basic -> validate_basic, validate -> validate_custom and identity to validate_identity which are a bit more expressive IMO.

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.

3 participants