-
Notifications
You must be signed in to change notification settings - Fork 155
chore(cicd): cdk examples and e2e tests for metrics #326
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
Conversation
98918af
to
e72dfd6
Compare
Pushed a change in the Opened also a separate issue to work on exposing better APIs for manual instrumentation: #334 |
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.
Happy to merge this
593443a
to
663854b
Compare
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.
Overall, it looks good to me. My comments are nitpicking. We can discuss and change them in a separate PR.
/* If you don't specify 'env', this stack will be environment-agnostic. | ||
* Account/Region-dependent features and context lookups will not work, | ||
* but a single synthesized template can be deployed anywhere. */ | ||
|
||
/* Uncomment the next line to specialize this stack for the AWS Account | ||
* and Region that are implied by the current CLI configuration. */ | ||
// env: { account: process.env.CDK_DEFAULT_ACCOUNT, region: process.env.CDK_DEFAULT_REGION }, | ||
|
||
/* Uncomment the next line if you know exactly what Account and Region you | ||
* want to deploy the stack to. */ | ||
// env: { account: '123456789012', region: 'us-east-1' }, | ||
|
||
/* For more information, see https://docs.aws.amazon.com/cdk/latest/guide/environments.html */ |
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.
Can we remove this?
|
||
// Invoke all functions twice | ||
for (let i = 0; i < 2; i++) { | ||
new custom_resources.AwsCustomResource(this, `Invoke-std-func-${i}`, { |
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.
Should we wrap these 3 resource creations in to a function? They are almost the same except the passed function.
defaultDimensions: { environment: 'example', type: 'withDecorator' }, | ||
}) | ||
public handler(_event: unknown, _context: Context, _callback: Callback<unknown>): void | Promise<unknown> { | ||
// ### Experiment logger |
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.
Why is there a word "Experiment"?
@@ -0,0 +1,17 @@ | |||
// import * as cdk from 'aws-cdk-lib'; |
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 can also remove these unused comments.
// Despite lambda has been called twice, coldstart metric sum should only be 1 | ||
const singleDataPoint = coldStartMetricStat.Datapoints ? coldStartMetricStat.Datapoints[0] : {}; | ||
expect(singleDataPoint.Sum).toBe(1); | ||
}, 9000000); |
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.
That's 2.5 hr. Do you intend to be 15 mins? (one less 0)
stack: stackArtifact, | ||
}); | ||
} | ||
}, 9000000); |
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.
That's 2.5 hr. Do you intend to be 15 mins? (one less 0)
@saragerion @ijemmy @flochaz - looks like this lock file was generated by node@16, which will break node@14 users when the want to do |
Thx @michaelbrewer. I am opening an issue and will fix asap. |
For those coming here in the future, the issue was that we recently started recommending Using |
Sorry about this. I guess things where in flux (so now with the clear contributor guide and the gitpod/CodeSpaces support). It should be clear what the local setup should be. |
Description of your changes
How to verify this change
Global example
cd examples/cdk npm install npm run build npm run cdk deploy
This will deploy 3 lambdas and invoke them twice (to have one cold start and one warm)
Metrics e2e test
Run procedure
Doc (might be worth to move that in CONTRIBUTING.md ?)
Tests are under
test
folder and splitted into two categories: Unit tests and e2e tests.This split happen thanks to jest-runner-groups.
Unit tests, under
test/unit
folder, don't need AWS Account to run and can only verify your construct's logic and it's capability to be synthesised to Cloud Formation.Integration tests, under
test/e2e
folder, will test the deployability (cfn stack deploy) of your construct, potentially its functionality and its deprovisioning capability (cfn stack destroy) and therefore will need an AWS account.Unit testing
Write
As mentioned before, tests are splitted thanks to jest-runner-groups and therefore needs to be tagged properly by adding the following comments in your unit test file:
Run
npm run test
You can run selective tests by restricting the group to the one you want. For instance
npx jest --group=unit/metrics/all
.e2e testing
Write
As mentioned before, tests are splitted thanks to jest-runner-groups and therefore needs to be tagged properly by adding the following comments in your unit test file:
and leverage
aws-cdk
package to programatically deploy and destroy stacks. Seemetrics/tests/e2e/decorator.test.ts
as an example.Run
To run unit tests you can either use projen task
npm run test:e2e
which will only run jest integ testsnpx jest --group=e2e
You can run selective tests by restricting the group to the one you want. For instance
npx jest --group=e2e/other/example
.Two important env variable can be used:
AWS_PROFILE
to use the right credentialsDISABLE_TEARDOWN
if you don't want your stack to be destroyed at the end of the test (useful in dev mode when iterating over your code).Example:
DISABLE_TEARDOWN=true AWS_PROFILE=ara npx jest --group=integ/other/example
Related issues, RFCs
#136
PR status
Is this ready for review?: YES
Is it a breaking change?: NO
Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.