UI visual regression testing to cover UI widgets visibility #674

Ygnas · 2024-09-18T14:59:18Z

Issue link

https://issues.redhat.com/browse/RHOAIENG-12525

Depends on:

ipywidgets UI create delete #668

What changes have been made

Added I visual regression testing to cover UI widgets visibility

Verification steps

Passing tests.
At the moment I have made a separate workflow but I think it would be best to integrate them into the guided notebook tests, as my workflow is almost identical to that one.

Checks

I've made sure the tests are passing.
Testing Strategy
- Unit tests
- Manual tests
- Testing is not required for this change

.github/workflows/ui_notebooks_test.yaml

KPostOffice · 2024-09-19T13:13:41Z

.github/workflows/ui_notebooks_test.yaml

+        uses: actions/setup-python@v5
+        with:
+          python-version: "3.9"
+          cache: "pip" # caching pip dependencies


Is there a reason to use pip over poetry here?

Not sure about this one as that's how it was done in the guided notebook tests workflow

.github/workflows/ui_notebooks_test.yaml

ui-tests/tests/0_basic_ray.test.ts

KPostOffice

I have a few comments on the test

demo-notebooks/guided-demos/widget_notebook_example.ipynb

ui-tests/tests/widget_notebook_example.test.ts

Bobbins228

Just a nit on the notebook name and verification on cluster deletion.

demo-notebooks/guided-demos/widget_notebook_example.ipynb

.github/workflows/ui_notebooks_test.yaml

demo-notebooks/guided-demos/widget_notebook_example.ipynb

ui-tests/tests/widget_notebook_example.test.ts

openshift-ci · 2024-09-20T12:15:13Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from bobbins228. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

KPostOffice · 2024-09-20T16:39:41Z

ui-tests/tests/widget_notebook_example.test.ts

+      const successMessage = await page.waitForSelector('text=Ray Cluster: \'raytest\' has successfully been created', { timeout: 10000 });
+      expect(successMessage).not.toBeNull();
+
+      const resourcesMessage = await page.waitForSelector('text=Waiting for requested resources to be set up...');
+      expect(resourcesMessage).not.toBeNull();
+
+      const upAndRunningMessage = await page.waitForSelector('text=Requested cluster is up and running!');
+      expect(upAndRunningMessage).not.toBeNull();
+
+      const dashboardReadyMessage = await page.waitForSelector('text=Dashboard is ready!');


Should we use timeouts for all of these just incase the Dashboard doesn't come up (or someone changes the text but forgets to change the test)?

It will still wait for the timeout of the set time out

Yeah, that's fine. I just mean so that if the changes are pushed the test doesn't hang indefinitely waiting for the condition

But if it wait's for the cluster to be ready and it can take sometime or even fail, what is the timeout for that? The cluster is ready at different time every single time

KPostOffice

2 small comments, other than that, I think this is g2g

KPostOffice · 2024-09-20T17:25:43Z

ui-tests/tests/widget_notebook_example.test.ts

+  }
+}
+
+async function runLastCell(page, cellCount, expectedMessage) {


Suggested change

async function runLastCell(page, cellCount, expectedMessage) {

async function runPreviousCell(page, cellCount, expectedMessage) {

Small nit

KPostOffice · 2024-09-20T17:26:11Z

ui-tests/tests/widget_notebook_example.test.ts

+import { expect } from "@playwright/test";
+import * as path from "path";
+
+test.setTimeout(460000);


add to playwright config as discussed

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 18, 2024

Ygnas force-pushed the RHOAIENG-12525 branch from 1e9e7d8 to 1ad654b Compare September 18, 2024 15:07

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 18, 2024

Ygnas force-pushed the RHOAIENG-12525 branch from 1ad654b to 875268d Compare September 18, 2024 15:09

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 18, 2024

Ygnas force-pushed the RHOAIENG-12525 branch from 875268d to 2128799 Compare September 18, 2024 15:14

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 18, 2024

Ygnas force-pushed the RHOAIENG-12525 branch from 2128799 to 5d17d5e Compare September 18, 2024 15:16

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 18, 2024

Ygnas force-pushed the RHOAIENG-12525 branch 7 times, most recently from 942fb9f to 89ac3c3 Compare September 19, 2024 08:22

Ygnas marked this pull request as ready for review September 19, 2024 08:36

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 19, 2024

openshift-ci bot requested review from ChristianZaccaria and MichaelClifford September 19, 2024 08:36

Ygnas requested review from KPostOffice and Bobbins228 and removed request for MichaelClifford September 19, 2024 08:37

KPostOffice reviewed Sep 19, 2024

View reviewed changes

Ygnas force-pushed the RHOAIENG-12525 branch 6 times, most recently from baf4658 to cee54a1 Compare September 19, 2024 15:58

Ygnas force-pushed the RHOAIENG-12525 branch 2 times, most recently from f62e61a to ad221de Compare September 19, 2024 16:43

KPostOffice reviewed Sep 19, 2024

View reviewed changes

demo-notebooks/guided-demos/widget_notebook_example.ipynb Outdated Show resolved Hide resolved

ui-tests/tests/widget_notebook_example.test.ts Show resolved Hide resolved

ui-tests/tests/widget_notebook_example.test.ts Show resolved Hide resolved

Ygnas force-pushed the RHOAIENG-12525 branch 3 times, most recently from be97842 to c0e616f Compare September 20, 2024 10:50

Ygnas requested a review from KPostOffice September 20, 2024 11:00

Bobbins228 requested changes Sep 20, 2024

View reviewed changes

openshift-ci bot assigned Bobbins228 Sep 20, 2024

Ygnas force-pushed the RHOAIENG-12525 branch from c0e616f to 10f4d97 Compare September 20, 2024 12:15

Ygnas force-pushed the RHOAIENG-12525 branch from 10f4d97 to d1ae990 Compare September 20, 2024 13:41

Ygnas added the test-ui-notebooks Run PR check to verify UI notebooks label Sep 20, 2024

Ygnas force-pushed the RHOAIENG-12525 branch from d1ae990 to bde3957 Compare September 20, 2024 14:05

Ygnas added 2 commits September 20, 2024 15:08

UI visual regression testing to cover UI widgets visibility

9d5df26

Test to cover the widgets functionality

3e07369

Ygnas force-pushed the RHOAIENG-12525 branch from bde3957 to 3e07369 Compare September 20, 2024 14:08

Ygnas added test-ui-notebooks Run PR check to verify UI notebooks and removed test-ui-notebooks Run PR check to verify UI notebooks labels Sep 20, 2024

KPostOffice reviewed Sep 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI visual regression testing to cover UI widgets visibility #674

UI visual regression testing to cover UI widgets visibility #674

Ygnas commented Sep 18, 2024 •

edited

Loading

KPostOffice Sep 19, 2024

Ygnas Sep 19, 2024

KPostOffice left a comment

Bobbins228 left a comment

openshift-ci bot commented Sep 20, 2024

KPostOffice Sep 20, 2024

Ygnas Sep 20, 2024

KPostOffice Sep 20, 2024

Ygnas Sep 20, 2024

KPostOffice left a comment

KPostOffice Sep 20, 2024

KPostOffice Sep 20, 2024

	async function runLastCell(page, cellCount, expectedMessage) {
	async function runPreviousCell(page, cellCount, expectedMessage) {

UI visual regression testing to cover UI widgets visibility #674

Are you sure you want to change the base?

UI visual regression testing to cover UI widgets visibility #674

Conversation

Ygnas commented Sep 18, 2024 • edited Loading

Issue link

Depends on:

What changes have been made

Verification steps

Checks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KPostOffice left a comment

Choose a reason for hiding this comment

Bobbins228 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Sep 20, 2024

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

KPostOffice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ygnas commented Sep 18, 2024 •

edited

Loading