test: add end to end tests for subscriptions #578
Conversation
There was a problem hiding this comment.
Looks good. There are several minor changes to be made.
Also, can you add tests for Subscription validation? those ones are easy, just post invalid Subscriptions (non-allowlisted endpoint, unsupported channel, invalid criteria) and assert that you get a 400 error
| const postPatientResult = await client.post('Patient', resourceThatMatchesSubscription); | ||
| expect(postPatientResult.status).toEqual(201); | ||
| // 3. Verify that notifications are received | ||
| // give 2 minutes for notification to be placed in ddb table |
There was a problem hiding this comment.
actually we need to wait one minute after creating the subscription but BEFORE posting the Patient that matches the subscription.(otherwise the Subscription may not be enabled yet due to caching)
I think that this needs to be fixed on tenant isolation tests as well
| expect(postPatientResult.status).toEqual(201); | ||
| // 3. Verify that notifications are received | ||
| // give 2 minutes for notification to be placed in ddb table | ||
| await new Promise((r) => setTimeout(r, 120_000)); |
There was a problem hiding this comment.
This is used several times, It'd be better to declare this helper function once.
You can move an existing sleep fn to test utils and then use it everywhere:
| // now we search for our patient to make sure it was updated correctly | ||
| const searchResult = await client.get( | ||
| `${subResource.criteria}&_lastUpdated=gt${new Date(new Date().getTime() - 180_000).toISOString()}`, | ||
| ); |
There was a problem hiding this comment.
Not really needed. There are other existing tests for update/search
| let notifications = await subscriptionsHelper.getNotifications( | ||
| `/${uuid}/Patient/${postPatientResult.data.id}`, | ||
| ); | ||
| expect(notifications).not.toEqual([]); | ||
| expect(notifications[0].httpMethod).toEqual('PUT'); | ||
| expect(notifications[0].body).toBeNull(); |
There was a problem hiding this comment.
This would be faster using waitForExpect. That way you don't need to wait an entire minute.
Also, can you assert that the headers match?
| // 4. Delete the Subscription | ||
| const deleteSubscriptionResult = await client.delete(`Subscription/${postSubscriptionResult.data.id}`); | ||
| expect(deleteSubscriptionResult.status).toEqual(200); | ||
| await new Promise((r) => setTimeout(r, 120_000)); |
There was a problem hiding this comment.
This wait can be only 1 minute
| // 2. Create/Update a resource that matches the subscription. | ||
| const postPatientResult = await client.post('Patient', resourceThatMatchesSubscription); |
There was a problem hiding this comment.
We need to test Update as well.
| notifications = await subscriptionsHelper.getNotifications(`/${uuid}/Patient/${postPatientResult.data.id}`); | ||
| // we still have the one notification from earlier in the test, but no more | ||
| expect(notifications.length).toEqual(1); |
There was a problem hiding this comment.
I think that we also need to lookup notifications for postAnotherPatientResult.data.id and make sure that there are none.
| await expect(client.post('Subscription', subResource)).rejects.toMatchObject({ | ||
| response: { status: 400 }, | ||
| }); | ||
| subResource.channel.type = 'SMS'; |
There was a problem hiding this comment.
this needs to be sms (case sensitive). Make sure that you are using the codes as they appear on https://fhir-ru.github.io/valueset-subscription-channel-type.html#expansion across this tests
Here you are getting a 404 because 'SMS' is an invalid code, so the validator (either HAPI or JSON schema) reject it, but what we want to test is the custom logic we wrote to reject valid channels that we don't support
There was a problem hiding this comment.
Gotcha, I've updated the test to the correct spelling and also updated websocket to match the standard as well.
| const notifications = await subscriptionsHelper.getNotifications( | ||
| `/${uuid}/Patient/${postPatientResult.data.id}`, | ||
| ); | ||
| expect(notifications).not.toEqual([]); |
There was a problem hiding this comment.
you can remove this line. The length check on the next line is sufficient
| expect(notifications[0].httpMethod).toEqual('PUT'); | ||
| expect(notifications[0].body).toBeNull(); | ||
| expect(notifications[0].headers).toHaveProperty('x-api-key', SUBSCRIPTIONS_API_KEY); |
There was a problem hiding this comment.
for completeness, can you also make this assertions for notifications[1]?
| expect(notifications[0].httpMethod).toEqual('POST'); | ||
| expect(notifications[0].body).toBeNull(); | ||
| expect(notifications[0].headers).toHaveProperty('x-api-key', SUBSCRIPTIONS_API_KEY); |
There was a problem hiding this comment.
Same as above, let's assert the same for notifications[1]
* 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:
Description of changes:
Added end to end tests for empty notification and id notification scenarios. Tested using
yarn int-testand making sure that the notification table and output looked as expected through the AWS Console.Checklist:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.