-
Notifications
You must be signed in to change notification settings - Fork 1
Improve test coverage from 29.7% to 51.3% - Add comprehensive tests for core modules #105
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.
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
Conversation
Co-authored-by: talltechy <43618761+talltechy@users.noreply.github.com>
… 46.0% Co-authored-by: talltechy <43618761+talltechy@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR focuses on improving test coverage for the InsightVM-Python codebase, targeting a minimum of 70% coverage as specified in CONTRIBUTING.md. The work includes comprehensive unit tests for core utility modules to increase coverage from 29.7% to 46%.
Key changes:
- Added comprehensive test coverage for the UI module (72.9% coverage)
- Added comprehensive test coverage for the Config module (86% coverage)
- Updated .gitignore to exclude coverage.json files
Reviewed Changes
Copilot reviewed 2 out of 13 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/test_ui.py | Comprehensive unit tests for UI module covering Color enum, UI class methods, SimpleProgressBar, and Rich library integration |
| tests/test_config.py | Comprehensive unit tests for Config class covering initialization, file operations, preferences, tool configurations, and state management |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bandit found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
| import json | ||
| import pytest | ||
| from pathlib import Path | ||
| from unittest.mock import Mock, patch, mock_open |
Check warning
Code scanning / Prospector (reported by Codacy)
'unittest.mock.Mock' imported but unused (F401) Warning test
| """ | ||
|
|
||
| import pytest | ||
| from unittest.mock import Mock, patch, MagicMock |
Check warning
Code scanning / Prospector (reported by Codacy)
'unittest.mock.MagicMock' imported but unused (F401) Warning test
|
|
||
| import pytest | ||
| from unittest.mock import Mock, patch, MagicMock | ||
| from io import StringIO |
Check warning
Code scanning / Prospector (reported by Codacy)
'io.StringIO' imported but unused (F401) Warning test
| import pytest | ||
| from unittest.mock import Mock, patch, MagicMock | ||
| from io import StringIO | ||
| import sys |
Check warning
Code scanning / Prospector (reported by Codacy)
'sys' imported but unused (F401) Warning test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bandit (reported by Codacy) found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pylintpython3 (reported by Codacy) found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
…> 50.6% Co-authored-by: talltechy <43618761+talltechy@users.noreply.github.com>
Co-authored-by: talltechy <43618761+talltechy@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 5 out of 19 changed files in this pull request and generated 3 comments.
| """Mock authentication for testing.""" | ||
| auth = Mock() | ||
| auth.base_url = "https://test.insightvm.example.com:3780" | ||
| auth.auth = Mock() | ||
| return auth |
Copilot
AI
Oct 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mock_auth fixture is duplicated across all test files. This should be moved to conftest.py to avoid code duplication and ensure consistency across tests.
| def mock_auth(self): | ||
| """Mock authentication for testing.""" | ||
| auth = Mock() | ||
| auth.base_url = "https://test.insightvm.example.com:3780" | ||
| auth.auth = Mock() | ||
| return auth |
Copilot
AI
Oct 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mock_auth fixture is duplicated across all test files. This should be moved to conftest.py to avoid code duplication and ensure consistency across tests.
| def mock_auth(self): | ||
| """Mock authentication for testing.""" | ||
| auth = Mock() | ||
| auth.base_url = "https://test.insightvm.example.com:3780" | ||
| auth.auth = Mock() | ||
| return auth |
Copilot
AI
Oct 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mock_auth fixture is duplicated across all test files. This should be moved to conftest.py to avoid code duplication and ensure consistency across tests.
| report_id = 123 | ||
| mock_request.return_value = {} | ||
|
|
||
| result = reports_api.delete_report(report_id) |
Check warning
Code scanning / Prospector (reported by Codacy)
local variable 'result' is assigned to but never used (F841) Warning test
| scan_id = 123 | ||
| mock_request.return_value = {} | ||
|
|
||
| result = scans_api.stop_scan(scan_id) |
Check warning
Code scanning / Prospector (reported by Codacy)
local variable 'result' is assigned to but never used (F841) Warning test
| scan_id = 123 | ||
| mock_request.return_value = {} | ||
|
|
||
| result = scans_api.pause_scan(scan_id) |
Check warning
Code scanning / Prospector (reported by Codacy)
local variable 'result' is assigned to but never used (F841) Warning test
| scan_id = 123 | ||
| mock_request.return_value = {} | ||
|
|
||
| result = scans_api.resume_scan(scan_id) |
Check warning
Code scanning / Prospector (reported by Codacy)
local variable 'result' is assigned to but never used (F841) Warning test
| @patch('rapid7.api.sites.BaseAPI._request') | ||
| def test_create_site(self, mock_request, sites_api): | ||
| """Test creating a new site.""" | ||
| site_config = { |
Check warning
Code scanning / Prospector (reported by Codacy)
local variable 'site_config' is assigned to but never used (F841) Warning test
| site_id = 123 | ||
| mock_request.return_value = {} | ||
|
|
||
| result = sites_api.delete_site(site_id) |
Check warning
Code scanning / Prospector (reported by Codacy)
local variable 'result' is assigned to but never used (F841) Warning test
| template_id = "full-audit" | ||
| mock_request.return_value = {} | ||
|
|
||
| result = sites_api.set_scan_template(site_id, template_id) |
Check warning
Code scanning / Prospector (reported by Codacy)
local variable 'result' is assigned to but never used (F841) Warning test
| engine_id = 5 | ||
| mock_request.return_value = {} | ||
|
|
||
| result = sites_api.set_scan_engine(site_id, engine_id) |
Check warning
Code scanning / Prospector (reported by Codacy)
local variable 'result' is assigned to but never used (F841) Warning test
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
Overview
This PR significantly improves test coverage for the InsightVM-Python project, increasing overall coverage from 29.7% to 51.3% (+21.6 percentage points, a 72.7% relative increase). The changes add 128+ new test cases across 5 new test files, following established testing patterns and maintaining consistency with existing code.
Motivation
The project's test coverage was below the minimum target of 70% outlined in CONTRIBUTING.md. Critical modules like
config.pyandui.pyhad 0% coverage, while key API modules (Scans, Reports, Sites) had coverage below 50%. This lack of testing made it difficult to:Changes Made
New Test Files
1. Configuration Management (
tests/test_config.py)2. UI Utilities (
tests/test_ui.py)3. Scans API (
tests/test_rapid7/test_scans.py)4. Reports API (
tests/test_rapid7/test_reports.py)return_raw=True5. Sites API (
tests/test_rapid7/test_sites.py)Testing Patterns Established
All new tests follow consistent patterns:
mock_authand API client fixturesunittest.mockto avoid real API callspytest.raisesAdditional Improvements
.gitignoreto excludecoverage.jsontest_assets.pyandtest_base.pyconftest.pyTest Results
Coverage by Module:
base.py: 100% (complete)auth.py: 100% (complete)client.py: 100% (complete)constants.py: 100% (complete)config.py: 86.0% (+86%)assets.py: 78.6% (existing)ui.py: 72.9% (+72.9%)sites.py: 68.1% (+21.3%)scans.py: 67.5% (+46.7%)reports.py: 42.1% (+12.6%)Verification
All new tests can be run with:
Individual test files:
Future Work
While this PR brings coverage to 51.3%, additional tests could be added for:
These improvements provide a solid foundation and testing patterns that can be extended to remaining modules to reach the 70%+ target.
Related Issues
Closes #[issue-number]
Original prompt
Created from VS Code via the GitHub Pull Request extension.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.