feat(integ-runner): Support non-TypeScript tests #22521

karakter98 · 2022-10-16T16:03:25Z

At the moment, the integ-tests library is only useful to TypeScript users of CDK, because integ-runner defaults to evaluating integration test files with node.

This PR adds a new parameter to the integ-runner CLI, to specify a different run command for the test files. The default run command is still node, to keep backwards compatibility.

Example usage:
integ-runner --test-run-command python3

Specifying the run command like this is needed because integration tests don't have a cdk.json file where their --app flag can be found (as mentioned in this thread).

Another important change is to the expected naming convention of integration tests. Up until now, all tests were expected to be prefixed with the "integ." prefix. This is fine for TypeScript/JavaScript, but other CDK languages have different naming conventions for source files:

Python (and to my knowledge, Go) use snake_case
Java and C# use PascalCase
TypeScript and JavaScript can use snake case with different separators, as we can see in the CDK codebase itself:
- runner-base.ts
- extract_worker.ts
- integ.test.ts

To accommodate all of these different naming conventions, the test discovery logic was changed to use a RegEx that works with the following prefix patterns:

IntegTest.cs
integ_test.py
integ.test.ts
integ-test.ts

Also, another RegEx for valid file extensions was added (.js, .py, etc) to distinguish files generated by compiled languages (for example, TypeScript generates .js files, and the .js files should be tested, not the .ts ones). I am not sure about the valid extensions for C# and Java tests, so if someone with experience in those languages can take a look, it would be much appreciated.

I checked the changes locally with this command and an integration test written in Python:
integ-runner --directory test/test-data-python --update-on-failed --dry-run --test-run-command python3

And Java:
export $CLASSPATH="$(cd test/test-data-java; mvn dependency:build-classpath -q -Dmdep.outputFile=/dev/stdout)"; integ-runner --directory test/test-data-java/src/main/java/com/myorg --update-on-failed --dry-run --test-run-command="java -cp $CLASSPATH"

The Java command is a little awkward, but I couldn't find a way to pass mvn a file name, so I used the single-file source code feature from JDK 11 to test it. I assume there are better ways, but that's the best I could come up with.

I committed the generated snapshot and a unit test. I don't think integration tests are needed, since there are no changes to existing constructs.

Fixes #21169

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

…or the tests. This allows for tests written in other languages than TypeScript to be evaluated. Changed integration test file discovery to work with the naming conventions of all languages supported by CDK. Updated unit tests for integ-runner.

gitpod-io · 2022-10-16T16:03:33Z

…was problematic and broke some existing tests (unit tests in TS named integration-tests.ts for example). Changed to use a different regex pattern to discover tests with different extensions.

mrgrain · 2022-10-17T10:52:51Z

Hey @karakter98 this is great! I've started similar work in #22058 but your PR looks much better.

One note though that @corymhall and I have been discussing a slightly different interface:

--language [javascript|typescript|python|java] Use a preset to run integration tests for the selected language.

--app [string] The app to execute integration tests with. Use `{filePath}` to mark the path of the app under test: `python {filePath}`

--test-match [array<string>] The glob patterns used to detect integration test files. See documentation for the defaults for each language.

Naming of these options aside, adding --language presets as an easy way to switch is what's important to us.

--app is what you've called --test-run-command with the addition of {filePath}. Do you think this would make the Java command any easier?

--test-match is really just an option to change the regex for test files. I'd be okay with this not being implemented right away, although it's probably not hard. I went with glob patterns but there's no real reasons for this.

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

karakter98 · 2022-10-17T12:58:46Z

Hey @karakter98 this is great! I've started similar work in #22058 but your PR looks much better.

Sorry for stepping over the existing draft PR, but the last activity on it was 23 days ago, so I just assumed it went stale and went ahead to open a new one

karakter98 · 2022-10-17T13:11:04Z

--language [javascript|typescript|python|java] Use a preset to run integration tests for the selected language.

--app [string] The app to execute integration tests with. Use {filePath} to mark the path of the app under test: python {filePath}

--test-match [array] The glob patterns used to detect integration test files. See documentation for the defaults for each language.

This actually looks great to me, I did struggle a bit to test the Java version, so some default implementation would be a great UX improvement. I also like the idea of marking {filePath} in the --app flag, it allows for more complex commands.

I assume that the custom match pattern falls back to some defaults per language, as in my current implementation? I believe it would be trivial to implement it.

About the naming, I wanted to avoid having an --app parameter to not be confused with the CDK CLI --app parameter which had a little different structure, but if we introduce {filePath} the format is kind of the same:
CDK CLI: --app "python3 app.py"
integ-runner: --app "python3 {filePath}"

mrgrain · 2022-10-17T13:12:48Z

Sorry for stepping over the existing draft PR, but the last activity on it was 23 days ago, so I just assumed it went stale and went ahead to open a new one

All good, you are 100% right about my PR becoming stale. :D This was mostly for reference, but it sounds like you did already come across it anyway.

mrgrain · 2022-10-17T13:17:58Z

I assume that the custom match pattern falls back to some defaults per language, as in my current implementation? I believe it would be trivial to implement it.

Yes, pretty much like what you did. --language is what most users would use and server as a preset for --app and --test-matches unless they are explicitly provided.

About the naming, I wanted to avoid having an --app parameter to not be confused with the CDK CLI --app parameter which had a little different structure, but if we introduce {filePath} the format is kind of the same: CDK CLI: --app "python3 app.py" integ-runner: --app "python3 {filePath}"

Tbh, we went back on forth on it. Like you we liked the similarity to CDK CLI. But --test-run-command is equally good.

karakter98 · 2022-10-17T13:20:20Z

What do you think about, instead of another --language flag for the presets, we would just infer the language from the file extension? At least right now, all CDK languages have different extensions, so it seems doable, and makes it even easier for the end user.

mrgrain · 2022-10-17T13:31:12Z

What do you think about, instead of another --language flag for the presets, we would just infer the language from the file extension? At least right now, all CDK languages have different extensions, so it seems doable, and makes it even easier for the end user.

I'm definitely open to this idea. What I'm not sure about is how this would work in practice. I imagine we would have to scan for all patterns and would then run tests for multiple languages all at once? Is that a good thing? How would it work with co-located JS/TS files, would these tests just run twice or would we give preference to one of them?

karakter98 · 2022-10-17T13:56:32Z

It would allow for tests in different languages to run at the same time, since the app command is set per test in runner_base.ts. I don't necessarily believe it's a bad thing. Maybe someone will have a use case for this. If the codebase uses a single language for the integ tests, it doesn't really change anything, but if you're multi-language (e.g. Python and TypeScript monorepo), this might be desirable.

We could just say "if you need custom --app commands in a multi-language project, then separate the tests by language", so we don't overcomplicate to allow for different --app commands per language at the same time. Using only the default commands in multi-language projects seems good enough.

Regarding TS/JS interaction, if we have a test written in TS named integ.test_ts.ts and one written in JS named integ.test_js.js, they would both run as JS tests, since the TS one would be compiled to a file named integ.test_ts.js. It would be the same as 2 TypeScript or 2 JavaScript tests in the same directory.

If the TS and JS files had the same name, but different extensions, then the JS file would get overwritten at build time by the compiled TS version, but this is not something we can solve from our code, it's just the way the build process works. Since they need to have the same file name, it seems too narrow of an edge case.

I think it's safe to do it, but if there are any concerns, we can just go the --language flag route. I just think users aren't used to this, because the CDK CLI usually knows the language already, so it would be a small difference that might trip people up (and it's probably easy to miss in the docs).

mrgrain · 2022-10-17T14:26:49Z

It would allow for tests in different languages to run at the same time, since the app command is set per test in runner_base.ts. I don't necessarily believe it's a bad thing. Maybe someone will have a use case for this. If the codebase uses a single language for the integ tests, it doesn't really change anything, but if you're multi-language (e.g. Python and TypeScript monorepo), this might be desirable.

Yep, that makes a lot of sense.

Regarding TS/JS interaction, if we have a test written in TS named integ.test_ts.ts and one written in JS named integ.test_js.js, they would both run as JS tests, since the TS one would be compiled to a file named integ.test_ts.js. It would be the same as 2 TypeScript or 2 JavaScript tests in the same directory.

If the TS and JS files had the same name, but different extensions, then the JS file would get overwritten at build time by the compiled TS version, but this is not something we can solve from our code, it's just the way the build process works. Since they need to have the same file name, it seems too narrow of an edge case.

I'm coming from the assumption that we need support for running TypeScript tests directly via ts-node. In this scenario there is no compiled JS file. This is a pattern we encourage users to use via the app template as well as projen.

On the other hand, we have projects where source TS files and compiled JS files are placed in the same directory (aws-cdk itself does that). So they are exactly the same test, not just two files that happen to have the same name (in that latter case I fully agree with you).

So I'm really only concerned about TS projects that are compiled to JS and places in the same directory. This appears to be a quite common scenario to me.

I think it's safe to do it, but if there are any concerns, we can just go the --language flag route. I just think users aren't used to this, because the CDK CLI usually knows the language already, so it would be a small difference that might trip people up (and it's probably easy to miss in the docs).

I'd like to solve it, because I think you're right that auto-detection would be the much better UX. Either way we should have --language to allow selecting the behaviour. Tbh it can probably be a list as well, so a user could do integ-runner -l java -l python and explicitly run Java and Python integ tests.

To me this now sounds like we have only one open question: What should the default value of --language be?

none - always force user to specify a language
javascript - very dumb and safe default
all languages - look for tests in all supported languages
all languages but be "clever" around JS/TS, e.g. prefer one of the other if two files are named the same
something else?

karakter98 · 2022-10-17T14:59:51Z

Let's go with

all languages but be "clever" around JS/TS, e.g. prefer one of the other if two files are named the same

If we suppose that same filename (without extension) for .js and .ts files is due to compilation, and not user error, then it means they are effectively the same test. We can filter the duplicates out in the discover() call, after gathering all the tests.

We can actually go further, and filter out tests with the same name in general for any languages, because they would overwrite one another's snapshots anyway.

In this case, the TS RegEx also should exclude .d.ts generated files.

If all sounds good, I'll get to work on this.

mrgrain · 2022-10-17T15:13:04Z

If all sounds good, I'll get to work on this.

I'm happy with this, thanks for the discussion & contribution! 🎉
Just a little caveat that @corymhall might have opinions on the language default. I think our approach is sound though.

While you're on the code, I can get to work and figure out the default RegEx and apps for each language.
I've already done another iteration and this is what I currently have for every language:

{
  javascript: new IntegrationTestPreset('node {filePath}', [new RegExp(/^integ\..*\.js$/)]),
  typescript: new IntegrationTestPreset('node -r ts-node/register {filePath}', [new RegExp(/^integ\.(?!.*\.d\.ts$).*\.ts$/)]),
  python: new IntegrationTestPreset(`${pythonExecutable()} {filePath}`, [new RegExp(/^integ_.*\.py$/)]),

  // these are guessed from the template app and still need some testing
  go: new IntegrationTestPreset('go mod download && go run {filePath}', [new RegExp(/^integ_.*\.go$/)]),
  csharp: new IntegrationTestPreset('dotnet run {filePath}', [new RegExp(/^Integ.*\.cs$/)]),
  fsharp: new IntegrationTestPreset('dotnet run {filePath}', [new RegExp(/^Integ.*\.fs$/)]),

  // No idea, yours will be good enough for now
  java: new IntegrationTestPreset('tbd', [new RegExp(/^Integ.*\.java$/)]),
};

Now implements default run commands for all languages. Implemented different matching patterns for different file extensions. Now filters out duplicate test names, and makes sure to give precedence to .ts files over their compiled .js files. Java default Maven run command is now inferred from the directory structure of the Java project. The classpath for each individual test will automatically be discovered, together with the closest pom.xml file in the project hierarchy.

…mpiled file, but broke if run with ts-node directly.

aws-cdk-automation · 2022-10-18T15:37:12Z

AWS CodeBuild CI Report

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

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

karakter98 · 2022-10-18T20:27:37Z

@mrgrain I found a solution to get the Java files executed properly in Maven projects, and implemented the rest of the stuff we talked about, please let me know if there's anything else you would like to address in this PR. Thanks for the input!

mrgrain · 2022-10-18T20:30:02Z

That's awesome! I'll review it tomorrow and double check the c/fsharp & go app strings.

mrgrain

Good stuff @karakter98! I dropped a few minor suggestions in. The bigger issues we need to address tough are:

Language Integration tests - Great work here so far. I'll need to check if/how we can handle them in our CI process. From what I can see we don't run them right now, which might be okay. I will also provide additional tests for the other languages.
We should support configuring the new options via config file as well. See fromFile in packages/@aws-cdk/integ-runner/lib/runner/integration-tests.ts
Give .js preference over .ts (see inline comment)
I've missed to explicitly state an additional functional requirement: Fully custom integration tests. With these changes, we would like the following to be possible: integ-runner --app 'cat {filePath}' --test-regex 'integ-.*\.yaml' (assuming cat {filePath} would provide a valid cdk app)
Or in other words: support running integ tests with completely custom apps and unknown file types, as long as both --app and --test-regex are provided.
I think with the current approach this is not possible. I can see a couple of options here, but from a UX perspective I wonder if this would make sense:
- if --language is provided we limit any automatics to the stored regular expressions for the selected language
- if only --app is provided we don't use automatics to detect the app command. We still use the regular expressions for all listed languages to detect files.
  Using --app only makes sense if either --test-regex is provided, or --language is explicitly provided or the test directory only contains tests in one language. We could also enforce this.
- if only--test-regex is provided, we use only the provided regex to match full filenames (not split in prefix/suffix)
  We would still use the (language filtered) suffix list to auto-detect the app command for a given file.
  - --test-regex can now also be an array option
- if both --app and --test-regex are provided we do not do any automatics. Effectively --language is ignored.

Let me know if you have any questions, especially re the fully custom integration tests feature. Apologies for not making this explicit earlier.

mrgrain · 2022-10-19T12:23:38Z

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

+
+    const language = new IntegrationTests(this.directory).getLanguage(parsed.base);
+    if (!language) {
+      throw new Error('Given file does not match any of the allowed languages for this test');


Suggested change

throw new Error('Given file does not match any of the allowed languages for this test');

throw new Error(`Integration test '${parsed.base}' does not match any of the supported languages.');

mrgrain · 2022-10-19T12:27:38Z

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

+    const suffix = this.suffixMapping.get(language);
+    const prefix = this.prefixMapping.get(language);
+
+    return <boolean>(suffix?.test(fileName) && prefix?.test(fileName));


Suggested change

return <boolean>(suffix?.test(fileName) && prefix?.test(fileName));

return Boolean(suffix?.test(fileName) && prefix?.test(fileName));

mrgrain · 2022-10-19T12:36:26Z

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

+    // Additionally, to give precedence to .ts files over their compiled .js version,
+    // use descending lexicographic ordering, so the .ts files are picked up first.


I've discussed this with the team and we now think that .js files should be given priority over .ts files.

In scenarios where TS is explicitly compiled to JS, it's usually preferred to run tests from the compiled version. Otherwise a user would have setup "on-the-fly" processes with ts-node and never produce compiled js files. This is also consistent with how ts-node works if a .js of the same file is present.

mrgrain · 2022-10-19T12:40:22Z

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

+    for (const integFileName of integs.sort().reverse()) {
+      const testName = this.stripPrefixAndSuffix(path.basename(integFileName));
+      if (!discoveredTestNames.has(testName)) {
+        integsWithoutDuplicates.push(integFileName);
+      }
+      discoveredTestNames.add(testName);
+    }


Let's add print a logger.warning() for every duplicate test that has been discarded.

mrgrain · 2022-10-19T12:42:35Z

packages/aws-cdk/lib/index.ts

@@ -1,2 +1,3 @@
 export * from './api';
 export { cli } from './cli';
+export { pythonExecutable } from './init';


Revert (see note about pythonExecutable)

Suggested change

export { pythonExecutable } from './init';

mrgrain · 2022-10-19T12:42:48Z

packages/aws-cdk/lib/init.ts

@@ -46,7 +46,7 @@ export async function cliInit(type?: string, language?: string, canUseNetwork =
 /**
 * Returns the name of the Python executable for this OS
 */
-function pythonExecutable() {
+export function pythonExecutable() {


Revert (see note about pythonExecutable)

Suggested change

export function pythonExecutable() {

function pythonExecutable() {

mrgrain · 2022-10-19T12:46:08Z

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

@@ -1,6 +1,7 @@
 import * as path from 'path';
 import { TestCase, DefaultCdkOptions } from '@aws-cdk/cloud-assembly-schema';
 import { AVAILABILITY_ZONE_FALLBACK_CONTEXT_KEY, FUTURE_FLAGS, TARGET_PARTITIONS, FUTURE_FLAGS_EXPIRED } from '@aws-cdk/cx-api';
+import { pythonExecutable } from 'aws-cdk/lib/init';


Thanks for trying to avoid duplication! In this case we avoid importing from submodules and the functions is not something we'd like to be part of a public interface.

Let's keeps the duplication here for now.

Suggested change

import { pythonExecutable } from 'aws-cdk/lib/init';

mrgrain · 2022-10-19T12:47:37Z

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

+   * You can specify a custom run command, and it will be applied to all test files.
+   * If it contains {filePath}, the test file names will be substituted at that place in the command for each run.
+   *
+   * @default - test run command will be `node`


Suggested change

* @default - test run command will be `node`

* @default - a default run command based on the language the test is written in

mrgrain · 2022-10-19T12:51:01Z

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

+    .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 name should be inserted. Example: --app="python3.8 {filePath}"' })
+    .option('language', { type: 'array', default: ['csharp', 'fsharp', 'go', 'java', 'javascript', 'python', 'typescript'], desc: 'The languages that tests will search for. Defaults to all languages supported by CDK.' })
+    .option('test-regex', { type: 'string', default: undefined, desc: 'A custom pattern in the JS RegExp format to match integration test file prefixes.' })


Suggested change

.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 name should be inserted. Example: --app="python3.8 {filePath}"' })

.option('language', { type: 'array', default: ['csharp', 'fsharp', 'go', 'java', 'javascript', 'python', 'typescript'], desc: 'The languages that tests will search for. Defaults to all languages supported by CDK.' })

.option('test-regex', { type: 'string', default: undefined, desc: 'A custom pattern in the JS RegExp format to match integration test file prefixes.' })

.option('app', { type: 'string', default: undefined, desc: 'The command used by the test runner to synth the test files. Uses default run commands based on the language the test is written in. You can use {filePath} in the command to specify where the test file name should be inserted in the command. Example: `--app "python3.8 {filePath}"`' })

.option('language', { type: 'array', default: ['csharp', 'fsharp', 'go', 'java', 'javascript', 'python', 'typescript'], desc: 'The language to discover tests for. Defaults to all CDK-supported languages.' })

.option('test-regex', { type: 'string', default: undefined, desc: 'A custom pattern in the JS RegExp format to match integration test file prefixes. Defaults are set based on naming conventions for the language the test is written in. Example: `--test-regex "^Integ\."`' })

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

…of multi-language integration tests (#22716) To support multi language integration tests better (see #22521), we need to rename the snapshot directories. Other languages have different naming conventions, so it's not trivial anymore to generate a "nice" name just from a test file. Instead we now use the full filename. This new pattern is inspired by Jest etc that all use the full filename. E.g. `my-test-name.integ.snapshot` -> `integ.my-test-name.js.snapshot` This PR changes the code generating the snapshot names and then does the rename in one go via the following commits: * chore: change code for generating the integ test snapshot directory names * chore: rename test data in `integ-runner` * chore: update `.gitignore` files with new snapshot patterns * chore: rename all integ-test snapshots * chore: update integ test snapshot paths in docs * chore: update `.npmignore` files with new snapshot pattern * chore: update prlinter to detect new snapshot pattern --- Review by commit ([start here](a884681)). The [middle commit](fee824d) contains most of the renames, is HUGE and takes a long time to load in the GitHub UI. However it only contains renames (use `--diff-filter=r` to exclude renames from the diff): ``` > git show --diff-filter=r fee824d (empty result) ``` ---- ### All Submissions: * [x] 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 * [x] 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*

mrgrain · 2022-11-01T13:08:51Z

@karakter98 the snapshot rename has been merged.

I'll start preparing --test-regex next.

aws-cdk-automation · 2022-11-01T15:02:04Z