tests(idempotency): add utility to workspace unit tests and CI #1160
Conversation
pull-request-size
bot
added
the
size/XL
| process.env._X_AMZN_TRACE_ID = '1-abcdef12-3456abcdef123456abcdef12'; | ||
| process.env.AWS_LAMBDA_FUNCTION_NAME = 'my-lambda-function'; | ||
| process.env.AWS_EXECUTION_ENV = 'nodejs16.x'; | ||
| process.env.AWS_LAMBDA_FUNCTION_MEMORY_SIZE = '128'; | ||
| process.env.AWS_REGION = 'eu-west-1'; | ||
| process.env._HANDLER = 'index.handler'; |
There was a problem hiding this comment.
(Question) Do we really need all of these env vars? Wouldn't only AWS_LAMBDA_FUNCTION_NAME be enough?
There was a problem hiding this comment.
We use only the name at the moment, and we'll be using also the region in the e2e tests.
I have added them mostly for consistency so that all the unit tests in the repo have the same environment. Mostly for cognitive load.
What do you think?
| describe('Method: constructor', () => { | ||
|
|
||
| test('when instantiated with minimum options it creates an instance with default values', () => { | ||
|
|
||
| // Prepare | ||
| const persistenceLayer = new TestDynamoDBPersistenceLayer({ tableName: dummyTableName }); | ||
|
|
||
| // Act / Assess | ||
| expect(persistenceLayer).toEqual(expect.objectContaining({ | ||
| tableName: dummyTableName, | ||
| keyAttr: 'id', | ||
| statusAttr: 'status', | ||
| expiryAttr: 'expiration', | ||
| inProgressExpiryAttr: 'in_progress_expiry_attr', | ||
| dataAttr: 'data' | ||
| })); | ||
|
|
||
| }); | ||
|
|
||
| test('when instantiated with specific options it creates an instance with correct values', () => { | ||
|
|
||
| // Prepare | ||
| const testDynamoDBPersistenceLayerOptions: DynamoPersistenceConstructorOptions = { | ||
| tableName: dummyTableName, | ||
| keyAttr: dummyKey, | ||
| statusAttr: 'someStatusAttr', | ||
| expiryAttr: 'someExpiryAttr', | ||
| inProgressExpiryAttr: 'someInProgressExpiryAttr', | ||
| dataAttr: 'someDataAttr' | ||
| }; | ||
| const persistenceLayer = new TestDynamoDBPersistenceLayer(testDynamoDBPersistenceLayerOptions); | ||
|
|
||
| // Act / Assess | ||
| expect(persistenceLayer).toEqual(expect.objectContaining({ | ||
| tableName: dummyTableName, | ||
| keyAttr: dummyKey, | ||
| statusAttr: testDynamoDBPersistenceLayerOptions.statusAttr, | ||
| expiryAttr: testDynamoDBPersistenceLayerOptions.expiryAttr, | ||
| inProgressExpiryAttr: testDynamoDBPersistenceLayerOptions.inProgressExpiryAttr, | ||
| dataAttr: testDynamoDBPersistenceLayerOptions.dataAttr | ||
| })); | ||
|
|
||
| }); | ||
|
|
||
| }); | ||
|
|
There was a problem hiding this comment.
All member variables in this class are private. Should we break the abstraction here?
The private value are indirectly tested in the _putRecord() and other protected fields. Those test cases would detect an issue if the private values are incorrect.
There was a problem hiding this comment.
I see your point.
The reason why I introduced these tests is primarily to reach 100% coverage.
If we remove this test, coverage drops:
and the reason, I think, is because we have conditional statements in the constructor:
this.tableName = constructorOptions.tableName;
this.keyAttr = constructorOptions.keyAttr ?? 'id';
this.statusAttr = constructorOptions.statusAttr ?? 'status';
this.expiryAttr = constructorOptions.expiryAttr ?? 'expiration';
this.inProgressExpiryAttr = constructorOptions.inProgressExpiryAttr ?? 'in_progress_expiry_attr';
this.dataAttr = constructorOptions.dataAttr ?? 'data';Since in all other test cases, including _putRecord we are always using the defaults, we never testing the left-side of the ?? (except for tableName).
Now, I could have an additional test to one of the other methods (i.e. _putRecord) and pass some values when initializing the class, however I saw that test as really testing the constructor, and so I added its own case.
If there's another way I'm happy to change it though, I have to admit I went this way also because this is how I've done it in Tracer.
There was a problem hiding this comment.
I see your point. I'll prefer to have additional _putRecord. But let's keep it as it is for now. I would like us to have unit test run in workflow early. We could refactor the tests afterward.
Co-authored-by: ijemmy <[email protected]>
|
I will be merging this PR so that it can be included in this week's release. In case of any comment please feel free to either leave one here or open an issue, we'll address any change in a new PR. |
Description of your changes
As described in #1159, this PR makes it so that the existing unit tests for the Idempotency utility are now run as part of the PR checks as well as on merge & pre-push hooks.
Additionally, this PR introduces the following changes:
makeFunctionIdempotentfrom test coverage (to be reintroduced once implemented)How to verify this change
Check the results of the automated checks below this PR.
Related issues, RFCs
Issue number: #1159
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.