Ja/dpc -4984 - Adding smoke test for patient everything#2906
Ja/dpc -4984 - Adding smoke test for patient everything#2906Jose-verdance wants to merge 14 commits intomainfrom
Conversation
MEspositoE14s
left a comment
MEspositoE14s
left a comment
There was a problem hiding this comment.
A couple of nitpicks, but they can probably be spun off into another ticket if you're ready to deploy.
jdettmannnava
left a comment
jdettmannnava
left a comment
There was a problem hiding this comment.
Just some ideas...
|
Works across all remote environments ✅
Patient $everything covered for both sync and async ✅
The only obvious thing popping out for me is that we have multiple k6 workflows that monitor bulk exports and we probably should share that code (already called out above ☝️ ) |
| { | ||
| 'get patient everything returns 200': res => res.status == 200, | ||
| } | ||
| ) |
There was a problem hiding this comment.
Should probably check the output (see monitorJob from bulk_export.js,)
There was a problem hiding this comment.
Could you please check for errors?
There was a problem hiding this comment.
Hey @jdettmannnava, can you elaborate on what errors you have in mind? My understanding is that the synchronous call will either succeed and return 200 or it will fail with an operation outcome with 403 or 404 if patient id is not found. This would be covered by this simple smoke test check but I might be missing something?
There was a problem hiding this comment.
Doesn't patient everything sync return the actual data?
There was a problem hiding this comment.
Hey @jdettmannnava I didn't quite catch the elaboration on that last point, but I've added an additional check for the response content. Let me know if that addresses what you had in mind.
manojwadhwa81
left a comment
manojwadhwa81
left a comment
There was a problem hiding this comment.
pollJobStatus could be extracted out as a reusable function. Also it's going to sleep for 20 seconds for the first iteration in all environments except local.. check out if we can reduce that based upon the actual time it takes. Looks good to me otherwise.
Hi @manojwadhwa81 I refactored it to reuse the monitorJob method and moved it to a smoke_test_utils. Also, the 20 seconds was from monitorJob which is doing so for rate limit reasons. |
Jose-verdance
requested review from
MEspositoE14s,
jdettmannnava,
lukey-luke and
manojwadhwa81
| jobResponse, | ||
| { | ||
| 'job response code was 200': res => res.status === 200, | ||
| 'no job output errors': res => res.json().error.length <= JOB_OUPUT_ERROR_LENGTH, |
There was a problem hiding this comment.
@Jose-verdance : this is the check I am talking about
There was a problem hiding this comment.
Hi @jdettmannnava I am now using the monitorJob for the async call so this check is being done. The sync version of patient everything doesn't return "error" in the response. It returns a resourceType of OperationOutcome if an error occurs.
There was a problem hiding this comment.
You are calling monitorJob for the async export -- I am asking you to check for errors in the synchronous export for parity.
lukey-luke
left a comment
lukey-luke
left a comment
There was a problem hiding this comment.
Thanks for breaking out monitorJob() to a shared function! LGTM
5 participants
🎫 Ticket
https://jira.cms.gov/browse/DPC-4984
🛠 Changes
This change adds new smoke test for both synchronous and asynchronous patient everything endpoints.
ℹ️ Context
This adds smoke test coverage to this high use endpoint.
🧪 Validation
Confirmed the smoke test succeed locally and in higher environments.
Dev
https://github.com/CMSgov/dpc-app/actions/runs/21990516533
Test
https://github.com/CMSgov/dpc-app/actions/runs/21958462870
Sandbox
https://github.com/CMSgov/dpc-app/actions/runs/21959205829
Prod
https://github.com/CMSgov/dpc-app/actions/runs/21960235280