Skip to content

Commit

Permalink
chore(cli-integ): try to fix some integ test failures (#24091)
Browse files Browse the repository at this point in the history
While investigating cli test failures I found some things that need to be cleaned up.

1. Ensure that the bootstrap tests do not also run `ensureBootstrapped` since the test itself is responsible for bootstrapping.
2. Ensure that the correct context is passed to each test

---------

Co-authored-by: Rico Hermans <rix0rrr@gmail.com>
  • Loading branch information
corymhall and rix0rrr authored Feb 13, 2023
1 parent 68f1ff8 commit c4ad3ac
Show file tree
Hide file tree
Showing 7 changed files with 90 additions and 32 deletions.
6 changes: 6 additions & 0 deletions packages/@aws-cdk-testing/cli-integ/bin/run-suite.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@ async function main() {
type: 'string',
requiresArg: true,
})
.option('test-file', {
description: 'The specific test file to run',
type: 'string',
requiresArg: true,
})
.option('use-source', {
descripton: 'Use TypeScript packages from the given source repository (or "auto")',
alias: 's',
Expand Down Expand Up @@ -121,6 +126,7 @@ async function main() {
...args.test ? ['-t', args.test] : [],
...args.verbose ? ['--verbose'] : [],
...passWithNoTests ? ['--passWithNoTests'] : [],
...args['test-file'] ? [args['test-file']] : [],
], path.resolve(__dirname, '..', 'resources', 'integ.jest.config.js'));

} finally {
Expand Down
4 changes: 2 additions & 2 deletions packages/@aws-cdk-testing/cli-integ/lib/integ-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ export function integTest(
name: string,
callback: (context: TestContext) => Promise<void>,
timeoutMillis?: number,
) {
): void {

// Integ tests can run concurrently, and are responsible for blocking
// themselves if they cannot. Because `test.concurrent` executes the test
Expand Down Expand Up @@ -62,4 +62,4 @@ function shouldSkip(testName: string) {
export function randomString() {
// Crazy
return Math.random().toString(36).replace(/[^a-z0-9]+/g, '');
}
}
12 changes: 8 additions & 4 deletions packages/@aws-cdk-testing/cli-integ/lib/with-aws.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { AwsClients } from './aws';
import { TestContext } from './integ-test';
import { ResourcePool } from './resource-pool';
import { DisableBootstrapContext } from './with-cdk-app';

export type AwsContext = { readonly aws: AwsClients };

Expand All @@ -9,12 +10,15 @@ export type AwsContext = { readonly aws: AwsClients };
*
* Allocate the next region from the REGION pool and dispose it afterwards.
*/
export function withAws<A extends TestContext>(block: (context: A & AwsContext) => Promise<void>) {
return (context: A) => regionPool().using(async (region) => {
export function withAws(
block: (context: TestContext & AwsContext & DisableBootstrapContext) => Promise<void>,
disableBootstrap: boolean = false,
): (context: TestContext) => Promise<void> {
return (context: TestContext) => regionPool().using(async (region) => {
const aws = await AwsClients.forRegion(region, context.output);
await sanityCheck(aws);

return block({ ...context, aws });
return block({ ...context, disableBootstrap, aws });
});
}

Expand Down Expand Up @@ -60,4 +64,4 @@ async function sanityCheck(aws: AwsClients) {
throw new Error('AWS credentials probably not configured, see previous error');
}
}
let sanityChecked: boolean | undefined;
let sanityChecked: boolean | undefined;
36 changes: 31 additions & 5 deletions packages/@aws-cdk-testing/cli-integ/lib/with-cdk-app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,10 @@ import { AwsContext, withAws } from './with-aws';
* For backwards compatibility with existing tests (so we don't have to change
* too much) the inner block is expected to take a `TestFixture` object.
*/
export function withCdkApp<A extends TestContext & AwsContext>(block: (context: TestFixture) => Promise<void>) {
return async (context: A) => {
export function withCdkApp(
block: (context: TestFixture) => Promise<void>,
): (context: TestContext & AwsContext & DisableBootstrapContext) => Promise<void> {
return async (context: TestContext & AwsContext & DisableBootstrapContext) => {
const randy = context.randomString;
const stackNamePrefix = `cdktest-${randy}`;
const integTestDir = path.join(os.tmpdir(), `cdk-integ-${randy}`);
Expand Down Expand Up @@ -61,7 +63,9 @@ export function withCdkApp<A extends TestContext & AwsContext>(block: (context:
});
}

await ensureBootstrapped(fixture);
if (!context.disableBootstrap) {
await ensureBootstrapped(fixture);
}

await block(fixture);
} catch (e) {
Expand Down Expand Up @@ -131,8 +135,28 @@ export function withMonolithicCfnIncludeCdkApp<A extends TestContext>(block: (co
* test declaration but centralizing it is going to make it convenient to modify in the future.
*/
export function withDefaultFixture(block: (context: TestFixture) => Promise<void>) {
return withAws<TestContext>(withCdkApp(block));
// ^~~~~~ this is disappointing TypeScript! Feels like you should have been able to derive this.
return withAws(withCdkApp(block));
}

export interface DisableBootstrapContext {
/**
* Whether to disable creating the default bootstrap
* stack prior to running the test
*
* This should be set to true when running tests that
* explicitly create a bootstrap stack
*
* @default false
*/
readonly disableBootstrap?: boolean;
}

/**
* To be used in place of `withDefaultFixture` when the test
* should not create the default bootstrap stack
*/
export function withoutBootstrap(block: (context: TestFixture) => Promise<void>) {
return withAws(withCdkApp(block), true);
}

export interface CdkCliOptions extends ShellOptions {
Expand Down Expand Up @@ -246,6 +270,8 @@ export class TestFixture extends ShellHelper {
return this.cdk(['deploy',
...(neverRequireApproval ? ['--require-approval=never'] : []), // Default to no approval in an unattended test
...(options.options ?? []),
// use events because bar renders bad in tests
'--progress', 'events',
...this.fullStackName(stackNames)], options);
}

Expand Down
4 changes: 2 additions & 2 deletions packages/@aws-cdk-testing/cli-integ/lib/with-sam.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ function errorCausedByGoPkg(error: string) {
* SAM Integration test fixture for CDK - SAM integration test cases
*/
export function withSamIntegrationFixture(block: (context: SamIntegrationTestFixture) => Promise<void>) {
return withAws<TestContext>(withSamIntegrationCdkApp(block));
return withAws(withSamIntegrationCdkApp(block));
}

export class SamIntegrationTestFixture extends TestFixture {
Expand Down Expand Up @@ -263,4 +263,4 @@ function killSubProcess(child: child_process.ChildProcess, command: string) {
child.kill('SIGINT');
}

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ if (process.env.PACKAGE_LAYOUT_VERSION === '1') {
} else {
var cdk = require('aws-cdk-lib');
var {
DefaultStackSynthesizer,
LegacyStackSynthesizer,
aws_ec2: ec2,
aws_s3: s3,
aws_ssm: ssm,
Expand Down Expand Up @@ -216,7 +218,19 @@ class MissingSSMParameterStack extends cdk.Stack {

class LambdaStack extends cdk.Stack {
constructor(parent, id, props) {
super(parent, id, props);
// sometimes we need to specify the custom bootstrap bucket to use
// see the 'upgrade legacy bootstrap stack' test
const synthesizer = parent.node.tryGetContext('legacySynth') === 'true' ?
new LegacyStackSynthesizer({
fileAssetsBucketName: parent.node.tryGetContext('bootstrapBucket'),
})
: new DefaultStackSynthesizer({
fileAssetsBucketName: parent.node.tryGetContext('bootstrapBucket'),
})
super(parent, id, {
...props,
synthesizer: synthesizer,
});

const fn = new lambda.Function(this, 'my-function', {
code: lambda.Code.asset(path.join(__dirname, 'lambda')),
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import * as fs from 'fs';
import * as path from 'path';
import { integTest, randomString, withDefaultFixture } from '../../lib';
import { integTest, randomString, withoutBootstrap } from '../../lib';

jest.setTimeout(2 * 60 * 60_000); // Includes the time to acquire locks, worst-case single-threaded runtime

integTest('can bootstrap without execution', withDefaultFixture(async (fixture) => {
integTest('can bootstrap without execution', withoutBootstrap(async (fixture) => {
const bootstrapStackName = fixture.bootstrapStackName;

await fixture.cdkBootstrapLegacy({
Expand All @@ -19,7 +19,7 @@ integTest('can bootstrap without execution', withDefaultFixture(async (fixture)
expect(resp.Stacks?.[0].StackStatus).toEqual('REVIEW_IN_PROGRESS');
}));

integTest('upgrade legacy bootstrap stack to new bootstrap stack while in use', withDefaultFixture(async (fixture) => {
integTest('upgrade legacy bootstrap stack to new bootstrap stack while in use', withoutBootstrap(async (fixture) => {
const bootstrapStackName = fixture.bootstrapStackName;

const legacyBootstrapBucketName = `aws-cdk-bootstrap-integ-test-legacy-bckt-${randomString()}`;
Expand All @@ -35,7 +35,12 @@ integTest('upgrade legacy bootstrap stack to new bootstrap stack while in use',

// Deploy stack that uses file assets
await fixture.cdkDeploy('lambda', {
options: ['--toolkit-stack-name', bootstrapStackName],
options: [
'--context', `bootstrapBucket=${legacyBootstrapBucketName}`,
'--context', 'legacySynth=true',
'--context', `@aws-cdk/core:bootstrapQualifier=${fixture.qualifier}`,
'--toolkit-stack-name', bootstrapStackName,
],
});

// Upgrade bootstrap stack to "new" style
Expand All @@ -49,13 +54,15 @@ integTest('upgrade legacy bootstrap stack to new bootstrap stack while in use',
// --force to bypass the check which says that the template hasn't changed.
await fixture.cdkDeploy('lambda', {
options: [
'--context', `bootstrapBucket=${newBootstrapBucketName}`,
'--context', `@aws-cdk/core:bootstrapQualifier=${fixture.qualifier}`,
'--toolkit-stack-name', bootstrapStackName,
'--force',
],
});
}));

integTest('can and deploy if omitting execution policies', withDefaultFixture(async (fixture) => {
integTest('can and deploy if omitting execution policies', withoutBootstrap(async (fixture) => {
const bootstrapStackName = fixture.bootstrapStackName;

await fixture.cdkBootstrapModern({
Expand All @@ -72,7 +79,7 @@ integTest('can and deploy if omitting execution policies', withDefaultFixture(as
});
}));

integTest('deploy new style synthesis to new style bootstrap', withDefaultFixture(async (fixture) => {
integTest('deploy new style synthesis to new style bootstrap', withoutBootstrap(async (fixture) => {
const bootstrapStackName = fixture.bootstrapStackName;

await fixture.cdkBootstrapModern({
Expand All @@ -90,7 +97,7 @@ integTest('deploy new style synthesis to new style bootstrap', withDefaultFixtur
});
}));

integTest('deploy new style synthesis to new style bootstrap (with docker image)', withDefaultFixture(async (fixture) => {
integTest('deploy new style synthesis to new style bootstrap (with docker image)', withoutBootstrap(async (fixture) => {
const bootstrapStackName = fixture.bootstrapStackName;

await fixture.cdkBootstrapModern({
Expand All @@ -108,7 +115,7 @@ integTest('deploy new style synthesis to new style bootstrap (with docker image)
});
}));

integTest('deploy old style synthesis to new style bootstrap', withDefaultFixture(async (fixture) => {
integTest('deploy old style synthesis to new style bootstrap', withoutBootstrap(async (fixture) => {
const bootstrapStackName = fixture.bootstrapStackName;

await fixture.cdkBootstrapModern({
Expand All @@ -119,12 +126,13 @@ integTest('deploy old style synthesis to new style bootstrap', withDefaultFixtur
// Deploy stack that uses file assets
await fixture.cdkDeploy('lambda', {
options: [
'--context', `@aws-cdk/core:bootstrapQualifier=${fixture.qualifier}`,
'--toolkit-stack-name', bootstrapStackName,
],
});
}));

integTest('can create a legacy bootstrap stack with --public-access-block-configuration=false', withDefaultFixture(async (fixture) => {
integTest('can create a legacy bootstrap stack with --public-access-block-configuration=false', withoutBootstrap(async (fixture) => {
const bootstrapStackName = fixture.bootstrapStackName;

await fixture.cdkBootstrapLegacy({
Expand All @@ -140,7 +148,7 @@ integTest('can create a legacy bootstrap stack with --public-access-block-config
]);
}));

integTest('can create multiple legacy bootstrap stacks', withDefaultFixture(async (fixture) => {
integTest('can create multiple legacy bootstrap stacks', withoutBootstrap(async (fixture) => {
const bootstrapStackName1 = `${fixture.bootstrapStackName}-1`;
const bootstrapStackName2 = `${fixture.bootstrapStackName}-2`;

Expand All @@ -162,7 +170,7 @@ integTest('can create multiple legacy bootstrap stacks', withDefaultFixture(asyn
]);
}));

integTest('can dump the template, modify and use it to deploy a custom bootstrap stack', withDefaultFixture(async (fixture) => {
integTest('can dump the template, modify and use it to deploy a custom bootstrap stack', withoutBootstrap(async (fixture) => {
let template = await fixture.cdkBootstrapModern({
// toolkitStackName doesn't matter for this particular invocation
toolkitStackName: fixture.bootstrapStackName,
Expand All @@ -188,7 +196,7 @@ integTest('can dump the template, modify and use it to deploy a custom bootstrap
});
}));

integTest('can use the default permissions boundary to bootstrap', withDefaultFixture(async (fixture) => {
integTest('can use the default permissions boundary to bootstrap', withoutBootstrap(async (fixture) => {
let template = await fixture.cdkBootstrapModern({
// toolkitStackName doesn't matter for this particular invocation
toolkitStackName: fixture.bootstrapStackName,
Expand All @@ -199,7 +207,7 @@ integTest('can use the default permissions boundary to bootstrap', withDefaultFi
expect(template).toContain('PermissionsBoundary');
}));

integTest('can use the custom permissions boundary to bootstrap', withDefaultFixture(async (fixture) => {
integTest('can use the custom permissions boundary to bootstrap', withoutBootstrap(async (fixture) => {
let template = await fixture.cdkBootstrapModern({
// toolkitStackName doesn't matter for this particular invocation
toolkitStackName: fixture.bootstrapStackName,
Expand All @@ -210,7 +218,7 @@ integTest('can use the custom permissions boundary to bootstrap', withDefaultFix
expect(template).toContain('permission-boundary-name');
}));

integTest('switch on termination protection, switch is left alone on re-bootstrap', withDefaultFixture(async (fixture) => {
integTest('switch on termination protection, switch is left alone on re-bootstrap', withoutBootstrap(async (fixture) => {
const bootstrapStackName = fixture.bootstrapStackName;

await fixture.cdkBootstrapModern({
Expand All @@ -229,7 +237,7 @@ integTest('switch on termination protection, switch is left alone on re-bootstra
expect(response.Stacks?.[0].EnableTerminationProtection).toEqual(true);
}));

integTest('add tags, left alone on re-bootstrap', withDefaultFixture(async (fixture) => {
integTest('add tags, left alone on re-bootstrap', withoutBootstrap(async (fixture) => {
const bootstrapStackName = fixture.bootstrapStackName;

await fixture.cdkBootstrapModern({
Expand All @@ -250,7 +258,7 @@ integTest('add tags, left alone on re-bootstrap', withDefaultFixture(async (fixt
]);
}));

integTest('can add tags then update tags during re-bootstrap', withDefaultFixture(async (fixture) => {
integTest('can add tags then update tags during re-bootstrap', withoutBootstrap(async (fixture) => {
const bootstrapStackName = fixture.bootstrapStackName;

await fixture.cdkBootstrapModern({
Expand All @@ -273,7 +281,7 @@ integTest('can add tags then update tags during re-bootstrap', withDefaultFixtur
]);
}));

integTest('can deploy modern-synthesized stack even if bootstrap stack name is unknown', withDefaultFixture(async (fixture) => {
integTest('can deploy modern-synthesized stack even if bootstrap stack name is unknown', withoutBootstrap(async (fixture) => {
const bootstrapStackName = fixture.bootstrapStackName;

await fixture.cdkBootstrapModern({
Expand All @@ -293,7 +301,7 @@ integTest('can deploy modern-synthesized stack even if bootstrap stack name is u
});
}));

integTest('create ECR with tag IMMUTABILITY to set on', withDefaultFixture(async (fixture) => {
integTest('create ECR with tag IMMUTABILITY to set on', withoutBootstrap(async (fixture) => {
const bootstrapStackName = fixture.bootstrapStackName;

await fixture.cdkBootstrapModern({
Expand Down

0 comments on commit c4ad3ac

Please sign in to comment.