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

[WD-11114] Feat: Improving tests by using text assertion #789

Merged
merged 1 commit into from
Jun 6, 2024

Conversation

Kxiru
Copy link
Contributor

@Kxiru Kxiru commented Jun 5, 2024

Done

  • Renamed assertModificationStatus to assertTextVisible and replaced instances of the former.
  • Reviewed all usages of "getByText()" in /lxd-ui/tests/ and replaced with assertTextVisible where appropriate.
  • Rewrote the function to conditionally account for getByText instances where an Exact match was required.
export const assertTextVisible = async (
  page: Page,
  textValue: string,
  exact = false,
) => {
  const textLocator = page.getByText(textValue, { exact: exact });
  await expect(textLocator).toBeVisible();
};

QA

  1. Run the LXD-UI:
    • On the demo server via the link posted by @webteam-app below. This is only available for PRs created by collaborators of the repo. Ask @mas-who or @edlerd for access.
    • With a local copy of this branch, run as described here.
  2. Perform the following QA steps:
    • N/A

Screenshots

N/A

@webteam-app
Copy link

@Kxiru
Copy link
Contributor Author

Kxiru commented Jun 5, 2024

Observations from WD-11114:

  • In projects.spec.ts , test "project edit configuration" fails for both lxd-5.0-edge and lxd-latest-edge but for different reasons. For 5.0-edge, the checkbox option "Network Zones" is expected to be hidden, but it is visible.
  • Continued from above, for lxd-latest-edge, the same test fails because "Clusters" does not exist on the page ("Cluster" does, but this does not hold any clusters and thus the test would still fail".
  • In storage.spec.ts test "storage volume edit snapshot configuration" passes for Chromium:lxd-latest-edge but not for chromium:lxd-5.0-edge, as the test seems to have issues on the page.getByPlaceholder() line.

@Kxiru Kxiru force-pushed the improve-tests-with-text-assertion branch from 9b07be7 to 00b264e Compare June 5, 2024 15:08
@edlerd
Copy link
Collaborator

edlerd commented Jun 5, 2024

  • In projects.spec.ts , test "project edit configuration" fails for both lxd-5.0-edge and lxd-latest-edge but for different reasons. For 5.0-edge, the checkbox option "Network Zones" is expected to be hidden, but it is visible.

Did you run the 5.0-edge tests against a LXD backend from the 5.0/edge channel? The backend behaves differently depending on its version.

  • Continued from above, for lxd-latest-edge, the same test fails because "Clusters" does not exist on the page ("Cluster" does, but this does not hold any clusters and thus the test would still fail".

The cluster test relies on the clustering to be enabled. But be careful to enable it on your local host, it might block LXD on your machine in case the external ip changes -- which is not an expected behaviour for a cluster.

tests/helpers/permissions.ts Outdated Show resolved Hide resolved
tests/iso-volumes.spec.ts Outdated Show resolved Hide resolved
@Kxiru
Copy link
Contributor Author

Kxiru commented Jun 5, 2024

  • In projects.spec.ts , test "project edit configuration" fails for both lxd-5.0-edge and lxd-latest-edge but for different reasons. For 5.0-edge, the checkbox option "Network Zones" is expected to be hidden, but it is visible.

Did you run the 5.0-edge tests against a LXD backend from the 5.0/edge channel? The backend behaves differently depending on its version.

  • Continued from above, for lxd-latest-edge, the same test fails because "Clusters" does not exist on the page ("Cluster" does, but this does not hold any clusters and thus the test would still fail".

The cluster test relies on the clustering to be enabled. But be careful to enable it on your local host, it might block LXD on your machine in case the external ip changes -- which is not an expected behaviour for a cluster.

I'm not sure what this means. Are you referring to the different Playwright projects / browsers that the tests can be performed on? If so, yes.

@Kxiru Kxiru force-pushed the improve-tests-with-text-assertion branch 2 times, most recently from a7646c7 to 01a68c4 Compare June 5, 2024 16:46
tests/projects.spec.ts Outdated Show resolved Hide resolved
@edlerd
Copy link
Collaborator

edlerd commented Jun 5, 2024

I'm not sure what this means. Are you referring to the different Playwright projects / browsers that the tests can be performed on? If so, yes.

The playwright tests for 5.0-edge are meant to run against a LXD backend with version 5.0 from the edge channel. If you run that test suite against a LXD version latest/stable or alike they will fail. You can check with lxd version in your back end.

I'd suggest to setup lxd inside a container or vm with various version (i.e. 1 container for 5.0 with snap refresh lxd --channel=5.0/edge or snap install lxd --channel=5.0/edge. Then another container with the latest/edge channel. And on the host you can keep the 5.21/stable version. That way you can switch out the LXD_UI_BACKEND_IP in your .env.local file between host and the two containers, then restart dotrun, and you have a backend with a different version in seconds.

- Renamed assertModificationStatus to assertTextValue and replaced instances of the former.
- Reviewed all usages of "getByText()" in /lxd-ui/tests/ and replaced with assertTextValue where appropriate.
- Rewrote the function to conditionally account for getByText instances where an Exact match was required.

Signed-off-by: Nkeiruka <[email protected]>
@Kxiru Kxiru force-pushed the improve-tests-with-text-assertion branch from 01a68c4 to 203611c Compare June 6, 2024 11:51
await page.locator(".view-lines").click();
await page.keyboard.press("PageDown");
await page.keyboard.press("PageDown");
await page.keyboard.press("PageDown");
Copy link
Contributor

Choose a reason for hiding this comment

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

why was this removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test wasn't running with them in. The item that needed to be viewed was not visible after 3 pagedown clicks.

@mas-who
Copy link
Contributor

mas-who commented Jun 6, 2024

Looks good, thanks for doing this! 👍 Just one small clarification from my side.

@Kxiru Kxiru requested review from edlerd and mas-who June 6, 2024 13:01
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for improving the tests!

@Kxiru Kxiru merged commit 129c7a1 into canonical:main Jun 6, 2024
12 checks passed
github-actions bot pushed a commit that referenced this pull request Jun 6, 2024
@Kxiru Kxiru deleted the improve-tests-with-text-assertion branch June 17, 2024 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants