-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Changes from all commits
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 |
---|---|---|
|
@@ -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
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. This feels out of place here. See comment further down for a possible refactoring. |
||
/** | ||
* The name of the file to run | ||
* | ||
|
@@ -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 | ||
|
@@ -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 | ||
* | ||
|
@@ -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[]; | ||
} | ||
|
||
|
||
|
@@ -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
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 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) { | ||
} | ||
|
||
|
@@ -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
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. Okay so here's an idea how we could streamline the language presets a bit. Instead of {
"app command": ["test regex"],
// e.g.
"node {filePath}": ['^integ\\..*\\.js$']
} (In case of 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 |
||
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
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 wonder if still need a generic de-duplication or just a special case Either way could we reduce duplication if we added the files to be ignored to |
||
|
||
return this.request(integsWithoutDuplicates, options); | ||
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. With the above changes, I think it's time to just merge |
||
} | ||
|
||
private request(files: string[], options: IntegrationTestsDiscoveryOptions): IntegTest[] { | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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$"']); | ||||||
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
Otherwise this is just testing the regex, right? 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. The test won't be picked up if we don't specify the regex, because it doesn't have the standard prefix 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. Yes, see comment re renaming the test file. |
||||||
|
||||||
expect(stdoutMock.mock.calls).toEqual([[ | ||||||
[ | ||||||
'xxxxx.typescript-test.ts', | ||||||
'', | ||||||
].join('\n'), | ||||||
]]); | ||||||
}); | ||||||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
import { App, Stack } from '@aws-cdk/core'; | ||
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. This one is okay to be named (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" | ||
} | ||
} | ||
} |
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.
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.
We might want to check if adding
defaultDescription: 'all supported languages'
provides a better ux