Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test result list #92

Merged
merged 17 commits into from
Nov 17, 2022
Merged

Test result list #92

merged 17 commits into from
Nov 17, 2022

Conversation

stefpiatek
Copy link
Collaborator

@stefpiatek stefpiatek commented Nov 14, 2022

Adding in automated tests for result list, also ensured use of identifiers that should have been there before

Closes #89

@stefpiatek stefpiatek temporarily deployed to test-env November 15, 2022 10:14 Inactive
@stefpiatek stefpiatek temporarily deployed to test-env November 15, 2022 10:26 Inactive
@stefpiatek stefpiatek temporarily deployed to test-env November 15, 2022 10:46 Inactive
Copy link
Collaborator

@jhughes982 jhughes982 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

Comment on lines +164 to +167
const renderTestReportForm = () => {
render(<ContextAndModal children={<ReportForm initialValues={noValues} />} />, { wrapper: BrowserRouter });
};

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed in order to render a modal during the test?

One thing to consider. It could be beneficial to weigh up having more integration tests vs. having a small number of end-to-end tests that go through the form like a human would. You could use a tool called Puppeteer or Cypress to do this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep it is! though also a router is also required so that we can use useNavigate, even if we're mocking it. Agreed, though this style of integration testing also feels similar to how a user would interact with the site, that would give us the benefit of not having to create our component for testing

Comment on lines +109 to +111
beforeEach(() => {
return clearFhirAndSendReports();
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this test delete the data already in the development fhir server if its run locally? I'm just thinking whether it's desirable for a test to change the data? I guess it doesn't matter too much, but thought I'd raise the question.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it does, I think that if this were rolled out further into production then it might be worth adding some guards to the setup functions so that they can't be run on a real server.

@stefpiatek stefpiatek merged commit 01f0b9b into main Nov 17, 2022
@stefpiatek stefpiatek deleted the test-results branch November 17, 2022 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

automated test for result list page
3 participants