test: E2E test case for the creation of the podman machine with user mode enabled #8528

danivilla9 · 2024-08-21T06:53:15Z

What does this PR do?

This PR extends the E2E test suite by adding the test case of creating a rootfull/rootless Podman machine with user mode networking enabled

What issues does this PR fix or reference?

#4666

How to test this PR?

Run the e2e tests with yarn test:e2e

ScrewTSW

LGTM, just a comment on timeout

tests/playwright/src/utility/operations.ts

cbr7

Left some comments.

xbabalov · 2024-09-03T08:29:56Z

tests/playwright/src/specs/podman-machine-rootless.spec.ts

+  await pdRunner.close();
+});
+
+test.describe.serial('Rootless Podman machine Verification', () => {


Is there a need for this file in testing the user mode machine?

No, it's not needed. I updated it because it prompted an error when testing it on GH Actions. If you have a different version you can substitute it completely.

I'm still working on that file and it's not on containers main yet, just my branch where I'm testing it, so you don't have to update it in your PR.

xbabalov · 2024-09-03T08:33:08Z

tests/playwright/src/model/pages/create-machine-page.ts

+  async handleConnectionDialog(machineName: string, setAsDefault: boolean): Promise<void> {
+    const connectionDialog = this.page.getByRole('dialog', { name: 'Podman' });
+    const dialogMessage = connectionDialog.getByLabel('Dialog Message');
+    if ((await connectionDialog.isVisible()) && (await dialogMessage.isVisible())) {


I was checking the regex of dialog message so I could be sure the connection dialog was regarding specifically the newly created machine, but if we don't want to check that we don't need to check the visibility of the dialog message if we know the dialog is already there.

xbabalov · 2024-09-03T08:34:43Z

tests/playwright/src/utility/operations.ts

  } else {
    console.log(`Podman machine [${machineVisibleName}] not present, skipping deletion.`);
  }
 }

+export async function handleResetDefaultConnectionDialog(page: Page): Promise<void> {


This function is defined but never used

cbr7 · 2024-09-04T07:03:46Z

tests/playwright/src/model/pages/create-machine-page.ts

+    isRootful: boolean = true,
+    enableUserNet: boolean = false,
+    startNow: boolean = true,
+    setAsDefault: boolean = true,


Please refactor this to use object descructuring, these are too many optional params not to be descructured.

cbr7 · 2024-09-04T07:09:25Z

tests/playwright/src/model/pages/create-machine-page.ts

+  async isEnabled(checkbox: Locator): Promise<boolean> {
+    await playExpect(checkbox).toBeVisible();
+    const upperElement = checkbox.locator('..').locator('..');
+    const clickableCheckbox = upperElement.getByText('Enabled');
+    return await clickableCheckbox.isVisible();
+  }


why do you not just check if the checkbox isChecked()?

cbr7 · 2024-09-04T07:10:26Z

tests/playwright/src/model/pages/create-machine-page.ts

+  async switchCheckbox(checkbox: Locator): Promise<void> {
+    await playExpect(checkbox).toBeVisible();
+    const upperElement = checkbox.locator('..').locator('..');
+
+    const wasEnabled = await this.isEnabled(checkbox);
+    let checkText;
+    if (wasEnabled) {
+      checkText = 'Enabled';
+    } else {
+      checkText = 'Disabled';
+    }
+
+    const clickableCheckbox = upperElement.getByText(checkText);
+    await clickableCheckbox.click();
+  }


Same as above, the checkbox should be checkable for the isChecked() state.

cbr7 · 2024-09-04T07:13:32Z

tests/playwright/src/model/pages/create-machine-page.ts

+    if (await connectionDialog.isVisible()) {
+      let handleButtonName = 'Yes';
+      if (!setAsDefault) {
+        handleButtonName = 'Ignore';
+      }
+      const handleButton = connectionDialog.getByRole('button', { name: handleButtonName });
+      await handleButton.click();
+    }


This is not the right way to check this, the method isVisible() always returns immediately, it has no innate waiter, as such it can return false just as easily as true, simply because of system lag, this is very flaky behavior.

cbr7 · 2024-09-04T07:16:30Z

tests/playwright/src/model/pages/create-machine-page.ts

+    await this.page.waitForTimeout(2_000); //wait for dialog to appear, in the needed cases
+    await this.handleConnectionDialog(setAsDefault);


This is not the best approach, you should be waiting until the element that you need is available, I would say that in this case you should use await playExpect(setAsDefault).toBeVisible() instead of waitForTimeout(), which could very well introduce flakyness.

By waiting on the specific element it also allows the implementation of handleConnectionDialog() method to be refactored, as pointed out below.

cbr7 · 2024-09-04T07:22:10Z

tests/playwright/src/model/pages/create-machine-page.ts

+    if (isRootful !== (await this.isEnabled(this.rootPriviledgesCheckbox))) {
+      await this.switchCheckbox(this.rootPriviledgesCheckbox);
+    }
+
+    if (enableUserNet !== (await this.isEnabled(this.userModeNetworkingCheckbox))) {
+      await this.switchCheckbox(this.userModeNetworkingCheckbox);
+    }
+
+    if (startNow !== (await this.isEnabled(this.startNowCheckbox))) {
+      await this.switchCheckbox(this.startNowCheckbox);
+    }


These can be refactor in order for the if statements to not be required, there should be some method named ensureCheckboxState() that accepts the checkbox locator and the desired state of the checkbox which checks the initial state and switches the state in case it does not match the desired state.

cbr7 · 2024-09-04T07:23:50Z

tests/playwright/src/model/pages/create-machine-page.ts

@@ -0,0 +1,134 @@
+/**********************************************************************


File should be added to index.ts file to be exported.

cbr7 · 2024-09-04T07:25:14Z

tests/playwright/src/specs/podman-machine-rootless.spec.ts

+
+test.afterAll(async () => {
+  await deletePodmanMachine(page, MACHINE_VISIBLE_NAME);
+  await pdRunner.close();


the close() method call should always be in a finally block, in order to ensure that it's executed even if some exception is thrown in the deletePodmanMachine() method.

cbr7 · 2024-09-04T07:27:17Z

tests/playwright/src/specs/podman-machine-rootless.spec.ts

+    const connectionStatusLabel = await machineBox.resourceElementConnectionStatus.textContent();
+    playExpect(connectionStatusLabel === 'RUNNING').toBeTruthy();


Suggested change

const connectionStatusLabel = await machineBox.resourceElementConnectionStatus.textContent();

playExpect(connectionStatusLabel === 'RUNNING').toBeTruthy();

await playExpect(await machineBox.resourceElementConnectionStatus).toContain('RUNNING');

cbr7 · 2024-09-04T07:28:45Z

tests/playwright/src/specs/podman-machine-user-mode.spec.ts

+  pdRunner = new PodmanDesktopRunner();
+  page = await pdRunner.start();
+  pdRunner.setVideoAndTraceName('podman-machine-user-mode');
+  process.env.KEEP_TRACES_ON_PASS = 'true';


Why are we setting this env var here?

cbr7 · 2024-09-04T07:29:16Z

tests/playwright/src/specs/podman-machine-user-mode.spec.ts

+    const connectionStatusLabel = await machineBox.resourceElementConnectionStatus.textContent();
+    playExpect(connectionStatusLabel === 'RUNNING').toBeTruthy();


Suggested change

const connectionStatusLabel = await machineBox.resourceElementConnectionStatus.textContent();

playExpect(connectionStatusLabel === 'RUNNING').toBeTruthy();

await playExpect(await machineBox.resourceElementConnectionStatus).toContain('RUNNING');

cbr7 · 2024-09-04T07:29:38Z

tests/playwright/src/specs/podman-machine-user-mode.spec.ts

+    const connectionStatusLabel = await machineBox.resourceElementConnectionStatus.textContent();
+    playExpect(connectionStatusLabel === 'RUNNING').toBeTruthy();


Suggested change

const connectionStatusLabel = await machineBox.resourceElementConnectionStatus.textContent();

playExpect(connectionStatusLabel === 'RUNNING').toBeTruthy();

await playExpect(await machineBox.resourceElementConnectionStatus).toContain('RUNNING');

cbr7 · 2024-09-04T07:32:17Z

tests/playwright/src/utility/operations.ts

+export function checkForFailedTest(result: TaskResult, runner: PodmanDesktopRunner): void {
+  if (result.errors && result.errors.length > 0) runner.setTestPassed(false);
+}
+


What is this method used for?

cbr7 · 2024-09-04T07:33:13Z

tests/playwright/src/utility/operations.ts

@@ -137,7 +137,7 @@ export async function deletePod(page: Page, name: string): Promise<void> {
        async () => {
          return !!(await pods.getPodRowByName(name));
        },
-        { timeout: 20000 },
+        { timeout: 20_000 },


Are all these timeout refactorings required, what is the point of them?

cbr7

Left some comments in the PR.

Signed-off-by: Tamara Babalova <tbabalov@redhat.com>

Signed-off-by: xbabalov <t.babalova.17@gmail.com>

Signed-off-by: Daniel Villanueva <davillan@redhat.com>

ScrewTSW approved these changes Aug 21, 2024

View reviewed changes

tests/playwright/src/utility/operations.ts Outdated Show resolved Hide resolved

danivilla9 force-pushed the 4666-e2e-create-podman-machine-windows branch from 37cec22 to d3bc498 Compare August 29, 2024 14:55

cbr7 reviewed Aug 30, 2024

View reviewed changes

tests/playwright/src/utility/operations.ts Outdated Show resolved Hide resolved

cbr7 reviewed Aug 30, 2024

View reviewed changes

tests/playwright/src/utility/operations.ts Outdated Show resolved Hide resolved

cbr7 reviewed Aug 30, 2024

View reviewed changes

tests/playwright/src/utility/operations.ts Outdated Show resolved Hide resolved

cbr7 requested changes Aug 30, 2024

View reviewed changes

danivilla9 force-pushed the 4666-e2e-create-podman-machine-windows branch from d3bc498 to ea31515 Compare September 2, 2024 14:38

danivilla9 marked this pull request as ready for review September 3, 2024 07:15

danivilla9 requested review from benoitf and a team as code owners September 3, 2024 07:15

danivilla9 requested review from cdrage, feloy and deboer-tim and removed request for a team September 3, 2024 07:15

xbabalov reviewed Sep 3, 2024

View reviewed changes

danivilla9 requested a review from cbr7 September 4, 2024 06:54

cbr7 reviewed Sep 4, 2024

View reviewed changes

cbr7 requested changes Sep 4, 2024

View reviewed changes

xbabalov and others added 18 commits September 5, 2024 12:10

chore(tests): rootless machine creation test added

ec151f6

Signed-off-by: Tamara Babalova <tbabalov@redhat.com>

chore(tests): creating rootless machine e2e test fixed

b82ecd9

Signed-off-by: Tamara Babalova <tbabalov@redhat.com>

chore(tests): create rootless fixed

7040be1

Signed-off-by: Tamara Babalova <tbabalov@redhat.com>

chore(tests): addressing changes 2

26dc76c

Signed-off-by: Tamara Babalova <tbabalov@redhat.com>

chore(tests): added connection dialog handling

cb5b31b

Signed-off-by: Tamara Babalova <tbabalov@redhat.com>

chore(tests): creating rootless machine e2e test fixed

fc49d55

Signed-off-by: Tamara Babalova <tbabalov@redhat.com>

chore(tests): create rootless fixed

bdd330a

Signed-off-by: Tamara Babalova <tbabalov@redhat.com>

chore(tests): solving conflicts

1e83b2d

Signed-off-by: Tamara Babalova <tbabalov@redhat.com>

chore(tests): added reset default connection after machine delete handle

9a5b4cf

Signed-off-by: xbabalov <t.babalova.17@gmail.com>

chore(tests): number refactoring

1c3ecc6

Signed-off-by: xbabalov <t.babalova.17@gmail.com>

test: draft podman machine creation user mode enabled

77102eb

Signed-off-by: Daniel Villanueva <davillan@redhat.com>

test: fixed errata machine name

2bd4978

Signed-off-by: Daniel Villanueva <davillan@redhat.com>

test: fixed test case and tweaked timeouts

d34d530

Signed-off-by: Daniel Villanueva <davillan@redhat.com>

test: migration to playwright

07410a3

Signed-off-by: Daniel Villanueva <davillan@redhat.com>

test: migrate podman-machine-rootless.spec.ts to playwright

31614d7

Signed-off-by: Daniel Villanueva <davillan@redhat.com>

test: fixes from @xbabalov's feedback

3ef97ef

Signed-off-by: Daniel Villanueva <davillan@redhat.com>

test: more fixes from testing

f313b5f

Signed-off-by: Daniel Villanueva <davillan@redhat.com>

test: refactor to use fixtures and other improvements

afd4492

Signed-off-by: Daniel Villanueva <davillan@redhat.com>

danivilla9 force-pushed the 4666-e2e-create-podman-machine-windows branch from d5717a1 to afd4492 Compare September 5, 2024 11:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: E2E test case for the creation of the podman machine with user mode enabled #8528

test: E2E test case for the creation of the podman machine with user mode enabled #8528

danivilla9 commented Aug 21, 2024

ScrewTSW left a comment

cbr7 left a comment

xbabalov Sep 3, 2024

danivilla9 Sep 3, 2024

xbabalov Sep 3, 2024

xbabalov Sep 3, 2024 •

edited

Loading

xbabalov Sep 3, 2024

cbr7 Sep 4, 2024

cbr7 Sep 4, 2024

cbr7 Sep 4, 2024

cbr7 Sep 4, 2024

cbr7 Sep 4, 2024

cbr7 Sep 4, 2024

cbr7 Sep 4, 2024

cbr7 Sep 4, 2024

cbr7 Sep 4, 2024

cbr7 Sep 4, 2024

cbr7 Sep 4, 2024

cbr7 Sep 4, 2024

cbr7 Sep 4, 2024

cbr7 Sep 4, 2024

cbr7 left a comment

		await this.page.waitForTimeout(2_000); //wait for dialog to appear, in the needed cases
		await this.handleConnectionDialog(setAsDefault);

		@@ -0,0 +1,134 @@
		/**********************************************************************

		const connectionStatusLabel = await machineBox.resourceElementConnectionStatus.textContent();
		playExpect(connectionStatusLabel === 'RUNNING').toBeTruthy();

	const connectionStatusLabel = await machineBox.resourceElementConnectionStatus.textContent();
	playExpect(connectionStatusLabel === 'RUNNING').toBeTruthy();
	await playExpect(await machineBox.resourceElementConnectionStatus).toContain('RUNNING');

test: E2E test case for the creation of the podman machine with user mode enabled #8528

Are you sure you want to change the base?

test: E2E test case for the creation of the podman machine with user mode enabled #8528

Conversation

danivilla9 commented Aug 21, 2024

What does this PR do?

What issues does this PR fix or reference?

How to test this PR?

ScrewTSW left a comment

Choose a reason for hiding this comment

cbr7 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

xbabalov Sep 3, 2024 • edited Loading

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

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

cbr7 left a comment

Choose a reason for hiding this comment

xbabalov Sep 3, 2024 •

edited

Loading