Fix outdated tests #7
Conversation
Reviewer's GuideThis PR overhauls the outdated tests in the compliance_report and key_rotation modules to align with recent refactors, including reloading modules for threshold tests, shifting from status dictionaries to compliance metrics, updating test data schemas and method names, and tightening mocks and assertions. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughThe test suites for IAM compliance reporting and key rotation were updated to align with recent implementation changes. Updates include improved module reloading for environment isolation, changes in expected data formats, refactoring of test logic to match new method names and data structures, and adjustments to mocking approaches. No public API changes were made. Changes
Poem
✨ Finishing Touches
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 (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @tblakex01 - I've reviewed your changes - here's some feedback:
- The tests contain a lot of repeated user_data setup; consider extracting a helper or fixture (or using pytest parametrization) to reduce duplication and improve readability.
- Avoid using importlib.reload to reset module state for threshold tests and instead use monkeypatch or dependency injection to override environment variables more cleanly.
- Relying on datetime.now in tests makes them non-deterministic—consider freezing time or injecting a clock to ensure consistent, repeatable results.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The tests contain a lot of repeated user_data setup; consider extracting a helper or fixture (or using pytest parametrization) to reduce duplication and improve readability.
- Avoid using importlib.reload to reset module state for threshold tests and instead use monkeypatch or dependency injection to override environment variables more cleanly.
- Relying on datetime.now in tests makes them non-deterministic—consider freezing time or injecting a clock to ensure consistent, repeatable results.
## Individual Comments
### Comment 1
<location> `tests/test_compliance_report.py:234` </location>
<code_context>
- @patch("aws_iam_compliance_report.Progress")
- def test_process_users_data(self, mock_progress_class):
+ self.assertIsNone(metrics["password_age"])
+
+ @patch.object(compliance, "get_iam_client")
</code_context>
<issue_to_address>
No test for invalid or malformed date input in age calculation.
Add a test case for malformed or invalid date strings to verify error handling during parsing.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
self.assertIsNone(metrics["password_age"])
@patch.object(compliance, "get_iam_client")
def test_parse_credential_report(self, mock_get_client):
=======
self.assertIsNone(metrics["password_age"])
def test_calculate_compliance_metrics_with_invalid_date(self):
"""Test that invalid or malformed date strings are handled gracefully in age calculation."""
report = compliance.IAMComplianceReport()
# Malformed date string for password_last_changed
user_data = {
"user": "test-user",
"password_enabled": True,
"password_last_changed": "not-a-date",
"access_key_1_active": False,
"access_key_1_last_rotated": None,
"access_key_2_active": False,
"access_key_2_last_rotated": None,
}
metrics = report.calculate_compliance_metrics(user_data)
# Expect password_age to be None or handled gracefully
self.assertIsNone(metrics["password_age"])
@patch.object(compliance, "get_iam_client")
def test_parse_credential_report(self, mock_get_client):
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `tests/test_compliance_report.py:429` </location>
<code_context>
+ mock_report.summary_stats = {"expired_keys": 0, "expired_passwords": 0}
- compliance.main()
+ with self.assertRaises(SystemExit) as cm:
+ compliance.main()
+ self.assertEqual(cm.exception.code, 0)
# Verify methods were called
</code_context>
<issue_to_address>
Test for main function does not cover all CLI argument combinations.
Please add tests for additional CLI options like CSV export, detailed report, and summary only to ensure all code paths are tested.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| self.assertIsNone(metrics["password_age"]) | ||
|
|
||
| @patch.object(compliance, "get_iam_client") | ||
| def test_parse_credential_report(self, mock_get_client): |
There was a problem hiding this comment.
suggestion (testing): No test for invalid or malformed date input in age calculation.
Add a test case for malformed or invalid date strings to verify error handling during parsing.
| self.assertIsNone(metrics["password_age"]) | |
| @patch.object(compliance, "get_iam_client") | |
| def test_parse_credential_report(self, mock_get_client): | |
| self.assertIsNone(metrics["password_age"]) | |
| def test_calculate_compliance_metrics_with_invalid_date(self): | |
| """Test that invalid or malformed date strings are handled gracefully in age calculation.""" | |
| report = compliance.IAMComplianceReport() | |
| # Malformed date string for password_last_changed | |
| user_data = { | |
| "user": "test-user", | |
| "password_enabled": True, | |
| "password_last_changed": "not-a-date", | |
| "access_key_1_active": False, | |
| "access_key_1_last_rotated": None, | |
| "access_key_2_active": False, | |
| "access_key_2_last_rotated": None, | |
| } | |
| metrics = report.calculate_compliance_metrics(user_data) | |
| # Expect password_age to be None or handled gracefully | |
| self.assertIsNone(metrics["password_age"]) | |
| @patch.object(compliance, "get_iam_client") | |
| def test_parse_credential_report(self, mock_get_client): |
| with self.assertRaises(SystemExit) as cm: | ||
| compliance.main() | ||
| self.assertEqual(cm.exception.code, 0) |
There was a problem hiding this comment.
suggestion (testing): Test for main function does not cover all CLI argument combinations.
Please add tests for additional CLI options like CSV export, detailed report, and summary only to ensure all code paths are tested.
There was a problem hiding this comment.
Pull Request Overview
This PR updates and fixes outdated tests for the IAM compliance report and key rotation modules to align with recent API and behavior changes. The changes ensure tests work correctly with the updated module interfaces and expected data structures.
Key changes include:
- Updated compliance report tests to use new method names (
calculate_compliance_metrics,parse_credential_report) and data structures - Modified key rotation tests to patch the correct boto3 methods and update client caching behavior
- Adjusted test assertions to match new compliance status values and CSV export formats
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/test_key_rotation.py | Updates client mocking to patch boto3.client directly, changes Mock to MagicMock, and removes outdated assertion |
| tests/test_compliance_report.py | Extensive updates to match new API methods, data structures, compliance status values, and export formats |
| import importlib | ||
|
|
||
| importlib.reload(compliance) |
There was a problem hiding this comment.
Using importlib.reload() in tests can lead to unpredictable behavior and side effects. Consider using dependency injection or mocking environment variables instead of relying on module reloading.
| import importlib | |
| importlib.reload(compliance) |
| import importlib | ||
|
|
||
| importlib.reload(compliance) |
There was a problem hiding this comment.
Using importlib.reload() in tests can lead to unpredictable behavior and side effects. Consider using dependency injection or mocking environment variables instead of relying on module reloading.
| import importlib | |
| importlib.reload(compliance) |
| import importlib | ||
|
|
||
| importlib.reload(compliance) | ||
| try: | ||
| report = compliance.IAMComplianceReport() | ||
|
|
||
| self.assertEqual(report.KEY_WARNING_THRESHOLD, 60) | ||
| self.assertEqual(report.KEY_NON_COMPLIANT_THRESHOLD, 80) | ||
| self.assertEqual(report.PASSWORD_WARNING_THRESHOLD, 70) | ||
| self.assertEqual(report.PASSWORD_NON_COMPLIANT_THRESHOLD, 85) | ||
| self.assertEqual(report.KEY_WARNING_THRESHOLD, 60) | ||
| self.assertEqual(report.KEY_NON_COMPLIANT_THRESHOLD, 80) | ||
| self.assertEqual(report.PASSWORD_WARNING_THRESHOLD, 70) | ||
| self.assertEqual(report.PASSWORD_NON_COMPLIANT_THRESHOLD, 85) | ||
| finally: | ||
| importlib.reload(compliance) | ||
|
|
There was a problem hiding this comment.
Using importlib.reload() in tests can lead to unpredictable behavior and side effects. Consider using dependency injection or mocking environment variables instead of relying on module reloading.
| import importlib | |
| importlib.reload(compliance) | |
| try: | |
| report = compliance.IAMComplianceReport() | |
| self.assertEqual(report.KEY_WARNING_THRESHOLD, 60) | |
| self.assertEqual(report.KEY_NON_COMPLIANT_THRESHOLD, 80) | |
| self.assertEqual(report.PASSWORD_WARNING_THRESHOLD, 70) | |
| self.assertEqual(report.PASSWORD_NON_COMPLIANT_THRESHOLD, 85) | |
| self.assertEqual(report.KEY_WARNING_THRESHOLD, 60) | |
| self.assertEqual(report.KEY_NON_COMPLIANT_THRESHOLD, 80) | |
| self.assertEqual(report.PASSWORD_WARNING_THRESHOLD, 70) | |
| self.assertEqual(report.PASSWORD_NON_COMPLIANT_THRESHOLD, 85) | |
| finally: | |
| importlib.reload(compliance) | |
| report = compliance.IAMComplianceReport( | |
| key_warning_threshold=60, | |
| key_non_compliant_threshold=80, | |
| password_warning_threshold=70, | |
| password_non_compliant_threshold=85, | |
| ) | |
| self.assertEqual(report.KEY_WARNING_THRESHOLD, 60) | |
| self.assertEqual(report.KEY_NON_COMPLIANT_THRESHOLD, 80) | |
| self.assertEqual(report.PASSWORD_WARNING_THRESHOLD, 70) | |
| self.assertEqual(report.PASSWORD_NON_COMPLIANT_THRESHOLD, 85) |
|
|
||
| mock_client = Mock() | ||
| mock_client.list_user_tags.return_value = {"Tags": []} | ||
| mock_client.list_access_keys.return_value = {"AccessKeyMetadata": [{}]} |
There was a problem hiding this comment.
[nitpick] The empty dictionary in the list could be more explicit about what AccessKeyMetadata structure is expected. Consider using a more realistic mock structure with actual key fields.
| mock_client.list_access_keys.return_value = {"AccessKeyMetadata": [{}]} | |
| mock_client.list_access_keys.return_value = { | |
| "AccessKeyMetadata": [ | |
| { | |
| "AccessKeyId": "AKIAEXAMPLE123456", | |
| "Status": "Active", | |
| "CreateDate": "2023-01-01T00:00:00+00:00" | |
| } | |
| ] | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_key_rotation.py (1)
24-25: Update the outdated comment to match the code change.The comment still refers to "global session" but the code now resets
_iam_client. This inconsistency should be fixed for clarity.- def setUp(self): - """Reset global session before each test""" - key_rotation._iam_client = None + def setUp(self): + """Reset global IAM client before each test""" + key_rotation._iam_client = None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/test_compliance_report.py(12 hunks)tests/test_key_rotation.py(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
PR: tblakex01/iam-key-rotation#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-02T14:07:15.163Z
Learning: Applies to scripts/aws_iam_self_service_key_rotation.py : Scripts follow AWS best practices for key rotation.
tests/test_key_rotation.py (2)
Learnt from: CR
PR: tblakex01/iam-key-rotation#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-02T14:07:15.163Z
Learning: Applies to scripts/aws_iam_self_service_key_rotation.py : Scripts follow AWS best practices for key rotation.
Learnt from: CR
PR: tblakex01/iam-key-rotation#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-02T14:07:15.163Z
Learning: Applies to scripts/aws_iam_{self_service,user}_password_reset.py : Password generation uses Python's `secrets` module for cryptographic security.
tests/test_compliance_report.py (1)
Learnt from: CR
PR: tblakex01/iam-key-rotation#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-02T14:07:15.163Z
Learning: Applies to lambda/password_notification/password_notification.py : Lambda function must have an IAM role with 'iam:GenerateCredentialReport', 'iam:GetCredentialReport', 'iam:ListUserTags', and 'ses:SendEmail' permissions.
🧬 Code Graph Analysis (1)
tests/test_key_rotation.py (1)
tests/integration/conftest.py (1)
mock_client(33-36)
🔇 Additional comments (10)
tests/test_key_rotation.py (2)
7-7: Good addition of MagicMock import.The addition of
MagicMockis appropriate for mocking objects that need to support magic methods like__setitem__and__getitem__.
498-501: Good use of MagicMock for ConfigParser.The change to
MagicMock()is appropriate for mocking ConfigParser's dictionary-like behavior. The explicit setup of__contains__and__getitem__ensures proper test behavior.tests/test_compliance_report.py (8)
28-30: Excellent test isolation using module reload.The use of
importlib.reload()with a try-finally pattern ensures proper environment variable isolation between tests. This prevents test pollution when modules read environment variables at import time.Also applies to: 53-64
90-90: Correct assertion for string return type.The test properly validates that
generate_credential_reportnow returns a decoded string instead of bytes, which simplifies downstream CSV parsing.
124-136: Well-structured compliance metrics refactoring.The migration from
calculate_compliance_statustocalculate_compliance_metricswith detailed user data structures improves testability and provides richer compliance information. The explicit fields for each access key make the data model clearer.Also applies to: 143-155, 162-175
236-244: Good use of patch.object for explicit mocking.The use of
@patch.objectclearly indicates what's being mocked, and the mock client setup includes all necessary IAM operations for the test.
247-260: Comprehensive CSV test data structure.The updated CSV content includes additional access key usage fields (last_used_date, region, service) which align with AWS credential report format and enable more detailed compliance monitoring.
288-290: Good adoption of pathlib for file operations.Using
pathlib.Pathobjects instead of strings for file operations is a Python best practice that provides better cross-platform compatibility and cleaner APIs.Also applies to: 335-340
429-431: Explicit exit code validation improves test precision.Testing for
SystemExitwith specific exit codes (0 for success, 1 for error) makes the expected behavior clearer and follows CLI best practices.Also applies to: 457-459, 487-489
493-493: Cleaner error message formatting.The simplified error format with
[red]Error:[/red]prefix provides better separation between the error indicator and the actual message.
| @patch("aws_iam_self_service_key_rotation.boto3.client") | ||
| def test_get_iam_client_creates_session(self, mock_boto_client): | ||
| """Test that IAM client creates and caches session""" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update test method name and docstring to reflect client creation instead of session.
The test method name and docstring still reference "session" but the implementation now creates an IAM client directly.
- @patch("aws_iam_self_service_key_rotation.boto3.client")
- def test_get_iam_client_creates_session(self, mock_boto_client):
- """Test that IAM client creates and caches session"""
+ @patch("aws_iam_self_service_key_rotation.boto3.client")
+ def test_get_iam_client_creates_and_caches_client(self, mock_boto_client):
+ """Test that IAM client creates and caches client"""📝 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.
| @patch("aws_iam_self_service_key_rotation.boto3.client") | |
| def test_get_iam_client_creates_session(self, mock_boto_client): | |
| """Test that IAM client creates and caches session""" | |
| @patch("aws_iam_self_service_key_rotation.boto3.client") | |
| def test_get_iam_client_creates_and_caches_client(self, mock_boto_client): | |
| """Test that IAM client creates and caches client""" |
🤖 Prompt for AI Agents
In tests/test_key_rotation.py around lines 27 to 29, the test method name and
docstring incorrectly mention "session" while the code actually creates an IAM
client. Rename the test method to reflect client creation instead of session and
update the docstring accordingly to describe testing IAM client creation and
caching.
Summary
Testing
black tests/test_compliance_report.py tests/test_key_rotation.pyflake8 scripts/ lambda/ tests/ --max-line-length=120 --ignore=E501,W503,E203mypy scripts/ lambda/bandit -r scripts/ lambda/pytesthttps://chatgpt.com/codex/tasks/task_e_686974023b7483329893bea53c1eb47b
Summary by Sourcery
Update and fix outdated tests for IAM compliance report and key rotation modules to align with recent API and behavior changes
Tests:
Summary by CodeRabbit