Adding integration tests of /config endpoint. Moving auth and request fictures to conftest.py. #733
Conversation
… fixtures to conftest.py.
WalkthroughAdds shared integration test fixtures (current_config, test_request, test_auth) in Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Integration Test
participant Handler as config_endpoint_handler
participant Config as configuration module
Test->>Handler: invoke(auth, request)
alt configuration loaded
Handler->>Config: read configuration
Config-->>Handler: configuration object
Handler-->>Test: return configuration.configuration
else configuration missing
Handler->>Config: read configuration
Config-->>Handler: returns None / raises
Handler-->>Test: raise LogicError("configuration is not loaded")
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used📓 Path-based instructions (3)**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
tests/{unit,integration}/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
tests/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-09-18T16:46:33.353ZApplied to files:
📚 Learning: 2025-09-18T16:46:33.353ZApplied to files:
⏰ 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)
🔇 Additional comments (2)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/integration/endpoints/test_config_integration.py (1)
61-82: Test logic is correct, but docstring could be more accurate.The test correctly verifies that
LogicErroris raised when configuration is not loaded. However, the docstring (line 66) and comment (line 77) incorrectly attribute the failure to "authorization" when it's actually the endpoint handler checking configuration state.Consider clarifying:
- Line 66: "Test that endpoint fails when configuration is not loaded"
- Line 77: "Verify that LogicError is raised when endpoint handler tries to access config"
- """Test that authorization fails when configuration is not loaded. + """Test that endpoint handler fails when configuration is not loaded. This integration test verifies: - LogicError is raised when configuration is not loaded - Error message indicates configuration is not loaded Args: test_request: FastAPI request test_auth: noop authentication tuple """ - # Verify that LogicError is raised when authorization tries to access config + # Verify that LogicError is raised when endpoint handler tries to access config with pytest.raises(LogicError) as exc_info:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/integration/conftest.py(3 hunks)tests/integration/endpoints/test_config_integration.py(1 hunks)tests/integration/endpoints/test_info_integration.py(0 hunks)
💤 Files with no reviewable changes (1)
- tests/integration/endpoints/test_info_integration.py
🧰 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/integration/endpoints/test_config_integration.pytests/integration/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/integration/endpoints/test_config_integration.pytests/integration/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/integration/endpoints/test_config_integration.pytests/integration/conftest.py
tests/**/conftest.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use conftest.py for shared pytest fixtures
Files:
tests/integration/conftest.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: lightspeed-core/lightspeed-stack#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-18T16:46:33.353Z
Learning: Applies to tests/**/conftest.py : Use conftest.py for shared pytest fixtures
Learnt from: CR
PR: lightspeed-core/lightspeed-stack#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-18T16:46:33.353Z
Learning: Applies to tests/{unit,integration}/**/*.py : Use pytest for all unit and integration tests
Learnt from: CR
PR: lightspeed-core/lightspeed-stack#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-18T16:46:33.353Z
Learning: Applies to tests/{unit,integration}/**/*.py : Do not use unittest in tests; pytest is the standard
📚 Learning: 2025-09-18T16:46:33.353Z
Learnt from: CR
PR: lightspeed-core/lightspeed-stack#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-18T16:46:33.353Z
Learning: Applies to tests/**/conftest.py : Use conftest.py for shared pytest fixtures
Applied to files:
tests/integration/conftest.py
🧬 Code graph analysis (2)
tests/integration/endpoints/test_config_integration.py (2)
src/configuration.py (2)
configuration(73-77)LogicError(35-36)src/app/endpoints/config.py (1)
config_endpoint_handler(63-85)
tests/integration/conftest.py (2)
src/authentication/noop.py (1)
NoopAuthDependency(17-42)src/configuration.py (2)
configuration(73-77)load_configuration(56-62)
⏰ 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). (2)
- GitHub Check: e2e_tests (azure)
- GitHub Check: e2e_tests (ci)
🔇 Additional comments (7)
tests/integration/endpoints/test_config_integration.py (3)
1-9: LGTM: Clean module structure with proper docstring.The module docstring and imports are well-organized. All imports are used appropriately in the integration tests.
11-33: LGTM: Well-structured integration test with comprehensive documentation.The test properly verifies the integration between the endpoint handler and configuration system. The docstring clearly explains what is being tested.
36-58: LGTM: Good coverage for root configuration loading.This test verifies the endpoint works correctly with the root configuration file, complementing the test config verification.
tests/integration/conftest.py (4)
5-6: LGTM: Necessary imports for new fixtures.The imports support the new
test_requestandtest_authfixtures added below.
51-67: LGTM: Fixture correctly loads root configuration.The fixture follows the same pattern as
test_config_fixturebut loads from the project root. The path construction is correct, and the cleanup is properly handled.
120-128: LGTM: Excellent integration test approach using real authentication.This fixture correctly uses the actual
NoopAuthDependencyimplementation rather than mocking, making these true integration tests. The fixture properly handles the async nature of the auth dependency and returns the expected authentication tuple.Based on learnings
108-117: Request scope is sufficient for current usage.The
test_request_fixtureuses a minimal scope that works correctly with the codebase. Integration tests successfully use this fixture with the realNoopAuthDependency, which accessesquery_params(dependent onquery_string✓). Unit tests throughout the codebase use even more minimal scopes (e.g.,{"type": "http"}alone) without issues, indicating Starlette's Request lazily loads fields as needed. The ASGI spec documents theoretical requirements, but Starlette does not enforce all fields at initialization.
tisnik
left a comment
tisnik
left a comment
There was a problem hiding this comment.
LGTM in overall. Just few nitpicks
tests/integration/conftest.py
Outdated
|
|
||
|
|
||
| @pytest.fixture(name="test_auth") | ||
| async def test_auth_fixture(test_request: Request) -> tuple[str, str, bool, str]: |
There was a problem hiding this comment.
nitpick: the return type is AuthTuple
from authentication.interface import AuthTuple
| async def test_config_endpoint_returns_config( | ||
| test_config: AppConfig, | ||
| test_request: Request, | ||
| test_auth: tuple[str, str, bool, str], |
3 participants
… fixtures to conftest.py.
Description
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit