From 6068c2f515559723ed97e7f1f5ac783fac674299 Mon Sep 17 00:00:00 2001 From: Karan Jhaveri Date: Mon, 20 Jul 2026 22:21:02 +0800 Subject: [PATCH] test(unit): fix cross_account pollution + standardize unit-test setup test_lambda_runtime_tool.py replaced shared.cross_account in sys.modules at module scope and never restored it, breaking all of test_cross_account.py in a full `make test-unit` run. Replace with a scoped `cross_account_stub` fixture (monkeypatch.setitem, auto-restored). Also standardize the unit suite so this class of drift is less likely: - add tests/unit/conftest.py: centralizes deterministic AWS test env (region + dummy creds at import time) and auto-applies the `unit` marker to the whole package, so `pytest -m unit` selects all of it. - drop now-redundant per-file AWS_REGION setdefaults + the lone pytestmark. - docs/development.md: add a "Testing Conventions" section codifying handler loading, the no-global-sys.modules-stub rule, env handling, moto-vs-MagicMock, and markers. 488 unit+topology tests pass. --- docs/development.md | 11 +++++ tests/unit/conftest.py | 47 +++++++++++++++++++ tests/unit/test_health_events_collector.py | 3 +- tests/unit/test_lambda_runtime_tool.py | 52 +++++++++++++++------- tests/unit/test_tag_governance_tool.py | 4 +- tests/unit/test_topology.py | 3 +- 6 files changed, 99 insertions(+), 21 deletions(-) create mode 100644 tests/unit/conftest.py diff --git a/docs/development.md b/docs/development.md index 9e91b6e..8b60430 100644 --- a/docs/development.md +++ b/docs/development.md @@ -105,6 +105,17 @@ These three rules prevent the most common behavioral regressions: `DEPLOY_MODE` in `.env` gates build phases via `DEPLOY_FLAG_*` flags and maps to Terraform `deploy_*` vars. Modes: `full` (default), `agents-only` (skips Next.js/S3/CloudFront), `gateway-only`, `tools-only`. `DEPLOY_TOOLS` filters Lambda tools by `tools.json` keys. `GATEWAY_AUTH` is `iam` (default) or `oauth` (adds Cognito JWT authorizer for external clients). +## Testing Conventions + +Unit tests live in `tests/unit/` (run by `make test-unit` alongside `tests/topology/`); moto/DynamoDB fixtures are shared via `tests/conftest.py`; package-wide setup is in `tests/unit/conftest.py`. Follow these so new tests don't drift: + +- **Loading a Lambda handler**: use `importlib.util.spec_from_file_location(...)` + `exec_module` under a **namespaced** module name (e.g. `lambda_runtime_handler`, not `handler`) so multiple `handler.py` files don't collide. Reference: `tests/unit/test_tag_governance_tool.py`. +- **NEVER stub `shared.*` (or any real package) via a module-scope `sys.modules[...] = MagicMock()`.** It never gets removed and pollutes every later test file (this once broke all of `test_cross_account.py`). Handlers import `shared.cross_account` **lazily inside functions**, so `exec_module` does not need it stubbed to import. If a test exercises that path, stub it in a **fixture** with `monkeypatch.setitem(sys.modules, ...)` (auto-restored) — see the `cross_account_stub` fixture in `test_lambda_runtime_tool.py`. +- **AWS env**: don't set `AWS_REGION`/credentials per file. `tests/unit/conftest.py` sets deterministic region + dummy creds for the whole package at import time. Only set genuinely test-specific env (e.g. a tool's own `*_TABLE_NAME`) in the file, and prefer `monkeypatch.setenv` inside a test over module-scope mutation. +- **Mocking**: DynamoDB-heavy tests use **moto** (`mock_aws` via the shared fixtures); handler-routing/logic tests use **`MagicMock` + `monkeypatch`** on the handler's own seams (e.g. `monkeypatch.setattr(handler, "_get_lambda_client", ...)`). Don't reach into `sys.modules` to patch — patch the handler reference. +- **Markers**: `tests/unit/conftest.py` auto-applies the `unit` marker to everything under `tests/unit/`, so `pytest -m unit` selects the whole suite. No per-file `pytestmark` needed. +- **Prove a regression test fails on the bug first** (stash the fix → test fails → restore → passes) — see the report-mode example in `temp/PR-PLAYBOOK.md`. + ## Observability Runtime → X-Ray trace delivery and runtime → CWL application-log delivery are wired in `terraform/modules/core/observability/main.tf`. Spans land in `aws/spans`; application logs vend to `/aws/vendedlogs/bedrock-agentcore/runtime/{project}`. Runtime execution roles (both `agent-runtime-base` and `agentcore-runtime`) grant four X-Ray actions — `PutTraceSegments`, `PutTelemetryRecords`, `GetSamplingRules`, `GetSamplingTargets`. The two `Get*` are load-bearing: ADOT's sampler calls them every ~10s; without them spans get silently dropped. diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py new file mode 100644 index 0000000..1231b89 --- /dev/null +++ b/tests/unit/conftest.py @@ -0,0 +1,47 @@ +"""Shared setup for the unit-test package. + +Two things are centralized here so individual test modules don't each +re-implement them (which is how conventions drift — see the sys.modules +pollution that once lived in test_lambda_runtime_tool.py): + +1. Deterministic AWS env for import time. Several Lambda handlers read + AWS_REGION (and clients like MemoryClient default to us-west-2 if it is + unset — see CLAUDE.md), and they are imported at test-collection time via + importlib. conftest.py is imported BEFORE the test modules in its + directory, so setting these at module scope here guarantees they are in + place before any handler import. Dummy credentials ensure a stray boto3 + call can never reach a real account from the unit suite. + +2. Automatic `unit` marker. Every test under tests/unit/ is a unit test, so + rather than repeat `pytestmark = pytest.mark.unit` in 15 files (only one + did), we tag them all here. This makes `pytest -m unit` actually select + the whole unit suite. +""" + +import os + +import pytest + +# --- 1. Deterministic AWS env (module scope → runs at conftest import, +# before test modules import their handlers) -------------------------- +os.environ.setdefault("AWS_REGION", "us-east-1") +os.environ.setdefault("AWS_DEFAULT_REGION", "us-east-1") +# Dummy creds so no unit test can accidentally hit a real account. setdefault +# means a developer who has real creds exported still gets them overridden +# only if unset — but the unit suite never makes live calls, so this is a +# backstop, not a dependency. +os.environ.setdefault("AWS_ACCESS_KEY_ID", "testing") +os.environ.setdefault("AWS_SECRET_ACCESS_KEY", "testing") +os.environ.setdefault("AWS_SESSION_TOKEN", "testing") + + +# --- 2. Auto-apply the `unit` marker to everything in this package ---------- +def pytest_collection_modifyitems(config, items): + """Tag every test collected under tests/unit/ with the `unit` marker, + so `pytest -m unit` selects the full suite without each file having to + declare `pytestmark = pytest.mark.unit`. + """ + unit_root = os.path.dirname(__file__) + for item in items: + if str(item.fspath).startswith(unit_root): + item.add_marker(pytest.mark.unit) diff --git a/tests/unit/test_health_events_collector.py b/tests/unit/test_health_events_collector.py index 76ae617..50d7a80 100644 --- a/tests/unit/test_health_events_collector.py +++ b/tests/unit/test_health_events_collector.py @@ -26,8 +26,9 @@ _REPO_ROOT = Path(__file__).resolve().parents[2] +# AWS_REGION is set for the whole unit package in tests/unit/conftest.py. +# These two are test-specific config the collector reads at import time. os.environ.setdefault("HEALTH_EVENTS_TABLE_NAME", "test-table") -os.environ.setdefault("AWS_REGION", "us-east-1") # Disable LLM by default so tests don't hit Bedrock. Individual tests opt in. os.environ["ENRICHMENT_MODEL_ID"] = "" diff --git a/tests/unit/test_lambda_runtime_tool.py b/tests/unit/test_lambda_runtime_tool.py index 3ba3dd8..2c6e784 100644 --- a/tests/unit/test_lambda_runtime_tool.py +++ b/tests/unit/test_lambda_runtime_tool.py @@ -30,20 +30,25 @@ _REPO_ROOT = Path(__file__).resolve().parents[2] -# Default region before importing the handler. -os.environ.setdefault("AWS_REGION", "us-east-1") +# AWS_REGION is set for the whole unit package in tests/unit/conftest.py +# (imported before this module), so the handler sees it at import time. # Load the handler under a namespaced module name so it doesn't collide with -# other `handler.py` modules. +# other `handler.py` modules (network-resilience, collectors, etc.). +# +# NOTE: the handler imports `shared.cross_account` LAZILY (inside functions, +# not at module scope), so exec_module succeeds without stubbing `shared`. +# Tests that exercise those code paths request the `cross_account_stub` +# fixture below — we must NOT install a global sys.modules["shared.cross_account"] +# here, because a module-scope stub that is never removed pollutes the whole +# test session and breaks the real tests/unit/test_cross_account.py. _HANDLER_PATH = _REPO_ROOT / "src" / "lambda" / "mcp" / "lambda-runtime" / "handler.py" _spec = importlib.util.spec_from_file_location("lambda_runtime_handler", _HANDLER_PATH) handler = importlib.util.module_from_spec(_spec) - -# Mock the shared.cross_account import that happens at module level -sys.modules["shared"] = MagicMock() -sys.modules["shared.cross_account"] = MagicMock() _spec.loader.exec_module(handler) -# Register the module so patch() can resolve it +# Register the module under its own name so patch("lambda_runtime_handler...") +# targets resolve. This is namespaced to this handler and does not shadow any +# real package, so it is safe to leave in sys.modules. sys.modules["lambda_runtime_handler"] = handler @@ -52,6 +57,23 @@ # --------------------------------------------------------------------------- +@pytest.fixture +def cross_account_stub(monkeypatch): + """Provide a scoped stub for `shared.cross_account` for the tests that + exercise the handler's lazy `from shared.cross_account import get_aws_client`. + + monkeypatch.setitem restores the previous sys.modules state (including + absence) on teardown, so this never leaks into other test files — unlike + a bare module-level `sys.modules["shared.cross_account"] = MagicMock()`. + Returns the stub module so the test can set `get_aws_client` on it. + """ + shared_pkg = sys.modules.get("shared") or MagicMock() + stub = MagicMock() + monkeypatch.setitem(sys.modules, "shared", shared_pkg) + monkeypatch.setitem(sys.modules, "shared.cross_account", stub) + return stub + + def _make_context(tool_name: str) -> SimpleNamespace: """Build the AgentCore Gateway context shape the dispatcher reads.""" return SimpleNamespace( @@ -446,7 +468,7 @@ def mock_client_factory(region=None): class TestDiscoverLambdaRegions: - def test_success(self, monkeypatch): + def test_success(self, monkeypatch, cross_account_stub): """Discovers regions with Lambda functions.""" mock_ec2 = MagicMock() mock_ec2.describe_regions.return_value = { @@ -474,17 +496,15 @@ def mock_get_client(service, region_name=None): client.get_paginator.return_value = paginator return client - # Patch the shared.cross_account.get_aws_client used inside + # Stub the shared.cross_account.get_aws_client the handler imports lazily. + cross_account_stub.get_aws_client = mock_get_client with patch("lambda_runtime_handler.boto3"): - monkeypatch.setattr( - sys.modules["shared.cross_account"], "get_aws_client", mock_get_client - ) result = handler.handle_discover_lambda_regions({}) assert result["total_functions"] == 2 assert result["total_regions"] == 1 # only us-east-1 has functions - def test_region_scan_error_handled(self, monkeypatch): + def test_region_scan_error_handled(self, monkeypatch, cross_account_stub): """A region that throws during scan doesn't crash discovery.""" mock_ec2 = MagicMock() mock_ec2.describe_regions.return_value = { @@ -496,10 +516,8 @@ def mock_get_client(service, region_name=None): return mock_ec2 raise Exception("Connection timeout") + cross_account_stub.get_aws_client = mock_get_client with patch("lambda_runtime_handler.boto3"): - monkeypatch.setattr( - sys.modules["shared.cross_account"], "get_aws_client", mock_get_client - ) result = handler.handle_discover_lambda_regions({}) # Should return empty results, not crash diff --git a/tests/unit/test_tag_governance_tool.py b/tests/unit/test_tag_governance_tool.py index 0d7dd81..342c0e9 100644 --- a/tests/unit/test_tag_governance_tool.py +++ b/tests/unit/test_tag_governance_tool.py @@ -30,8 +30,8 @@ _REPO_ROOT = Path(__file__).resolve().parents[2] -# Default region before importing the handler (it reads AWS_REGION at import). -os.environ.setdefault("AWS_REGION", "us-east-1") +# AWS_REGION is set for the whole unit package in tests/unit/conftest.py +# (imported before this module), so the handler sees it at import time. # Load the handler under a namespaced module name so it doesn't collide with # other `handler.py` modules (network-resilience, collectors, etc.). diff --git a/tests/unit/test_topology.py b/tests/unit/test_topology.py index daa41ea..d620b74 100644 --- a/tests/unit/test_topology.py +++ b/tests/unit/test_topology.py @@ -35,7 +35,8 @@ import pytest -pytestmark = pytest.mark.unit +# The `unit` marker is applied to every test under tests/unit/ by +# tests/unit/conftest.py — no per-file pytestmark needed. _REPO_ROOT = Path(__file__).resolve().parents[2] _HIERARCHY_PATH = _REPO_ROOT / "src" / "agents" / "hierarchy.json"