feat: add comprehensive code coverage with Codacy integration#93
feat: add comprehensive code coverage with Codacy integration#93
Conversation
- Add pytest, pytest-cov, coverage dependencies to requirements.txt - Create pyproject.toml with pytest and coverage configuration - Implement 73+ test cases across authentication, client, and API modules - Set up GitHub Actions workflow for automated testing on Python 3.8-3.12 - Configure Codacy coverage reporter integration - Update documentation with testing instructions - Add .codacy/ to .gitignore for CLI tool cleanup Coverage baseline: 18.7% across core modules CI/CD pipeline will upload reports to Codacy automatically
There was a problem hiding this comment.
Pull Request Overview
This PR establishes comprehensive code coverage testing infrastructure with Codacy integration for the insightvm-python project. The implementation adds automated testing pipelines, coverage reporting, and establishes a baseline coverage of 18.7% across 73+ test cases.
- Added pytest-based testing framework with comprehensive configuration in pyproject.toml
- Created GitHub Actions workflow for automated testing across Python 3.8-3.12 with Codacy coverage upload
- Updated README with detailed testing instructions and coverage information
Reviewed Changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pyproject.toml | Complete project configuration with pytest, coverage, and dependency management |
| README.md | Added comprehensive testing documentation and coverage reporting instructions |
| .github/workflows/test-coverage.yml | CI/CD pipeline for automated testing and Codacy coverage integration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
pyproject.toml
Outdated
| name = "insightvm-python" | ||
| version = "2.0.0" | ||
| description = "Python library for rapid7 insightvm and palo alto cortex xdr apis" | ||
| authors = [{name = "Your Name"}] |
There was a problem hiding this comment.
The author field contains placeholder text 'Your Name' instead of actual project maintainer information. This should be updated with real author details.
| authors = [{name = "Your Name"}] | |
| authors = [{name = "Jane Doe", email = "jane.doe@example.com"}] |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- Update pyproject.toml author field with real maintainer information - Add permissions block to test-coverage workflow for security best practice - Limit GITHUB_TOKEN to contents:read (minimum required permissions) Addresses review comments from PR #93
✅ Addressed Review CommentsI've pushed updates to address both automated review comments: 1. pyproject.toml Author FieldFixed in commit
2. Workflow PermissionsFixed in commit
Both issues have been resolved. The workflow will re-run with these updates. |
- Add Dockerfile.test for consistent CI/CD-matching test environment - Add .docker-test.sh script for easy local Docker testing - Optional but recommended for pre-push validation
🐳 Docker Testing AddedCommit: Added optional Docker testing utilities for local pre-push validation: Files Added
Usage# Quick method
./.docker-test.sh
# Manual method
docker build -f Dockerfile.test -t insightvm-test:local .
docker run --rm insightvm-test:localBenefits✅ Consistent environment matching GitHub Actions Documentation updated in README. |
✅ Addressing Review CommentsGitHub Advanced Security - Workflow PermissionsThe permissions concern has been addressed. The current workflow file includes: permissions:
contents: readThis limits the GITHUB_TOKEN to read-only access as recommended. Copilot - Author FieldThe placeholder author name in Security Fix - Codacy Action✅ Fixed: Replaced the unpinned third-party GitHub Action with Codacy's recommended bash script method. This eliminates the security warning about unpinned actions while using the officially supported approach. |
Security fix: Replaced unpinned third-party GitHub Action with Codacy's recommended bash script method to eliminate security warning. Benefits: - No third-party action pinning concerns - Uses Codacy's officially recommended approach - Auto-updates to latest reporter version - Built-in checksum validation - Simpler and more maintainable Addresses Codacy CLI security warning on PR #93
- Add Dockerfile.test for consistent CI/CD-matching test environment - Add .docker-test.sh script for easy local Docker testing - Update README with Docker testing instructions - Optional but recommended for pre-push validation
- Fixed .gitignore to allow tests/ directory to be tracked - Added all test files including __pycache__ directories - Resolves workflow failures due to missing tests directory Fixes #92
There was a problem hiding this comment.
Bandit found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
| auth = InsightVMAuth() | ||
| repr_str = repr(auth) | ||
| assert "InsightVMAuth" in repr_str | ||
| assert "test.insightvm.example.com" in repr_str |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High test
Copilot Autofix
AI 4 months ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| auth = PlatformAuth() | ||
| repr_str = repr(auth) | ||
| assert "PlatformAuth" in repr_str | ||
| assert "us.api.insight.rapid7.com" in repr_str |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 months ago
To fix this issue, replace the substring check with a strict check of the host value by parsing the base URL in the object being tested—in this case, from the auth.base_url attribute of the PlatformAuth instance. Parse this URL (using Python's urllib.parse module), extract the actual hostname, and assert that it matches the expected domain name "us.api.insight.rapid7.com". Make this change in the assertion on line 128, replacing the substring assertion with an explicit domain extraction and comparison.
You will need to import urlparse from urllib.parse at the top of the file if not already present. The replacement block should update the test so it uses the actual host value and asserts strict equality.
| @@ -7,7 +7,7 @@ | ||
| import pytest | ||
| from unittest.mock import patch | ||
| from requests.auth import HTTPBasicAuth | ||
|
|
||
| from urllib.parse import urlparse | ||
| from rapid7.auth import InsightVMAuth, PlatformAuth | ||
|
|
||
|
|
||
| @@ -125,9 +125,9 @@ | ||
| auth = PlatformAuth() | ||
| repr_str = repr(auth) | ||
| assert "PlatformAuth" in repr_str | ||
| assert "us.api.insight.rapid7.com" in repr_str | ||
| # Strictly check that the host is "us.api.insight.rapid7.com" | ||
| assert urlparse(auth.base_url).hostname == "us.api.insight.rapid7.com" | ||
|
|
||
|
|
||
| class TestAuthIntegration: | ||
| """Test authentication integration scenarios.""" | ||
|
|
| client = InsightVMClient() | ||
| repr_str = repr(client) | ||
| assert "InsightVMClient" in repr_str | ||
| assert "test.insightvm.example.com" in repr_str |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 months ago
The best way to fix this problem is to parse the URL from the string representation and ensure that its hostname matches exactly what is expected (e.g., "test.insightvm.example.com") rather than relying on substring or endswith checks. This makes the test more robust against false positives. To implement this:
- Use the standard Python
urllib.parselibrary to extract the base URL fromrepr_str. - Parse the base URL to get its hostname, and assert equality.
- Only change the test on line 95 in
tests/test_client.py. - Import
urllib.parseif it is not already imported.
| @@ -6,7 +6,7 @@ | ||
| """ | ||
| import pytest | ||
| from unittest.mock import patch, MagicMock | ||
|
|
||
| import urllib.parse | ||
| from rapid7.client import InsightVMClient, create_client | ||
|
|
||
|
|
||
| @@ -92,8 +92,14 @@ | ||
| client = InsightVMClient() | ||
| repr_str = repr(client) | ||
| assert "InsightVMClient" in repr_str | ||
| assert "test.insightvm.example.com" in repr_str | ||
|
|
||
| # Extract base_url from repr string and parse hostname | ||
| # Assuming the repr includes the base_url, e.g. "InsightVMClient(base_url='https://test.insightvm.example.com:3780', ...)" | ||
| import re | ||
| m = re.search(r"base_url=['\"](.+?)['\"]", repr_str) | ||
| assert m, "base_url not found in repr string" | ||
| base_url = m.group(1) | ||
| parsed_url = urllib.parse.urlparse(base_url) | ||
| assert parsed_url.hostname == "test.insightvm.example.com" | ||
| def test_context_manager(self): | ||
| """Test client context manager functionality.""" | ||
| with InsightVMClient() as client: |
| assert client2.auth.username == "user2" | ||
| assert client1.auth.username != client2.auth.username | ||
|
|
||
| assert "server1.example.com" in client1.auth.base_url |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 months ago
The correct way to fix this is to parse the base_url using urlparse and check that the hostname attribute matches the expected host exactly. In the context of these tests, instead of assert "server1.example.com" in client1.auth.base_url, use:
from urllib.parse import urlparse
assert urlparse(client1.auth.base_url).hostname == "server1.example.com"This change should be made for both the assertion for server1 (line 144) and server2 (line 145), since both use substring checks. Additionally, since urlparse will be needed, an import line for urlparse should be added at the top of the file.
| @@ -6,7 +6,7 @@ | ||
| """ | ||
| import pytest | ||
| from unittest.mock import patch, MagicMock | ||
|
|
||
| from urllib.parse import urlparse | ||
| from rapid7.client import InsightVMClient, create_client | ||
|
|
||
|
|
||
| @@ -141,8 +141,8 @@ | ||
| assert client2.auth.username == "user2" | ||
| assert client1.auth.username != client2.auth.username | ||
|
|
||
| assert "server1.example.com" in client1.auth.base_url | ||
| assert "server2.example.com" in client2.auth.base_url | ||
| assert urlparse(client1.auth.base_url).hostname == "server1.example.com" | ||
| assert urlparse(client2.auth.base_url).hostname == "server2.example.com" | ||
|
|
||
| def test_api_module_access_patterns(self): | ||
| """Test typical API access patterns work.""" |
| assert client1.auth.username != client2.auth.username | ||
|
|
||
| assert "server1.example.com" in client1.auth.base_url | ||
| assert "server2.example.com" in client2.auth.base_url |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 months ago
The best way to fix the problem is to avoid substring matches for hostnames in URL assertions. Instead, parse the URL using Python's urllib.parse.urlparse and check the hostname (or .netloc) directly. For the file tests/test_client.py, in the test_multiple_clients_isolation test, replace
assert "server2.example.com" in client2.auth.base_urlwith:
from urllib.parse import urlparse
...
assert urlparse(client2.auth.base_url).hostname == "server2.example.com"Also update the similar assertion for client1.auth.base_url for consistency.
Add the necessary import at the top if not already present.
| @@ -6,6 +6,7 @@ | ||
| """ | ||
| import pytest | ||
| from unittest.mock import patch, MagicMock | ||
| from urllib.parse import urlparse | ||
|
|
||
| from rapid7.client import InsightVMClient, create_client | ||
|
|
||
| @@ -141,8 +142,8 @@ | ||
| assert client2.auth.username == "user2" | ||
| assert client1.auth.username != client2.auth.username | ||
|
|
||
| assert "server1.example.com" in client1.auth.base_url | ||
| assert "server2.example.com" in client2.auth.base_url | ||
| assert urlparse(client1.auth.base_url).hostname == "server1.example.com" | ||
| assert urlparse(client2.auth.base_url).hostname == "server2.example.com" | ||
|
|
||
| def test_api_module_access_patterns(self): | ||
| """Test typical API access patterns work.""" |
- Replace deprecated third-party bandit action with direct implementation - Update to github/codeql-action/upload-sarif@v3 (v1 and v2 deprecated) - Use latest checkout@v4 and setup-python@v5 actions - Install bandit with SARIF support directly - Maintains same security scanning functionality Resolves CodeQL Action deprecation warning per: https://github.blog/changelog/2025-01-10-code-scanning-codeql-action-v2-is-now-deprecated/
1 participant
🧪 Code Coverage with Codacy Integration
✨ What's New
🔧 Implementation Details
Test Infrastructure
tests/conftest.py)CI/CD Pipeline (
test-coverage.yml)Configuration
pyproject.tomlwith pytest and coverage configuration.codacy/to.gitignorefor CLI tool cleanup📊 Coverage Baseline
✅ Testing Status
🔒 Security Improvements
Fixed: Codacy CLI security warning about unpinned third-party action
Before:
After (Codacy's recommended approach):
Benefits:
This PR establishes a solid foundation for automated testing and code quality monitoring through Codacy. Future PRs should maintain or improve the coverage baseline.
🚀 Ready for merge once CI passes!