Skip to content

Fix test for eth_newFilter creation #6104

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
sudo-shashank merged 1 commit into from
Sep 23, 2025
Merged

Fix test for eth_newFilter creation #6104

sudo-shashank merged 1 commit into from
Sep 23, 2025

Conversation

@sudo-shashank
Copy link
Contributor

@sudo-shashank sudo-shashank commented Sep 22, 2025 •
edited by coderabbitai bot
Loading

Summary of changes

Changes introduced in this pull request:

  • We already have a filter creation limit, fixed test and re-enable create_eth_new_filter_limit_test

Reference issue to close (if applicable)

Closes #5915

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
    • Improved robustness of failure assertions by making error message checks case-insensitive.
    • Updated a filter-limit test to run with an expected failure, removing the ignore marker.
    • No public-facing behavior or APIs changed.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 22, 2025 •
edited
Loading

Walkthrough

Adjusts stateful test logic to compare expected failure messages case-insensitively and updates the eth_newFilter limit test by removing its ignore attribute while retaining the failure expectation. No public API signatures changed.

Changes

Cohort / File(s) Summary
Stateful test adjustments
src/tool/subcommands/api_cmd/stateful_tests.rs		 - run_tests: compare error strings and expected messages case-insensitively
- create_tests: removed ignore on “eth_newFilter over limit” test; kept should_fail_with expectation
- No public API changes

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

  • LesnyRumcajs
  • akaladarshi

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Linked Issues Check ⚠️ Warning Issue [#5915] requests enforcing a cap on eth_newFilter creations so the related test can run unignored; while the PR objectives claim the limit was introduced and the test fixed, the provided raw_summary shows only test-file changes (case-insensitive error check and re-enabling the test) and contains no evidence of implementation changes that actually enforce a filter-creation cap, so the core coding requirement is not verifiably met by the supplied summary. Provide the implementation changes that enforce the eth_newFilter creation cap (or update the summary to include the files where the limit is implemented), ensure the tests exercise that implementation, and rerun the stateful tests so compliance with issue [#5915] can be verified.
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 (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title "Fix test for eth_newFilter creation" is concise, single-sentence, and directly reflects the primary change shown in the provided diff (adjustments to the eth_newFilter creation test and re-enabling the filter-limit test), so it clearly communicates the main intent to reviewers.
Out of Scope Changes Check ✅ Passed The raw_summary shows edits confined to src/tool/subcommands/api_cmd/stateful_tests.rs that adjust test behavior for the eth_newFilter limit; these modifications are directly related to the linked issue's test expectations and there is no indication of unrelated or out-of-scope code changes in the provided summary.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch shashank/fix-filter-limit

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

@sudo-shashank sudo-shashank marked this pull request as ready for review September 22, 2025 13:54
@sudo-shashank sudo-shashank requested a review from a team as a code owner September 22, 2025 13:54
@sudo-shashank sudo-shashank requested review from LesnyRumcajs and akaladarshi and removed request for a team September 22, 2025 13:54
coderabbitai[bot]
coderabbitai bot reviewed Sep 22, 2025
View reviewed changes
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: 0

🧹 Nitpick comments (1)
src/tool/subcommands/api_cmd/stateful_tests.rs (1)

148-151: Reduce allocations and match across error chain for robustness.

Precompute lowercase once and check all error causes; avoids multiple allocations and improves resilience if the message is wrapped.

-                    let err_str = format!("{e:#}");
-                    if err_str
-                        .to_lowercase()
-                        .contains(&expected_msg.to_lowercase())
+                    let expected = expected_msg.to_lowercase();
+                    let err_lc = format!("{e:#}").to_lowercase();
+                    let contains_expected = err_lc.contains(&expected)
+                        || e.chain().any(|cause| cause.to_string().to_lowercase().contains(&expected));
+                    if contains_expected
                     {
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0f2becb and 1680640.

📒 Files selected for processing (1)
  • src/tool/subcommands/api_cmd/stateful_tests.rs (2 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). (3)
  • GitHub Check: tests-release
  • GitHub Check: tests
  • GitHub Check: Build forest binaries on Linux AMD64
🔇 Additional comments (1)
src/tool/subcommands/api_cmd/stateful_tests.rs (1)

630-630: Verify exact server error text to avoid brittle matching. Test expects "maximum number of filters registered" (src/tool/subcommands/api_cmd/stateful_tests.rs:630). Repository search did not locate that string in server code — either shorten to a stable fragment like "maximum number of filters" or run the server and update the test to match the actual emitted message.

@sudo-shashank sudo-shashank added this pull request to the merge queue Sep 23, 2025
Merged via the queue into main with commit 1b9189d Sep 23, 2025
68 of 69 checks passed
@sudo-shashank sudo-shashank deleted the shashank/fix-filter-limit branch September 23, 2025 12:07
@coderabbitai coderabbitai bot mentioned this pull request Sep 24, 2025
Merged
4 tasks
@coderabbitai coderabbitai bot mentioned this pull request Dec 18, 2025
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

@LesnyRumcajs LesnyRumcajs LesnyRumcajs approved these changes

@akaladarshi akaladarshi akaladarshi approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

eth_newFilter creation is not limited

4 participants

@sudo-shashank @LesnyRumcajs @akaladarshi