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

(chore) O3-3574 : Refactor E2E test for appointments #1287

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

chimanwadike
Copy link
Contributor

Requirements

  • This PR has a title that briefly describes the work done including the ticket number. If there is a ticket, make sure your PR title includes a conventional commit label. See existing PR titles for inspiration.
  • My work conforms to the OpenMRS 3.0 Styleguide and design documentation.
  • My work includes tests or is validated by existing tests.

Summary

This PR refactors existing e2e tests for appointments by writing separate tests for adding, updating, and canceling.
This is to increase test clarity and visibility.

Related Issue

https://issues.openmrs.org/browse/O3-3574

test('Add an appointment', async ({ page, api }) => {
const appointmentsPage = new AppointmentsPage(page);
test.describe.serial('Running appointment tests sequentially', () => {
test('Add an appointment', async ({ page }) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jayasanka-sack @kdaud I would like to seek your views concerning the use of test.describe in scenario of sequential operations e.g. create, edit and delete like this one.
This is to improve some of our e2e tests going forward and prepare towards writing more advanced test cases.

Copy link
Member

@kdaud kdaud left a comment

Choose a reason for hiding this comment

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

@chimanwadike the refactor should involve decoupling the test scenarios for adding, editing, and canceling an appointment.

@chimanwadike
Copy link
Contributor Author

@chimanwadike the refactor should involve decoupling the test scenarios for adding, editing, and canceling an appointment.

@kdaud this is noted.

@chimanwadike
Copy link
Contributor Author

@chimanwadike the refactor should involve decoupling the test scenarios for adding, editing, and canceling an appointment.

@kdaud I've been away for a while now. Following your last comment, it appears we would have to create separate appointments to test edit and cancel if these test cases must be decoupled that far.

In order to decouple the tests the way you've explained and how I understand it, we could run into overheads of more API calls and could lead to slower tests in the grand scheme which is neither what we need.

I'm thinking there maybe no need for this PR, what do you think?

@brandones
Copy link
Contributor

@kdaud What do you think of what @chimanwadike wrote? Should we close this PR?

@chimanwadike
Copy link
Contributor Author

@brandones Thanks for revisiting this PR.
I still believe there are obvious improvements in this PR that should be maintained going forward even though I haven't splitted 'sequential' test cases to separate files as was suggested.

  1. Use of beforeAll and afterAll instead of beforeEach and afterEach where appropriate to maximise resource
  2. More test case readability, clarity and visibility.

So, to rephrase the question, should we accept the little improvement or close the PR?

Thanks

@brandones
Copy link
Contributor

Ok, @kdaud ?

@kdaud
Copy link
Member

kdaud commented Nov 14, 2024

it appears we would have to create separate appointments to test edit and cancel if these test cases must be decoupled that far.

Yes, this should be possible via the API. See an example here.

I'm thinking there maybe no need for this PR, what do you think?

It makes sense to refactor the PR by creating appointment via the API, then using it in edit and delete scenarios.

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.

3 participants