Skip to content

Conversation

@akaladarshi
Copy link
Collaborator

@akaladarshi akaladarshi commented Dec 1, 2025

Summary of changes

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

  • Chores
    • Updated test filter configurations to exclude ChainGetEvents from API comparison validations in both standard and offline test scenarios.
    • Added explanatory comments and a TODO note referencing a related issue to clarify why ChainGetEvents is being ignored and to aid future reviews.

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

@akaladarshi akaladarshi requested a review from a team as a code owner December 1, 2025 15:21
@akaladarshi akaladarshi requested review from hanabi1224 and sudo-shashank and removed request for a team December 1, 2025 15:21
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 1, 2025

Walkthrough

Adds Filecoin.ChainGetEvents exclusion entries and explanatory comments to both the online and offline API comparison filter lists; no changes to exported/public declarations or control flow.

Changes

Cohort / File(s) Summary
API comparison filter exclusions
scripts/tests/api_compare/filter-list, scripts/tests/api_compare/filter-list-offline
Inserted Filecoin.ChainGetEvents into both filter lists and added comments noting the exclusion (uncertainty around duplicate event handling, with a TODO referencing forest issue). No other logic or public API changes.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

  • Files to spot-check: scripts/tests/api_compare/filter-list, scripts/tests/api_compare/filter-list-offline.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding ChainGetEvents to the filter list, which aligns with modifications to both filter-list and filter-list-offline files.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch akaladarshi/update-filter-list

📜 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 96a294b and 5c5a437.

📒 Files selected for processing (2)
  • scripts/tests/api_compare/filter-list (1 hunks)
  • scripts/tests/api_compare/filter-list-offline (1 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). (7)
  • GitHub Check: Build MacOS
  • GitHub Check: cargo-publish-dry-run
  • GitHub Check: Build Ubuntu
  • GitHub Check: tests
  • GitHub Check: tests-release
  • GitHub Check: Build forest binaries on Linux AMD64
  • GitHub Check: All lint checks
🔇 Additional comments (2)
scripts/tests/api_compare/filter-list (1)

4-6: Good, temporary exclusion is clearly documented and linked to the tracking issue

The rationale for ignoring ChainGetEvents and the inline TODO pointing to issue 6271 make this temporary test filter explicit and easy to revisit. The !Filecoin.ChainGetEvents entry matches the existing filter-list style.

scripts/tests/api_compare/filter-list-offline (1)

30-32: Offline filter mirrors the online behavior and is well annotated

Adding the same comment, TODO for issue 6271, and !Filecoin.ChainGetEvents here keeps offline API comparisons consistent with the online filter-list and makes the temporary nature of this exclusion clear.


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

hanabi1224
hanabi1224 previously approved these changes Dec 1, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 82f6a34 and 1a93f52.

📒 Files selected for processing (2)
  • scripts/tests/api_compare/filter-list (1 hunks)
  • scripts/tests/api_compare/filter-list-offline (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-11T13:50:04.489Z
Learnt from: elmattic
Repo: ChainSafe/forest PR: 5836
File: src/tool/subcommands/api_cmd/api_run_tests.rs:211-268
Timestamp: 2025-08-11T13:50:04.489Z
Learning: In Filecoin WebSocket channels (used for methods like `Filecoin.ChainNotify`), the protocol only uses Text messages for communication. Non-Text WebSocket message types (Binary, Ping, Pong, etc.) can be safely ignored when processing Filecoin channel notifications.

Applied to files:

  • scripts/tests/api_compare/filter-list-offline
⏰ 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 MacOS
  • GitHub Check: cargo-publish-dry-run
  • GitHub Check: Build Ubuntu
  • GitHub Check: All lint checks
  • GitHub Check: Build forest binaries on Linux AMD64
  • GitHub Check: tests-release
  • GitHub Check: tests
🔇 Additional comments (2)
scripts/tests/api_compare/filter-list (1)

5-5: LGTM!

The exclusion entry syntax is correct and aligns with the PR objective to temporarily filter out ChainGetEvents until issue #6271 is resolved.

scripts/tests/api_compare/filter-list-offline (1)

31-31: LGTM!

The exclusion entry syntax is correct and maintains consistency with the online filter-list, appropriately filtering out ChainGetEvents in offline API comparisons.

@hanabi1224 hanabi1224 enabled auto-merge December 1, 2025 15:27
@hanabi1224 hanabi1224 disabled auto-merge December 1, 2025 15:27
hanabi1224
hanabi1224 previously approved these changes Dec 1, 2025
# This list contains potentially broken methods (or tests) that are ignored.
# They should be considered bugged, and not used until the root cause is resolved.

# Ignoring ChainGetEvents for now, as we are not sure if we should get duplicated events or not.
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a tracking issue to the comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@akaladarshi akaladarshi added this pull request to the merge queue Dec 1, 2025
Merged via the queue into main with commit aa8ba1f Dec 1, 2025
41 checks passed
@akaladarshi akaladarshi deleted the akaladarshi/update-filter-list branch December 1, 2025 16:42
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.

4 participants