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 custom --app commands #22761

Merged
merged 3 commits into from
Nov 7, 2022

Conversation

karakter98
Copy link
Contributor

To prepare for supporting other languages than TypeScript (see #22521) we need to be able to run test files with commands other than the current default node. With this PR, users can specify a custom --app command that will substitute the string {filePath} with the filename of each discovered test, and synth each test with that command.

Example usage: integ-runner --app '"node --no-warnings {filePath}"'

Until we also allow discovery of different file prefixes, this can't be used to run languages like Python, because the default prefix used now (integ.) contains a . which is not a valid character for Python module names.


All Submissions:

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
Copy link

gitpod-io bot commented Nov 3, 2022

@github-actions github-actions bot added p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Nov 3, 2022
@aws-cdk-automation aws-cdk-automation requested a review from a team November 3, 2022 10:31
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

@mrgrain mrgrain self-assigned this Nov 3, 2022
mrgrain
mrgrain previously requested changes Nov 4, 2022
Copy link
Contributor

@mrgrain mrgrain left a comment

Choose a reason for hiding this comment

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

Hi @karakter98 Apologies for the delay in reviewing this.

Just got a minor comment and would like a clarification around the single quote thing.

@@ -68,7 +68,8 @@ to be a self contained CDK app. The runner will execute the following for each f
Read the list of tests from this file
- `--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 name should be inserted. Example: --app='"python3.8 {filePath}"'. It is recommended to use single quotes to wrap the double quotes of the command into a literal string, to avoid parsing errors.
Copy link
Contributor

@mrgrain mrgrain Nov 4, 2022

Choose a reason for hiding this comment

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

file path instead of file name

Copy link
Contributor

Choose a reason for hiding this comment

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

It is recommended to use single quotes to wrap the double quotes of the command into a literal string, to avoid parsing errors.

Could you elaborate on this recommendation?

packages/@aws-cdk/integ-runner/lib/cli.ts Outdated Show resolved Hide resolved
@mrgrain mrgrain changed the title feat(integ-runner): Support custom --app commands feat(integ-runner): support custom --app commands Nov 4, 2022
@mrgrain mrgrain added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Nov 7, 2022
@aws-cdk-automation aws-cdk-automation dismissed their stale review November 7, 2022 10:09

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

@mergify mergify bot dismissed mrgrain’s stale review November 7, 2022 10:22

Pull request has been modified.

mrgrain
mrgrain previously approved these changes Nov 7, 2022
@mrgrain
Copy link
Contributor

mrgrain commented Nov 7, 2022

@karakter98 I've applied my suggestions and approved to PR to move this forward. Let me know if you can clarify this recommendation. Very happy to add it back in if it makes sense.

It is recommended to use single quotes to wrap the double quotes of the command into a literal string, to avoid parsing errors.

@mergify
Copy link
Contributor

mergify bot commented Nov 7, 2022

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot dismissed mrgrain’s stale review November 7, 2022 11:12

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify
Copy link
Contributor

mergify bot commented Nov 7, 2022

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit a7bb6e1 into aws:main Nov 7, 2022
mergify bot pushed a commit that referenced this pull request Nov 10, 2022
… files (#22786)

Follow-up to #22761. To support other languages than JavaScript (see #22521) we need to be able to detect test files with any patterns. With this PR, users can specify a number of custom `--test-regex` patterns that will bed used to discover integration test files. Together with `--app` this can already be used to run integ tests in arbitrary languages.

Example usage: `integ-runner --app="python3 {filePath}" --test-regex="^integ_.*\.py$"`

Also contains a minor refactor to make `--app` available via `IntegrationTests.fromFile()`. This is in preparation of an upcoming change to reestablish support for an integration test config file.

----

### 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

* [ ] 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*
@karakter98
Copy link
Contributor Author

@mrgrain sorry for the delay, the recommendation was put there because of limitations in the yargs CLI parsing library.

This comment explains the problem well, if you need to add flags to the app command string (like node --enable-fips {filePath}) and wrap it only in double-quotes, yargs doesn't handle it correctly. Wrapping the argument in single-quotes and then double-quotes makes sure that yargs receives the string double-quoted and knows it's supposed to parse it as a string and not as CLI arguments.

@mrgrain
Copy link
Contributor

mrgrain commented Nov 15, 2022

Thanks for the clarification. I cannot reproduce the problem on my end, plus the linked yargs issues has been closed as completed since 2019. It's also not a recommendation we make elsewhere. Let's monitor, but I'm inclined to keep it out for now (like it has been merged) unless we get an influx of issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 pr-linter/exempt-integ-test The PR linter will not require integ test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants