Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(integ-runner): support --language presets for TypeScript and JavaScript #22927

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion packages/@aws-cdk/integ-runner/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,24 @@ to be a self contained CDK app. The runner will execute the following for each f
- `--disable-update-workflow` (default=`false`)
If this is set to `true` then the [update workflow](#update-workflow) will be disabled
- `--app`
The custom CLI command that will be used to run the test files. You can include {filePath} to specify where in the command the test file path should be inserted. Example: --app="python3.8 {filePath}".
The custom CLI command that will be used to run the test files. You can include {filePath} to specify where in the command the test file path should be inserted. Example: --app="python3.8 {filePath}".

Takes precedence over language presets.
- `--test-regex`
Detect integration test files matching this JavaScript regex pattern. If used multiple times, all files matching any one of the patterns are detected.

Takes precedence over language presets.
- `--language`
The language presets to use. You can discover and run tests written in multiple languages by passing this flag multiple times (`--language javascript --language typescript`). Defaults to all supported languages. Currently supported language presets are:
- `javascript`:
- File RegExp: `^integ\..*\.js$`
- App run command: `node {filePath}`
- `typescript`:
- File RegExp: `^integ\..*(?<!\.d)\.ts$`
- App run command: `node -r ts-node/register {filePath}`

For TypeScript files compiled to JavaScript, the JS tests will take precedence and the TS ones won't be evaluated.

Example:

```bash
Expand Down
5 changes: 4 additions & 1 deletion packages/@aws-cdk/integ-runner/lib/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export async function main(args: string[]) {
.option('disable-update-workflow', { type: 'boolean', default: false, desc: 'If this is "true" then the stack update workflow will be disabled' })
.option('app', { type: 'string', default: undefined, desc: 'The custom CLI command that will be used to run the test files. You can include {filePath} to specify where in the command the test file path should be inserted. Example: --app="python3.8 {filePath}".' })
.option('test-regex', { type: 'array', desc: 'Detect integration test files matching this JavaScript regex pattern. If used multiple times, all files matching any one of the patterns are detected.', default: [] })
.option('language', { type: 'array', default: ['javascript', 'typescript'], desc: 'The language presets to run tests for. Only javascript and typescript are supported for now.' })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.option('language', { type: 'array', default: ['javascript', 'typescript'], desc: 'The language presets to run tests for. Only javascript and typescript are supported for now.' })
.option('language', { type: 'array', choices: ['javascript', 'typescript'], default: ['javascript', 'typescript'], desc: 'The language presets to run tests for.' })

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to check if adding defaultDescription: 'all supported languages' provides a better ux

.strict()
.parse(args);

Expand All @@ -51,6 +52,7 @@ export async function main(args: string[]) {
const fromFile: string | undefined = argv['from-file'];
const exclude: boolean = argv.exclude;
const app: string | undefined = argv.app;
const language = arrayFromYargs(argv.language);

let failedSnapshots: IntegTestWorkerConfig[] = [];
if (argv['max-workers'] < testRegions.length * (profiles ?? [1]).length) {
Expand All @@ -60,7 +62,7 @@ export async function main(args: string[]) {
let testsSucceeded = false;
try {
if (argv.list) {
const tests = await new IntegrationTests(argv.directory).fromCliArgs({ testRegex, app });
const tests = await new IntegrationTests(argv.directory).fromCliArgs({ testRegex, app, language });
process.stdout.write(tests.map(t => t.discoveryRelativeFileName).join('\n') + '\n');
return;
}
Expand All @@ -77,6 +79,7 @@ export async function main(args: string[]) {
testRegex,
tests: requestedTests,
exclude,
language,
})));

// always run snapshot tests, but if '--force' is passed then
Expand Down
76 changes: 71 additions & 5 deletions packages/@aws-cdk/integ-runner/lib/runner/integration-tests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,22 @@ export interface IntegTestInfo {
* Derived information for IntegTests
*/
export class IntegTest {
private static readonly defaultSuffixes = new Map<string, RegExp>([
['javascript', new RegExp(/\.js$/)],
// Allow files ending in .ts but not in .d.ts
['typescript', new RegExp(/(?<!\.d)\.ts$/)],
]);

private static readonly defaultAppCommands = new Map<string, string>([
['javascript', 'node {filePath}'],
['typescript', 'node -r ts-node/register {filePath}'],
]);

private static getLanguage(fileName: string): string | undefined {
const [language] = Array.from(IntegTest.defaultSuffixes.entries()).find(([, regex]) => regex.test(fileName)) ?? [undefined, undefined];
return language;
}

Comment on lines +40 to +55
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels out of place here. See comment further down for a possible refactoring.

/**
* The name of the file to run
*
Expand Down Expand Up @@ -95,15 +111,22 @@ export class IntegTest {
*/
readonly appCommand: string;

/**
* Language the test is written in
*/
public readonly language?: string;

constructor(public readonly info: IntegTestInfo) {
this.appCommand = info.appCommand ?? 'node {filePath}';
this.absoluteFileName = path.resolve(info.fileName);
this.fileName = path.relative(process.cwd(), info.fileName);

const parsed = path.parse(this.fileName);
this.discoveryRelativeFileName = path.relative(info.discoveryRoot, info.fileName);
this.directory = parsed.dir;

this.language = IntegTest.getLanguage(parsed.base);
this.appCommand = info.appCommand ?? this.getDefaultAppCommand();

// if we are running in a package directory then just use the fileName
// as the testname, but if we are running in a parent directory with
// multiple packages then use the directory/filename as the testname
Expand All @@ -119,6 +142,19 @@ export class IntegTest {
this.temporaryOutputDir = path.join(this.directory, `${CDK_OUTDIR_PREFIX}.${parsed.base}.snapshot`);
}

private getDefaultAppCommand(): string {
if (!this.language) {
throw new Error(`Integration test '${this.fileName}' does not match any of the supported languages.`);
}

const defaultAppCommand = IntegTest.defaultAppCommands.get(this.language);
if (!defaultAppCommand) {
throw new Error(`No default app run command defined for language ${this.language}`);
}

return defaultAppCommand;
}

/**
* Whether this test matches the user-given name
*
Expand Down Expand Up @@ -172,6 +208,13 @@ export interface IntegrationTestsDiscoveryOptions {
* @default - test run command will be `node {filePath}`
*/
readonly app?: string;

/**
* List of language presets to discover tests for.
*
* @default - all supported languages
*/
readonly language?: string[];
}


Expand All @@ -190,6 +233,11 @@ export interface IntegrationTestFileConfig extends IntegrationTestsDiscoveryOpti
* Discover integration tests
*/
export class IntegrationTests {
private static readonly defaultDiscoveryRegexes = new Map<string, RegExp>([
['javascript', new RegExp(/^integ\..*\.js$/)],
// Allow files ending in .ts but not in .d.ts
['typescript', new RegExp(/^integ\..*(?<!\.d)\.ts$/)],
]);
Comment on lines +236 to +240
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see why you had to split the regexes and app commands. But the split and duplication with language detection feels odd.

constructor(private readonly directory: string) {
}

Expand Down Expand Up @@ -248,15 +296,33 @@ export class IntegrationTests {
}

private async discover(options: IntegrationTestsDiscoveryOptions): Promise<IntegTest[]> {
const patterns = options.testRegex ?? ['^integ\\..*\\.js$'];
const languagePresets = options.language ?? Array.from(IntegrationTests.defaultDiscoveryRegexes.keys());
const patterns = options.testRegex?.map((pattern) => new RegExp(pattern))
?? Array.from(IntegrationTests.defaultDiscoveryRegexes.entries()).filter(
([language]) => languagePresets.includes(language),
).map(([_, regex]) => regex);

const files = await this.readTree();
const integs = files.filter(fileName => patterns.some((p) => {
const regex = new RegExp(p);
const integs = files.filter(fileName => patterns.some((regex) => {
return regex.test(fileName) || regex.test(path.basename(fileName));
}));

Comment on lines -251 to 309
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay so here's an idea how we could streamline the language presets a bit. Instead of IntegrationTestsDiscoveryOptions having all three: testRegex app and language we introduce a new map:

{
  "app command": ["test regex"],
  // e.g.
  "node {filePath}": ['^integ\\..*\\.js$']
}

(In case of --app and/or --test-regex we just amend the map accordingly)

That way you could mostly keep the current code, but add an outer loop around it:

for (const [appCommand, patterns] of Object.entries(options.theNewMapThingy)) {
  // do the old thing here
  // but also map the matched files to IntegTests
  const integs = files
    .filter(fileName => patterns.some((pattern) => {
      const regex = new RegExp(pattern);
      return regex.test(fileName) || regex.test(path.basename(fileName));
    }))
    .map(fileName => new IntegTest({
      discoveryRoot: this.directory,
      fileName,
      appCommand, // this is now the `appCommand` from `theNewMapThingy`
    })); 
}

What do you think?

This would probably mean we need to move the conversion from list of language presets and/or --app/--test-regex to somewhere and call it from cli.ts.

return this.request(integs, options);
const discoveredTestNames = new Set<string>();
const integsWithoutDuplicates = new Array<string>();

// Remove tests with duplicate names.
// To make sure the precendence of files is deterministic, iterate the files in lexicographic order.
// Additionally, to give precedence to compiled .js files over their .ts source,
// use ascending lexicographic ordering, so the .ts files are picked up first.
for (const integFileName of integs.sort()) {
const testName = path.parse(integFileName).name;
if (!discoveredTestNames.has(testName)) {
integsWithoutDuplicates.push(integFileName);
}
discoveredTestNames.add(testName);
}
Comment on lines +317 to +323
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if still need a generic de-duplication or just a special case if languages includes 'typescript' AND 'javascript'

Either way could we reduce duplication if we added the files to be ignored to options.exclude and let filterTests() do its work?


return this.request(integsWithoutDuplicates, options);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the above changes, I think it's time to just merge request() into this function. Will make it bit easier.

}

private request(files: string[], options: IntegrationTestsDiscoveryOptions): IntegTest[] {
Expand Down
9 changes: 6 additions & 3 deletions packages/@aws-cdk/integ-runner/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -52,15 +52,18 @@
"license": "Apache-2.0",
"devDependencies": {
"@aws-cdk/cdk-build-tools": "0.0.0",
"@types/mock-fs": "^4.13.1",
"mock-fs": "^4.14.0",
"@aws-cdk/core": "^0.0.0",
"@aws-cdk/integ-tests": "^0.0.0",
"@aws-cdk/pkglint": "0.0.0",
"@types/fs-extra": "^8.1.2",
"@types/jest": "^27.5.2",
"@types/mock-fs": "^4.13.1",
"@types/node": "^14.18.33",
"@types/workerpool": "^6.1.0",
"@types/yargs": "^15.0.14",
"jest": "^27.5.1"
"constructs": "^10.0.0",
"jest": "^27.5.1",
"mock-fs": "^4.14.0"
},
"dependencies": {
"@aws-cdk/cloud-assembly-schema": "0.0.0",
Expand Down
11 changes: 11 additions & 0 deletions packages/@aws-cdk/integ-runner/test/cli.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,15 @@ describe('CLI', () => {
].join('\n'),
]]);
});

test('find only TypeScript files', async () => {
await main(['--list', '--language', 'typescript', '--directory=test/test-data-typescript', '--test-regex="^xxxxx\\..*(?<!\\.d)\\.ts$"']);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
await main(['--list', '--language', 'typescript', '--directory=test/test-data-typescript', '--test-regex="^xxxxx\\..*(?<!\\.d)\\.ts$"']);
await main(['--list', '--language', 'typescript', '--directory=test/test-data-typescript']);

Otherwise this is just testing the regex, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test won't be picked up if we don't specify the regex, because it doesn't have the standard prefix integ. but rather xxxxx.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, see comment re renaming the test file.
#22927 (comment)


expect(stdoutMock.mock.calls).toEqual([[
[
'xxxxx.typescript-test.ts',
'',
].join('\n'),
]]);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -588,4 +588,32 @@ describe('IntegTest runIntegTests', () => {
app: 'node --no-warnings xxxxx.test-with-snapshot.js',
}));
});

test('with default preset for TypeScript', () => {
// WHEN
const integTest = new IntegTestRunner({
cdk: cdkMock.cdk,
test: new IntegTest({
fileName: 'test/test-data-typescript/xxxxx.typescript-test.ts',
discoveryRoot: 'test/test-data-typescript',
}),
});
integTest.runIntegTestCase({
testCaseName: 'xxxxx.typescript-test',
});

// THEN
expect(deployMock).toHaveBeenCalledTimes(1);
expect(destroyMock).toHaveBeenCalledTimes(1);
expect(synthFastMock).toHaveBeenCalledTimes(1);
expect(deployMock).toHaveBeenCalledWith(expect.objectContaining({
app: 'node -r ts-node/register xxxxx.typescript-test.ts',
}));
expect(synthFastMock).toHaveBeenCalledWith(expect.objectContaining({
execCmd: ['node', '-r', 'ts-node/register', 'xxxxx.typescript-test.ts'],
}));
expect(destroyMock).toHaveBeenCalledWith(expect.objectContaining({
app: 'node -r ts-node/register xxxxx.typescript-test.ts',
}));
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,14 @@ describe('IntegrationTests', () => {
mockfs({
'test/test-data': {
'integ.integ-test1.js': 'content',
'integ.integ-test1.ts': 'content',
'integ.integ-test1.d.ts': 'should not match',
'integ.integ-test2.js': 'content',
'integ.integ-test2.ts': 'content',
'integ.integ-test2.d.ts': 'should not match',
'integ.integ-test3.js': 'content',
'integ.integ-test3.ts': 'content',
'integ.integ-test3.d.ts': 'should not match',
'integration.test.js': 'should not match',
},
'other/other-data': {
Expand Down Expand Up @@ -166,5 +172,31 @@ describe('IntegrationTests', () => {
expect(integTests.length).toEqual(3);
expect(integTests[0].appCommand).toEqual('node --no-warnings {filePath}');
});

test('TypeScript compiled to JavaScript, does not pick up the compiled tests for both .ts and .js versions', async () => {
const tsCompiledTests = new IntegrationTests('test');
const integTests = await tsCompiledTests.fromCliArgs();

expect(integTests.length).toEqual(3);
});

test('TypeScript compiled to JavaScript, gives precedence to JavaScript files', async () => {
const tsCompiledTests = new IntegrationTests('test');
const integTests = await tsCompiledTests.fromCliArgs();

for (const test of integTests) {
expect(test.fileName).toEqual(expect.stringMatching(/integ.integ-test[1-3].js/));
}
});

test('TypeScript .d.ts files should be ignored', async () => {
writeConfig({ language: ['typescript'] });

const tsCompiledTests = new IntegrationTests('test');
const integTests = await tsCompiledTests.fromFile(configFile);

expect(integTests.length).toEqual(3);
expect(integTests[0].fileName).toEqual(expect.stringMatching(/integ.integ-test1.ts/));
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import { App, Stack } from '@aws-cdk/core';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is okay to be named integ.typescript-test.ts

(The other ones aren't because they are empty files that only need to exist for mocking purposes and we should probably try to just get rid of them at some point)

import { IntegTest } from '@aws-cdk/integ-tests';

const app = new App();
const stack = new Stack();

new IntegTest(app, 'Integ', { testCases: [stack] });

app.synth();
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
{
"version": "21.0.0",
"files": {
"21fbb51d7b23f6a6c262b46a9caee79d744a3ac019fd45422d988b96d44b2a22": {
"source": {
"path": "IntegDefaultTestDeployAssert4E6713E1.template.json",
"packaging": "file"
},
"destinations": {
"current_account-current_region": {
"bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}",
"objectKey": "21fbb51d7b23f6a6c262b46a9caee79d744a3ac019fd45422d988b96d44b2a22.json",
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}"
}
}
}
},
"dockerImages": {}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
{
"Parameters": {
"BootstrapVersion": {
"Type": "AWS::SSM::Parameter::Value<String>",
"Default": "/cdk-bootstrap/hnb659fds/version",
"Description": "Version of the CDK Bootstrap resources in this environment, automatically retrieved from SSM Parameter Store. [cdk:skip]"
}
},
"Rules": {
"CheckBootstrapVersion": {
"Assertions": [
{
"Assert": {
"Fn::Not": [
{
"Fn::Contains": [
[
"1",
"2",
"3",
"4",
"5"
],
{
"Ref": "BootstrapVersion"
}
]
}
]
},
"AssertDescription": "CDK bootstrap stack version 6 required. Please run 'cdk bootstrap' with a recent version of the CDK CLI."
}
]
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"version":"21.0.0"}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
"version": "21.0.0",
"testCases": {
"Integ/DefaultTest": {
"stacks": [
"Default"
],
"assertionStack": "Integ/DefaultTest/DeployAssert",
"assertionStackName": "IntegDefaultTestDeployAssert4E6713E1"
}
}
}
Loading