Skip to content

Commit 17eba2f

Browse files
perf(test): disable Lambda bundling in CDK unit-test synths (~15x per synth) (#366) (#371)
* perf(test): disable Lambda bundling in CDK unit-test synths (#366) Template.fromStack() triggers a full CDK synth that bundles every NodejsFunction via esbuild — ~28s for the 413-resource AgentStack. Unit tests assert on CloudFormation structure, not bundled Lambda code, so the bundling is pure overhead. A jest setupFiles hook sets aws:cdk:bundling-stacks: [] via CDK_CONTEXT_JSON, which a bare new App() picks up with no call-site changes. Measured: single AgentStack synth 28.7s -> 1.9s (~15x); github-tags.test.ts ~135s -> 9s; full //cdk:test 155s -> 25s (8-core local). All 2177 tests pass; coverage thresholds unchanged (lines 92.36 >= 91). Durable regression guards added: AGENTS.md "Common mistakes" entry and a .abca/commands/review_pr.md checklist item — do not re-enable bundling in tests unless asserting on a bundled asset, and minimize full-stack synths (synth once in beforeAll). Both link docs/design/CI_BUILD_PERFORMANCE.md (PR #364). Refs #366. Part of #363. Likely supersedes the sharding approach (#365) — to be re-evaluated once this lands. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(test): correct bundling opt-out to postCliContext (#366) The documented per-test opt-out used constructor `context`, which CDK silently overwrites with CDK_CONTEXT_JSON in App.loadContext — so `new App({ context: { 'aws:cdk:bundling-stacks': ['**'] } })` does NOT re-enable bundling. Only `postCliContext` (applied last) overrides the global disable. Corrects the opt-out instruction and precedence comment in all three documented locations (disable-bundling.ts, AGENTS.md, review_pr.md). Verified empirically against aws-cdk-lib@2.257.0. Comment/doc-only; runtime logic unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(test): clarify bundling opt-out is for asset output, not synth (#366) Disabling bundling does not stop tests from synthesizing — `Template.fromStack()` still runs a full synth; it only skips the esbuild asset-bundling step. The `postCliContext` opt-out exists solely for the rare test that needs real bundled-asset output (asset hash / S3 key), where an unbundled synth would silently yield a placeholder. Makes this explicit in disable-bundling.ts and AGENTS.md. Doc-only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(test): drop dead CI_BUILD_PERFORMANCE.md links (#366) The doc lives in draft PR #364 and exists on neither this branch nor main, so the three links (disable-bundling.ts, AGENTS.md, review_pr.md) are dead. Drop them and keep the stable #366 reference, removing the merge-order dependency on #364 and the link-checker (#300) risk. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(test): harden bundling-disable against ambient CDK_CONTEXT_JSON (#366) Two robustness fixes to the global bundling-disable setup file, both from PR #371 review (finding 1 hardening nits): - Spread BUNDLING_DISABLED_CONTEXT LAST when merging a pre-set CDK_CONTEXT_JSON, so an ambient `aws:cdk:bundling-stacks` value can no longer silently win and re-enable bundling. This is safe precisely because the documented per-test opt-out is `postCliContext` (applied after the env var regardless), so the escape hatch still works. - Guard `JSON.parse` of the pre-set var with try/catch. A malformed value previously threw inside setupFiles and failed every test in every file with an opaque parse error; now the blame is localized to the offending value. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(cdk): add executable guard that Lambda bundling is disabled (#366) PR #371 review finding 1: the ~15× synth speedup rested entirely on the `disable-bundling.ts` setupFiles entry, but the only regression guards were honor-system prose (AGENTS.md) and a review_pr.md checklist item. Dropping the setupFiles wiring, reordering Jest setup, or a CDK rename of the context key would silently revert the suite to full-bundling synths with no failing check. Adds two assertions: the synth context carries `aws:cdk:bundling-stacks: []`, and a bare-App stack reports `bundlingRequired === false`. Verified empirically against aws-cdk-lib@2.259.0 that without the disable a bare App yields `bundlingRequired === true`, so these discriminate a real regression. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(cdk): cover the postCliContext bundling opt-out precedence (#366) PR #371 review finding 2: the documented per-test opt-out (`postCliContext`) and the `loadContext` precedence this PR corrected had zero test coverage, so a future CDK version reordering context application could silently break the one supported escape hatch. Locks the contract with two assertions verified against aws-cdk-lib@2.259.0: `postCliContext: { 'aws:cdk:bundling-stacks': ['**'] }` re-enables bundling (`bundlingRequired === true`), while constructor `context` does NOT (the env var set by the global disable clobbers it). Uses a bare `Stack` rather than a heavy AgentStack since the precedence is a property of CDK context loading, not of any particular stack. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(test): reframe bundling as overhead-plus-smoke-test, not "pure overhead" (#366) PR #371 review finding 3: "pure overhead" undersold the tradeoff. esbuild bundling also validates that every Lambda `entry` resolves and the handler compiles, so disabling it in unit tests means a broken entry path or handler TS error stops surfacing at `Template.fromStack()` and only fails at real synth/deploy or in the `agent/` build. The tradeoff is still acceptable — real synth/deploy bundles, and handler code has its own compile/test under `agent/` — but the wording now names what unit tests cede. Updated in both places the phrase lived: the disable-bundling.ts header and AGENTS.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: bgagent <345885+scottschreckengaust@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 97fe24c commit 17eba2f

5 files changed

Lines changed: 153 additions & 0 deletions

File tree

.abca/commands/review_pr.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,13 @@ Then apply principal-architect judgment over the diff:
9797
IDs, cdk-nag clean. Watch for cost and operational footguns.
9898
- **Tests** — Are unit tests added/updated under the matching `*/test/` tree? Do they cover the
9999
new behavior and failure paths, not just the happy path?
100+
- **Test performance (CDK synth)** — New/changed CDK tests must not re-enable Lambda bundling at
101+
synth or synthesize the same stack repeatedly. `cdk/` disables bundling globally via
102+
`test/setup/disable-bundling.ts` (~15× faster synth); flag any test that turns
103+
`aws:cdk:bundling-stacks` back on (only valid via `postCliContext`, not constructor
104+
`context` — the env var overwrites the latter) without asserting on a bundled asset, or
105+
that calls `new App()` + `Template.fromStack()` per-test instead of once in `beforeAll`.
106+
See #366.
100107
- **Routing** — Changes should land in the right package per the AGENTS.md routing table
101108
(agent runtime in `agent/`, API/Lambdas in `cdk/`, CLI in `cli/`).
102109

AGENTS.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ Handler entry tests: `cdk/test/handlers/orchestrate-task.test.ts`, `create-task.
4545
- Adding or editing files in **`docs/design/`** or **`docs/guides/`** without running **`cd docs && node scripts/sync-starlight.mjs`** — CI will reject ("Fail build on mutation") because the Starlight mirror files in `docs/src/content/docs/` are stale. Always commit the regenerated mirrors alongside source changes.
4646
- Changing **`cdk/.../types.ts`** without updating **`cli/src/types.ts`** — CLI and API drift.
4747
- Running raw **`jest`/`tsc`/`cdk`** from muscle memory — prefer **`mise //cdk:test`**, **`mise //cdk:compile`**, **`mise //cdk:synth`** (see [Commands you can use](#commands-you-can-use)).
48+
- **Bundling Lambda assets in CDK unit tests** — `Template.fromStack()` triggers a full synth that bundles every `NodejsFunction` via esbuild (~28s for `AgentStack`). Unit tests assert on CloudFormation structure, not bundled code, so for those assertions it is overhead — plus a smoke-test of Lambda `entry` resolution and handler compilation that unit tests intentionally cede to the `agent/` build (a broken entry path or handler TS error stops surfacing at `Template.fromStack()` and instead fails at real synth/deploy or in the `agent/` build). The `cdk/` Jest config disables it globally via `test/setup/disable-bundling.ts` (sets `aws:cdk:bundling-stacks: []` in `CDK_CONTEXT_JSON`), which a bare `new App()` picks up automatically. This does not stop tests from synthesizing — `Template.fromStack()` still runs a full synth; it only skips the esbuild step. **Do not re-enable bundling** in a test unless you are specifically asserting on the bundled-asset output (hash / S3 key) — and if you must, opt out narrowly via that test's `App` `postCliContext` (e.g. `new App({ postCliContext: { 'aws:cdk:bundling-stacks': ['**'] } })`), not globally. Note that constructor `context` does **not** work for this key: CDK overwrites it with `CDK_CONTEXT_JSON`, so only `postCliContext` (applied last) overrides the global disable. Minimize full-stack synths regardless: synthesize each distinct stack config once in `beforeAll` and assert against the cached `Template`. See #366.
4849
- **`MISE_EXPERIMENTAL=1`** — required for namespaced tasks like **`mise //cdk:build`** (see [CONTRIBUTING.md](./CONTRIBUTING.md)).
4950
- **`mise run build`** builds **`//agent:quality`** alongside **`//cdk:build`** (the deployed image bundles **`agent/`**, so agent quality is part of the build) — these run as parallel `depends`, not in a fixed order; agent changes belong in the **`agent/`** tree.
5051
- **`prek install`** fails if Git **`core.hooksPath`** is set — another hook manager owns hooks; see [CONTRIBUTING.md](./CONTRIBUTING.md).

cdk/package.json

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,9 @@
106106
"watchPathIgnorePatterns": [
107107
"/node_modules/"
108108
],
109+
"setupFiles": [
110+
"<rootDir>/test/setup/disable-bundling.ts"
111+
],
109112
"setupFilesAfterEnv": [
110113
"aws-cdk-lib/testhelpers/jest-autoclean"
111114
],
Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
/**
2+
* MIT No Attribution
3+
*
4+
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
5+
*
6+
* Permission is hereby granted, free of charge, to any person obtaining a copy of
7+
* the Software without restriction, including without limitation the rights to
8+
* use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of
9+
* the Software, and to permit persons to whom the Software is furnished to do so.
10+
*
11+
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
12+
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
13+
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
14+
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
15+
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
16+
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
17+
* SOFTWARE.
18+
*/
19+
20+
// Executable guard for the global Lambda-bundling disable (#366).
21+
//
22+
// The speedup in this package depends entirely on `test/setup/disable-bundling.ts`
23+
// running as a Jest `setupFiles` entry. Until now the only regression guards
24+
// were prose in AGENTS.md and a checklist item in review_pr.md — both
25+
// honor-system. If someone drops the `setupFiles` wiring, reorders Jest setup,
26+
// or a CDK rename breaks the context key, the suite silently reverts to a
27+
// full-bundling (~15× slower) synth with no failing check. These assertions
28+
// make that floor machine-enforced.
29+
import { App, Stack } from 'aws-cdk-lib';
30+
31+
describe('global Lambda bundling disable', () => {
32+
it('sets aws:cdk:bundling-stacks to [] in the synth context', () => {
33+
// A bare `new App()` reads CDK_CONTEXT_JSON, which setupFiles populated.
34+
expect(new App().node.tryGetContext('aws:cdk:bundling-stacks')).toEqual([]);
35+
});
36+
37+
it('makes a bare-App stack report bundlingRequired === false', () => {
38+
const stack = new Stack(new App(), 'BundlingProbe', {
39+
env: { account: '123456789012', region: 'us-east-1' },
40+
});
41+
expect(stack.bundlingRequired).toBe(false);
42+
});
43+
});
44+
45+
describe('per-test bundling opt-out (#366)', () => {
46+
// The documented escape hatch for a test that needs real bundled-asset output
47+
// is `postCliContext`, NOT constructor `context`. CDK's
48+
// `App.loadContext(props.context, props.postCliContext)` applies
49+
// `props.context` first, overwrites it with CDK_CONTEXT_JSON (which the global
50+
// disable sets), then applies `postCliContext` last — so only the latter wins.
51+
// These lock that precedence contract against a future CDK reordering; without
52+
// them the one supported opt-out path is unguarded.
53+
it('re-enables bundling via postCliContext (the documented opt-out)', () => {
54+
const app = new App({ postCliContext: { 'aws:cdk:bundling-stacks': ['**'] } });
55+
const stack = new Stack(app, 'OptOutProbe', {
56+
env: { account: '123456789012', region: 'us-east-1' },
57+
});
58+
expect(stack.bundlingRequired).toBe(true);
59+
});
60+
61+
it('does NOT re-enable bundling via constructor context (env var clobbers it)', () => {
62+
const app = new App({ context: { 'aws:cdk:bundling-stacks': ['**'] } });
63+
const stack = new Stack(app, 'ContextOptOutProbe', {
64+
env: { account: '123456789012', region: 'us-east-1' },
65+
});
66+
expect(stack.bundlingRequired).toBe(false);
67+
});
68+
});

cdk/test/setup/disable-bundling.ts

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
/**
2+
* MIT No Attribution
3+
*
4+
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
5+
*
6+
* Permission is hereby granted, free of charge, to any person obtaining a copy of
7+
* the Software without restriction, including without limitation the rights to
8+
* use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of
9+
* the Software, and to permit persons to whom the Software is furnished to do so.
10+
*
11+
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
12+
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
13+
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
14+
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
15+
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
16+
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
17+
* SOFTWARE.
18+
*/
19+
20+
// Disable Lambda asset bundling during unit-test synthesis.
21+
//
22+
// `Template.fromStack()` triggers a full CDK synth, which bundles every
23+
// NodejsFunction via esbuild — ~28s for the AgentStack (413 resources). Unit
24+
// tests assert on CloudFormation structure/properties, not bundled Lambda code,
25+
// so for those assertions bundling is overhead — plus a smoke-test of Lambda
26+
// `entry` resolution and handler compilation that unit tests intentionally cede
27+
// to the `agent/` build (real synth/deploy still bundles). Skipping it cuts a
28+
// single synth ~15× (~28.7s -> ~1.9s). See #366.
29+
//
30+
// `aws:cdk:bundling-stacks: []` tells the CLI/synth to bundle no stacks. CDK
31+
// reads CDK_CONTEXT_JSON when an `App` is constructed, so a bare `new App()`
32+
// (the overwhelming-majority pattern in our tests) picks this up with no
33+
// call-site changes.
34+
//
35+
// Precedence matters for the opt-out. CDK's `App.loadContext(props.context,
36+
// props.postCliContext)` applies `props.context` FIRST, then overwrites it with
37+
// CDK_CONTEXT_JSON, then applies `postCliContext` LAST. So for this key the env
38+
// var beats constructor `context` — `new App({ context: { 'aws:cdk:bundling-
39+
// stacks': ['**'] } })` does NOT re-enable bundling (the env var clobbers it).
40+
//
41+
// This does NOT stop tests from synthesizing — `Template.fromStack()` still
42+
// runs a full synth; it only skips the esbuild asset-bundling step within that
43+
// synth. The opt-out below exists solely for the rare test that needs the
44+
// *bundled-asset output itself* (e.g. asserting on a real asset hash / S3 key),
45+
// where an unbundled synth would silently yield a placeholder value. Such a
46+
// test must opt out via `postCliContext` (which wins over the env var),
47+
// constructing its `App` with
48+
// `new App({ postCliContext: { 'aws:cdk:bundling-stacks': ['**'] } })`
49+
// (or the specific stack id).
50+
const BUNDLING_DISABLED_CONTEXT = { 'aws:cdk:bundling-stacks': [] as string[] };
51+
52+
const existing = process.env.CDK_CONTEXT_JSON;
53+
if (existing) {
54+
// Preserve any ambient context, but let the bundling disable win
55+
// unconditionally: a pre-set CDK_CONTEXT_JSON that carries
56+
// `aws:cdk:bundling-stacks` must not silently re-enable bundling and defeat
57+
// the speedup. The documented per-test opt-out is `postCliContext` (applied
58+
// AFTER the env var regardless — see above), so spreading our key LAST keeps
59+
// that escape hatch working while making the global disable robust to
60+
// whatever the environment already set.
61+
let ambient: Record<string, unknown>;
62+
try {
63+
ambient = JSON.parse(existing);
64+
} catch {
65+
// Without this guard a malformed pre-set var throws here, inside
66+
// `setupFiles`, and fails EVERY test in EVERY file with an opaque parse
67+
// error. Re-throw with the offending value so the blame is localized.
68+
throw new Error(`malformed CDK_CONTEXT_JSON in environment (must be valid JSON): ${existing}`);
69+
}
70+
const merged = { ...ambient, ...BUNDLING_DISABLED_CONTEXT };
71+
process.env.CDK_CONTEXT_JSON = JSON.stringify(merged);
72+
} else {
73+
process.env.CDK_CONTEXT_JSON = JSON.stringify(BUNDLING_DISABLED_CONTEXT);
74+
}

0 commit comments

Comments
 (0)