test(localstack): add integration tests for create-investigation Lambda - #202
Conversation
Add 6 integration tests that exercise the create-investigation Lambda handler against LocalStack, validating AWS-facing logic (ECS, EFS, STS) end-to-end. These complement existing unit tests that mock all AWS calls. Non-slow tests (4): - test_create_investigation_creates_efs_access_point: validates EFS AP path, POSIX user (UID/GID 1000), and tags via real EFS API - test_create_investigation_registers_task_definition: validates task def family pattern, EFS volume config, and baked env vars (CLUSTER_ID, INVESTIGATION_ID, OC_VERSION, TASK_TIMEOUT, S3_AUDIT_BUCKET) - test_skip_task_creates_access_point_only: validates skip_task creates AP but no task def or ECS task - test_idempotent_access_point_reuse: validates tag-based AP lookup returns same AP ID on second invocation Slow tests (2, require ECS_EXECUTOR != local): - test_create_investigation_launches_ecs_task: full e2e including tag roundtrip, deadline arithmetic, and startedBy verification - test_duplicate_investigation_returns_409: validates startedBy-based duplicate detection against real ECS list_tasks API Handler loading strategy: - Uses importlib with unique module names per call to handle module-level boto3 client creation - Sets AWS_ENDPOINT_URL before loading so clients connect to LocalStack - Patches validate_oidc_token to bypass Keycloak JWKS validation Tests cut from original spec as redundant with unit tests: - get_config (no AWS interaction, identical to unit test) - custom OC version (subset of task def registration test) - missing OIDC token / invalid cluster_id (input validation, no AWS) - response shape (pure Python formatting) - deadline tag arithmetic (merged into task launch test)
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughAdded a LocalStack integration-test suite for the create-investigation Lambda handler. The suite covers resource creation, task configuration, task launch metadata, idempotent reuse, skip_task behavior, cleanup, and duplicate-investigation responses. ChangesCreate-investigation integration coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
🤖 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 `@tests/localstack/integration/test_create_investigation.py`:
- Around line 154-156: Update the fixture resource-name generation around
cluster_name and base_task_family to use a collision-resistant single
per-instance identifier, such as one UUID or time.time_ns() value, instead of
int(time.time()). Apply that shared identifier consistently to all related ECS
cluster, task-definition family, and IAM role names in the fixture.
- Around line 481-489: Update the task-readiness polling loop in the
duplicate-detection test to call pytest.fail instead of pytest.skip when the
task never reaches RUNNING, ensuring ECS startup failures fail the test rather
than being treated as an unavailable environment.
🪄 Autofix
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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 6dff8e3e-ee83-49b3-9035-3f157891ac97
📒 Files selected for processing (1)
tests/localstack/integration/test_create_investigation.py
| ts = int(time.time()) | ||
| cluster_name = f'test-create-inv-{ts}' | ||
| base_task_family = f'rosa-boundary-base-{ts}' |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use collision-resistant fixture resource names.
int(time.time()) has one-second resolution. Concurrent or rapid test execution can create the same ECS cluster, task-definition family, and IAM role names. LocalStack can then return AlreadyExists and make the suite flaky. Use one UUID or time.time_ns() value for this fixture instance.
🤖 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 `@tests/localstack/integration/test_create_investigation.py` around lines 154 -
156, Update the fixture resource-name generation around cluster_name and
base_task_family to use a collision-resistant single per-instance identifier,
such as one UUID or time.time_ns() value, instead of int(time.time()). Apply
that shared identifier consistently to all related ECS cluster, task-definition
family, and IAM role names in the fixture.
| for _ in range(24): # 24 × 5s = 120s max | ||
| desc = ecs_client.describe_tasks( | ||
| cluster=handler_env['cluster_name'], tasks=[task_arn] | ||
| ) | ||
| if desc['tasks'][0].get('lastStatus') == 'RUNNING': | ||
| break | ||
| time.sleep(5) | ||
| else: | ||
| pytest.skip("Task never reached RUNNING — cannot test duplicate detection") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail when the task does not reach RUNNING.
After ECS_EXECUTOR != 'local' selects this test, a task that never starts means duplicate detection was not validated. pytest.skip() hides broken task configuration or ECS execution. Replace it with pytest.fail().
Proposed fix
else:
- pytest.skip("Task never reached RUNNING — cannot test duplicate detection")
+ pytest.fail("Task never reached RUNNING; duplicate detection was not tested")📝 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.
| for _ in range(24): # 24 × 5s = 120s max | |
| desc = ecs_client.describe_tasks( | |
| cluster=handler_env['cluster_name'], tasks=[task_arn] | |
| ) | |
| if desc['tasks'][0].get('lastStatus') == 'RUNNING': | |
| break | |
| time.sleep(5) | |
| else: | |
| pytest.skip("Task never reached RUNNING — cannot test duplicate detection") | |
| for _ in range(24): # 24 × 5s = 120s max | |
| desc = ecs_client.describe_tasks( | |
| cluster=handler_env['cluster_name'], tasks=[task_arn] | |
| ) | |
| if desc['tasks'][0].get('lastStatus') == 'RUNNING': | |
| break | |
| time.sleep(5) | |
| else: | |
| pytest.fail("Task never reached RUNNING; duplicate detection was not tested") |
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 481-481: Comment contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF003)
🤖 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 `@tests/localstack/integration/test_create_investigation.py` around lines 481 -
489, Update the task-readiness polling loop in the duplicate-detection test to
call pytest.fail instead of pytest.skip when the task never reaches RUNNING,
ensuring ECS startup failures fail the test rather than being treated as an
unavailable environment.
|
/hold Don't review yet - I'd like to spend more time reviewing this first, its almost entirely AI generated with some hand-holding. |
openshift-ci
Bot
added
the
do-not-merge/hold
|
@tiwillia: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
Summary
Add 6 LocalStack integration tests for the
create-investigationLambda handler, exercising AWS-facing logic (ECS, EFS, STS) end-to-end. These complement the existing unit tests that mock all AWS calls.Tests
Non-slow (4 tests,
@pytest.mark.integration)test_create_investigation_creates_efs_access_pointtest_create_investigation_registers_task_definitiontest_skip_task_creates_access_point_onlytest_idempotent_access_point_reuseSlow (2 tests,
@pytest.mark.integration @pytest.mark.slow, requireECS_EXECUTOR != local)test_create_investigation_launches_ecs_tasktest_duplicate_investigation_returns_409list_tasksAPIDesign decisions
importlibwith unique module names per call. The handler creates boto3 clients at module scope, soAWS_ENDPOINT_URLmust be set before loading. Each test gets a fresh module instance.validate_oidc_tokenis patched on the loaded module to return known-good claims. All other AWS interactions (ECS, EFS, STS) hit LocalStack directly.invoke_handlerhelper: Deduplicates the load/patch/invoke/parse/cleanup pattern across all tests. Each test body is pure assertion logic.register_investigation_task_definition()directly instead of throughlambda_handler()to avoidrun_taskside effects in the local ECS executor.Verification
make test-localstack-fast): 43 passed, 3 skipped, 0 failures — no regressionsSummary by CodeRabbit