Skip to content

Conversation

@elmattic
Copy link
Contributor

@elmattic elmattic commented Oct 30, 2025

Summary of changes

Changes introduced in this pull request:

  • Add PassWithQuasiIdenticalError policy for eth_getLogs api compare tests

Also tested the fix locally using the problematic params:

"params": [
  {
    "address": [],
    "fromBlock": "0x300a85",
    "toBlock": "0x300ae9",
    "topics": null
  }
]
| RPC Method              | Forest | Lotus | Status                     |
|-------------------------|--------|-------|----------------------------|
| Filecoin.EthGetLogs     | 1/1    | 1/1   | ✅ All Passed              |

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
    • Enhanced API comparison test evaluation to treat quasi-identical error patterns as acceptable outcomes in rejection scenarios, improving test reliability for error handling validation across multiple test cases.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 30, 2025

Walkthrough

Multiple RpcTest builders in API comparison tests are now configured with .policy_on_rejected(PolicyOnRejected::PassWithQuasiIdenticalError) to treat quasi-identical error patterns as acceptable during rejection evaluation, affecting test pass/fail outcomes across EthGetLogs and related test blocks.

Changes

Cohort / File(s) Summary
Test Rejection Policy Configuration
src/tool/subcommands/api_cmd/api_compare_tests.rs
Added .policy_on_rejected(PolicyOnRejected::PassWithQuasiIdenticalError) chained calls to multiple RpcTest builders previously configured with .sort_policy(SortPolicy::All). Changes applied to EthGetLogs/EthGetFilterLogs and related test blocks to handle error rejection scenarios with quasi-identical error tolerance.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10–15 minutes

  • Review the policy semantics of PolicyOnRejected::PassWithQuasiIdenticalError and verify it is the intended behavior for all affected test blocks
  • Confirm consistency across all test builder additions and check for any test blocks that should have received this policy but were missed
  • Verify that the quasi-identical error matching logic aligns with expected test outcomes

Suggested reviewers

  • sudo-shashank
  • akaladarshi
  • LesnyRumcajs

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.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 PR title "Use PassWithQuasiIdenticalError policy to match lotus error message" directly and accurately describes the primary change in the pull request. The changeset adds the PassWithQuasiIdenticalError policy to multiple RpcTest builders across API compare tests to address error message mismatches between Forest and Lotus. The title is specific, concise, and clearly communicates both what is being changed (the specific policy being used) and the intent (matching lotus error messages), making it immediately understandable to someone reviewing the git history.
✨ 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 elmattic/collect-events-wo-limit

📜 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 4712a9f and 3353d3a.

📒 Files selected for processing (1)
  • src/tool/subcommands/api_cmd/api_compare_tests.rs (4 hunks)
⏰ 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). (4)
  • GitHub Check: tests-release
  • GitHub Check: tests
  • GitHub Check: Build forest binaries on Linux AMD64
  • GitHub Check: Diff snapshot export checks
🔇 Additional comments (1)
src/tool/subcommands/api_cmd/api_compare_tests.rs (1)

1749-1750: LGTM! Consistent application of the quasi-identical error policy.

The addition of .policy_on_rejected(PolicyOnRejected::PassWithQuasiIdenticalError) to all four EthGetLogs test cases is appropriate for handling error message formatting differences between Forest and Lotus. The policy allows tests to pass when one error message is a substring of the other (as implemented in lines 2334-2336), which aligns with the comment that "We don't always bubble up errors and format the error chain like Lotus."

The changes are applied consistently across all relevant test cases and follow the established pattern of chaining after .sort_policy(SortPolicy::All).

Also applies to: 1760-1761, 1770-1771, 1781-1782


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

@elmattic elmattic added the RPC requires calibnet RPC checks to run on CI label Oct 30, 2025
@elmattic elmattic changed the title Remove limit check Use PassWithQuasiIdenticalError policy to match lotus error message Oct 30, 2025
@elmattic elmattic marked this pull request as ready for review October 30, 2025 18:37
@elmattic elmattic requested a review from a team as a code owner October 30, 2025 18:37
@elmattic elmattic requested review from hanabi1224 and sudo-shashank and removed request for a team October 30, 2025 18:37
@elmattic elmattic added this pull request to the merge queue Nov 3, 2025
Merged via the queue into main with commit d143ac5 Nov 3, 2025
45 checks passed
@elmattic elmattic deleted the elmattic/collect-events-wo-limit branch November 3, 2025 09:00
@coderabbitai coderabbitai bot mentioned this pull request Dec 8, 2025
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

RPC requires calibnet RPC checks to run on CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants