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

fix(integ-runner): catch snapshot errors, treat --from-file as command-line #20523

Merged
merged 5 commits into from
May 31, 2022

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented May 27, 2022

Snapshot errors

The constructor of IntegSnapshotRunner calls loadManifest(), which
on my computer happened to fail and stopped the entire test suite
because this error happened outside the try/catch block. Same for
the integ runner itself.

Move it inside the try/catch block, needed a bit of refactoring to
make sure we could still get at the test name.

--from-file

Instead of having --from-file require a JSON file with its own
structure, interpret it as a text file which gets treated exactly
the same as the [TEST [..]] arguments on the command line.

This still allows for the --exclude behavior by setting that flag
on the command-line.

Also be very liberal on the pattern (file name or test name or display
name) that we accept, encoded in the IntegTest.matches() class.

Refactoring

Moved the logic around determining test names and directories into a
class (IntegTest) which is a convenience class on top of a static data
record (IntegTestInfo). The split between data and logic is so that we
can pass the data to worker threads where we can hydrate the helper
class on top again. I tried to be consistent: in memory, all the fields are
with respect to process.cwd(), so valid file paths in the current
process. Only when they are passed to the CLI wrapper are the paths
made relative to the CLI wrapper directory.

Print snapshot validations that are running for a long time (1 minute).
This helps diagnose what is stuck, if anything is. On my machine, it
was tests using Docker because there was some issue with it, and this
lost me a day. Also change the test reporting formatting slightly.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@rix0rrr rix0rrr requested a review from a team May 27, 2022 13:01
@rix0rrr rix0rrr self-assigned this May 27, 2022
@gitpod-io
Copy link

gitpod-io bot commented May 27, 2022

@aws-cdk-automation aws-cdk-automation requested a review from a team May 27, 2022 13:01
@github-actions github-actions bot added the p2 label May 27, 2022
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label May 27, 2022
@@ -157,8 +197,8 @@ export abstract class IntegRunner {
...options.env,
},
});
this.cdkOutDir = options.integOutDir ?? `${CDK_OUTDIR_PREFIX}.${testName}`;
this.cdkApp = `node ${parsed.base}`;
this.cdkOutDir = options.integOutDir ?? `${CDK_OUTDIR_PREFIX}.${this.testName}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

this.testName needs to be the baseTestName from testNameFromFile

Copy link
Contributor

Choose a reason for hiding this comment

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

And actually can you just add baseTestName to the TestNameParts. Maybe call it stableTestName? The testName (really the testDisplayName) is something that changes depending on where you run the test from, but we also need a stable test name that doesn't change to use here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I'm running into some trouble with the other change I'm trying to make here as well. They seem to come together.

…and-line

Snapshot errors
---------------

The constructor of `IntegSnapshotRunner` calls `loadManifest()`, which
on my computer happened to fail and stopped the entire test suite
because this error happened outside the `try/catch` block.

Move it inside the try/catch block, needed a bit of refactoring to
make sure we could still get at the test name.

`--from-file`
-------------

Instead of having `--from-file` require a JSON file with its own
structure, interpret it as a text file which gets treated exactly
the same as the `[TEST [..]]` arguments on the command line.

This still allows for the `--exclude` behavior by setting that flag
on the command-line.

Refactoring
-----------

Moved the logic around determining test names and directories into a
class (`IntegTest`) which is a convenience class on top of a static data
record (`IntegTestInfo`). We pass the data record to worker threads.
@rix0rrr rix0rrr force-pushed the huijbers/integ-runner-catch branch from fb10189 to 2007431 Compare May 30, 2022 12:49
@rix0rrr rix0rrr changed the title fix(integ-runner): catch errors that happen during snapshot generation fix(integ-runner): catch snapshot errors, treat --from-file as command-line May 30, 2022
@rix0rrr rix0rrr requested a review from corymhall May 30, 2022 12:50
Print snapshot validations that are running for a long time (1 minute).
This helps diagnose what is stuck, if anything is. On my machine, it
was tests using Docker because there was some issue with it, and this
lost me a day.

Also change the test reporting formatting slightly, and catch errors
during actual test running.
Copy link
Contributor

@corymhall corymhall left a comment

Choose a reason for hiding this comment

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

Looks great! Love these improvements ❤️

* - The suite name
* - The absolute filename
*/
public matches(name: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

oooh I like this!

@@ -57,6 +50,9 @@ export interface IntegRunnerOptions {
readonly cdk?: ICdk;
}

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this comment left over?

@corymhall corymhall added the pr/do-not-merge This PR should not be merged at this time. label May 31, 2022
@rix0rrr rix0rrr removed the pr/do-not-merge This PR should not be merged at this time. label May 31, 2022
@mergify
Copy link
Contributor

mergify bot commented May 31, 2022

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 8995f98
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify mergify bot merged commit cedfde8 into master May 31, 2022
@mergify mergify bot deleted the huijbers/integ-runner-catch branch May 31, 2022 14:32
@mergify
Copy link
Contributor

mergify bot commented May 31, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants