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) O3-3601 : E2E tests for User onboarding #7

Merged
merged 12 commits into from
Aug 5, 2024

Conversation

Vijaykv5
Copy link
Contributor

@Vijaykv5 Vijaykv5 commented Jul 15, 2024

Requirements

  • This PR has a title that briefly describes the work done including a conventional commit type prefix and a Jira ticket number if applicable. See existing PR titles for inspiration.

For changes to apps

If applicable

  • My work includes tests or is validated by existing tests.

Summary

This PR adds E2E test for basic user onboarding walkthroughs.

Screenshots

Related Issue

O3-3601

Other

@Vijaykv5 Vijaykv5 marked this pull request as draft July 15, 2024 08:46
@Vijaykv5
Copy link
Contributor Author

Screenshot 2024-07-15 at 3 45 20 PM

Getting this error while running E2E tests and failure in getting the first step itself for getting the help menu button.
Any possible reason for it? @Piumal1999 @jayasanka-sack

Screenshot 2024-07-15 at 3 54 18 PM

@Piumal1999
Copy link
Collaborator

Hi @Vijaykv5 ,

I couldn't reproduce the above-mentioned error. Did that occur when you were running the tests locally?

Btw, the GitHub action is failing due to an incorrect script. To fix it, just replace the e2e/support/github/run-e2e-docker-env.sh file with the new file given here. And then add the esm-help-menu app to the list of required modules like this.

@Piumal1999
Copy link
Collaborator

The test fails because it starts before the page is fully loaded. The simplest way to fix this is by using the Playwright waitForLoadState function, like this:

home-page.ts file:

async goto() {
  await this.page.goto(`home`);
  await this.page.waitForLoadState('networkidle');
}

This will make it wait until there are no network connections for at least 500 ms. However, the Playwright documentation discourages using this method, as the best practice is to rely on visual components.

Another way to fix this issue is to wait for a specific component to load, such as the search button, which usually loads last. However, this method does not guarantee that all other components are fully loaded, so the error might occur again.

@jayasanka-sack WDYT?

@jayasanka-sack
Copy link
Member

This is very helpful @Piumal1999 !

I prefer the second option; making sure all relevant elements are loaded before triggering the tutorial by adding some assertions.

@jayasanka-sack jayasanka-sack marked this pull request as ready for review July 26, 2024 13:33
Copy link
Collaborator

@Piumal1999 Piumal1999 left a comment

Choose a reason for hiding this comment

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

@Vijaykv5 Looks like some the selectors you used are no longer there in the UI. Can you recheck those locally and update the tests?

e2e/specs/onboarding-test.spec.ts Outdated Show resolved Hide resolved
@Vijaykv5
Copy link
Contributor Author

Still the same issue in second step to wait for the search icon element, but working fine locally.

playwright.config.ts Outdated Show resolved Hide resolved
Copy link
Member

@jayasanka-sack jayasanka-sack left a comment

Choose a reason for hiding this comment

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

@Vijaykv5 could you please address these comments (it applies to all other places.)

I'd recommend keeping videos on all the time because:

  • If a test fails, we know something went wrong.
  • If a test passes, we can see all the steps, but we still can't be sure the tooltips are in the right place. So, having videos helps us manually confirm when needed.

});

await test.step('And I click on the Basic Tutorial walkthrough', async () => {
await page.getByText('Walkthrough').nth(1).click();
Copy link
Member

Choose a reason for hiding this comment

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

We can't guarantee the order of the list. The order may change when we add new tutorials. So, we need to think of a better selector.

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 I didn't figure out how we can select those walkthoroughs.
An approach that came to my mind is having a selector in the tutorial modal #L49 and searching for the Heading and corresponding walkthrough. WDYT?

Copy link
Member

@jayasanka-sack jayasanka-sack Aug 2, 2024

Choose a reason for hiding this comment

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

First of all, I've noticed a few adjustments needed for the tutorial to follow best practices:

  • Change the list to an unordered list. This will be beneficial for both testing and accessibility.
  • Currently, the walkthrough button is a div. It should be a button. (A ghost button would be the option.)

Here is how your code should look (there won't be any visual changes):

<ul>
   {tutorials.map -> tutorial}
      <li>
          <h3>{title}</h3>
          <p>{description}</p>
          <Button kind="ghost">Button</Button>
      </li>
   {/tutorials}
</ul>

Once you've done that, you can select the button with this:

const tutorialTitle = "Basic ..."
await page.locator('li', { hasText: 'my item' }).locator('button').click();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a small visual change in the alignment of buttons. is this fine?

walkthrough-btn.mov

Copy link
Member

Choose a reason for hiding this comment

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

That's fine. Btw, can we get the close button fixed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fine. Btw, can we get the close button fixed?

This is due to the upgrade in carbon dependencies in this PR

This would be the fix #215, I'll talk with dennis regarding merge of that PR in todays coffee call.

});

await test.step('Then I should see the first Joyride tooltip', async () => {
await expect(page.locator('div').filter({ hasText: 'Welcome to OpenMRS!' }).nth(1)).toBeVisible();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
await expect(page.locator('div').filter({ hasText: 'Welcome to OpenMRS!' }).nth(1)).toBeVisible();
await expect(page.getByText('Welcome to OpenMRS!')).toBeVisible();

e2e/specs/onboarding-test.spec.ts Show resolved Hide resolved
});

await test.step('Then I should see the user icon Joyride tooltip', async () => {
await expect(page.getByText('The user icon')).toBeVisible();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
await expect(page.getByText('The user icon')).toBeVisible();
await expect(page.getByText('The user icon. Click here to change your user preferences and settings.')).toBeVisible();

Copy link
Member

Choose a reason for hiding this comment

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

Make sure you check for the exact full text.

Copy link
Member

Choose a reason for hiding this comment

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

This applies to other places as well.

playwright.config.ts Outdated Show resolved Hide resolved
@jayasanka-sack jayasanka-sack merged commit 5240200 into openmrs:main Aug 5, 2024
2 checks passed
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