tests(parameters): integration tests for SecretsProvider
#1263
Conversation
|
Should I keep it as a draft until we merge #1260 for all types? |
There was a problem hiding this comment.
Thank you for the PR, appreciate it!
Left a couple of minor comments to understand the function code.
Happy to merge this once review is ready, no need to wait for other PRs.
Whenever you have time, could you please leave a comment under the linked issue so that I can assign it to you?
| import { Context } from 'aws-lambda'; | ||
| import { SecretsProvider } from '../../lib/secrets'; | ||
| import { TinyLogger } from '../helpers/tinyLogger'; | ||
| import { transform } from 'esbuild'; |
There was a problem hiding this comment.
Why do we import this and use it in the conditional? Not challenging, just asking because I'm not clear on it
There was a problem hiding this comment.
This does not make any sense 🤔 , it should be options in the conditional. Will change that.
| const secretNameObjectWithSuffix = process.env.SECRET_NAME_OBJECT_WITH_SUFFIX || ''; | ||
| const secretNameBinaryWithSuffix = process.env.SECRET_NAME_BINARY_WITH_SUFFIX || ''; | ||
|
|
||
| const _call_get = async (paramName: string, testName: string, options?: SecretsGetOptionsInterface) : Promise<void> => { |
There was a problem hiding this comment.
How will this work once we introduce multiple providers (i.e. once #1222 is merged and we need multiple SecretsProviders)? Should we add a new parameter?
There was a problem hiding this comment.
Right, lets pass the provider in the signature. Should we wait until #1222 is merged, or can I change it now?
There was a problem hiding this comment.
Let's do it now, so later we can just add a new block at the end of the main handler
There was a problem hiding this comment.
Hey guys! #1222 is ready for review.
dreamorosi
added
parameters
|
I added two test cases, one for caching and other for force fetch. I am not sure what other cases with custom SDK client we should cover with e2e tests. The major benefit comes from injecting clients for testing, which is not something we should assess with e2e. What do you think? |
Completely agree, the fact that we can check the caching already proves the injection works correctly. I'm okay with the current test surface for now. |
|
I have ran the integration tests locally after checking out your branch and they are passing:
|
|
Thank you for the work on this Alex, really appreciate it! |
Description of your changes
As part of #1039 , we need to implement integration tests for the SecretsProvider which is part of the upcoming Parameters utility.
The test cases cover string, json, binary and auto transformer. We don't cover the cases for sdk client options and I will add them after #1222 is resolved.
Related issues, RFCs
Issue number: #1241
PR status
Is this ready for review?: YES
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.