test: add tests for tenant isolation of subscriptions #577
Conversation
| // make sure the end date isn't caught by the reaper before the test completes | ||
| subResource.end = new Date(new Date().getTime() + 100000).toISOString(); |
There was a problem hiding this comment.
I suggest increasing time to at least 10 minutes (600_000). There's no need for it to be such a tight time window
There was a problem hiding this comment.
Made this the default time window for subscriptions created in utils.ts, and just changed the end time for the one test that needed things to expire quickly.
| test('tenant isolation', async () => { | ||
| const clientAnotherTenant = await getFhirClient({ tenant: 'tenant2' }); | ||
| // tenant 1 creates a subscription | ||
| const subResource = clone(subscriptionResource); |
There was a problem hiding this comment.
I suggest to create a function that generates Subscriptions instead of cloning the same one over and over. An example function to generate test Patients is:
I know some tests do the clone stuff, but it's not a good practice and we try not to d that anymore.
There was a problem hiding this comment.
Gotcha, I will avoid clone in the future. For now I've created a rather basic method to create subscriptions that takes in a uuid in utils.ts
| // give SLA of 20 seconds for notification to be placed in ddb table | ||
| await new Promise((r) => setTimeout(r, 20000)); | ||
| // make sure no notification was receieved for first tenant |
There was a problem hiding this comment.
We need to wait more time. Subscriptions may take up to a minute to actually start sending notifications(due to the cache in the SubscriptionMatcher), also, we need leniency on the SLA of 20 seconds (a system with steady traffic sends notifications well below 20 seconds, but with very little traffic (like integ tests envs) subscriptions face cold starts + batching windows in stream/sqs processing that make them slower)
I'd say update it to 2 minutes waiting.
There was a problem hiding this comment.
Updated to 2 min!
| await new Promise((r) => setTimeout(r, 20000)); | ||
| // make sure no notification was receieved for first tenant | ||
| const notifications = await subscriptionsHelper.getNotifications( | ||
| `/Patient/${postPatientResult.data.id}`, |
There was a problem hiding this comment.
This can get mixed up with Subscriptions created by other tests.
I suggest you add a UUID to the endpoint in all test Subscriptions, so that you can lookup in DDB for the path with the exact same UUID.
endpoint: `${SUBSCRIPTIONS_ENDPOINT}/<uuid>`There was a problem hiding this comment.
Used the uuid package to generate uuids for each test that generates subscriptions so no test interferes with another.
* feat: add GSI to Resource DDB Table (#533) * feat: Add data validation for subscription (#543) * fix: remove _subsciptionStatus from export result field (#555) * feat: sns, sqs, dlq for Subscriptions (#554) * feat: Rest hook Lambda (#558) * feat: subscriptionReaper (#557) * feat: add subscriptionsMatcher Lambda (#559) * test: Add Subscriptions test infrastructure/helper (#569) * fix: update unit tests for subscription reaper (#567) * test: add subscriptions env vars in gh actions (#572) * fix: encrypt logs for new Lambda fns (#574) * test: add Subscription reaper tests (#575) * feat: emit end to end latency metric from rest-hook Lambda (#570) * test: add tests for tenant isolation of subscriptions (#577) * feat: add DLQ for matcher Lambda (#576) * test: add end to end tests for subscriptions (#578) * perf: partial failures for restHook Lambda (#579) * docs: add Subscription docs (#582) Co-authored-by: Sukeerth Vegaraju <ssvegaraju@yahoo.co.in> Co-authored-by: zheyanyu <zheyanyu@amazon.com> Co-authored-by: Yanyu Zheng <yz2690@columbia.edu> Co-authored-by: brndhpkn <98061326+brndhpkn@users.noreply.github.com>
Issue #, if available:
FHIR -764
Description of changes:
Added tests for tenant isolation on subscriptions. The only modifications made to the other tests were to extract common elements such as subscription resource construction.
Tested by running
yarn int-testand ensuring through the AWS Console that the lambda function ran as expected and the notifications were present in the DDB Table.Checklist:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.