-
Notifications
You must be signed in to change notification settings - Fork 155
test(common,logger,metrics): run e2e tests for metrics in both Node runtimes #678
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
…tests for InvocationLogs
Co-authored-by: Andrea Amorosi <[email protected]>
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.
Thanks a lot for the PR, I like the direction you're giving to the integration tests. It'll make them much easier to maintain moving forward.
My only concern relates to exporting the e2e utilities in the commons package
. This package is depended and bundled by all the other utilities and as such it will be included in the deployment package of each function that uses Powertools.
While I think there's definitely value in providing test utilities (see #445), I don't know if it should be in commons
and in general, if it's code that should be deployed in Lambda.
What's your take?
I totally agree with you. We'll unnecessarily increase package size. I'll change this. |
…nd don't expose it outside of the package
I've remove the module from the --group param.
Description of your changes
Changes in this PR.
commons
How to verify this change
Run
npm run lerna-test:e2e
at the root.Every test should pass and metrics tests should be on both node14 and node12
Related issues, RFCs
#497
PR status
Is this ready for review?: NO
Is it a breaking change?: NO
Checklist
Breaking change checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.