-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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 non-TypeScript tests #22521
Changes from all commits
8d0a523
574581f
5359a18
4a16192
fa8a56e
4d21159
d110bce
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -79,6 +79,11 @@ export class IntegTest { | |||||
*/ | ||||||
public readonly temporaryOutputDir: string; | ||||||
|
||||||
/** | ||||||
* Language the test is written in | ||||||
*/ | ||||||
public readonly language: string; | ||||||
|
||||||
constructor(public readonly info: IntegTestInfo) { | ||||||
this.absoluteFileName = path.resolve(info.fileName); | ||||||
this.fileName = path.relative(process.cwd(), info.fileName); | ||||||
|
@@ -97,10 +102,16 @@ export class IntegTest { | |||||
? parsed.name | ||||||
: path.join(path.relative(this.info.discoveryRoot, parsed.dir), parsed.name); | ||||||
|
||||||
const nakedTestName = parsed.name.slice(6); // Leave name without 'integ.' and '.ts' | ||||||
const nakedTestName = new IntegrationTests(this.directory).stripPrefixAndSuffix(parsed.base); // Leave name without 'integ.' and '.ts' | ||||||
this.normalizedTestName = parsed.name; | ||||||
this.snapshotDir = path.join(this.directory, `${nakedTestName}.integ.snapshot`); | ||||||
this.temporaryOutputDir = path.join(this.directory, `${CDK_OUTDIR_PREFIX}.${nakedTestName}`); | ||||||
|
||||||
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'); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
} | ||||||
this.language = language; | ||||||
} | ||||||
|
||||||
/** | ||||||
|
@@ -146,7 +157,75 @@ export interface IntegrationTestFileConfig { | |||||
* Discover integration tests | ||||||
*/ | ||||||
export class IntegrationTests { | ||||||
constructor(private readonly directory: string) { | ||||||
/** | ||||||
* Will discover integration tests with naming conventions typical to each language. Examples: | ||||||
* - TypeScript/JavaScript: integ.test.ts or integ-test.ts or integ_test.ts | ||||||
* - Python/Go: integ_test.py | ||||||
* - Java/C#: IntegTest.cs | ||||||
* @private | ||||||
*/ | ||||||
private readonly prefixMapping: Map<string, RegExp> = new Map<string, RegExp>([ | ||||||
['csharp', new RegExp(/^Integ/)], | ||||||
['fsharp', new RegExp(/^Integ/)], | ||||||
['go', new RegExp(/^integ_/)], | ||||||
['java', new RegExp(/^Integ/)], | ||||||
['javascript', new RegExp(/^integ\./)], | ||||||
['python', new RegExp(/^integ_/)], | ||||||
['typescript', new RegExp(/^integ\./)], | ||||||
]); | ||||||
private readonly suffixMapping: Map<string, RegExp> = new Map<string, RegExp>([ | ||||||
['csharp', new RegExp(/\.cs$/)], | ||||||
['fsharp', new RegExp(/\.fs$/)], | ||||||
['go', new RegExp(/\.go$/)], | ||||||
['java', new RegExp(/\.java$/)], | ||||||
['javascript', new RegExp(/\.js$/)], | ||||||
['python', new RegExp(/\.py$/)], | ||||||
// Allow files ending in .ts but not in .d.ts | ||||||
['typescript', new RegExp(/(?<!\.d)\.ts$/)], | ||||||
]); | ||||||
|
||||||
constructor(private readonly directory: string, customPrefix?: RegExp, allowedLanguages?: Array<string>) { | ||||||
if (customPrefix) { | ||||||
for (const language of this.prefixMapping.keys()) { | ||||||
this.prefixMapping.set(language, customPrefix); | ||||||
} | ||||||
} | ||||||
if (allowedLanguages) { | ||||||
const disallowedLanguages = Array.from(this.prefixMapping.keys()).filter((language) => !allowedLanguages.includes(language)); | ||||||
for (const disallowedLanguage of disallowedLanguages) { | ||||||
this.prefixMapping.delete(disallowedLanguage); | ||||||
this.suffixMapping.delete(disallowedLanguage); | ||||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
public stripPrefixAndSuffix(fileName: string): string { | ||||||
const language = this.getLanguage(fileName); | ||||||
if (!language) { | ||||||
return fileName; | ||||||
} | ||||||
|
||||||
const suffix = this.suffixMapping.get(language) ?? ''; | ||||||
const prefix = this.prefixMapping.get(language) ?? ''; | ||||||
|
||||||
return fileName.replace(prefix, '').replace(suffix, ''); | ||||||
} | ||||||
|
||||||
private hasValidPrefixSuffix(fileName: string): boolean { | ||||||
const language = this.getLanguage(fileName); | ||||||
if (!language) { | ||||||
return false; | ||||||
} | ||||||
|
||||||
const suffix = this.suffixMapping.get(language); | ||||||
const prefix = this.prefixMapping.get(language); | ||||||
|
||||||
return <boolean>(suffix?.test(fileName) && prefix?.test(fileName)); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
} | ||||||
|
||||||
public getLanguage(fileName: string): string | undefined { | ||||||
const [language] = Array.from(this.suffixMapping.entries()).find(([, regex]) => regex.test(fileName)) ?? [undefined, undefined]; | ||||||
return language; | ||||||
} | ||||||
|
||||||
/** | ||||||
|
@@ -212,8 +291,24 @@ export class IntegrationTests { | |||||
|
||||||
private async discover(): Promise<IntegTest[]> { | ||||||
const files = await this.readTree(); | ||||||
const integs = files.filter(fileName => path.basename(fileName).startsWith('integ.') && path.basename(fileName).endsWith('.js')); | ||||||
return this.request(integs); | ||||||
const integs = files.filter(fileName => this.hasValidPrefixSuffix(path.basename(fileName))); | ||||||
|
||||||
const discoveredTestNames = new Set<string>(); | ||||||
const integsWithoutDuplicates = new Array<string>(); | ||||||
|
||||||
// Remove duplicate test names that would just overwrite each other's snapshots anyway. | ||||||
// To make sure the precendence of files is deterministic, iterate the files in lexicographic order. | ||||||
// Additionally, to give precedence to .ts files over their compiled .js version, | ||||||
// use descending lexicographic ordering, so the .ts files are picked up first. | ||||||
Comment on lines
+301
to
+302
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||||||
for (const integFileName of integs.sort().reverse()) { | ||||||
const testName = this.stripPrefixAndSuffix(path.basename(integFileName)); | ||||||
if (!discoveredTestNames.has(testName)) { | ||||||
integsWithoutDuplicates.push(integFileName); | ||||||
} | ||||||
discoveredTestNames.add(testName); | ||||||
} | ||||||
Comment on lines
+303
to
+309
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's add print a |
||||||
|
||||||
return this.request(integsWithoutDuplicates); | ||||||
} | ||||||
|
||||||
private request(files: string[]): IntegTest[] { | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -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'; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 { CdkCliWrapper, ICdk } from 'cdk-cli-wrapper'; | ||||||
import * as fs from 'fs-extra'; | ||||||
import { flatten } from '../utils'; | ||||||
|
@@ -49,6 +50,14 @@ export interface IntegRunnerOptions { | |||||
*/ | ||||||
readonly cdk?: ICdk; | ||||||
|
||||||
/** | ||||||
* 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` | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
*/ | ||||||
readonly appCommand?: string; | ||||||
|
||||||
/** | ||||||
* Show output from running integration tests | ||||||
* | ||||||
|
@@ -64,6 +73,41 @@ export interface IntegRunnerOptions { | |||||
* Represents an Integration test runner | ||||||
*/ | ||||||
export abstract class IntegRunner { | ||||||
|
||||||
// Best-effort reconstruction of the classpath of the file in the project. | ||||||
// Example: /absolute/file/path/src/main/java/com/myorg/IntegTest.java -> com.myorg.IntegTest | ||||||
mrgrain marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
// Should work for standard Java project layouts. | ||||||
private static getJavaClassPath(absoluteFilePath: string): string | undefined { | ||||||
const packagePath = absoluteFilePath.split('/java/').slice(-1)[0]; | ||||||
if (!packagePath) { | ||||||
return undefined; | ||||||
} | ||||||
// string.replaceAll isn't available in the TS version the project uses | ||||||
return packagePath.split('/').join('.').replace('.java', ''); | ||||||
} | ||||||
// Will find the closest pom.xml file in the directory structure for this file | ||||||
private static getJavaPomPath(executionDirectoryPath: string): string | undefined { | ||||||
// Will generate the full list of ancestors in the directory path | ||||||
// Example: ['/', '/home', '/home/MyUser', '/home/MyUser/Desktop'] | ||||||
const dirHierarchy = executionDirectoryPath | ||||||
.split(path.sep) | ||||||
.filter(dirName => dirName !== '') | ||||||
.reduce((hierarchy, segment) => { | ||||||
hierarchy.push(path.join(hierarchy[hierarchy.length - 1], segment)); | ||||||
return hierarchy; | ||||||
}, | ||||||
// For Windows support | ||||||
[path.toNamespacedPath('/')]); | ||||||
|
||||||
for (const parentDir of dirHierarchy.reverse()) { | ||||||
const searchPath = path.join(parentDir, 'pom.xml'); | ||||||
if (fs.existsSync(searchPath)) { | ||||||
return path.relative(executionDirectoryPath, searchPath); | ||||||
} | ||||||
} | ||||||
return undefined; | ||||||
} | ||||||
|
||||||
/** | ||||||
* The directory where the snapshot will be stored | ||||||
*/ | ||||||
|
@@ -150,7 +194,24 @@ export abstract class IntegRunner { | |||||
}, | ||||||
}); | ||||||
this.cdkOutDir = options.integOutDir ?? this.test.temporaryOutputDir; | ||||||
this.cdkApp = `node ${path.relative(this.directory, this.test.fileName)}`; | ||||||
|
||||||
const defaultAppCommands = new Map<string, string>([ | ||||||
['javascript', 'node {filePath}'], | ||||||
['typescript', 'node -r ts-node/register {filePath}'], | ||||||
['python', `${pythonExecutable()} {filePath}`], | ||||||
['go', 'go mod download && go run {filePath}'], | ||||||
['csharp', 'dotnet run {filePath}'], | ||||||
['fsharp', 'dotnet run {filePath}'], | ||||||
['java', `mvn -f ${IntegRunner.getJavaPomPath(path.dirname(options.test.absoluteFileName))} -e -q compile exec:java clean -Dexec.mainClass=${IntegRunner.getJavaClassPath(options.test.absoluteFileName)}`], | ||||||
]); | ||||||
const testRunCommand = options.appCommand ?? defaultAppCommands.get(this.test.language); | ||||||
|
||||||
if (!testRunCommand) { | ||||||
throw new Error('Could not find default run command for this file extension. Try specifying a custom one with the --app flag.'); | ||||||
} | ||||||
|
||||||
this.cdkApp = testRunCommand.replace('{filePath}', path.relative(this.directory, this.test.fileName)); | ||||||
|
||||||
this.profile = options.profile; | ||||||
if (this.hasSnapshot()) { | ||||||
this.expectedTestSuite = this.loadManifest(); | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.