Expand moto-backed coverage and infrastructure validation #11
Conversation
Reviewer's GuideThis PR broadens test coverage by introducing moto-backed unit tests for the password notification Lambda, expanding CLI regression tests for password reset and secret rotation, adding end-to-end CLI integration tests, validating Terraform configs in CI, enforcing stricter pytest coverage thresholds, and updating tooling dependencies along with a global mypy configuration. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughAdds type-checker and test configs, expands test requirements, and introduces multiple new tests: CLI end-to-end, password reset workflow, password notification Lambda (Moto), secret key rotation to S3 (Moto and CLI), Terraform validation, and a compliance-report regression. No production code changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as Key Rotation CLI
participant boto3 as boto3 (IAM/S3)
participant AWS as AWS (Moto/Real)
User->>CLI: Invoke --list --json
CLI->>boto3: client('iam').list_access_keys
boto3->>AWS: ListAccessKeys
AWS-->>boto3: Keys
boto3-->>CLI: Keys
CLI-->>User: Print JSON payload
sequenceDiagram
autonumber
participant Event as Event Trigger
participant Lambda as Password Notification Lambda
participant IAM as boto3.client('iam')
participant SES as boto3.client('ses')
Event->>Lambda: lambda_handler(event, context)
Lambda->>IAM: get_credential_report
IAM-->>Lambda: CSV credential data
Lambda->>Lambda: Parse, filter expiring users with email
Lambda->>SES: send_email(to=user.email)
SES-->>Lambda: MessageId
Lambda-->>Event: "Password expiry notifications sent."
sequenceDiagram
autonumber
actor User
participant CLI as Password Reset CLI
participant IAM as boto3.client('iam')
User->>CLI: Run main flow
CLI->>IAM: GetLoginProfile
alt Exists
CLI->>CLI: Prompt for old password
CLI->>CLI: Generate new password
CLI->>IAM: ChangePassword(old, new)
alt Success
CLI-->>User: Print new password, exit 0
else Error
CLI-->>User: Log error, exit 1
end
else NoSuchEntity
CLI-->>User: Exit 1
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- I’d recommend unifying the test suite to a single framework (pytest or unittest) rather than mixing TestCase classes and pytest fixtures, which will simplify configuration and improve readability.
- The CLI tests rely on runpy and patching sys.argv/boto3, which can be brittle—consider refactoring your scripts to expose core functions or a testable entry point so you can invoke them directly without runpy hacks.
- The Terraform validation test runs real terraform commands in CI, which can slow down or fail tests—consider isolating it behind a dedicated marker or mocking providers to keep the main test suite fast and reliable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- I’d recommend unifying the test suite to a single framework (pytest or unittest) rather than mixing TestCase classes and pytest fixtures, which will simplify configuration and improve readability.
- The CLI tests rely on runpy and patching sys.argv/boto3, which can be brittle—consider refactoring your scripts to expose core functions or a testable entry point so you can invoke them directly without runpy hacks.
- The Terraform validation test runs real terraform commands in CI, which can slow down or fail tests—consider isolating it behind a dedicated marker or mocking providers to keep the main test suite fast and reliable.
## Individual Comments
### Comment 1
<location> `tests/test_password_notification_lambda.py:69-78` </location>
<code_context>
+def test_lambda_sends_notifications_for_expiring_passwords(
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for SES send_email failures.
Please add a test case where SES send_email raises an exception to verify the Lambda handles errors correctly and does not misreport success.
Suggested implementation:
```python
def test_lambda_sends_notifications_for_expiring_passwords(
password_notification_module,
):
"""Verify that users beyond the warning threshold receive an email."""
module = password_notification_module
iam_client = module.iam_client
iam_client.create_user(UserName="alice")
iam_client.tag_user(
UserName="alice", Tags=[{"Key": "email", "Value": "alice@example.com"}]
def test_lambda_handles_ses_send_email_failure(password_notification_module, mocker):
"""Verify that the Lambda handles SES send_email failures gracefully."""
module = password_notification_module
iam_client = module.iam_client
ses_client = module.ses_client
iam_client.create_user(UserName="bob")
iam_client.tag_user(
UserName="bob", Tags=[{"Key": "email", "Value": "bob@example.com"}]
)
# Simulate password expiration for bob
# (You may need to set up the password last changed date here as in other tests)
# Mock SES send_email to raise an exception
mocker.patch.object(ses_client, "send_email", side_effect=Exception("SES failure"))
# Run the Lambda handler
try:
module.lambda_handler({}, {})
except Exception:
pytest.fail("Lambda should handle SES send_email exceptions gracefully")
# Optionally, check logs or other error handling mechanisms if available
```
- You may need to adjust the password expiration setup for "bob" to match your test conventions.
- If your Lambda logs errors or sets a status, you can add assertions to check those behaviors.
- Ensure `pytest` and `mocker` are available in your test environment.
</issue_to_address>
### Comment 2
<location> `tests/test_password_reset.py:401-402` </location>
<code_context>
+ @patch(
+ "aws_iam_self_service_password_reset.validate_aws_connection", return_value=True
+ )
+ def test_main_handles_unexpected_exception(
+ self,
+ mock_validate,
+ mock_get_user,
+ mock_boto_client,
+ mock_passwordgen,
+ mock_getpass,
+ mock_exit,
+ mock_file,
+ ):
+ iam_client = Mock()
+ mock_boto_client.return_value = iam_client
+ iam_client.get_login_profile.return_value = {}
+ iam_client.change_password.side_effect = RuntimeError("boom")
+
+ with patch("aws_iam_self_service_password_reset.rprint"):
+ with self.assertRaises(SystemExit):
+ pwd_reset.main()
+
+ mock_exit.assert_called_with(1)
+ mock_file().write.assert_called()
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider asserting the content written to the file in error cases.
Please add assertions to verify the actual content written to the file during error handling, such as the error message or traceback, to confirm correct error reporting.
```suggestion
mock_exit.assert_called_with(1)
mock_file().write.assert_called()
# Assert the error message or traceback is written to the file
written_content = "".join(
call.args[0] for call in mock_file().write.call_args_list
)
self.assertIn("boom", written_content)
```
</issue_to_address>
### Comment 3
<location> `tests/test_secret_key_rotation_s3.py:143-152` </location>
<code_context>
+ def test_cli_propagates_client_errors(self):
</code_context>
<issue_to_address>
**suggestion (testing):** Consider testing for other error codes and exception types.
Adding tests for other ClientError codes and non-ClientError exceptions will help verify error handling and logging are robust.
</issue_to_address>
### Comment 4
<location> `tests/test_terraform_validation.py:45-46` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 5
<location> `tests/test_terraform_validation.py:54-55` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 6
<location> `tests/test_terraform_validation.py:58-59` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 7
<location> `tests/test_password_notification_lambda.py:53` </location>
<code_context>
def _credential_report_for(users: list[tuple[str, datetime]]):
"""Generate a credential report CSV for the provided users."""
rows = [
"user,arn,user_creation_time,password_enabled,password_last_used,password_last_changed",
]
for username, changed_at in users:
rows.append(
",".join(
[
username,
f"arn:aws:iam::123456789012:user/{username}",
"2023-01-01T00:00:00+00:00",
"true",
"N/A",
changed_at.astimezone(timezone.utc).isoformat(),
]
)
)
return "\n".join(rows).encode("utf-8")
</code_context>
<issue_to_address>
**issue (code-quality):** Replace a for append loop with list extend ([`for-append-to-extend`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/for-append-to-extend/))
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if shutil.which("terraform") is None: | ||
| pytest.skip("Terraform CLI is not installed") |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if init_result.returncode != 0: | ||
| pytest.fail(f"terraform init failed: {init_result.stderr}") |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if validate_result.returncode != 0: | ||
| pytest.fail(f"terraform validate failed: {validate_result.stderr}") |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| rows = [ | ||
| "user,arn,user_creation_time,password_enabled,password_last_used,password_last_changed", | ||
| ] | ||
| for username, changed_at in users: |
There was a problem hiding this comment.
issue (code-quality): Replace a for append loop with list extend (for-append-to-extend)
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (15)
mypy.ini (1)
1-2: Consider adding stricter type checking optionsWhile
ignore_missing_imports = Truehelps with third-party libraries lacking type stubs, consider adding more strict options to improve type safety gradually:[mypy] ignore_missing_imports = True +python_version = 3.8 +warn_return_any = True +warn_unused_configs = True +disallow_untyped_defs = FalseThis would provide better type checking while still allowing incremental adoption.
tests/test_terraform_validation.py (2)
1-2: Remove shebang from non-executable test fileThe shebang line is present but the file is not executable. Either make the file executable or remove the shebang.
-#!/usr/bin/env python3 """Lightweight infrastructure validation executed during the test suite."""
15-26: Consider adding timeout protection for subprocess callsThe
subprocess.runcall could hang indefinitely if Terraform encounters an interactive prompt or network issue. Consider adding a timeout.result = subprocess.run( ["terraform", *args], cwd=work_dir, check=False, capture_output=True, text=True, env=env, + timeout=60, # 60 second timeout for Terraform operations )tests/test_password_notification_lambda.py (2)
1-2: Remove shebang from non-executable test fileThe shebang line is present but the file is not executable.
-#!/usr/bin/env python3 """Tests for the password notification Lambda using moto-backed AWS services."""
47-47: Improve type hint for better clarityThe type hint could be more specific about the expected user data structure.
-def _credential_report_for(users: list[tuple[str, datetime]]): +def _credential_report_for(users: list[tuple[str, datetime]]) -> bytes:tests/test_password_reset.py (1)
289-310: Clean up unused mock argumentsSeveral mock arguments are injected but not used in the test methods. While they're needed for patching, consider using
_prefix to indicate they're intentionally unused.def test_main_successful_reset( self, - mock_validate, - mock_get_user, + _mock_validate, + _mock_get_user, mock_boto_client, - mock_passwordgen, - mock_getpass, + _mock_passwordgen, + _mock_getpass, mock_display, mock_log, ):Apply similar changes to other test methods with unused mock arguments.
tests/integration/test_cli_end_to_end.py (5)
1-1: Remove non-executable shebang or make the file executable.Ruff flags EXE001; tests don’t need a shebang.
Apply:
-#!/usr/bin/env python3
41-47: Prefer pytest’s capsys over manual stdout redirection.Simpler and less brittle.
Example:
- stdout = io.StringIO() - ... - with patch.object(sys, "argv", argv): - with patch("boto3.client", return_value=_FakeIAM()): - with redirect_stdout(stdout): - runpy.run_path(script, run_name="__main__") - - payload = json.loads(stdout.getvalue()) + with patch.object(sys, "argv", argv), patch("boto3.client", return_value=_FakeIAM()): + runpy.run_path(script, run_name="__main__") + captured = capsys.readouterr() + payload = json.loads(captured.out)
49-50: Guard JSON parsing from incidental stdout noise.If the CLI logs to stdout, json.loads will fail. Consider emitting JSON only (logs to stderr) when --json is set, or parse the last JSON-looking line in the test.
59-63: Avoid env leakage; patch env in a scoped context.setdefault can leave globals mutated across tests.
- os.environ.setdefault("AWS_DEFAULT_REGION", "us-east-1") - os.environ.setdefault("AWS_ACCESS_KEY_ID", "testing") - os.environ.setdefault("AWS_SECRET_ACCESS_KEY", "testing") + with patch.dict( + os.environ, + { + "AWS_DEFAULT_REGION": "us-east-1", + "AWS_ACCESS_KEY_ID": "testing", + "AWS_SECRET_ACCESS_KEY": "testing", + }, + clear=False, + ): + iam = boto3.client("iam", region_name="us-east-1") + iam.create_user(UserName="cli-user-one") + iam.create_user(UserName="cli-user-two") + stdout = io.StringIO() + argv = [script.name, "list-users"] + with patch.object(sys, "argv", argv), redirect_stdout(stdout): + runpy.run_path(script, run_name="__main__") + output = stdout.getvalue().splitlines()
75-77: Make the header assertion resilient to extra output.Don’t assume it’s the first line.
- assert "Found 2 IAM users:" in output[0] + assert any("Found 2 IAM users:" in line for line in output)tests/test_secret_key_rotation_s3.py (4)
1-1: Remove non-executable shebang or chmod +x.Keep tests shebang-free to satisfy EXE001.
-#!/usr/bin/env python3
64-67: Avoid shared module state; restore cached clients after test.Resetting rotate._iam_client/_s3_client without restore risks cross-test flakiness.
- def test_rotate_and_store_with_moto(self): - rotate._iam_client = None - rotate._s3_client = None + def test_rotate_and_store_with_moto(self): + prev_iam, prev_s3 = rotate._iam_client, rotate._s3_client + rotate._iam_client = None + rotate._s3_client = None try: - with mock_aws(): + with patch.dict( + os.environ, + { + "AWS_DEFAULT_REGION": "us-east-1", + "AWS_ACCESS_KEY_ID": "testing", + "AWS_SECRET_ACCESS_KEY": "testing", + }, + clear=False, + ), mock_aws(): iam = boto3.client("iam", region_name="us-east-1") s3 = boto3.client("s3", region_name="us-east-1") ... - self.assertTrue(url.startswith("https://")) + self.assertTrue(url.startswith("https://")) + finally: + rotate._iam_client, rotate._s3_client = prev_iam, prev_s3
150-158: Silence Ruff ARG002 by marking unused parameters.These dummies aren’t used; prefix with underscore.
- class _ErroringIAM: - def list_access_keys(self, UserName): + class _ErroringIAM: + def list_access_keys(self, _username): return {"AccessKeyMetadata": []} - def create_access_key(self, UserName): + def create_access_key(self, _username): raise ClientError( {"Error": {"Code": "AccessDenied", "Message": "denied"}}, "CreateAccessKey", )
181-186: Assert on stable signal (“AccessDenied”) rather than log copy.Log text “AWS error” is brittle; assert the code appears in logs.
- self.assertTrue(any("AWS error" in message for message in logs.output)) + self.assertTrue(any("AccessDenied" in message for message in logs.output))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
mypy.ini(1 hunks)pytest.ini(1 hunks)scripts/requirements.txt(1 hunks)tests/integration/test_cli_end_to_end.py(1 hunks)tests/requirements.txt(1 hunks)tests/test_compliance_report.py(1 hunks)tests/test_password_notification_lambda.py(1 hunks)tests/test_password_reset.py(2 hunks)tests/test_secret_key_rotation_s3.py(2 hunks)tests/test_terraform_validation.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Flake8 for linting the application. E501 errors can be ignored up to 120 lines.
Always use Black for formatting the application.
Always use Pyright or mypy for type checking the application.
Files:
tests/test_compliance_report.pytests/integration/test_cli_end_to_end.pytests/test_password_notification_lambda.pytests/test_secret_key_rotation_s3.pytests/test_terraform_validation.pytests/test_password_reset.py
🧬 Code graph analysis (4)
tests/test_compliance_report.py (1)
scripts/aws_iam_compliance_report.py (1)
parse_credential_report(118-132)
tests/integration/test_cli_end_to_end.py (1)
tests/test_secret_key_rotation_s3.py (3)
_FakeIAM(89-108)list_access_keys(93-95)list_access_keys(151-152)
tests/test_secret_key_rotation_s3.py (2)
scripts/aws_iam_secret_key_rotation_s3.py (2)
rotate_and_store_credentials(78-116)generate_presigned_url(68-75)tests/integration/test_cli_end_to_end.py (2)
_FakeIAM(29-39)list_access_keys(30-39)
tests/test_password_reset.py (1)
scripts/aws_iam_self_service_password_reset.py (1)
main(192-302)
🪛 Ruff (0.12.2)
tests/integration/test_cli_end_to_end.py
1-1: Shebang is present but file is not executable
(EXE001)
tests/test_password_notification_lambda.py
1-1: Shebang is present but file is not executable
(EXE001)
tests/test_secret_key_rotation_s3.py
151-151: Unused method argument: UserName
(ARG002)
154-154: Unused method argument: UserName
(ARG002)
tests/test_terraform_validation.py
1-1: Shebang is present but file is not executable
(EXE001)
19-19: subprocess call: check for execution of untrusted input
(S603)
20-20: Starting a process with a partial executable path
(S607)
tests/test_password_reset.py
291-291: Unused method argument: mock_validate
(ARG002)
292-292: Unused method argument: mock_get_user
(ARG002)
294-294: Unused method argument: mock_passwordgen
(ARG002)
295-295: Unused method argument: mock_getpass
(ARG002)
322-322: Unused method argument: mock_validate
(ARG002)
322-322: Unused method argument: mock_get_user
(ARG002)
344-344: Unused method argument: mock_validate
(ARG002)
357-357: Unused method argument: mock_validate
(ARG002)
357-357: Unused method argument: mock_get_user
(ARG002)
379-379: Unused method argument: mock_validate
(ARG002)
380-380: Unused method argument: mock_get_user
(ARG002)
382-382: Unused method argument: mock_passwordgen
(ARG002)
383-383: Unused method argument: mock_getpass
(ARG002)
418-418: Unused method argument: mock_validate
(ARG002)
419-419: Unused method argument: mock_get_user
(ARG002)
421-421: Unused method argument: mock_passwordgen
(ARG002)
422-422: Unused method argument: mock_getpass
(ARG002)
🔇 Additional comments (12)
tests/requirements.txt (1)
6-7: LGTM! Type stubs addition aligns with mypy configurationThe addition of
types-python-dateutil>=2.8.19provides type hints for thepython-dateutillibrary, which complements the new mypy configuration. The coverage line appears unchanged despite the diff formatting.scripts/requirements.txt (1)
15-16: LGTM! Security scanning with Bandit addedThe addition of
bandit>=1.7.0for security analysis is a valuable addition to the development toolchain. The Black entry remains unchanged despite the diff formatting.pytest.ini (2)
15-15: Consider the coverage threshold implicationsSetting
--cov-fail-under=85enforces high test coverage, which is excellent. However, ensure the team is prepared for builds to fail if coverage drops below this threshold during development.
11-11: Resolved —lambda/directory present
--cov=lambdais valid; repository contains a top-levellambda/directory.tests/test_compliance_report.py (1)
203-211: Good defensive programming for malformed CSV dataThis regression test ensures the code gracefully handles incomplete CSV rows without crashing. This is important for robustness when dealing with potentially malformed credential reports.
tests/test_terraform_validation.py (1)
45-46: Consider usingshutil.whichconsistentlyGood use of
shutil.whichto check for Terraform. This is the Pythonic way to check for executable availability.tests/test_password_notification_lambda.py (1)
94-96: Well-structured mocking approachGood use of nested MonkeyPatch contexts to cleanly manage multiple patches. The approach of capturing calls while still invoking the original function is a solid testing pattern.
tests/test_password_reset.py (2)
269-437: Comprehensive test coverage for main workflowExcellent addition of the
TestMainFlowclass with thorough coverage of success and error scenarios. The tests properly mock all external dependencies and verify expected behavior.
365-402: Good error handling test coverageThe test properly verifies that policy violations are logged to the audit file. This ensures compliance and security requirements are met.
tests/test_secret_key_rotation_s3.py (2)
26-63: LGTM: solid unit test coverage of happy path with precise assertions.Mocks capture all critical calls; assertions verify S3 hardening and IAM deactivation.
143-186: No change required — CLI logs an ERROR-level message.
The script calls _logger.error("AWS error: %s", exc) in the ClientError handler (scripts/aws_iam_secret_key_rotation_s3.py), so assertLogs(level="ERROR") and the test's check for "AWS error" are correct.tests/integration/test_cli_end_to_end.py (1)
45-47: Resolved — CLI uses boto3.client so the test patch is effective.
scripts/aws_iam_self_service_key_rotation.py calls boto3.client("iam") at line 42, so patch("boto3.client", ...) will intercept the call.
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68cc1b444a34833294619cb0df8e9442
Summary by Sourcery
Expand and strengthen testing coverage and validation infrastructure, and update tooling requirements and static typing configuration.
Enhancements:
Tests:
Chores:
Summary by CodeRabbit
Tests
Chores