-
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
fix(integ-runner): catch snapshot errors, treat --from-file
as command-line
#20523
Conversation
@@ -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}`; |
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.
this.testName
needs to be the baseTestName
from testNameFromFile
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.
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
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.
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.
fb10189
to
2007431
Compare
--from-file
as command-line
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.
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.
Looks great! Love these improvements ❤️
* - The suite name | ||
* - The absolute filename | ||
*/ | ||
public matches(name: string) { |
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.
oooh I like this!
@@ -57,6 +50,9 @@ export interface IntegRunnerOptions { | |||
readonly cdk?: ICdk; | |||
} | |||
|
|||
/** |
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.
Is this comment left over?
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
Snapshot errors
The constructor of
IntegSnapshotRunner
callsloadManifest()
, whichon my computer happened to fail and stopped the entire test suite
because this error happened outside the
try/catch
block. Same forthe 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 ownstructure, 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 flagon 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 datarecord (
IntegTestInfo
). The split between data and logic is so that wecan 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 currentprocess. 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