LCORE-1394: chore: enforce avoiding unittest and fix existing test cases#1251
Merged
tisnik merged 2 commits intolightspeed-core:mainfrom Mar 3, 2026
Merged
LCORE-1394: chore: enforce avoiding unittest and fix existing test cases#1251tisnik merged 2 commits intolightspeed-core:mainfrom
tisnik merged 2 commits intolightspeed-core:mainfrom
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdded a Ruff lint block to pyproject.toml and migrated multiple unit tests from unittest.mock to pytest-mock’s Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/unit/a2a_storage/test_storage_factory.py (1)
19-27: Silence the new pylint warning on_FakeProperty.This helper triggers
R0903in CI; add a targeted disable on the class to keep lint output clean.Proposed minimal lint-only fix
-class _FakeProperty: +class _FakeProperty: # pylint: disable=too-few-public-methods🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/a2a_storage/test_storage_factory.py` around lines 19 - 27, Add a targeted pylint disable for the "too few public methods" warning on the _FakeProperty test helper by adding a class-level comment like "class _FakeProperty: # pylint: disable=R0903" (or a preceding "# pylint: disable=R0903" immediately above the class) so the R0903 warning is silenced without changing behavior.tests/unit/authentication/test_rh_identity.py (1)
436-443: Optional: suppress pylint “too many arguments” for this parametrized test.This is test-only scaffolding; a local disable keeps CI output clean without changing behavior.
Proposed lint-only adjustment
- async def test_entitlement_validation( + async def test_entitlement_validation( # pylint: disable=too-many-arguments,too-many-positional-arguments🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/authentication/test_rh_identity.py` around lines 436 - 443, For the parametrized test function test_entitlement_validation, suppress the pylint warning by adding a lint-only disable for too-many-arguments (e.g., add "# pylint: disable=too-many-arguments" on the def line or immediately above it) and include a short comment stating this is test-only scaffolding so CI output remains clean; do not change the function signature or behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/authentication/test_rh_identity.py`:
- Line 328: Calls to create_request_with_header are missing a space after the
comma (e.g., create_request_with_header(mocker,header_value)), which causes
Black formatting failures; update each call site to add a space after the comma
(create_request_with_header(mocker, header_value)) including the additional
occurrences that use the same helper so all instances (the ones near tests
referencing create_request_with_header) are consistently fixed.
---
Nitpick comments:
In `@tests/unit/a2a_storage/test_storage_factory.py`:
- Around line 19-27: Add a targeted pylint disable for the "too few public
methods" warning on the _FakeProperty test helper by adding a class-level
comment like "class _FakeProperty: # pylint: disable=R0903" (or a preceding "#
pylint: disable=R0903" immediately above the class) so the R0903 warning is
silenced without changing behavior.
In `@tests/unit/authentication/test_rh_identity.py`:
- Around line 436-443: For the parametrized test function
test_entitlement_validation, suppress the pylint warning by adding a lint-only
disable for too-many-arguments (e.g., add "# pylint: disable=too-many-arguments"
on the def line or immediately above it) and include a short comment stating
this is test-only scaffolding so CI output remains clean; do not change the
function signature or behavior.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
pyproject.tomltests/unit/a2a_storage/test_storage_factory.pytests/unit/app/endpoints/test_a2a.pytests/unit/app/test_main_middleware.pytests/unit/authentication/test_rh_identity.pytests/unit/observability/formats/test_rlsapi.pytests/unit/observability/test_splunk.py
tisnik
reviewed
Mar 2, 2026
Contributor
tisnik
left a comment
There was a problem hiding this comment.
Should not you update AGENTS.md too?
Contributor
Author
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/authentication/test_rh_identity.py (1)
436-443:⚠️ Potential issue | 🟡 MinorLine 436 still triggers pylint argument-count warnings.
Pipeline reports
R0913andR0917here after addingmocker. Please either reduce parameters or locally disable for this test to keep lint clean.💡 Minimal fix
- async def test_entitlement_validation( + async def test_entitlement_validation( # pylint: disable=too-many-arguments,too-many-positional-arguments self, mocker: MockerFixture, user_identity_data: dict, required_entitlements: Optional[list[str]], should_raise: bool, expected_error: Optional[str], ) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/authentication/test_rh_identity.py` around lines 436 - 443, The test function test_entitlement_validation currently trips pylint R0913/R0917 due to too many parameters; fix by either reducing parameters (convert some to fixtures or bundle into a single dict) or add a local pylint disable immediately above the test definition to silence the warnings (e.g., add a comment like "# pylint: disable=R0913,R0917" directly above async def test_entitlement_validation(...)). Reference the test name test_entitlement_validation when making the change so you modify the correct function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/unit/authentication/test_rh_identity.py`:
- Around line 436-443: The test function test_entitlement_validation currently
trips pylint R0913/R0917 due to too many parameters; fix by either reducing
parameters (convert some to fixtures or bundle into a single dict) or add a
local pylint disable immediately above the test definition to silence the
warnings (e.g., add a comment like "# pylint: disable=R0913,R0917" directly
above async def test_entitlement_validation(...)). Reference the test name
test_entitlement_validation when making the change so you modify the correct
function.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
pyproject.tomltests/unit/a2a_storage/test_storage_factory.pytests/unit/app/endpoints/test_a2a.pytests/unit/app/test_main_middleware.pytests/unit/authentication/test_rh_identity.pytests/unit/observability/formats/test_rlsapi.pytests/unit/observability/test_splunk.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/unit/a2a_storage/test_storage_factory.py
- tests/unit/app/test_main_middleware.py
- pyproject.toml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
We have agreed to use pytest insted of unittest. This PR is doing below
Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
Tests
Chores