(test) O3-2428: E2E test for appointment scheduling #1664

Vijaykv5 · 2024-02-14T16:48:22Z

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 covers E2E test to test the appointment workflow.

The test involves adding an appointment for a patient, edit the added appointment, and cancel it.

Screenshots

appointments-test.mov

Related Issue

O3-2428

e2e/specs/add-appointments.spec.ts

Vijaykv5 · 2024-02-15T14:24:35Z

@jayasanka-sack May you can look into this PR.
I'm failing to add this test case -

Then I should see the Schedule appointment workspace

Vijaykv5 · 2024-02-17T12:31:11Z

@jayasanka-sack @RandilaP May you can check this PR?
I've added the test video in description
Thanks

jayasanka-sack

Great progress, @Vijaykv5! 🎉 I've reviewed the changes and added some comments. Well done!

jayasanka-sack · 2024-02-19T13:32:17Z

e2e/specs/add-appointments.spec.ts

+  await test.step('When I click on appointment tab', async () => {
+    await appointmentsPage.goto(patient.uuid);
+  });
+  //test to add appointment


The test steps are sufficiently descriptive; therefore, additional comments are unnecessary.

jayasanka-sack · 2024-02-19T13:33:20Z

e2e/specs/add-appointments.spec.ts

+  const appointmentsPage = new AppointmentsPage(page);
+  await test.step('When I click on appointment tab', async () => {


Suggested change

const appointmentsPage = new AppointmentsPage(page);

await test.step('When I click on appointment tab', async () => {

const appointmentsPage = new AppointmentsPage(page);

await test.step('When I go to the appointment tab in the patient chart', async () => {

jayasanka-sack · 2024-02-19T13:33:40Z

e2e/specs/add-appointments.spec.ts

+    await page.click('button:has-text("Add")');
+  });
+
+  await test.step('When I select Mobile Clinic location', async () => {


Suggested change

await test.step('When I select Mobile Clinic location', async () => {

await test.step('And I select Mobile Clinic location', async () => {

jayasanka-sack · 2024-02-19T13:35:49Z

e2e/specs/add-appointments.spec.ts

+  });
+
+  await test.step('When I select Mobile Clinic location', async () => {
+    await page.locator('#location').selectOption('8d9045ad-50f0-45b8-93c8-3ed4bce19dbf');


Please refer to @ibacher 's comment on slack:

Always prefer grabbing the element by user-facing text (in this case the ARIA label) rather than technical details like id / class / tag as we want our tests to be robust to markup changes.

@jayasanka-sack There's no element under options other than changing their values of location.
We have used the same here

Can we try with something akin to what's showcased in the Playwright documentation (link)?

await page.getByLabel('Choose a color').selectOption({ label: 'Blue' });

This applies for other fields as well.

Oops I missed that!
Fixed now :)
Thanks

jayasanka-sack · 2024-02-19T13:50:25Z

e2e/specs/add-appointments.spec.ts

+    await page.locator('#datePickerInput').fill('18/02/2024');
+  });
+
+  await test.step('And I set the “Duration”', async () => {


Suggested change

await test.step('And I set the “Duration”', async () => {

await test.step('And I set the “Duration” to 60', async () => {

jayasanka-sack · 2024-02-19T13:53:18Z

e2e/specs/add-appointments.spec.ts

+    await expect(page.getByText(/Appointment scheduled/i)).toBeVisible();
+  });
+  // test for editing the appointment
+  await test.step('And I choose the "Edit" option from the popup menu', async () => {


Break this down to two steps:

When I click the options kebab menu in the appointment

And I choose the "Edit" option
This applies to the deletion too.

Also add the following step

Then I should see the Edit an appointment workspace

jayasanka-sack · 2024-02-19T13:55:50Z

e2e/specs/add-appointments.spec.ts

+    await page.getByRole('menuitem', { name: 'Cancel' }).click();
+  });
+
+  await test.step('And I cancel the appointmen', async () => {


Then I should see a confirmation modal

When I click the "Cancel appointment" button to confirm

RandilaP

Good job @Vijaykv5

RandilaP · 2024-02-26T11:22:36Z

e2e/pages/appointments-page.ts

+export class AppointmentsPage {
+  constructor(readonly page: Page) {}
+
+  readonly appointmentsTable = () => this.page.getByRole('table', { name: /appointments/i });


Suggested change

readonly appointmentsTable = () => this.page.getByRole('table', { name: /appointments/i });

readonly appointmentsTable = () => this.page.getByRole('table', { name: /appointments/i });

I think it's best to Add a getByTestId to the table locator

It's better to avoid the use of ids, classes as much as possible. #1664 (comment)

RandilaP · 2024-02-26T11:27:19Z

e2e/specs/add-appointments.spec.ts

+  });
+
+  await test.step('And I set the “Duration” to 60', async () => {
+    await page.locator('#duration').fill('60');


Suggested change

await page.locator('#duration').fill('60');

await page.locator('#duration').fill('60');

Use getById

RandilaP · 2024-02-26T11:27:36Z

e2e/specs/add-appointments.spec.ts

+  });
+
+  await test.step('And I set the “Duration” of the appointment”', async () => {
+    await page.locator('#duration').fill('80');


Suggested change

await page.locator('#duration').fill('80');

await page.locator('#duration').fill('80');

Here as well

Vijaykv5 · 2024-02-26T15:42:52Z

Thanks for the Review @jayasanka-sack @RandilaP
I've made changes as per the suggestions.
I wonder why the Test fails in the PR? although it's working fine locally when I tried to test particular component

RandilaP

@Vijaykv5 Good to see your progress

RandilaP · 2024-02-27T06:01:26Z

e2e/specs/add-appointments.spec.ts

+  await test.step('And I set date for tomorrow', async () => {
+    await page.fill('input[placeholder="dd/mm/yyyy"]', '30/03/2024');
+  });


Suggested change

await test.step('And I set date for tomorrow', async () => {

await page.fill('input[placeholder="dd/mm/yyyy"]', '30/03/2024');

});

await test.step('And I set date for tomorrow', async () => {

await page.fill('input[placeholder="dd/mm/yyyy"]', '30/03/2024');

});

Nice to see your progress in tests @Vijaykv5. I would like to highlight that we are using these E2E tests again and again when someone sends a PR. So after the date that u added in this test these tests will fail because this appointment is going to move to the past appointment section. I would like to suggest you if you are adding a date manually make sure add a previous date and in the test make sure to go through the past appointment section.

You can use Js to get tomorrow's date.

ex:

const tomorrow = new Date(); tomorrow.setDate(tomorrow.getDate() + 1); await page.fill('input[placeholder="dd/mm/yyyy"]', tomorrow.toLocaleDateString('en-GB'));

btw, can't we use page.getByLabel().fill here?

RandilaP · 2024-02-27T06:01:46Z

e2e/specs/add-appointments.spec.ts

+  await test.step('And I change the date to Today', async () => {
+    await page.fill('input[placeholder="dd/mm/yyyy"]', '28/03/2024');
+  });


Here as well

Yes but I think as @jayasanka-sack It would best way to make into today's date.

RandilaP · 2024-02-28T03:57:11Z

@Vijaykv5 Make this PR a draft PR

jayasanka-sack · 2024-02-28T04:04:20Z

Thanks for the Review @jayasanka-sack @RandilaP I've made changes as per the suggestions. I wonder why the Test fails in the PR? although it's working fine locally when I tried to test particular component

You can try to view the report and check what's going wrong: https://o3-docs.openmrs.org/docs/frontend-modules/testing#view-test-reports-from-github-actions--bamboo

RandilaP

@Vijaykv5 Good job

RandilaP · 2024-03-05T02:36:36Z

e2e/specs/add-appointments.spec.ts

+  });
+
+  await test.step('When I change to “Inpatient ward” location', async () => {
+    await page.getByLabel('Select location').selectOption('Inpatient Ward');


Suggested change

await page.getByLabel('Select location').selectOption('Inpatient Ward');

await page.getByLabel('Select location').selectOption('Inpatient Ward');

Can you check in other E2E tests how they select options in the tests. Test is failing because of it. Download the test report and check the video where it's failing.

kdaud

Great work @Vijaykv5! 👍

I've added a few comments to address.

kdaud · 2024-03-06T16:01:21Z

e2e/specs/add-appointments.spec.ts

+let patient: Patient;
+
+test.beforeEach(async ({ api }) => {
+  patient = await generateRandomPatient(api);


Should we start patient visit before recording the appointment, @Vijaykv5 what do you think?

Oops I missed that out. Yes that right!
Thank you @kdaud
I've updated new changes

Appointments should be able to be scheduled without requiring an active visit. That's a kind of basic feature of an appointment scheduling application.

kdaud · 2024-03-06T16:04:27Z

e2e/specs/add-appointments.spec.ts

+  patient = await generateRandomPatient(api);
+});
+
+test('Add appointment for a patient, edit the added appointment and cancel it', async ({ page, api }) => {


I would suggest we re-title the test to

Add, edit and cancel an appointment

I would suggest this be broken up into three tests, as it's three features:

Add an appointment

Edit an existing appointment

Cancel an existing appointment

I would suggest this be broken up into three tests, as it's three features:

Add an appointment

Edit an existing appointment

Cancel an existing appointment

@ibacher we are following this doc for the tests. So for adding tests for appointment section, is it better to do it as a whole test for appointment section?

I would suggest this be broken up into three tests, as it's three features:

@jayasanka-sack what do you think concerning Ian's idea?

I suggested using a single flow to minimize API usage for the data population. But, after seeing how lengthy the test will be, I also think breaking it into three tests would simplify it.

I also think breaking into three tests would make it much simpler.

@jayasanka-sack will this apply across all tests with the add, edit and delete workflows? cc: @denniskigen

We've got just a few that contains in single flow. Let's leave them as is for the time being and beef them up later when we can. Our priority is to expand our coverage. But, if you happen to have some extra time, feel free to tweak them too.

There's definitely a balance of concerns, but if the main concern is how much data is created during, it should be possible, for example, to create a single patient and run a suite of tests using that single patient (i.e., there's no reason here that the patient needs to be created in a beforeEach() rather than a beforeAll(), especially since we're fundamentally only working on data attached to the patient record rather than the patient record itself).

My concern here is related to this point in that doc: "We want business-level users to easily be able to see a report that reassures them about what functionality is covered". Specifically, if the "Add appointment for a patient, edit the added appointment and cancel it" test fails, then, as a business user, I'm stuck regarding that whole feature as broken, which is much more concerning than saying "We can currently add appointments, but something broke the cancellation flow", for example.

I also agree with @jayasanka-sack's point that the priority should be improving test coverage, not going back and revising everything to match some "gold standard" for how tests are written.

Created a ticket to monitor the proposed E2E test approach.

kdaud · 2024-03-06T16:45:23Z

I would suggest to rename the spec file to 'appointments.spec.ts'. @Vijaykv5 let me know your thought?

kdaud · 2024-03-19T11:46:22Z

@Vijaykv5 as a result of O3-2891 we need to move the work on this PR to https://github.com/openmrs/openmrs-esm-patient-management/

Vijaykv5 · 2024-03-19T12:13:53Z

@Vijaykv5 as a result of O3-2891 we need to move the work on this PR to https://github.com/openmrs/openmrs-esm-patient-management/

Do I need to add the PR there?

kdaud · 2024-03-19T12:22:58Z

yes

denniskigen · 2024-03-25T19:06:00Z

I'm closing this in favour of a new PR in https://github.com/openmrs/openmrs-esm-patient-management following #1739. Let me know if you need any help setting that up, @Vijaykv5.

Vijaykv5 · 2024-03-25T19:16:09Z

@denniskigen I have made a New PR here
May you can check why it's failing there? It runs fine locally,I also have some issue regarding the commit history there.
Appreciate if you could help on that
Thanks!

denniskigen · 2024-03-25T19:46:00Z

I've responded to your question in the other PR.

(test)O3-2428: E2E test for appointment workflow

53c045e

Vijaykv5 marked this pull request as draft February 14, 2024 16:48

Vijaykv5 commented Feb 14, 2024

View reviewed changes

e2e/specs/add-appointments.spec.ts Outdated Show resolved Hide resolved

Merge branch 'main' into test/O3-2428

5c36eaa

Vijaykv5 added 3 commits February 17, 2024 00:02

Merge branch 'openmrs:main' into test/O3-2428

23a14a7

test for add appointment

bc721c0

added test for edit and cancel appointment

ac24aba

Vijaykv5 marked this pull request as ready for review February 17, 2024 12:30

Vijaykv5 requested a review from jayasanka-sack February 17, 2024 12:30

Merge branch 'main' into test/O3-2428

e0e6be8

jayasanka-sack requested changes Feb 19, 2024

View reviewed changes

Vijaykv5 added 2 commits February 21, 2024 19:06

Merge branch 'openmrs:main' into test/O3-2428

09f4967

Updated tests

7b0cdc6

Vijaykv5 requested a review from jayasanka-sack February 21, 2024 15:25

Vijaykv5 added 4 commits February 22, 2024 20:04

Merge branch 'openmrs:main' into test/O3-2428

bd19ac6

Minor changes made

f92e560

Merge branch 'main' into test/O3-2428

20ceaa3

Merge branch 'main' into test/O3-2428

d811207

RandilaP suggested changes Feb 26, 2024

View reviewed changes

Vijaykv5 added 2 commits February 26, 2024 19:59

Merge branch 'openmrs:main' into test/O3-2428

98e3e38

New changes made

a30e561

Merge branch 'main' into test/O3-2428

61b963a

Vijaykv5 requested a review from RandilaP February 26, 2024 15:44

RandilaP suggested changes Feb 27, 2024

View reviewed changes

Merge branch 'openmrs:main' into test/O3-2428

84dafc0

Vijaykv5 marked this pull request as draft February 28, 2024 16:52

fixed the date format

41e5296

Vijaykv5 requested a review from RandilaP March 1, 2024 16:18

Merge branch 'main' into test/O3-2428

297ead6

RandilaP reviewed Mar 5, 2024

View reviewed changes

jayasanka-sack mentioned this pull request Mar 5, 2024

(test) O3-2927: Add E2E tests for editing, and discontinuing a drug order #1714

Merged

3 tasks

Vijaykv5 added 4 commits March 5, 2024 19:38

Merge branch 'openmrs:main' into test/O3-2428

90e76d6

Merge branch 'openmrs:main' into test/O3-2428

ae74d4c

Fixed E2E test

eaa061d

Fixed E2E testing

087fc57

kdaud reviewed Mar 6, 2024

View reviewed changes

Added patient visit

38299a3

Renamed to appointments.spec.ts

a5a251b

denniskigen changed the title ~~(test)O3-2428: E2E test for appointment scheduling~~ (test) O3-2428: E2E test for appointment scheduling Mar 6, 2024

denniskigen closed this Mar 25, 2024

jayasanka-sack mentioned this pull request Mar 26, 2024

(test) O3-2428: E2E test for appointment scheduling openmrs/openmrs-esm-patient-management#1062

Merged

3 tasks

Vijaykv5 mentioned this pull request Apr 5, 2024

(test) O3-2943: Edit E2E test for the immunization recording workflow #1768

Open

3 tasks

		const appointmentsPage = new AppointmentsPage(page);
		await test.step('When I click on appointment tab', async () => {

	await test.step('When I select Mobile Clinic location', async () => {
	await test.step('And I select Mobile Clinic location', async () => {

	await test.step('And I set the “Duration”', async () => {
	await test.step('And I set the “Duration” to 60', async () => {

	readonly appointmentsTable = () => this.page.getByRole('table', { name: /appointments/i });
	readonly appointmentsTable = () => this.page.getByRole('table', { name: /appointments/i });

	await page.locator('#duration').fill('60');
	await page.locator('#duration').fill('60');

	await page.locator('#duration').fill('80');
	await page.locator('#duration').fill('80');

	await page.getByLabel('Select location').selectOption('Inpatient Ward');
	await page.getByLabel('Select location').selectOption('Inpatient Ward');

(test) O3-2428: E2E test for appointment scheduling #1664

(test) O3-2428: E2E test for appointment scheduling #1664

Conversation

Vijaykv5 commented Feb 14, 2024 • edited

Requirements

Summary

Screenshots

Related Issue

Vijaykv5 commented Feb 15, 2024

Vijaykv5 commented Feb 17, 2024 • edited

jayasanka-sack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RandilaP left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vijaykv5 commented Feb 26, 2024

RandilaP left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RandilaP commented Feb 28, 2024

jayasanka-sack commented Feb 28, 2024

RandilaP left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdaud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayasanka-sack Mar 7, 2024 • edited

Choose a reason for hiding this comment

kdaud Mar 7, 2024 • edited

Choose a reason for hiding this comment

jayasanka-sack Mar 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdaud commented Mar 6, 2024 • edited

kdaud commented Mar 19, 2024

Vijaykv5 commented Mar 19, 2024

kdaud commented Mar 19, 2024

denniskigen commented Mar 25, 2024

Vijaykv5 commented Mar 25, 2024

denniskigen commented Mar 25, 2024

Vijaykv5 commented Feb 14, 2024 •

edited

Vijaykv5 commented Feb 17, 2024 •

edited

jayasanka-sack Mar 7, 2024 •

edited

kdaud Mar 7, 2024 •

edited

jayasanka-sack Mar 7, 2024 •

edited

kdaud commented Mar 6, 2024 •

edited