Switch tests to pytest #5
Conversation
Reviewer's GuideMigrates the project’s test suite from a custom runner to pytest by removing the legacy runner, updating scripts and CI config, and refreshing documentation to use pytest commands. Flow diagram for test execution process migration to pytestflowchart TD
A[Start] --> B[Remove legacy test runner]
B --> C[Update documentation to use pytest]
C --> D[Update CI workflow to call pytest]
D --> E[Update integration test script to use pytest]
E --> F[End]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughThe testing workflow was updated to use 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 and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `README.md:327` </location>
<code_context>
# Check for hardcoded secrets
-python tests/run_tests.py # Includes security checks
+pytest -m integration --collect-only # Triggers security fixtures
```
</code_context>
<issue_to_address>
The comment about triggering security fixtures may be misleading.
The '--collect-only' flag does not execute tests or fixtures. Please update the comment to accurately reflect this behavior.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
pytest -m integration --collect-only # Triggers security fixtures
=======
pytest -m integration --collect-only # Lists integration tests without executing them or their fixtures
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| # Check for hardcoded secrets | ||
| python tests/run_tests.py # Includes security checks | ||
| pytest -m integration --collect-only # Triggers security fixtures |
There was a problem hiding this comment.
suggestion: The comment about triggering security fixtures may be misleading.
The '--collect-only' flag does not execute tests or fixtures. Please update the comment to accurately reflect this behavior.
| pytest -m integration --collect-only # Triggers security fixtures | |
| pytest -m integration --collect-only # Lists integration tests without executing them or their fixtures |
![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
🔭 Outside diff range comments (1)
.github/workflows/test.yml (1)
31-36:pytestwill not be found – install test-only deps before running it
pip install -r scripts/requirements.txtdoes not guarantee thatpytest(and any pytest plug-ins declared intests/requirements.txt) are available.
The step that immediately follows (pytest) will therefore fail on a fresh checkout.- pip install -r scripts/requirements.txt + pip install -r scripts/requirements.txt + # 🧪 Install testing requirements + if [ -f tests/requirements.txt ]; then + pip install -r tests/requirements.txt + else + # Fallback – at least ensure pytest itself is present + pip install pytest + fiAdding the extra install keeps the matrix green and avoids leaking this responsibility to the repo-wide
scripts/requirements.txt.Also applies to: 66-66
🧹 Nitpick comments (3)
run_tests.sh (1)
27-27: Prefer explicit marker expression quotingWhen passing marker expressions, wrap them in quotes so that shell word-splitting or glob-expansion cannot interfere on some platforms.
-pytest -m integration -v --cov=scripts --cov-report=term-missing +pytest -m "integration" -v --cov=scripts --cov-report=term-missingNot mandatory, but it prevents subtle bugs if a directory named
integrationever exists alongside the tests.AGENTS.md (1)
37-44: Unify integration-test invocation with the shell scriptEverywhere else (CI &
run_tests.sh) uses the marker formpytest -m integration.
For consistency (and to avoid accidentally skipping files outsidetests/integration/), consider aligning the docs:-```bash -pytest tests/integration -``` +```bash +pytest -m integration +```README.md (1)
312-318: Coverage command overlooks integration markerIf integration tests are slow, users may appreciate a coverage command that mirrors the default fast-unit-test invocation. Something like:
coverage run -m pytest -m "not integration"Consider clarifying this or explicitly stating that all tests run under coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/test.yml(1 hunks)AGENTS.md(1 hunks)README.md(3 hunks)run_tests.sh(1 hunks)tests/run_tests.py(0 hunks)
💤 Files with no reviewable changes (1)
- tests/run_tests.py
🧰 Additional context used
🧠 Learnings (2)
README.md (1)
Learnt from: CR
PR: tblakex01/iam-key-rotation#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-02T14:07:15.152Z
Learning: Applies to scripts/**/*.py : All scripts use boto3 for AWS API interactions.
AGENTS.md (3)
Learnt from: CR
PR: tblakex01/iam-key-rotation#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-02T14:07:15.152Z
Learning: Applies to terraform/iam/**/*.tf : Validate Terraform configuration syntax using 'terraform validate'.
Learnt from: CR
PR: tblakex01/iam-key-rotation#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-02T14:07:15.152Z
Learning: Applies to scripts/**/*.py : All scripts use boto3 for AWS API interactions.
Learnt from: CR
PR: tblakex01/iam-key-rotation#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-02T14:07:15.152Z
Learning: Applies to terraform/iam/**/*.tf : Terraform files must be formatted using 'terraform fmt -recursive'.
🔇 Additional comments (3)
run_tests.sh (1)
33-33: Nitpick: missing newline at EOF was fixed – good catchREADME.md (2)
327-328: Marker note – The comment says “Triggers security fixtures” but the command selectsintegrationtests.
If those fixtures are indeed attached to that marker, great; otherwise double-check the statement to avoid confusion.
445-445: Good updateDocumentation now points developers at the canonical
pytestentry point – no issues spotted.
Summary
pytestpytestpytestTesting
black .flake8 scripts/ lambda/ tests/ --max-line-length=120 --ignore=E501,W503,E203mypy scripts/ lambda/(fails: missing library stubs)bandit -r scripts/ lambda/pytest(fails: 16 failed, 116 passed)pytest tests/integrationhttps://chatgpt.com/codex/tasks/task_e_6865e633823c83329cd3b46fa7b119d3
Summary by Sourcery
Switch test suite to pytest, remove legacy test runner, and update scripts, CI, and documentation to use pytest
Enhancements:
CI:
Documentation:
Tests:
Summary by CodeRabbit
Documentation
pytestframework directly for running unit, integration, and security-related tests.Chores
pytestinstead of custom test runner scripts.