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

karakter98 · 2022-11-15T23:17:58Z

Users can now run integration tests written in any CDK language since #22761 and #22786 added support for custom --app run commands and --test-regex to discover files with naming conventions specific to their language.

Since it's cumbersome and error-prone to always specify these flags when running the tests, this PR makes the first step to introduce language presets with default app run commands and test file regex discovery for each language.

This PR only introduces javascript and typescript presets, but it can easily be extended to all the other CDK-supported languages.

All supported language presets are enabled by default, but users can also specify a single language or a list of languages to use for running the tests (integ-runner --language javascript --language typescript).

See the discussion at #22521 for more background on this topic.

Shoutout to @mrgrain for the amazing collaboration and insights for this feature!

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- 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

gitpod-io · 2022-11-15T23:18:03Z

…n. Added CLI test for typescript files.

aws-cdk-automation · 2022-11-16T12:11:38Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: d2b89f6
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mrgrain

Couple minor (but blocking) changes.

But also a bigger refactoring idea, non-blocking. Let me know what you think. I don't want you to do unnecessary work if you feel it's not really worth it (I can always refactor this myself later)

mrgrain · 2022-11-16T12:57:33Z

packages/@aws-cdk/integ-runner/test/cli.test.ts

@@ -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$"']);


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?

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.

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

mrgrain · 2022-11-16T13:00:50Z

packages/@aws-cdk/integ-runner/lib/cli.ts

@@ -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.' })


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.' })

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

mrgrain · 2022-11-16T13:16:05Z

packages/@aws-cdk/integ-runner/test/test-data-typescript/xxxxx.typescript-test.ts

@@ -0,0 +1,9 @@
+import { App, Stack } from '@aws-cdk/core';


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)

mrgrain · 2022-11-16T13:19:54Z

packages/@aws-cdk/integ-runner/lib/runner/integration-tests.ts

+  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;
+  }
+


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

mrgrain · 2022-11-16T13:20:44Z

packages/@aws-cdk/integ-runner/lib/runner/integration-tests.ts

+  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$/)],
+  ]);


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

mrgrain · 2022-11-16T13:22:09Z

packages/@aws-cdk/integ-runner/lib/runner/integration-tests.ts

+      discoveredTestNames.add(testName);
+    }
+
+    return this.request(integsWithoutDuplicates, options);


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

mrgrain · 2022-11-16T13:24:46Z

packages/@aws-cdk/integ-runner/lib/runner/integration-tests.ts

+    for (const integFileName of integs.sort()) {
+      const testName = path.parse(integFileName).name;
+      if (!discoveredTestNames.has(testName)) {
+        integsWithoutDuplicates.push(integFileName);
+      }
+      discoveredTestNames.add(testName);
+    }


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?

mrgrain · 2022-11-16T13:38:07Z

packages/@aws-cdk/integ-runner/lib/runner/integration-tests.ts

-    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));
    }));



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.

aws-cdk-automation · 2022-11-20T00:23:01Z