test(core): add integration tests for credentials and environment modules #320
Conversation
…ules Add 98 new integration tests that exercise real system operations: Credentials integration tests (43 tests): - Real encrypted file storage/retrieval with Fernet - File permission verification (0600) - Salt file management and key derivation - Credential manager operations with real storage - Edge cases: unicode metadata, special characters, naive datetimes - Keyring availability detection Environment integration tests (55 tests): - Real tool detection using shutil.which() - Actual subprocess calls for version extraction - Project type detection with realistic file structures - Full validation workflow with real installed tools - Version parsing with real tool output formats - Ecosystem detector coverage Also adds integration test fixtures to conftest.py: - integration_storage_dir, integration_credential_store - available_system_tools (session-scoped) - real_python_project, real_js_project, real_rust_project Closes #309
WalkthroughAdds comprehensive integration test fixtures and two large integration test modules covering credential storage/management and environment/tool detection using real system tools and generated sample projects. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Add integration tests for core credentials and environment modules in test_credentials_integration.py and test_environment_integration.py with shared fixtures in conftest.pyIntroduce integration suites covering credential storage, encryption, validation, and environment/tool detection, backed by session-scoped tool availability and real project fixtures in conftest.py. 📍Where to StartStart with the fixtures in conftest.py, then review credential tests in test_credentials_integration.py followed by environment tests in test_environment_integration.py. Macroscope summarized 407f50e. |
Code Review - Integration Tests for Credentials and Environment ModulesThis PR adds comprehensive integration test coverage for the credentials and environment modules, doubling the test count from 78 to 176. The tests exercise real system operations without mocking, which is exactly what integration tests should do. ✅ Strengths1. Well-Structured Test Organization
2. Proper Integration Test Fixtures
3. Security-Focused Testing
4. Comprehensive Edge Cases # Examples from test_credentials_integration.py:
- Corrupt encrypted files (graceful degradation)
- Invalid salt files (proper error handling)
- Expired credentials (returns None)
- Large credential values (~5KB)
- Multiple store instances sharing same directory
- Naive datetime handling5. Smart Test Skipping
6. Realistic Version Parsing Tests @pytest.mark.parametrize("version_str,expected", [
("git version 2.39.0", (2, 39, 0)),
("Python 3.11.4", (3, 11, 4)),
("cargo 1.75.0 (1d8b05cdd 2023-11-20)", (1, 75, 0)),
# ... 20+ real-world version formats
])This comprehensive parameterization tests actual tool output formats. 🔍 Observations & Minor Suggestions1. Environment Variable Handling Pattern env = os.environ.copy()
env.pop("ANTHROPIC_API_KEY", None)
with patch.dict(os.environ, env, clear=True):
value = manager.get_credential(CredentialProvider.LLM_ANTHROPIC)Suggestion: Consider using the existing 2. Test Marker Consistency pytestmark = [pytest.mark.integration, pytest.mark.v2]This is excellent - properly marks all tests and follows the v2 convention from CLAUDE.md. 3. Fixture Documentation # All integration tests should be marked with @pytest.mark.integration
# Run with: pytest -m integration4. Test Coverage Verification
This matches the claim. Well done! 🏗️ Alignment with CLAUDE.md✅ Follows v2 Architecture:
✅ Integration Testing Best Practices:
✅ Phase 1 CLI Completion: 🧪 Test Coverage AnalysisCredentials Module:
Environment Module:
🐛 Potential IssuesNone identified. The code is well-written, defensive, and follows best practices. 🔐 Security Considerations✅ Excellent security testing:
📊 Performance ConsiderationsSession-scoped ✅ Recommendations
📝 SummaryThis is excellent work. The tests are:
The 98 new tests significantly improve confidence in the credentials and environment modules, especially for real-world scenarios (actual encryption, tool detection, project structures). Recommendation: APPROVE AND MERGE ✅ Great job closing issue #309! 🎉 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/core/conftest.py (1)
6-19:⚠️ Potential issue | 🟡 MinorMake the v2 auto-mark OS-agnostic.
"/tests/core/" in str(item.fspath)is path-separator sensitive and can fail on Windows, leaving core tests unmarked. Convert to a POSIX path (or check parts) before matching.🔧 Suggested fix
def pytest_collection_modifyitems(items): """Automatically mark all tests in this directory as v2.""" for item in items: # Check if the test is in the core directory - if "/tests/core/" in str(item.fspath): + path = Path(str(item.fspath)).as_posix() + if "/tests/core/" in path: item.add_marker(pytest.mark.v2)
🤖 Fix all issues with AI agents
In `@tests/core/test_credentials_integration.py`:
- Around line 41-191: The POSIX permission assertions in tests
test_encrypted_file_permissions_are_secure and
test_salt_file_permissions_are_secure can fail on Windows/non-POSIX filesystems;
update those tests to skip or guard the 0o600 assertions when os.name != "posix"
(or os.name == "nt"), e.g., check os.name before calling stat.S_IMODE and
asserting, or use pytest.skip for non-posix platforms; modify the tests
referencing derive_encryption_key and CredentialStore/credentials.encrypted to
only assert file_mode == 0o600 on POSIX systems.
In `@tests/core/test_environment_integration.py`:
- Around line 506-540: The two edge-case tests are not asserting ToolDetector
behavior; update them to exercise ToolDetector.get_version (and keep
check_version_compatibility) by mocking subprocess.run: for
test_tool_with_slow_version_command, mock subprocess.run to raise
subprocess.TimeoutExpired and assert ToolDetector.get_version returns None or
handles timeout as expected; for test_version_extraction_from_stderr, mock
subprocess.run to return an object with stdout="" and stderr="some-tool version
1.2.3" and assert ToolDetector.get_version extracts "1.2.3" (or the detector's
expected parsed value). Ensure you reference ToolDetector.get_version and
ToolDetector.check_version_compatibility in the assertions and remove the unused
MockResult/inspect-based checks if you implement the mocks.
🧹 Nitpick comments (2)
tests/core/test_environment_integration.py (1)
1-35: Remove redundantpytest.mark.v2in tests/core/.Tests in
tests/core/automatically receivepytest.mark.v2from the conftest.py hook. Keep only the integration marker:Suggested change
-pytestmark = [pytest.mark.integration, pytest.mark.v2] +pytestmark = [pytest.mark.integration]tests/core/test_credentials_integration.py (1)
1-34: Remove redundant v2 marker from pytestmark.Tests in
tests/core/automatically receivepytest.mark.v2via the conftest.py hook, so explicitly declaring it creates duplication. Keep only the integration marker.♻️ Suggested simplification
-pytestmark = [pytest.mark.integration, pytest.mark.v2] +pytestmark = [pytest.mark.integration]
1. Add POSIX platform guards for file permission tests - Skip 0o600 permission assertions on Windows/non-POSIX platforms - Affects test_encrypted_file_permissions_are_secure - Affects test_salt_file_permissions_are_secure 2. Improve edge case tests with proper assertions - test_tool_with_slow_version_command_timeout: Mock TimeoutExpired - test_version_extraction_from_stderr: Mock stderr version output - test_version_extraction_subprocess_error: Test SubprocessError handling - test_detect_tool_version_incompatible: Add equality case
Code Review - Integration Tests for Credentials and Environment ModulesI've reviewed the changes in this PR, including the follow-up commit that addressed CodeRabbit's feedback. This is excellent work that significantly strengthens the test coverage for critical v2 modules. ✅ SummaryWhat Changed:
Status: ✅ APPROVE - Ready to merge 🎯 Strengths1. Proper Integration Testing PhilosophyThe tests correctly exercise real system operations without excessive mocking:
This is exactly what integration tests should do - validate that components work correctly with real dependencies. 2. Excellent Test IsolationThe fixtures ensure tests don't pollute the user's environment: integration_storage_dir # Isolated temp directory for credentials
TEST_KEYRING_SERVICE # Separate keyring service name
available_system_tools # Session-scoped for efficiency3. Security-Focused TestingCritical security assertions are properly validated:
The addition of POSIX platform guards ( 4. Comprehensive CoverageCredentials module:
Environment module:
5. Smart Test SkippingTests gracefully handle missing tools: if not available_system_tools.get("git"):
pytest.skip("git not available on system")This ensures tests pass in CI and on various developer machines. 6. Well-Organized Structure
🔍 Minor Observations1. Redundant
|
The CI environment has ANTHROPIC_API_KEY set, which causes get_credential() to return the env value instead of stored credentials. Fixed tests: - test_set_and_get_credential - test_rotate_credential_preserves_metadata - test_expired_credential_returns_none Now properly clear the env var with patch.dict before testing stored credential retrieval.
Final Code Review - Integration Tests for Credentials and Environment ModulesI've reviewed the complete PR including the follow-up commit (25675e6) that addressed CodeRabbit's feedback. This is excellent work that significantly strengthens test coverage for critical v2 modules. ✅ Status: APPROVED - Ready to MergeWhat This PR Delivers:
🎯 Key Strengths1. Proper Integration Testing PhilosophyTests exercise real system operations without excessive mocking:
2. Security-Focused TestingCritical security assertions are properly validated:
The addition of 3. Excellent Test IsolationFixtures prevent environmental pollution: integration_storage_dir # Isolated temp directory
TEST_KEYRING_SERVICE # Separate keyring service name
available_system_tools # Session-scoped for efficiency4. Smart Performance OptimizationThe 5. Comprehensive CoverageCredentials module tests:
Environment module tests:
🔍 Addressed Issues from Previous Review✅ Fixed: POSIX Permission Tests (Commit 25675e6)CodeRabbit correctly identified that permission tests would fail on Windows. The fix properly guards these tests: @pytest.mark.skipif(os.name != "posix", reason="POSIX permissions not supported on Windows")
def test_encrypted_file_permissions_are_secure(self, integration_storage_dir: Path):This is the correct approach. ✅ ✅ Fixed: Edge Case Test Assertions (Commit 25675e6)The edge case tests now properly mock and assert against 💡 Minor Observations (Non-blocking)1. Redundant
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/core/test_credentials_integration.py`:
- Around line 374-401: The test test_rotate_credential_preserves_metadata stores
initial metadata via manager.set_credential then rotates the key but never
asserts the metadata; after calling manager.rotate_credential (for provider
CredentialProvider.LLM_ANTHROPIC) fetch the stored metadata (via the manager
method that returns credential metadata — e.g., manager.get_credential_metadata
or equivalent) and assert it still contains the original fields {"scopes":
["read", "write"], "rotated_count": 0} (or the expected preserved structure) to
verify metadata was not lost by rotate_credential.
In `@tests/core/test_environment_integration.py`:
- Around line 155-180: The test_python_version_format currently only reads
result.stdout; change it to use stderr when stdout is empty (e.g., version_str =
(result.stdout or result.stderr).strip() or combine both) so parse_version gets
the version whether Python prints to stdout or stderr; update the comment
accordingly and keep references to the test test_python_version_format and the
parse_version call.
1. test_rotate_credential_preserves_metadata: Now actually verifies metadata is preserved after rotation by checking stored_credential.metadata and stored_credential.name match original values 2. test_python_version_format: Handle Python versions that output to stderr by using (result.stdout or result.stderr).strip()
✅ Final Review - LGTM (Approved)I've reviewed all commits in this PR, including the latest fixes in commit 407f50e. This PR is ready to merge. SummaryThis PR successfully adds 98 comprehensive integration tests for the credentials and environment modules, increasing test coverage from 78 to 176 tests. The tests properly exercise real system operations (encryption, tool detection, file I/O) without excessive mocking - exactly what integration tests should do. Closes: #309 from the V2 Strategic Roadmap ✅ What's Been FixedThe PR author has diligently addressed all feedback from CodeRabbit and previous reviews: ✅ Commit 25675e6 - Cross-platform compatibility
✅ Commit dcfd5b8 - Environment variable handling
✅ Commit 407f50e - Final refinements
Key Strengths1. Proper Integration Testing Philosophy
2. Security-Focused Testing
3. Excellent Test Isolationintegration_storage_dir # Isolated temp directory
TEST_KEYRING_SERVICE # Separate keyring service name
available_system_tools # Session-scoped for efficiency4. Smart CI/Developer Machine Compatibility
5. Comprehensive CoverageCredentials module (43 tests):
Environment module (55 tests):
Alignment with CLAUDE.md✅ V2 Architecture Compliance:
✅ Phase 1 CLI Completion:
✅ Testing Best Practices:
Minor Note (Non-blocking)The Security AnalysisNo security concerns identified. The tests properly validate:
Final Recommendation✅ APPROVE AND MERGE This PR:
The test count increase from 78 to 176 represents a significant improvement in confidence for the credentials and environment modules, especially for real-world scenarios. Excellent work! 🎉 This is production-ready. |
Summary
tests/core/conftest.pyChanges
New Test Files
test_credentials_integration.pytest_environment_integration.pyIntegration Fixtures Added
integration_storage_dir- Isolated temp directory for credential testsintegration_credential_store- CredentialStore with test storageavailable_system_tools- Session-scoped tool availability detectionreal_python_project,real_js_project,real_rust_project- Realistic project structuresTest Classes
Credentials:
TestCredentialStoreIntegration- Real encrypted file storage/retrievalTestEncryptionIntegration- Fernet encryption, corruption handlingTestCredentialManagerIntegration- High-level API with real storageTestCredentialValidationIntegration- Format validationTestCredentialEdgeCases- Unicode, special chars, multiple instancesTestKeyringIntegration- Keyring backend detectionEnvironment:
TestRealToolDetection- Real git, python, pytest, ruff detectionTestRealVersionExtraction- Actual subprocess version parsingTestRealProjectTypeDetection- Detection with realistic structuresTestRealEnvironmentValidation- Full validation workflowTestVersionParsingReal- Real tool version formatsTest plan
Related
Closes #309
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.