Implement infrastructure integration testing suite using LocalStack#5190
Open
arkid15r wants to merge 3 commits into
Open
Implement infrastructure integration testing suite using LocalStack#5190arkid15r wants to merge 3 commits into
arkid15r wants to merge 3 commits into
Conversation
…tack and Terraform (#5122) * feat: implement infrastructure integration testing suite using LocalStack and Terraform Signed-off-by: Nachiket Roy <nachiket.roy.2@gmail.com> fix: align AWS provider version constraint to ~> 6.36.0 across storage modules Signed-off-by: Nachiket Roy <nachiket.roy.2@gmail.com> Revert "fix: align AWS provider version constraint to ~> 6.36.0 across storage modules" This reverts commit 8403b28. fix: bump AWS provider to ~> 6.53.0 across all modules and address review feedback Signed-off-by: Nachiket Roy <nachiket.roy.2@gmail.com> * Guard against an empty LocalStack image tag before starting the container Signed-off-by: Nachiket Roy <nachiket.roy.2@gmail.com> * fix: correct region identifier usage in KMS policy and document TF_PLUGIN_CACHE_DIR requirement Signed-off-by: Nachiket Roy <nachiket.roy.2@gmail.com> * refactor: consolidate override file paths and resource names into a single array for easier management Signed-off-by: Nachiket Roy <nachiket.roy.2@gmail.com> * chore: update LocalStack Dockerfile to run as non-root user Signed-off-by: Nachiket Roy <nachiket.roy.2@gmail.com> * Update code * refactor: replace shell-based test and tag scripts with a consolidated Python implementation Signed-off-by: Nachiket Roy <nachiket.roy.2@gmail.com> * extract LocalStack readiness check into a helper function Signed-off-by: Nachiket Roy <nachiket.roy.2@gmail.com> * reduced the cognitive complexity of each function Signed-off-by: Nachiket Roy <nachiket.roy.2@gmail.com> * refactor: replace sys.exit calls with TestRunnerError and improve localstack container management Signed-off-by: Nachiket Roy <nachiket.roy.2@gmail.com> * Update code * Update permissions --------- Signed-off-by: Nachiket Roy <nachiket.roy.2@gmail.com> Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org>
github-actions
Bot
added
docs
Contributor
WalkthroughInfrastructure testing is reorganized around reusable Terraform and Poetry actions, Dockerized unit and LocalStack integration environments, Python-based orchestration, new Terraform module integration tests, and expanded CI workflow coverage. ChangesInfrastructure testing
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5190 +/- ##
=======================================
Coverage 98.86% 98.86%
=======================================
Files 536 536
Lines 17072 17072
Branches 2451 2451
=======================================
Hits 16878 16878
Misses 97 97
Partials 97 97
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 39 files
Confidence score: 5/5
- In
infrastructure/scripts/commands.py, command execution changes currently lack direct regression tests, so argv/subprocess option drift could silently break Terraform or Docker calls in a future edit; this looks low risk for now, but add mockedsubprocess.runtests around resolved executable/argument construction before or immediately after merging to de-risk follow-on regressions.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="infrastructure/scripts/commands.py">
<violation number="1" location="infrastructure/scripts/commands.py:30">
P3: Command execution has no direct regression coverage, so changes to argv construction or subprocess options can break Terraform/Docker invocation undetected. Add mocked `subprocess.run` coverage for resolved executable, arguments, and both options.
(Based on your team's feedback about tests for new utility logic.) [FEEDBACK_USED]</violation>
</file>
Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.
Re-trigger cubic
| ) -> subprocess.CompletedProcess[str]: | ||
| """Run an executable resolved from PATH with fixed argv arguments.""" | ||
| executable = self.require(command) | ||
| return subprocess.run( |
Contributor
There was a problem hiding this comment.
P3: Command execution has no direct regression coverage, so changes to argv construction or subprocess options can break Terraform/Docker invocation undetected. Add mocked subprocess.run coverage for resolved executable, arguments, and both options.
(Based on your team's feedback about tests for new utility logic.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At infrastructure/scripts/commands.py, line 30:
<comment>Command execution has no direct regression coverage, so changes to argv construction or subprocess options can break Terraform/Docker invocation undetected. Add mocked `subprocess.run` coverage for resolved executable, arguments, and both options.
(Based on your team's feedback about tests for new utility logic.) </comment>
<file context>
@@ -0,0 +1,35 @@
+ ) -> subprocess.CompletedProcess[str]:
+ """Run an executable resolved from PATH with fixed argv arguments."""
+ executable = self.require(command)
+ return subprocess.run(
+ [executable, *args],
+ check=check,
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docker/infrastructure/Dockerfile.tests`:
- Around line 4-6: Update the Terraform base image declaration in the terraform
stage of Dockerfile.tests from hashicorp/terraform:1.14.6 to
hashicorp/terraform:1.15.8, leaving the Python and LocalStack image pins
unchanged.
In `@infrastructure/Makefile`:
- Around line 50-51: Update the test-infrastructure-integration recipe around
build-infrastructure-test-image to stop immediately when the image build fails,
replacing the unconditional command sequencing so status=0 and subsequent
compose execution cannot overwrite or obscure the original build error.
In `@infrastructure/scripts/runner.py`:
- Around line 91-94: Update the runner’s finally block so self.localstack.stop()
executes in a nested finally after self.overrides.cleanup(), while retaining the
localstack_started guard. Ensure unexpected cleanup failures still propagate
without preventing shutdown of a LocalStack instance started by the runner.
In `@infrastructure/scripts/terraform_tests.py`:
- Around line 86-110: Update run_module_tests so both Terraform
self.commands.run invocations use capture_output=True, then append each result’s
stderr to its corresponding TestRunnerError message while retaining the module
directory context.
In `@infrastructure/tests/scripts/localstack_test.py`:
- Around line 62-104: Add negative-path tests alongside test_image_info_pro,
test_image_info_standard, and test_start_success. Cover LocalStack.image_info()
raising the expected errors for a missing Dockerfile, no matching FROM line, and
an unparsable tag; also verify LocalStack.start() raises MissingAuthTokenError
when LOCALSTACK_AUTH_TOKEN is absent and TestRunnerError when CommandRunner.run
fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cca63467-7470-4f2e-9ca8-47b1c235b5e7
⛔ Files ignored due to path filters (1)
infrastructure/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (38)
.github/actions/apply-infrastructure-changes/action.yaml.github/actions/install-infrastructure-dependencies/action.yaml.github/actions/setup-terraform-environment/action.yaml.github/workflows/ci.yaml.github/workflows/run-code-checks.yaml.github/workflows/run-code-tests.yaml.github/workflows/run-infrastructure-tests.yaml.gitignore.trivyignore.yamlMakefilecspell/custom-dict.txtdocker-compose/infrastructure/compose.yamldocker/infrastructure/Dockerfile.testsdocker/localstack/Dockerfileinfrastructure/Makefileinfrastructure/README.mdinfrastructure/modules/ecr-cache/tests/integration.tftest.hclinfrastructure/modules/kms/tests/integration.tftest.hclinfrastructure/modules/parameters/tests/integration.tftest.hclinfrastructure/modules/storage/modules/s3-bucket/README.mdinfrastructure/modules/storage/modules/s3-bucket/outputs.tfinfrastructure/modules/storage/tests/integration.tftest.hclinfrastructure/poetry.tomlinfrastructure/pyproject.tomlinfrastructure/scripts/__init__.pyinfrastructure/scripts/commands.pyinfrastructure/scripts/errors.pyinfrastructure/scripts/localstack.pyinfrastructure/scripts/run_tests.pyinfrastructure/scripts/runner.pyinfrastructure/scripts/terraform_tests.pyinfrastructure/tests/__init__.pyinfrastructure/tests/scripts/__init__.pyinfrastructure/tests/scripts/commands_test.pyinfrastructure/tests/scripts/localstack_test.pyinfrastructure/tests/scripts/run_tests_test.pyinfrastructure/tests/scripts/runner_test.pyinfrastructure/tests/scripts/terraform_tests_test.py
|
Contribution validation failed:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
infrastructure/scripts/runner.py (1)
97-100: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard
stop()againstcleanup()failures to prevent container leaks.This concern was raised previously and remains unaddressed. If
self.overrides.cleanup()raises an unexpected (non-OSError) exception,self.localstack.stop()is skipped, leaving the container running. A nestedtry/finallyensures the container is always stopped when the runner started it.🔒 Proposed fix for finally block ordering
finally: - self.overrides.cleanup() - if localstack_started: - self.localstack.stop() + try: + self.overrides.cleanup() + finally: + if localstack_started: + self.localstack.stop()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infrastructure/scripts/runner.py` around lines 97 - 100, Update the cleanup logic in the runner’s finally block so self.localstack.stop() is executed in a nested finally whenever localstack_started is true, even if self.overrides.cleanup() raises an unexpected exception. Preserve the existing cleanup call and conditional container-stop behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@infrastructure/tests/scripts/commands_test.py`:
- Around line 63-69: In test_run_missing_command, instantiate CommandRunner
before entering the pytest.raises block, then invoke only its run method inside
the block while preserving the existing mocks and assertion that subprocess.run
is not called.
In `@infrastructure/tests/scripts/localstack_test.py`:
- Around line 87-91: Hoist LocalStack() construction outside each pytest.raises
block in the five flagged tests: test_image_info_missing_dockerfile, the
adjacent image_info error tests, and the start-related tests. Keep only the
intended image_info(...) or start(...) invocation inside pytest.raises so the
expected exception source is unambiguous.
In `@infrastructure/tests/scripts/terraform_tests_test.py`:
- Around line 70-82: Hoist the TerraformTests construction out of the
pytest.raises blocks in both init-failure tests: assign TerraformTests(commands)
to a local variable before each assertion, then invoke run_module_tests on that
variable inside pytest.raises while preserving the existing assertions.
---
Duplicate comments:
In `@infrastructure/scripts/runner.py`:
- Around line 97-100: Update the cleanup logic in the runner’s finally block so
self.localstack.stop() is executed in a nested finally whenever
localstack_started is true, even if self.overrides.cleanup() raises an
unexpected exception. Preserve the existing cleanup call and conditional
container-stop behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 22626072-f441-476b-b2bd-80b5ac174290
📒 Files selected for processing (12)
.github/workflows/run-infrastructure-tests.yamldocker/infrastructure/Dockerfile.testsinfrastructure/Makefileinfrastructure/scripts/localstack.pyinfrastructure/scripts/run_tests.pyinfrastructure/scripts/runner.pyinfrastructure/scripts/terraform_tests.pyinfrastructure/tests/scripts/commands_test.pyinfrastructure/tests/scripts/localstack_test.pyinfrastructure/tests/scripts/run_tests_test.pyinfrastructure/tests/scripts/runner_test.pyinfrastructure/tests/scripts/terraform_tests_test.py
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 12 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Merged
4 tasks
|
Contributor
There was a problem hiding this comment.
0 issues found across 5 files (changes from recent commits).
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
arkid15r
marked this pull request as ready for review
July 12, 2026 20:09
Collaborator
Author
|
@Nachiket-Roy @rudransh-shrivastava could you review this one? Thank you! |
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| main() |
Contributor
There was a problem hiding this comment.
We should add an unit test to assert try/finally cleanup execution & order
| """Raise if any override file already exists on disk.""" | ||
| for filepath, _ in self.overrides: | ||
| if Path(filepath).exists(): | ||
| raise OverrideExistsError(filepath) |
Contributor
There was a problem hiding this comment.
The makefile clean the temporary overrides files should it be added here too? to be safe?
|
|
||
| def find_test_dirs(self) -> list[str]: | ||
| """Return sorted ``tests`` directories under the search roots.""" | ||
| test_dirs: list[str] = [] |
Contributor
There was a problem hiding this comment.
We should prune .terraform from dirs in-place to avoid traversing provider cache trees and slowing down test runs.
| localstack.start.assert_not_called() | ||
| localstack.wait_ready.assert_called_once() | ||
| localstack.stop.assert_not_called() | ||
| terraform_tests.discover_and_run.assert_called_once_with("integration") |
Contributor
There was a problem hiding this comment.
No test verifies that discover_and_run() correctly raises a TestRunnerError if no tests are found/matched.
| cache_dir = Path.home() / ".terraform.d" / "plugin-cache" | ||
| try: | ||
| cache_dir.mkdir(parents=True, exist_ok=True) | ||
| os.environ["TF_PLUGIN_CACHE_DIR"] = str(cache_dir) |
Contributor
There was a problem hiding this comment.
We should check if TF_PLUGIN_CACHE_DIR is already set in the environment before unconditionally overwriting it.
| skip_metadata_api_check = true | ||
| skip_requesting_account_id = true | ||
| endpoints { | ||
| ecr = "http://localhost:4566" |
Collaborator
There was a problem hiding this comment.
I'm curious if its possible to use tflocal to run these tests instead of hardcoding http://localhost:4566
Contributor
There was a problem hiding this comment.
Hey, I checked out tflocal. It relies on generated override files and adds another dependency, whereas we can use AWS_ENDPOINT_URL which is supported natively by the AWS provider. Check it out and LMK
Contributor
There was a problem hiding this comment.
arkid15r
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed change
Merge infrastructure integration branch into
main.Checklist