Skip to content

Conversation

@tisnik
Copy link
Contributor

@tisnik tisnik commented Oct 28, 2025

Description

LCORE-740: Conftest type hint

Type of change

  • Refactor
  • New feature
  • Bug fix
  • CVE fix
  • Optimization
  • Documentation Update
  • Configuration Update
  • Bump-up service version
  • Bump-up dependent library
  • Bump-up library or tool used for development (does not change the final image)
  • CI configuration change
  • Konflux configuration change
  • Unit tests improvement
  • Integration tests improvement
  • End to end tests improvement

Related Tickets & Documents

  • Related Issue #LCORE-740

Summary by CodeRabbit

  • Tests
    • Added explicit type annotations to test fixtures to improve type safety and clarity.
    • Introduced a named fixture type alias describing the tuple of mock objects yielded by the fixture, improving tooling support and maintainability.

Note: Changes are limited to test code and do not affect end-user functionality.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 28, 2025

Walkthrough

Added precise type annotations and supporting imports to the pytest fixture prepare_agent_mocks_fixture in tests/unit/conftest.py: the mocker parameter is typed as MockerFixture and the fixture return is annotated using a public alias AgentFixtures = Generator[tuple[AsyncMockType, AsyncMockType], None, None].

Changes

Cohort / File(s) Summary
Type annotations for pytest fixture
tests/unit/conftest.py
Added imports and type hints: mocker parameter annotated as MockerFixture; introduced public type alias AgentFixtures = Generator[tuple[AsyncMockType, AsyncMockType], None, None]; updated fixture signature to def prepare_agent_mocks_fixture(mocker: MockerFixture) -> AgentFixtures:. Behavior unchanged.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

  • Purely typing/import additions; verify correct import names and that pytest fixture discovery remains unaffected.

Poem

🐇 I hop through lines and add a tag,
Two async mocks snug in a tidy bag.
The fixture whispers types so clear,
No runtime fuss, the tests stay near.
— a rabbit, nibbling on type-safe cheer.

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 PR title "LCORE-740: Conftest type hint" is fully related to the main change in the changeset. The summary shows that the pull request adds type hints to the prepare_agent_mocks_fixture function in tests/unit/conftest.py, introducing type annotations including a new public type alias AgentFixtures. The title is specific and concise, clearly conveying that type hints were added to the conftest file. A teammate scanning the commit history would immediately understand the primary change without ambiguity.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 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 a579c22 and 817ab8c.

📒 Files selected for processing (1)
  • tests/unit/conftest.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/unit/conftest.py
⏰ 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: build-pr
  • GitHub Check: e2e_tests (azure)
  • GitHub Check: e2e_tests (ci)

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.

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: 1

🧹 Nitpick comments (1)
tests/unit/conftest.py (1)

12-19: Use "Yields:" instead of "Returns:" in the docstring.

Generator functions should use "Yields:" rather than "Returns:" to accurately reflect that the function yields values.

Apply this diff to correct the docstring:

-    Returns:
+    Yields:
         tuple: (mock_client, mock_agent)
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0b873a4 and 5f0d09b.

📒 Files selected for processing (1)
  • tests/unit/conftest.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed

Files:

  • tests/unit/conftest.py
tests/{unit,integration}/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests
Do not use unittest in tests; pytest is the standard

Files:

  • tests/unit/conftest.py
tests/**/conftest.py

📄 CodeRabbit inference engine (CLAUDE.md)

Use conftest.py for shared pytest fixtures

Files:

  • tests/unit/conftest.py
tests/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

tests/**/*.py: Use pytest-mock to create AsyncMock objects for async interactions in tests
Use the shared auth mock constant: MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") in tests

Files:

  • tests/unit/conftest.py
⏰ 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: build-pr
  • GitHub Check: e2e_tests (ci)
  • GitHub Check: e2e_tests (azure)

@tisnik tisnik force-pushed the conftest-type-hint branch 2 times, most recently from a85bcfe to a579c22 Compare October 28, 2025 09:46
@tisnik
Copy link
Contributor Author

tisnik commented Oct 28, 2025

@coderabbitai review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 28, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

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)
tests/unit/conftest.py (1)

11-21: Type hints are complete and accurate.

The function signature now has complete type annotations that accurately reflect the fixture's behavior. The Generator[tuple[AsyncMockType, AsyncMockType], None, None] return type correctly specifies what is yielded, that no values are sent, and there's no return value.

The docstring could be slightly more precise by specifying the tuple type:

-    Yields:
-        tuple: (mock_client, mock_agent)
+    Yields:
+        tuple[AsyncMockType, AsyncMockType]: (mock_client, mock_agent)

However, this is a minor nitpick—the current docstring is acceptable.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a85bcfe and a579c22.

📒 Files selected for processing (1)
  • tests/unit/conftest.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed

Files:

  • tests/unit/conftest.py
tests/{unit,integration}/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests
Do not use unittest in tests; pytest is the standard

Files:

  • tests/unit/conftest.py
tests/**/conftest.py

📄 CodeRabbit inference engine (CLAUDE.md)

Use conftest.py for shared pytest fixtures

Files:

  • tests/unit/conftest.py
tests/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

tests/**/*.py: Use pytest-mock to create AsyncMock objects for async interactions in tests
Use the shared auth mock constant: MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") in tests

Files:

  • tests/unit/conftest.py
⏰ 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: build-pr
  • GitHub Check: e2e_tests (ci)
  • GitHub Check: e2e_tests (azure)
🔇 Additional comments (1)
tests/unit/conftest.py (1)

5-7: Excellent choice of type annotations!

The imports are correct and well-chosen. Using AsyncMockType instead of the generic Any (as suggested in the previous review) provides better type safety and IDE support. This is a meaningful improvement that makes the fixture's contract clearer.

@tisnik tisnik force-pushed the conftest-type-hint branch from a579c22 to 817ab8c Compare October 28, 2025 12:14
@tisnik tisnik merged commit 037c3dd into lightspeed-core:main Oct 28, 2025
18 of 20 checks passed
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.

1 participant