Skip to content

Commit

Permalink
fix(integ-runner): don't allow new legacy tests (#20614)
Browse files Browse the repository at this point in the history
All new integration tests that are created should use the new
`IntegTest` construct in the
[@aws-cdk/integ-tests](https://github.com/aws/aws-cdk/tree/main/packages/%40aws-cdk/integ-tests)
module.

We will eventually be migrating all of our tests to the new construct
and removing the "legacy" mode, so this PR will fail any new test that
is added that doesn't use the `IntegTest` construct.

I've also renamed some of the `test-data` files so that they will not be
picked up by the `integ-runner` if you execute it on the entire repo.


----

### 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 authored Jun 13, 2022
1 parent f98029e commit c946615
Show file tree
Hide file tree
Showing 20 changed files with 249 additions and 184 deletions.
24 changes: 8 additions & 16 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ Work your magic. Here are some guidelines:

Integration tests perform a few functions in the CDK code base -
1. Acts as a regression detector. It does this by running `cdk synth` on the integration test and comparing it against
the `*.expected.json` file. This highlights how a change affects the synthesized stacks.
the `*.integ.snapshot` directory. This highlights how a change affects the synthesized stacks.
2. Allows for a way to verify if the stacks are still valid CloudFormation templates, as part of an intrusive change.
This is done by running `yarn integ` which will run `cdk deploy` across all of the integration tests in that package. If you are developing a new integration test or for some other reason want to work on a single integration test over and over again without running through all the integration tests you can do so using `yarn integ integ.test-name.js`
Remember to set up AWS credentials before doing this.
Expand All @@ -275,9 +275,10 @@ new features unless there is a good reason why one is not needed.
4. Adding a new supported version (e.g. a new [AuroraMysqlEngineVersion](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_rds.AuroraMysqlEngineVersion.html))
5. Adding any functionality via a [Custom Resource](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.custom_resources-readme.html)

To the extent possible, include a section (like below) in the integration test file that specifies how the successfully
deployed stack can be verified for correctness. Correctness here implies that the resources have been set up correctly.
The steps here are usually AWS CLI commands but they need not be.
All integration tests going forward should use the [IntegTest](https://github.com/aws/aws-cdk/tree/main/packages/%40aws-cdk/integ-tests)
construct. Over time we will be updating all of our existing tests to use this construct. It
allows for more control over configuring each tests as well as the ability to perform
assertions against the deployed infrastructure.

```ts
/*
Expand All @@ -288,8 +289,8 @@ The steps here are usually AWS CLI commands but they need not be.
```

Examples:
* [integ.destinations.ts](https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk/aws-lambda-destinations/test/integ.destinations.ts#L7)
* [integ.token-authorizer.lit.ts](https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk/aws-apigateway/test/authorizers/integ.token-authorizer.lit.ts#L7-L12)
* [integ.destinations.ts](https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-lambda-destinations/test/integ.destinations.ts#L7)
* [integ.put-events.ts](https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk/aws-stepfunctions-tasks/test/eventbridge/integ.put-events.ts)

**What do do if you cannot run integration tests**

Expand Down Expand Up @@ -827,16 +828,7 @@ $ <path to the AWS CDK repo>/link-all.sh

### Running integration tests in parallel

Integration tests may take a long time to complete. We can speed this up by running them in parallel
in different regions.

```shell
# Install GNU parallel (may require uninstall 'moreutils' if you have it)
$ apt-get install parallel
$ brew install parallel
$ scripts/run-integ-parallel @aws-cdk/aws-ec2 @aws-cdk/aws-autoscaling ...
```
See the [Integration testing guide](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md#running-large-numbers-of-tests)

### Visualizing dependencies in a CloudFormation Template

Expand Down
52 changes: 51 additions & 1 deletion INTEGRATION_TESTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ _integ.lambda.ts_
import * as iam from '@aws-cdk/aws-iam';
import * as cdk from '@aws-cdk/core';
import * as lambda from '../lib';
import * as integ from '@aws-cdk/integ-tests';

const app = new cdk.App();

Expand All @@ -96,6 +97,10 @@ const fn = new lambda.Function(stack, 'MyLambda', {
runtime: lambda.Runtime.NODEJS_14_X,
});

new integ.IntegTest(app, 'LambdaTest', {
testCases: [stack],
});

app.synth();
```

Expand Down Expand Up @@ -223,7 +228,52 @@ but it will not validate that the Lambda function can be invoked. Because of thi
to deploy the Lambda Function _and_ then rerun the assertions to ensure that the function can still be invoked.

### Assertions
...Coming soon...

Sometimes it is necessary to perform some form of _assertion_ against the deployed infrastructure to validate that the
test succeeds. A good example of this is the `@aws-cdk/aws-stepfunctions-tasks` module which creates integrations between
AWS StepFunctions and other AWS services.

If we look at the [integ.put-events.ts](https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk/aws-stepfunctions-tasks/test/eventbridge/integ.put-events.ts)
integration test we can see that we are creating an `@aws-cdk/aws-events.EventBus` along with a `@aws-cdk/aws-stepfunctions.StateMachine`
which will send an event to the `EventBus`. In a typical integration test we would just deploy the test and the fact that the
infrastructure deployed successfully would be enough of a validation that the test succeeded. In this case though, we ideally
want to validate that the _integration_ connecting `StepFunctions` to the `EventBus` has been setup correctly, and the only
way to do that is to actually trigger the `StateMachine` and validate that it was successful.

```ts
declare const app: App;
declare const sm: sfn.StateMachine;
declare const stack: Stack;

const testCase = new IntegTest(app, 'PutEvents', {
testCases: [stack],
});

// Start an execution
const start = testCase.assertions.awsApiCall('StepFunctions', 'startExecution', {
stateMachineArn: sm.stateMachineArn,
});

// describe the results of the execution
const describe = testCase.assertions.awsApiCall('StepFunctions', 'describeExecution', {
executionArn: start.getAttString('executionArn'),
});

// assert the results
describe.expect(ExpectedResult.objectLike({
status: 'SUCCEEDED',
}));
```

Not every test requires an assertion. We typically do not need to assert CloudFormation behavior. For example, if we create an S3 Bucket
with Encryption, we do not need to assert that Encryption is set on the bucket. We can trust that the CloudFormation behavior works.
Some things you should look for in deciding if the test needs an assertion:

- Integrations between services (i.e. integration libraries like `@aws-cdk/aws-lambda-destinations`, `@aws-cdk/aws-stepfunctions-tasks`, etc)
- Anything that bundles or deploys custom code (i.e. does a Lambda function bundled with `@aws-cdk/aws-lambda-nodejs` still invoke or did we break bundling behavior)
- IAM/Networking connections.
- This one is a bit of a judgement call. Most things do not need assertions, but sometimes we handle complicated configurations involving IAM permissions or
Networking access.

## Running Integration Tests

Expand Down
10 changes: 10 additions & 0 deletions packages/@aws-cdk/integ-runner/lib/runner/integ-test-runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,16 @@ export class IntegTestRunner extends IntegRunner {
constructor(options: IntegRunnerOptions, destructiveChanges?: DestructiveChange[]) {
super(options);
this._destructiveChanges = destructiveChanges;

// We don't want new tests written in the legacy mode.
// If there is no existing snapshot _and_ this is a legacy
// test then point the user to the new `IntegTest` construct
if (!this.hasSnapshot() && this.isLegacyTest) {
throw new Error(`${this.testName} is a new test. Please use the IntegTest construct ` +
'to configure the test\n' +
'https://github.com/aws/aws-cdk/tree/main/packages/%40aws-cdk/integ-tests',
);
}
}

/**
Expand Down
2 changes: 2 additions & 0 deletions packages/@aws-cdk/integ-runner/lib/runner/runner-base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ export abstract class IntegRunner {

protected _destructiveChanges?: DestructiveChange[];
private legacyContext?: Record<string, any>;
protected isLegacyTest?: boolean;

constructor(options: IntegRunnerOptions) {
this.test = options.test;
Expand Down Expand Up @@ -214,6 +215,7 @@ export abstract class IntegRunner {
},
});
this.legacyContext = LegacyIntegTestSuite.getPragmaContext(this.test.fileName);
this.isLegacyTest = true;
return testCases;
}
}
Expand Down
Loading

0 comments on commit c946615

Please sign in to comment.