aws / aws/aws-cdk-cli

integ-runner always writes enableLookups: true to the golden snapshot manifest, ignoring the test's real value

Open
#1,895 1 comment 0 reactions 0 assignees View on GitHub
needs-triage
Dominant language
TypeScript
Stars
105
Forks
122
Avg merge
1d 17h
Merged PRs (30d)
71

Description

### Describe the bug

`CdkTestApp.cleanupGoldenSnapshot` unconditionally overwrites the test suite's `enableLookups` with a hardcoded `true` before saving the golden `integ.json` manifest:

https://github.com/aws/aws-cdk-cli/blob/main/packages/@aws-cdk/integ-runner/lib/runner/cdk-test-app.ts#L594-L605

```ts
const actualTestSuite = this.testSuite;
actualTestSuite.enableLookups = true; // <-- always true, regardless of the test's real setting

if (actualTestSuite instanceof LegacyIntegTestSuite) {
actualTestSuite.saveManifest(this.outputDirectory, this.legacyContext);
} else if (actualTestSuite instanceof IntegTestSuite) {
actualTestSuite.saveManifest(this.outputDirectory);
}
```

At this point `actualTestSuite.enableLookups` already correctly reflects the test's real setting — for a legacy test it comes from parsing the `pragma:enable-lookups` pragma in the test source (`LegacyIntegTestSuite.fromLegacy`); for a modern test it comes from the test's own synthesized `integ.json` (`IntegTestSuite.fromPath` → `IntegManifestReader`). The `= true` assignment clobbers that correct value right before persisting it.

### Impact

- Every golden snapshot's `integ.json` gets `"enableLookups": true` written into it, regardless of whether the test actually enables lookups.
- On the next run, `SnapshotTestRunner`/`synthForSnapshotComparison` reads this value back as its initial guess (`expectedSuite.enableLookups ?? true`) for whether to seed synth with lookup context. For any test whose real `enableLookups` is `false`, this wrong guess forces `synthForSnapshotComparison` to detect a mismatch and re-synth a second time on every single run — wasted work on every CI run for every such test.
- The persisted manifest permanently misrepresents the test's actual configuration, which is also used by the update workflow.

### Reproduction Steps / confirmation

`packages/@aws-cdk/integ-runner/test/test-data/xxxxx.test-with-snapshot.js` has no `pragma:enable-lookups`. Running it through `IntegTestRunner.runIntegTestCase` (which internally calls `cleanupGoldenSnapshot`) and inspecting the `Manifest.saveIntegManifest` call shows `enableLookups: true` written for this test, even though it should be `false`.

### Expected Behavior

The golden manifest should record the test's actual `enableLookups` value.

### Possible Solution

Remove the hardcoded `actualTestSuite.enableLookups = true;` line — the value is already correctly populated by the time `cleanupGoldenSnapshot` runs.

### Additional information / context

Found while auditing the integ-runner's snapshot-management logic. I have a fix + regression test ready and will open a PR referencing this issue.

### CDK CLI Version

latest (main)

### Framework Version

N/A

### Node.js Version

N/A

### OS

N/A

### Language

TypeScript

### Language Version

N/A

### Other information

N/A

Contributor guide

Open the contributing guide

Research direction

Start in packages/@aws-cdk/integ-runner/lib/runner/cdk-test-app.ts around cleanupGoldenSnapshot and inspect the existing test data at packages/@aws-cdk/integ-runner/test/test-data/xxxxx.test-with-snapshot.js. Run the reproduction through IntegTestRunner.runIntegTestCase and inspect the Manifest.saveIntegManifest call. Done means the golden integ.json preserves the test's actual enableLookups value and a regression test covers the false case.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
cli, testing
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.