fix(e2e): resolve repo root correctly in deploy helpers#1982
fix(e2e): resolve repo root correctly in deploy helpers#19820xDevNinja wants to merge 2 commits into
Conversation
The three AWS deploy helpers under tests/e2e/.../infrastructure_sdk/ computed `Path(__file__).resolve().parents[3]`, which lands on <repo>/tests rather than the repo root. Asset paths were then built with a `tests/<scenario>/...` prefix that double-stacked into <repo>/tests/tests/... and broke Lambda bundling and Docker build context resolution before any cloud resources were provisioned. - parents[3] -> parents[4] in all three helpers - scenario-specific asset paths now resolve under tests/e2e/<scenario> - shared assets continue to resolve under tests/shared/... - prefect helper introduces an explicit E2E_DIR alongside TESTS_DIR so the shared-vs-scenario split is obvious at the path table Refs Tracer-Cloud#1418
Imports each deploy helper via importlib and asserts that `module.project_root` equals the actual repository root, and that the key bundled asset paths (Lambda zip sources, Docker contexts, Alloy config) exist on disk. If a future edit drifts `parents[N]` back to 3 or re-introduces a `tests/<scenario>` double-prefix, the test fails locally instead of in the middle of a multi-minute provisioning run. Refs Tracer-Cloud#1418
Greptile code reviewThis repo uses Greptile for automated review. Before merge, aim for Confidence Score: 5/5 with zero unresolved review threads — see CONTRIBUTING.md. Run a review — add a PR comment with: Give it ~5-10 minutes (sometimes longer) for results, then fix feedback and re-trigger until you reach Confidence Score: 5/5. Optional: automate with the greploop skill. |
Greptile SummaryThis PR fixes a one-level
Confidence Score: 4/5The deploy helper changes are straightforward and correct; the only rough edges are in the new regression test's coverage depth and its implicit dependency on the full e2e package set. The three root-path fixes are accurate and every affected path constant was updated consistently. The regression test correctly computes REPO_ROOT and will catch a future parents[N] drift. Two test design notes: it checks asset existence against independently-hardcoded paths rather than the module's own path attributes, and exec_module pulls in all transitive imports which could surface as opaque import errors in environments lacking the full e2e dependency set. tests/e2e/test_infrastructure_deploy_paths.py - the test's coverage model and import-coupling are worth a second look before expanding it to additional scenarios. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["deploy.py (depth 4 from repo root)"] -->|"Path(__file__).resolve()"| B["Absolute file path"]
B -->|".parents[4] (was .parents[3])"| C["project_root = repo root"]
C --> D["TESTS_DIR = project_root / tests"]
D --> E["Shared assets: tests/shared/external_vendor_api, tests/shared/infrastructure_code/alloy_config"]
C --> F["E2E_DIR = TESTS_DIR / e2e (new)"]
F --> G["Scenario-specific assets: tests/e2e/scenario/pipeline_code, tests/e2e/scenario/infrastructure_code/Dockerfile"]
Reviews (1): Last reviewed commit: "test(e2e): regression coverage for deplo..." | Re-trigger Greptile |
| for relative in expected_assets: | ||
| asset = REPO_ROOT / relative | ||
| assert asset.exists(), f"{scenario}: expected asset path missing on disk: {asset}" |
There was a problem hiding this comment.
The loop checks that each expected path exists on disk (computed independently via REPO_ROOT), but never compares those expected paths against the module's own path attributes. For upstream_prefect_ecs_fargate, module.PREFECT_DOCKERFILE and module.TRIGGER_LAMBDA_CODE are module-level constants that could drift (e.g., someone drops E2E_DIR back to TESTS_DIR for one of them) without this test catching it, as long as module.project_root stays correct and the on-disk asset still exists. Adding assertions like assert module.PREFECT_DOCKERFILE == REPO_ROOT / "tests/e2e/upstream_prefect_ecs_fargate/infrastructure_code/prefect_image/Dockerfile" for each scenario that exposes module-level path constants would close this gap.
| def _load_deploy_module(scenario: str, deploy_file: Path) -> ModuleType: | ||
| module_name = f"_deploy_helper_under_test_{scenario}" | ||
| spec = importlib.util.spec_from_file_location(module_name, deploy_file) | ||
| assert spec is not None and spec.loader is not None | ||
| module = importlib.util.module_from_spec(spec) | ||
| sys.modules[module_name] = module | ||
| try: | ||
| spec.loader.exec_module(module) | ||
| except Exception: | ||
| sys.modules.pop(module_name, None) | ||
| raise | ||
| return module |
There was a problem hiding this comment.
exec_module couples the test to all transitive imports
spec.loader.exec_module(module) executes the entire deploy file, including its top-level imports (from app.utils.config import load_env, from botocore.exceptions import ClientError, etc.). If any of those packages are absent or unavailable in a lightweight CI environment (unit-test-only jobs, thin containers), the test raises ImportError rather than a meaningful path-assertion failure, masking the actual purpose of the test. Consider wrapping the exec in a pytest.importorskip-style guard, or document explicitly that this test requires the full e2e dependency set so it isn't accidentally included in a restricted job.
|
Closing this — I should have searched closed PRs against #1418 before opening. I now see #1419 (approved + Greptile 5/5), #1930, and #1912/#1936 were all closed with the same |
Fixes #1418
Describe the changes you have made in this PR -
Three E2E AWS deploy helpers were computing
project_rootasPath(__file__).resolve().parents[3], which lands on<repo>/testsinstead of the actual repo root. Subsequent asset paths then double-stacked into<repo>/tests/tests/upstream_lambda/pipeline_codeand similar nonexistent locations, breaking Lambda code bundling and Docker build-context resolution before any cloud resources were provisioned.This PR:
parents[3]→parents[4]in all three deploy helpers:tests/e2e/upstream_lambda/infrastructure_sdk/deploy.pytests/e2e/upstream_prefect_ecs_fargate/infrastructure_sdk/deploy.pytests/e2e/upstream_apache_flink_ecs/infrastructure_sdk/deploy.pytests/e2e/<scenario>/...(Lambdapipeline_code, Prefect Dockerfile + trigger lambda, Flink Dockerfile).tests/shared/...unchanged.E2E_DIRalongsideTESTS_DIRin the Prefect helper so the shared-vs-scenario split is obvious from the path table.tests/e2e/test_infrastructure_deploy_paths.py— a regression test that imports each deploy helper via importlib, assertsmodule.project_rootequals the actual repository root, and verifies the bundled asset paths (Lambda code, Dockerfiles, Alloy config) exist on disk. Catchesparents[N]drift locally before CI burns minutes on cloud provisioning that will fail atiterdir().Acceptance criteria from #1418:
tests/shared/....tests/e2e/...directories.Demo/Screenshot for feature changes and bug fixes -
Before (current main,
parents[3]):Composed pipeline_dir then resolves to
/Users/.../opensre/tests/tests/upstream_lambda/pipeline_code— does not exist,iterdir()raises during bundling.After (this PR):
Full
make test-cov: 6512 passed, 6 skipped in 92s on this branch.Code Understanding and AI Usage
Did you use AI assistance (ChatGPT, Claude, Copilot, etc.) to write any part of this code?
Explain your implementation approach:
The bug is a single-character index drift compounded by a string-concatenation mistake. Each deploy helper lives at depth 4 from the repo root (
tests/e2e/<scenario>/infrastructure_sdk/deploy.py), soPath(__file__).resolve().parents[N]must useN=4to reach the repo, notN=3. Once that is correct, the asset-path strings still need to include thee2e/segment for scenario-specific files (Lambda zips, Docker contexts) while leaving shared paths (tests/shared/...) alone.I considered two alternatives before settling on this one:
Path(__file__).resolve().parents[N]with a smallerNand rebase asset paths off the helper directory directly. Rejected because it makes shared-asset paths read../../../shared/...from inside the helper, which is harder to grep for and inconsistent with the existing pattern.pyproject.toml). Rejected because it adds runtime cost and a layer of indirection for a fix that should be a one-line index change.The regression test loads each helper via
importlib.util.spec_from_file_locationrather than a normalfrom tests.e2e... importbecause the per-scenario directories do not have__init__.pyfiles. Loading by file path keeps the test self-contained and avoids touching package layout.Checklist before requesting a review