From d0e0ab9d3744372edd56aa984daac4de26272673 Mon Sep 17 00:00:00 2001 From: Cory Hall <43035978+corymhall@users.noreply.github.com> Date: Fri, 16 Sep 2022 23:57:52 -0400 Subject: [PATCH] fix(integ-tests): can't enable lookups when creating an IntegTest (#22075) We were not exposing the `enableLookups` property when creating an `IntegTest`. This also updates `integ-runner` to ensure that we are correctly utilizing `enableLooups`. There was an undiscovered (because you couldn't set `enableLookups=true` :) ) bug which set the dummy context on _every_ command (i.e. deploy, destroy) when it should have only been set when synthing the snapshot. ---- ### All Submissions: * [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- .../lib/runner/integ-test-runner.ts | 4 +- .../integ-runner/lib/runner/runner-base.ts | 6 +-- .../lib/runner/snapshot-test-runner.ts | 5 +- .../test/runner/integ-test-runner.test.ts | 36 ++++++++++--- .../integ-tests/lib/manifest-synthesizer.ts | 11 ++-- .../@aws-cdk/integ-tests/lib/test-case.ts | 14 ++++- .../test/manifest-synthesizer.test.ts | 53 +++++++++++++++++++ 7 files changed, 109 insertions(+), 20 deletions(-) diff --git a/packages/@aws-cdk/integ-runner/lib/runner/integ-test-runner.ts b/packages/@aws-cdk/integ-runner/lib/runner/integ-test-runner.ts index 2845680063439..cf6fe6b9efb5b 100644 --- a/packages/@aws-cdk/integ-runner/lib/runner/integ-test-runner.ts +++ b/packages/@aws-cdk/integ-runner/lib/runner/integ-test-runner.ts @@ -176,7 +176,9 @@ export class IntegTestRunner extends IntegRunner { } else { const env: Record = { ...DEFAULT_SYNTH_OPTIONS.env, - CDK_CONTEXT_JSON: JSON.stringify(this.getContext()), + CDK_CONTEXT_JSON: JSON.stringify(this.getContext({ + ...this.actualTestSuite.enableLookups ? DEFAULT_SYNTH_OPTIONS.context : {}, + })), }; this.cdk.synthFast({ execCmd: this.cdkApp.split(' '), diff --git a/packages/@aws-cdk/integ-runner/lib/runner/runner-base.ts b/packages/@aws-cdk/integ-runner/lib/runner/runner-base.ts index b5db5b8c68829..c8fc073d89918 100644 --- a/packages/@aws-cdk/integ-runner/lib/runner/runner-base.ts +++ b/packages/@aws-cdk/integ-runner/lib/runner/runner-base.ts @@ -321,7 +321,7 @@ export abstract class IntegRunner { execCmd: this.cdkApp.split(' '), env: { ...DEFAULT_SYNTH_OPTIONS.env, - CDK_CONTEXT_JSON: JSON.stringify(this.getContext()), + CDK_CONTEXT_JSON: JSON.stringify(this.getContext(DEFAULT_SYNTH_OPTIONS.context)), }, output: path.relative(this.directory, this.snapshotDir), }); @@ -361,11 +361,7 @@ export abstract class IntegRunner { .filter(([k, _]) => !FUTURE_FLAGS_EXPIRED.includes(k)) .forEach(([k, v]) => futureFlags[k] = v); - const enableLookups = (this.actualTestSuite ?? this.expectedTestSuite)?.enableLookups; return { - // if lookups are enabled then we need to synth - // with the "dummy" context - ...enableLookups ? DEFAULT_SYNTH_OPTIONS.context : {}, ...futureFlags, ...this.legacyContext, ...additionalContext, diff --git a/packages/@aws-cdk/integ-runner/lib/runner/snapshot-test-runner.ts b/packages/@aws-cdk/integ-runner/lib/runner/snapshot-test-runner.ts index 3dbf55b4449fb..fd7ef514d51a0 100644 --- a/packages/@aws-cdk/integ-runner/lib/runner/snapshot-test-runner.ts +++ b/packages/@aws-cdk/integ-runner/lib/runner/snapshot-test-runner.ts @@ -41,7 +41,9 @@ export class IntegSnapshotRunner extends IntegRunner { // to produce the "correct" snapshot const env = { ...DEFAULT_SYNTH_OPTIONS.env, - CDK_CONTEXT_JSON: JSON.stringify(this.getContext()), + CDK_CONTEXT_JSON: JSON.stringify(this.getContext({ + ...this.actualTestSuite.enableLookups ? DEFAULT_SYNTH_OPTIONS.context : {}, + })), }; this.cdk.synthFast({ execCmd: this.cdkApp.split(' '), @@ -85,6 +87,7 @@ export class IntegSnapshotRunner extends IntegRunner { additionalMessages.push( 'Repro:', ` ${[...envCmd, 'cdk synth', `-a '${this.cdkApp}'`, `-o '${this.cdkOutDir}'`, ...Object.entries(this.getContext()).flatMap(([k, v]) => typeof v !== 'object' ? [`-c '${k}=${v}'`] : [])].join(' ')}`, + ); } diff --git a/packages/@aws-cdk/integ-runner/test/runner/integ-test-runner.test.ts b/packages/@aws-cdk/integ-runner/test/runner/integ-test-runner.test.ts index cb49ac23a907c..9fad8eb71a1a9 100644 --- a/packages/@aws-cdk/integ-runner/test/runner/integ-test-runner.test.ts +++ b/packages/@aws-cdk/integ-runner/test/runner/integ-test-runner.test.ts @@ -71,7 +71,11 @@ describe('IntegTest runIntegTests', () => { requireApproval: 'never', pathMetadata: false, assetMetadata: false, - context: expect.any(Object), + context: expect.not.objectContaining({ + 'vpc-provider:account=12345678:filter.isDefault=true:region=test-region:returnAsymmetricSubnets=true': expect.objectContaining({ + vpcId: 'vpc-60900905', + }), + }), profile: undefined, versionReporting: false, lookups: false, @@ -84,7 +88,11 @@ describe('IntegTest runIntegTests', () => { assetMetadata: false, output: 'cdk-integ.out.test-with-snapshot', profile: undefined, - context: expect.any(Object), + context: expect.not.objectContaining({ + 'vpc-provider:account=12345678:filter.isDefault=true:region=test-region:returnAsymmetricSubnets=true': expect.objectContaining({ + vpcId: 'vpc-60900905', + }), + }), versionReporting: false, lookups: false, rollback: false, @@ -94,7 +102,11 @@ describe('IntegTest runIntegTests', () => { app: 'node xxxxx.test-with-snapshot.js', pathMetadata: false, assetMetadata: false, - context: expect.any(Object), + context: expect.not.objectContaining({ + 'vpc-provider:account=12345678:filter.isDefault=true:region=test-region:returnAsymmetricSubnets=true': expect.objectContaining({ + vpcId: 'vpc-60900905', + }), + }), versionReporting: false, profile: undefined, force: true, @@ -169,7 +181,7 @@ describe('IntegTest runIntegTests', () => { requireApproval: 'never', pathMetadata: false, assetMetadata: false, - context: expect.objectContaining({ + context: expect.not.objectContaining({ 'vpc-provider:account=12345678:filter.isDefault=true:region=test-region:returnAsymmetricSubnets=true': expect.objectContaining({ vpcId: 'vpc-60900905', }), @@ -186,7 +198,7 @@ describe('IntegTest runIntegTests', () => { env: expect.objectContaining({ CDK_INTEG_ACCOUNT: '12345678', CDK_INTEG_REGION: 'test-region', - CDK_CONTEXT_JSON: expect.anything(), + CDK_CONTEXT_JSON: expect.stringContaining('"vpcId":"vpc-60900905"'), }), output: 'test-with-snapshot-assets-diff.integ.snapshot', }); @@ -194,7 +206,7 @@ describe('IntegTest runIntegTests', () => { app: 'node xxxxx.test-with-snapshot-assets-diff.js', pathMetadata: false, assetMetadata: false, - context: expect.objectContaining({ + context: expect.not.objectContaining({ 'vpc-provider:account=12345678:filter.isDefault=true:region=test-region:returnAsymmetricSubnets=true': expect.objectContaining({ vpcId: 'vpc-60900905', }), @@ -292,7 +304,11 @@ describe('IntegTest runIntegTests', () => { pathMetadata: false, assetMetadata: false, versionReporting: false, - context: expect.any(Object), + context: expect.not.objectContaining({ + 'vpc-provider:account=12345678:filter.isDefault=true:region=test-region:returnAsymmetricSubnets=true': expect.objectContaining({ + vpcId: 'vpc-60900905', + }), + }), profile: 'test-profile', rollback: false, lookups: false, @@ -304,7 +320,11 @@ describe('IntegTest runIntegTests', () => { pathMetadata: false, assetMetadata: false, versionReporting: false, - context: expect.any(Object), + context: expect.not.objectContaining({ + 'vpc-provider:account=12345678:filter.isDefault=true:region=test-region:returnAsymmetricSubnets=true': expect.objectContaining({ + vpcId: 'vpc-60900905', + }), + }), profile: 'test-profile', force: true, all: true, diff --git a/packages/@aws-cdk/integ-tests/lib/manifest-synthesizer.ts b/packages/@aws-cdk/integ-tests/lib/manifest-synthesizer.ts index 36230a8b7ad9f..61274f7c84ab9 100644 --- a/packages/@aws-cdk/integ-tests/lib/manifest-synthesizer.ts +++ b/packages/@aws-cdk/integ-tests/lib/manifest-synthesizer.ts @@ -9,12 +9,15 @@ const emptyManifest: IntegManifest = { }; export class IntegManifestSynthesizer { - constructor(private readonly testCases: IntegTestCase[]) {} + constructor(private readonly testCases: IntegTestCase[], private readonly enableLookups?: boolean) {} synthesize(session: ISynthesisSession) { - const manifest = this.testCases - .map(tc => tc.manifest) - .reduce(mergeManifests, emptyManifest); + const manifest: IntegManifest = { + enableLookups: this.enableLookups, + ...this.testCases + .map(tc => tc.manifest) + .reduce(mergeManifests, emptyManifest), + }; const snapshotDir = session.assembly.outdir; diff --git a/packages/@aws-cdk/integ-tests/lib/test-case.ts b/packages/@aws-cdk/integ-tests/lib/test-case.ts index febb4945bbe2f..1d3cce2026bb5 100644 --- a/packages/@aws-cdk/integ-tests/lib/test-case.ts +++ b/packages/@aws-cdk/integ-tests/lib/test-case.ts @@ -115,6 +115,16 @@ export interface IntegTestProps extends TestOptions { * List of test cases that make up this test */ readonly testCases: Stack[]; + + /** + * Enable lookups for this test. If lookups are enabled + * then `stackUpdateWorkflow` must be set to false. + * Lookups should only be enabled when you are explicitely testing + * lookups. + * + * @default false + */ + readonly enableLookups?: boolean; } /** @@ -127,9 +137,11 @@ export class IntegTest extends Construct { */ public readonly assertions: IDeployAssert; private readonly testCases: IntegTestCase[]; + private readonly enableLookups?: boolean; constructor(scope: Construct, id: string, props: IntegTestProps) { super(scope, id); + this.enableLookups = props.enableLookups; const defaultTestCase = new IntegTestCase(this, 'DefaultTest', { stacks: props.testCases.filter(stack => !IntegTestCaseStack.isIntegTestCaseStack(stack)), hooks: props.hooks, @@ -152,7 +164,7 @@ export class IntegTest extends Construct { validate: () => { attachCustomSynthesis(this, { onSynthesize: (session: ISynthesisSession) => { - const synthesizer = new IntegManifestSynthesizer(this.testCases); + const synthesizer = new IntegManifestSynthesizer(this.testCases, this.enableLookups); synthesizer.synthesize(session); }, }); diff --git a/packages/@aws-cdk/integ-tests/test/manifest-synthesizer.test.ts b/packages/@aws-cdk/integ-tests/test/manifest-synthesizer.test.ts index 7c448b7467a52..982079e7aedfb 100644 --- a/packages/@aws-cdk/integ-tests/test/manifest-synthesizer.test.ts +++ b/packages/@aws-cdk/integ-tests/test/manifest-synthesizer.test.ts @@ -82,6 +82,59 @@ describe(IntegManifestSynthesizer, () => { }); }); + test('with options', () => { + // GIVEN + const app = new App(); + const stack = new Stack(app, 'stack'); + + // WHEN + new IntegTest(app, 'Integ', { + testCases: [stack], + enableLookups: true, + hooks: { + preDeploy: ['echo "preDeploy"'], + }, + diffAssets: true, + allowDestroy: ['AWS::IAM::Role'], + stackUpdateWorkflow: false, + cdkCommandOptions: { + deploy: { + args: { + profile: 'profile', + }, + }, + }, + }); + const integAssembly = app.synth(); + const integManifest = Manifest.loadIntegManifest(path.join(integAssembly.directory, 'integ.json')); + + // THEN + expect(integManifest).toEqual({ + version: Manifest.version(), + enableLookups: true, + testCases: { + ['Integ/DefaultTest']: { + assertionStack: 'Integ/DefaultTest/DeployAssert', + assertionStackName: 'IntegDefaultTestDeployAssert4E6713E1', + stacks: ['stack'], + hooks: { + preDeploy: ['echo "preDeploy"'], + }, + diffAssets: true, + allowDestroy: ['AWS::IAM::Role'], + stackUpdateWorkflow: false, + cdkCommandOptions: { + deploy: { + args: { + profile: 'profile', + }, + }, + }, + }, + }, + }); + }); + test('with IntegTestCaseStack', () => { // GIVEN const app = new App();