Skip to content

Commit

Permalink
fix(integ-tests): can't enable lookups when creating an IntegTest (#2…
Browse files Browse the repository at this point in the history 
…2075)

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*
  • Loading branch information
@corymhall
corymhall committed Sep 17, 2022
1 parent 5fc7ca2 commit d0e0ab9
Show file tree
Hide file tree
Showing 7 changed files with 109 additions and 20 deletions.
4 changes: 3 additions & 1 deletion packages/@aws-cdk/integ-runner/lib/runner/integ-test-runner.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,9 @@ export class IntegTestRunner extends IntegRunner {
} else {
const env: Record<string, any> = {
...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(' '),
Expand Down
6 changes: 1 addition & 5 deletions packages/@aws-cdk/integ-runner/lib/runner/runner-base.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
});
Expand Down Expand Up @@ -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,
Expand Down
5 changes: 4 additions & 1 deletion packages/@aws-cdk/integ-runner/lib/runner/snapshot-test-runner.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(' '),
Expand Down Expand Up @@ -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(' ')}`,

);
}

Expand Down
36 changes: 28 additions & 8 deletions packages/@aws-cdk/integ-runner/test/runner/integ-test-runner.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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',
}),
Expand All @@ -186,15 +198,15 @@ 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',
});
expect(destroyMock).toHaveBeenCalledWith({
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',
}),
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down
11 changes: 7 additions & 4 deletions packages/@aws-cdk/integ-tests/lib/manifest-synthesizer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
14 changes: 13 additions & 1 deletion packages/@aws-cdk/integ-tests/lib/test-case.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

/**
Expand All @@ -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,
Expand All @@ -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);
},
});
Expand Down
53 changes: 53 additions & 0 deletions packages/@aws-cdk/integ-tests/test/manifest-synthesizer.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down

0 comments on commit d0e0ab9

Please sign in to comment.