tests: add zmap tests#11
Conversation
WalkthroughThis update introduces three new test modules targeting the API, configuration, and schema layers of the project. It also modifies an import statement in the API module to use absolute imports. The new tests leverage mocking and validation to verify correct endpoint responses, configuration handling, and schema initialization. Additionally, the CI workflow is enhanced to run tests, and a Makefile target is added to facilitate running pytest. Changes
Sequence Diagram(s)sequenceDiagram
participant TestClient
participant FastAPI_App
participant MockZMap
TestClient->>FastAPI_App: GET "/"
FastAPI_App->>MockZMap: get_version()
MockZMap-->>FastAPI_App: "1.2.3"
FastAPI_App-->>TestClient: JSON response with version info
TestClient->>FastAPI_App: GET "/openapi.json"
FastAPI_App-->>TestClient: OpenAPI schema
TestClient->>FastAPI_App: GET "/docs"
FastAPI_App-->>TestClient: Swagger UI HTML
TestClient->>FastAPI_App: GET "/redoc"
FastAPI_App-->>TestClient: Redoc HTML
Possibly related PRs
Suggested reviewers
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/test_config.py (1)
6-17: Consider expanding test coverage.The current test only covers basic attribute access and dictionary conversion. Consider adding tests for:
- Invalid bandwidth values
- Edge cases for rate and max_targets
- Validation of other configuration parameters
tests/test_schemas.py (1)
1-27: LGTM: Well-structured schema tests.The tests provide good coverage of the
ScanRequestschema with both minimal and comprehensive configurations. The assertions properly validate default values and explicit field assignments.Consider adding edge case tests for better coverage:
- Invalid subnet formats
- Invalid port ranges
- Invalid file paths
- Field validation boundaries
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/test_api.py(1 hunks)tests/test_config.py(1 hunks)tests/test_schemas.py(1 hunks)zmapsdk/api.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI
zmapsdk/api.py
[error] 1-20: isort: Imports are incorrectly sorted and/or formatted. Please run 'isort --write' to fix import order.
tests/test_config.py
[error] 1-10: isort: Imports are incorrectly sorted and/or formatted. Please run 'isort --write' to fix import order.
🔇 Additional comments (4)
zmapsdk/api.py (1)
18-18: LGTM: Import change improves consistency.The change from relative to absolute import aligns well with the package structure and maintains consistency with other imports in the file.
However, please address the pipeline failure regarding import sorting:
#!/bin/bash # Description: Check import order issues in the file # Expected: Show current import order and suggest fixes echo "Current import structure:" head -20 zmapsdk/api.py echo -e "\n\nRunning isort to check issues:" isort --check-only --diff zmapsdk/api.py || echo "Import sorting needed"tests/test_config.py (1)
1-4: Fix import sorting issues.The pipeline indicates import formatting problems. Please run
isort --writeto fix the import order.tests/test_api.py (2)
42-53: LGTM: Comprehensive endpoint testing.The test properly mocks the ZMap instance, validates the response structure, and ensures the mock method is called correctly.
56-68: LGTM: Good documentation endpoint coverage.The tests verify that all documentation endpoints are accessible and return expected content, which is essential for API usability.
| with pytest.raises(ZMapConfigError) as exc_info: | ||
| conf = ZMapScanConfig(source_port=-9999) | ||
| assert f"Invalid source port range: {conf.source_port}. Must be between 0 and 65535." in str(exc_info.value) |
There was a problem hiding this comment.
Fix test logic error: assertion should be outside the context manager.
The assertion on line 21 is incorrectly placed inside the pytest.raises context. It should be moved outside to properly validate the exception message.
Apply this fix:
with pytest.raises(ZMapConfigError) as exc_info:
conf = ZMapScanConfig(source_port=-9999)
- assert f"Invalid source port range: {conf.source_port}. Must be between 0 and 65535." in str(exc_info.value)
+ assert f"Invalid source port range: -9999. Must be between 0 and 65535." in str(exc_info.value)Note: Also fixed the reference to use the actual invalid value (-9999) directly since conf.source_port may not be accessible if the exception is raised during initialization.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with pytest.raises(ZMapConfigError) as exc_info: | |
| conf = ZMapScanConfig(source_port=-9999) | |
| assert f"Invalid source port range: {conf.source_port}. Must be between 0 and 65535." in str(exc_info.value) | |
| with pytest.raises(ZMapConfigError) as exc_info: | |
| conf = ZMapScanConfig(source_port=-9999) | |
| assert f"Invalid source port range: -9999. Must be between 0 and 65535." in str(exc_info.value) |
🤖 Prompt for AI Agents
In tests/test_config.py around lines 19 to 21, the assertion checking the
exception message is incorrectly inside the pytest.raises context manager. Move
the assertion outside the with block to properly verify the exception message
after the exception is caught. Also, replace conf.source_port with the literal
invalid value -9999 in the assertion message to avoid referencing conf when
initialization fails.
| def test_root_endpoint_success( | ||
| client: TestClient, | ||
| mock_zmap: Generator[MagicMock, Any, None] | ||
| ) -> None: |
There was a problem hiding this comment.
Fix incorrect type annotation.
The mock_zmap parameter should be typed as MagicMock, not Generator[MagicMock, Any, None].
def test_root_endpoint_success(
client: TestClient,
- mock_zmap: Generator[MagicMock, Any, None]
+ mock_zmap: MagicMock
) -> None:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_root_endpoint_success( | |
| client: TestClient, | |
| mock_zmap: Generator[MagicMock, Any, None] | |
| ) -> None: | |
| def test_root_endpoint_success( | |
| client: TestClient, | |
| mock_zmap: MagicMock | |
| ) -> None: |
🤖 Prompt for AI Agents
In tests/test_api.py around lines 38 to 41, the type annotation for the
mock_zmap parameter is incorrect; it is currently annotated as
Generator[MagicMock, Any, None]. Change the type annotation of mock_zmap to
MagicMock to correctly reflect its type.
| @pytest.fixture(autouse=True) | ||
| def mock_zmap() -> Generator[MagicMock, Any, None]: | ||
| """Automatically mock the ZMap instance before each test.""" | ||
| with patch('zmapsdk.api.ZMap') as mock_zmap: | ||
| mock_instance = MagicMock() | ||
| mock_zmap.return_value = mock_instance | ||
| app.state.zmap = mock_instance | ||
| yield mock_instance | ||
| if hasattr(app.state, 'zmap'): | ||
| del app.state.zmap | ||
|
|
||
| @pytest.fixture(autouse=True) | ||
| def setup_teardown() -> Generator[None, Any, None]: | ||
| """Fixture to handle setup and teardown for each test.""" | ||
| mock_zmap = MagicMock() | ||
| app.state.zmap = mock_zmap | ||
| yield | ||
| # Teardown - clean up | ||
| if hasattr(app.state, 'zmap'): | ||
| del app.state.zmap | ||
|
|
There was a problem hiding this comment.
Fix duplicate and conflicting fixtures.
You have two autouse fixtures (mock_zmap and setup_teardown) that both manipulate app.state.zmap, which can cause conflicts and unpredictable behavior.
Consolidate into a single fixture:
@pytest.fixture(autouse=True)
-def mock_zmap() -> Generator[MagicMock, Any, None]:
- """Automatically mock the ZMap instance before each test."""
- with patch('zmapsdk.api.ZMap') as mock_zmap:
- mock_instance = MagicMock()
- mock_zmap.return_value = mock_instance
- app.state.zmap = mock_instance
- yield mock_instance
- if hasattr(app.state, 'zmap'):
- del app.state.zmap
-
-@pytest.fixture(autouse=True)
-def setup_teardown() -> Generator[None, Any, None]:
- """Fixture to handle setup and teardown for each test."""
+def mock_zmap_instance() -> Generator[MagicMock, Any, None]:
+ """Automatically mock the ZMap instance before each test."""
mock_zmap = MagicMock()
app.state.zmap = mock_zmap
- yield
- # Teardown - clean up
+ yield mock_zmap
if hasattr(app.state, 'zmap'):
del app.state.zmapCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/test_api.py around lines 16 to 36, there are two autouse fixtures both
setting and deleting app.state.zmap, causing conflicts. Consolidate these into a
single autouse fixture that mocks ZMap, assigns the mock to app.state.zmap
before yielding, and deletes app.state.zmap after the test completes to ensure
consistent setup and teardown without duplication.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
39-41: Add a trailing newline to satisfy YAML lint
yamllintis already flagging the missing final EOL. While this doesn’t break GitHub Actions, it will keep the workflow file permanently “dirty” in lint checks.- make pytest + make pytest +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci.yml(1 hunks)Makefile(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Makefile
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[error] 41-41: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
35-38: Lint step is correctly wired into CIAdding a dedicated
make lintstep keeps style violations from masking test results. Looks good.
4 participants
add some unit tests
Summary by CodeRabbit