ISSUE-9: Add tests #10
Conversation
| setIps((ps) => ({...ps, history: history, isLoading: isLoading, error: error?.message})); | ||
| createIpsResource(uuid, abortController) | ||
| .then((response) => { | ||
| if (response.status === 200) { |
There was a problem hiding this comment.
What would happen if the response is not 200? I think we'd be better served by removing this check.
| isLowContrast: true, | ||
| kind: 'success', | ||
| title: t('ipsCreated', 'IPS'), | ||
| title: t('ipsCreationError', 'IPS'), |
There was a problem hiding this comment.
Shouldn't the title here be something like "Error updating IP"
| 'ipsNowAvailable', | ||
| 'The IPS has been updated and is now visible in the Patient History.', | ||
| 'checkForServerAvailability', | ||
| 'The Fhir server maybe unreachable or the IPS generation process exited with an error!', |
There was a problem hiding this comment.
FHIR like IPS is an acronym (Fast Healthcare Interoperability Resources)
| 'The Fhir server maybe unreachable or the IPS generation process exited with an error!', | |
| 'The FHIR server maybe unreachable or the IPS generation process exited with an error!', |
There was a problem hiding this comment.
It might be better to pass through a more detailed error from the backend. At the very least, the message should be logged to the console as an error.
| subtitle: t('checkForServerAvailability', 'The Fhir server maybe unreachable or the IPS generation process exited with an error!'), | ||
| }) | ||
| .finally(() => { | ||
| abortController.abort(); |
There was a problem hiding this comment.
It's actually kind of dangerous to call abort() on an instance that might be reused since further requests will already be aborted. It's probably better just to remove the abort controller from here altogether.
src/history/patient-history.test.tsx
Outdated
| const mockUsePatient = jest.mocked(usePatient); | ||
| const mockUseIpsResource = jest.mocked(useIpsResource); |
There was a problem hiding this comment.
Why not create the mocks once, outside of the it() block and then just change the return value?
src/history/patient-history.test.tsx
Outdated
|
|
||
| describe('Display patient IPS', () => { | ||
| beforeEach(() => { | ||
| const mockUseParams = useParams as jest.Mock; |
There was a problem hiding this comment.
| const mockUseParams = useParams as jest.Mock; | |
| const mockUseParams = jest.mocked(useParams); |
src/test.util.tsx
Outdated
| }, | ||
| }; | ||
|
|
||
| export const mockIPS = { |
There was a problem hiding this comment.
The usual convention would be to put this in a mocks folder rather than directly in src.
src/test.util.tsx
Outdated
There was a problem hiding this comment.
Use .ts for anything that doesn't actually use JSX.
| @@ -30,4 +33,5 @@ module.exports = { | |||
| testEnvironmentOptions: { | |||
| url: 'http://localhost/', | |||
| }, | |||
| }; | |||
| testTimeout: 25000, | |||
| }; | |||
jest.config.js
Outdated
| @@ -30,4 +33,5 @@ module.exports = { | |||
| testEnvironmentOptions: { | |||
| url: 'http://localhost/', | |||
| }, | |||
| }; | |||
| testTimeout: 25000, | |||
There was a problem hiding this comment.
Is this timeout strictly necessary? Things like this tend to make builds take a long time (since it potentially pauses for 25 seconds for a unit tests, which feels excessive).
Requirements
For changes to apps
If applicable
Summary
Issue: #9
Screenshots
Related Issue
Other