-
Notifications
You must be signed in to change notification settings - Fork 74
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
chore: upgrade integration test-suite to enable performance testing #405
Conversation
…nd testing sandbox process loop
🦋 Changeset detectedLatest commit: 7706134 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
/** | ||
* Kills the given process (equivalent of sending CTRL-C) | ||
* @param processInstance an instance of execa child process | ||
*/ | ||
export const killExecaProcess = async (processInstance: ExecaChildProcess) => { |
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 file should be called execa_process_killer
or execa_process_terminator
.
- https://blog.codinghorror.com/i-shall-call-it-somethingmanager/
- killing a subject is poor management practice .
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.
Updated
/** | ||
* Reusable predicates: Wait for sandbox to finish and emit "✨ Total time: xx.xxs" | ||
*/ | ||
export const waitForSandboxDeployment = () => | ||
new PredicatedActionBuilder().waitForLineIncludes('Total time'); |
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 one should also wait for line that synth and deployment was successful. Or perhaps we need a parameter or additional methods to wait for "any deployment" "successful deployment" etc.
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.
For now renamed it to be more specific, more macros can be added later as needed (or this changed when the need arises)
/** | ||
* Update the last predicated action to update backend code by copying files from | ||
* `from` location to `to` location. | ||
*/ | ||
updateBackendCode = (from: string, to: string) => { | ||
this.getLastPredicatedAction().then = { | ||
actionType: ActionType.MAKE_CODE_CHANGES, | ||
action: async () => { | ||
await fs.cp(from, to, { | ||
recursive: true, | ||
}); |
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.
if this is running cp
should this be updateFileContent
? We don't really assert that input is code.
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.
Updated.
/** | ||
* Update the last predicated action to validate that the deployment time is less than the one specified | ||
*/ | ||
ensureDeploymentTimeLessThan = (seconds: number) => { | ||
this.getLastPredicatedAction().then = { | ||
actionType: ActionType.ASSERT_ON_PROCESS_OUTPUT, | ||
action: (strWithDeploymentTime: string) => { | ||
const regex = /^✨ {2}Total time: (\d*.\d*).*$/; | ||
const deploymentTime = strWithDeploymentTime.match(regex); | ||
if ( | ||
deploymentTime && | ||
deploymentTime.length > 1 && | ||
!isNaN(+deploymentTime[1]) |
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.
Couple of ideas.
- Can we also capture Synth time and other counters emitted by CDK?
- Would it make sense to measure perceived latency from outside of process ? This would be likely pipeline deploy only.
- Since we have AWS account hooked up. Can we also emit these metrics to cloudwatch to observe trends ? (perhaps only for
main
branch). The threshold here is going to have some padding to catch big offenders, but we should persist this data somewhere for trend analytics. - Can we split CDK log parsing from calculations? I.e. parseCDKLog() => Some object model => assert and emit metrics?
- At some point we're going to have diagnostics/logging in our components. Perhaps we should invest into this earlier. I.e. create some components in
platform-core
that can capture telemetry and can be configured/injected into sandbox. And use that to get and assert data.
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.
upgrade integration test-suite to enable performance testing and testing sandbox process loop
Description of changes:
updates
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.