Skip to content

Commit

Permalink
fix(integ-runner): update workflow doesn't support resource replaceme…
Browse files Browse the repository at this point in the history
…nt (#24720)

For the update workflow we deploy the stack update with the
`--no-rollback` flag so that the assertion stack will deploy all the way
through instead of failing on the first assertion failure. This causes
issues if you are deploying an update that includes resource
replacement since resource replacement is not allowed when using
`--no-rollback`

To fix this, this PR splits the stack update deployment into two
separate deployments. The first deploys the test case stack and the
second deploys the assertion stack with `--no-rollback`.


----

*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 committed Mar 27, 2023
1 parent 20aeb0f commit 07d3aa7
Show file tree
Hide file tree
Showing 4 changed files with 75 additions and 11 deletions.
28 changes: 23 additions & 5 deletions packages/@aws-cdk/integ-runner/lib/runner/integ-test-runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -285,23 +285,41 @@ export class IntegTestRunner extends IntegRunner {
lookups: this.expectedTestSuite?.enableLookups,
});
}
// now deploy the "actual" test. If there are any assertions
// deploy the assertion stack as well
// now deploy the "actual" test.
this.cdk.deploy({
...deployArgs,
lookups: this.actualTestSuite.enableLookups,
stacks: [
...actualTestCase.stacks,
...actualTestCase.assertionStack ? [actualTestCase.assertionStack] : [],
],
rollback: false,
output: path.relative(this.directory, this.cdkOutDir),
...actualTestCase?.cdkCommandOptions?.deploy?.args,
...actualTestCase.assertionStack ? { outputsFile: path.relative(this.directory, path.join(this.cdkOutDir, 'assertion-results.json')) } : undefined,
context: this.getContext(actualTestCase?.cdkCommandOptions?.deploy?.args?.context),
app: this.cdkApp,
});

// If there are any assertions
// deploy the assertion stack as well
// This is separate from the above deployment because we want to
// set `rollback: false`. This allows the assertion stack to deploy all the
// assertions instead of failing at the first failed assertion
// combining it with the above deployment would prevent any replacement updates
if (actualTestCase.assertionStack) {
this.cdk.deploy({
...deployArgs,
lookups: this.actualTestSuite.enableLookups,
stacks: [
actualTestCase.assertionStack,
],
rollback: false,
output: path.relative(this.directory, this.cdkOutDir),
...actualTestCase?.cdkCommandOptions?.deploy?.args,
outputsFile: path.relative(this.directory, path.join(this.cdkOutDir, 'assertion-results.json')),
context: this.getContext(actualTestCase?.cdkCommandOptions?.deploy?.args?.context),
app: this.cdkApp,
});
}

if (actualTestCase.hooks?.postDeploy) {
actualTestCase.hooks.postDeploy.forEach(cmd => {
exec(chunks(cmd), {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ describe('IntegTest runIntegTests', () => {
});

// THEN
expect(cdkMock.mocks.deploy).toHaveBeenCalledTimes(2);
expect(cdkMock.mocks.deploy).toHaveBeenCalledTimes(3);
expect(cdkMock.mocks.destroy).toHaveBeenCalledTimes(1);
expect(cdkMock.mocks.synthFast).toHaveBeenCalledTimes(1);
expect(cdkMock.mocks.deploy).toHaveBeenCalledWith({
Expand Down Expand Up @@ -83,7 +83,6 @@ describe('IntegTest runIntegTests', () => {
}),
versionReporting: false,
lookups: false,
rollback: false,
stacks: ['test-stack', 'new-test-stack'],
});
expect(cdkMock.mocks.destroy).toHaveBeenCalledWith({
Expand Down Expand Up @@ -130,7 +129,6 @@ describe('IntegTest runIntegTests', () => {
context: expect.not.objectContaining({
[AVAILABILITY_ZONE_FALLBACK_CONTEXT_KEY]: ['test-region-1a', 'test-region-1b', 'test-region-1c'],
}),
rollback: false,
lookups: false,
stacks: ['stack1'],
output: 'cdk-integ.out.xxxxx.integ-test1.js.snapshot',
Expand Down Expand Up @@ -176,7 +174,6 @@ describe('IntegTest runIntegTests', () => {
}),
versionReporting: false,
lookups: true,
rollback: false,
stacks: ['test-stack'],
output: 'cdk-integ.out.xxxxx.test-with-snapshot-assets-diff.js.snapshot',
profile: undefined,
Expand Down Expand Up @@ -206,6 +203,52 @@ describe('IntegTest runIntegTests', () => {
});
});

test('with an assertion stack', () => {
// WHEN
const integTest = new IntegTestRunner({
cdk: cdkMock.cdk,
test: new IntegTest({
fileName: 'test/test-data/xxxxx.test-with-snapshot.js',
discoveryRoot: 'test/test-data',
}),
});
integTest.runIntegTestCase({
testCaseName: 'xxxxx.test-with-snapshot',
});

// THEN
expect(cdkMock.mocks.deploy).toHaveBeenCalledTimes(3);
expect(cdkMock.mocks.destroy).toHaveBeenCalledTimes(1);
expect(cdkMock.mocks.synthFast).toHaveBeenCalledTimes(1);
expect(cdkMock.mocks.deploy).toHaveBeenNthCalledWith(1, expect.objectContaining({
app: 'xxxxx.test-with-snapshot.js.snapshot',
context: expect.any(Object),
stacks: ['test-stack'],
}));
expect(cdkMock.mocks.deploy).toHaveBeenNthCalledWith(2, expect.not.objectContaining({
rollback: false,
}));
expect(cdkMock.mocks.deploy).toHaveBeenNthCalledWith(3, expect.objectContaining({
app: 'node xxxxx.test-with-snapshot.js',
stacks: ['Bundling/DefaultTest/DeployAssert'],
rollback: false,
}));
expect(cdkMock.mocks.destroy).toHaveBeenCalledWith({
app: 'node xxxxx.test-with-snapshot.js',
pathMetadata: false,
assetMetadata: false,
context: expect.not.objectContaining({
'vpc-provider:account=12345678:filter.isDefault=true:region=test-region:returnAsymmetricSubnets=true': expect.objectContaining({
vpcId: 'vpc-60900905',
}),
}),
versionReporting: false,
force: true,
all: true,
output: 'cdk-integ.out.xxxxx.test-with-snapshot.js.snapshot',
});
});

test('no clean', () => {
// WHEN
const integTest = new IntegTestRunner({
Expand Down Expand Up @@ -298,7 +341,6 @@ describe('IntegTest runIntegTests', () => {
}),
}),
profile: 'test-profile',
rollback: false,
lookups: false,
stacks: ['stack1'],
output: 'cdk-integ.out.xxxxx.integ-test1.js.snapshot',
Expand Down Expand Up @@ -564,7 +606,7 @@ describe('IntegTest runIntegTests', () => {
});

// THEN
expect(cdkMock.mocks.deploy).toHaveBeenCalledTimes(2);
expect(cdkMock.mocks.deploy).toHaveBeenCalledTimes(3);
expect(cdkMock.mocks.destroy).toHaveBeenCalledTimes(1);
expect(cdkMock.mocks.synthFast).toHaveBeenCalledTimes(1);
expect(cdkMock.mocks.deploy).toHaveBeenCalledWith(expect.objectContaining({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
"testCases": {
"xxxxx.test-with-snapshot": {
"stacks": ["test-stack", "new-test-stack"],
"assertionStack": "Bundling/DefaultTest/DeployAssert",
"assertionStackName": "BundlingDefaultTestDeployAssertAACA0CAF",
"diffAssets": true,
"allowDestroy": [
"AWS::IAM::Role"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
"testCases": {
"xxxxx.test-with-snapshot": {
"stacks": ["test-stack"],
"assertionStack": "Bundling/DefaultTest/DeployAssert",
"assertionStackName": "BundlingDefaultTestDeployAssertAACA0CAF",
"diffAssets": true,
"allowDestroy": [
"AWS::IAM::Role"
Expand Down

0 comments on commit 07d3aa7

Please sign in to comment.