Test: improve PW tests flakiness #452

dominikvagner · 2025-02-24T10:11:20Z

Summary

This PR changes/fixes a few things to combat test flakiness.

Changes a step in the PW actions from processing all repos to only snapshot the RH one that is used for tests. (as in HMS-5162: replace fedorapeople comps repos with github fixtures content-sources-backend#985)
Increases the timeout for the action that waits on the introspection.
Introduces 2 new helpers, one for retrying navigation (which sometimes just fails/times-out, because of console-dot) and another one which waits for a specific task to be picked up.
Also a custom ESLint rule was added that will throw linting errors when any local import is made inside of a PW test and imports anything outside the PW test folder. (This will be helpful for preventing failing of our integration PW tests)

marusak · 2025-02-27T10:03:09Z

_playwright-tests/UI/CustomRepo.spec.ts

@@ -1,15 +1,15 @@
 import { test, expect, type Page } from '@playwright/test';
 import { navigateToRepositories } from './helpers/navHelpers';
 import { deleteAllRepos } from './helpers/deleteRepositories';
-import { closePopupsIfExist } from './helpers/helpers';
+import { closePopupsIfExist, retry } from './helpers/helpers';


Although controversial, I agree with this :) I know that consoleDot sometimes gets stuck and I need to reload for it to load. But should we just not move this into the navigation function rather than introducing this to every single test? Would be cleaner and we would not forget to introduce it in other tests

forgetting this can be an issue, but I didn't want to force it into existing helpers and I think it's mostly wanted only on the inital page load (also had some trouble trying this while prototyping) so I just added a new one, but it could be changed 😅
@Andrewgdewar what's your opinion, retry included in nav helpers or it needs to be explicitly used when desired?

I think moving it to the navigation functions makes sense, and keeping the helper around in case we create other navigation based things!

marusak · 2025-02-27T10:08:40Z

_playwright-tests/UI/helpers/helpers.ts

+  expect(uuidList.length).toEqual(1);
+
+  const repoUuid = uuidList[0];
+  while (true) {


I am bit torn on this one. It might be necessary for stage/prod tests as we are not in control of queue or for parallel tests, but I feel that this might hide actual issues or make tests unpredictable. The golden rule of tests - if they pass, none cares about them would apply here, as if they pass but always wait for very long time, we would not notice it.

I am also a bit torn on this as it's sort of a foot gun for the reason you mentioned, but on the other hand the queue being bogged down on the runner isn't supposed to be the thing this test is verifying so I included this for that reason. 😅
but I am open to this being axed from this test (or the entire helper) @Andrewgdewar what ya think? 👀

_playwright-tests/UI/RedHatRepo.spec.ts

_playwright-tests/UI/helpers/helpers.ts

marusak · 2025-02-27T10:13:51Z

Nice, thanks! I have some comments, but in general I agree and like those changes. And nice work on commits ;) Was very easy to review commit by commit!
Have you tried running action on this to see if it fixes that one stubborn test?

dominikvagner · 2025-02-27T12:53:05Z

Nice, thanks! I have some comments, but in general I agree and like those changes. And nice work on commits ;) Was very easy to review commit by commit! Have you tried running action on this to see if it fixes that one stubborn test?

thanks 🎉😇
that one won't pass until the snapshot command will also introspect, PR is open for that 😅

This adds a `retry` helper and uses it for retrying all current navigations, which will be useful for mitigating flakiness during navigation in playwright tests that caused by consoledot slowness.

This adds a helper which is gonna indefinitely wait for a specific task to be picked up, before continuing. This will be useful for fixing the failures of tests that are waiting for a valid status of a repo, but can be broken by other tasks hogging up the task queue.

This moves a explicit timeout to the correct spot, we have a generous default test timeout, because as we should be setting explicit timeouts in the smallest units (i.e. actions, expects etc.).

This changes the closePopupsIfExist helper to not throw errors if multiple handlers try to close the same pop-up, it currently throws when a handler tries to close a pop-up has been already closed by a prior handler.

This fixes our `format` and `lint` npm commands to also check the PW test folder. Auto format and lint fix currently present violations.

This adds a new custom ESLint rule that will throw linting errors when any file inside the `_playwright-tests` folder will import any local code that is outside the PW test folder. This will be helpful for preventing breakages in our integration tests which pickup tests for that folder but not the other suff in the repo.

Andrewgdewar · 2025-03-05T17:38:01Z

_playwright-tests/UI/helpers/helpers.ts

+  expect(uuidList.length).toEqual(1);
+
+  const repoUuid = uuidList[0];
+  while (true) {


If I am reading this correctly we could wait forever.... also the timeout on the request below is excessive, I'd expect that to return in 5s or so?
Why would an api for tasks call take 60 seconds...

I like the idea of what you are doing here but lets have this while loop be based off off of a retry count, maybe configurable via passed in variable with a reasonable number set by default. maybe check this 10 times at maximum for example.

Andrewgdewar · 2025-03-05T17:41:45Z

_playwright-tests/UI/helpers/helpers.ts

+    await sleep(5000);
+  }
+
+  return;


unneeded return

Andrewgdewar · 2025-03-05T17:42:19Z

_playwright-tests/UI/helpers/helpers.ts

+    }
+  }
+
+  return;


this is the same as not having a return, I guess some prefer explicit returns. As long as you are aware 🤷

dominikvagner force-pushed the dvagner/pw-improve-flakiness branch from e12dd1c to 4d02a53 Compare February 26, 2025 17:05

dominikvagner requested a review from marusak February 26, 2025 17:06

marusak reviewed Feb 27, 2025

View reviewed changes

_playwright-tests/UI/RedHatRepo.spec.ts Outdated Show resolved Hide resolved

marusak reviewed Feb 27, 2025

View reviewed changes

_playwright-tests/UI/helpers/helpers.ts Show resolved Hide resolved

dominikvagner force-pushed the dvagner/pw-improve-flakiness branch 2 times, most recently from 3740056 to d7a8471 Compare March 3, 2025 11:07

dominikvagner added 6 commits March 3, 2025 12:07

fix: add retry helper, use it for navigation

25b414c

This adds a `retry` helper and uses it for retrying all current navigations, which will be useful for mitigating flakiness during navigation in playwright tests that caused by consoledot slowness.

fix: move a timeout to action from test

62758e4

This moves a explicit timeout to the correct spot, we have a generous default test timeout, because as we should be setting explicit timeouts in the smallest units (i.e. actions, expects etc.).

fix: make closePopupsIfExist not throwing

3b3f816

This changes the closePopupsIfExist helper to not throw errors if multiple handlers try to close the same pop-up, it currently throws when a handler tries to close a pop-up has been already closed by a prior handler.

fix+style: include PW tests in format+lint checks

3f5e757

This fixes our `format` and `lint` npm commands to also check the PW test folder. Auto format and lint fix currently present violations.

dominikvagner force-pushed the dvagner/pw-improve-flakiness branch from d7a8471 to fba8504 Compare March 3, 2025 11:07

Andrewgdewar reviewed Mar 5, 2025

View reviewed changes

_playwright-tests/UI/helpers/helpers.ts

await sleep(5000);

}

return;

Copy link

Member

Andrewgdewar Mar 5, 2025

Choose a reason for hiding this comment

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

unneeded return

Andrewgdewar reviewed Mar 5, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test: improve PW tests flakiness #452

Test: improve PW tests flakiness #452

dominikvagner commented Feb 24, 2025

marusak Feb 27, 2025

dominikvagner Feb 27, 2025 •

edited

Loading

Andrewgdewar Mar 5, 2025

marusak Feb 27, 2025

dominikvagner Feb 27, 2025

marusak commented Feb 27, 2025

dominikvagner commented Feb 27, 2025

Andrewgdewar Mar 5, 2025

Andrewgdewar Mar 5, 2025

Andrewgdewar Mar 5, 2025 •

edited

Loading

+                  }
+                }
+                return;

Test: improve PW tests flakiness #452

Are you sure you want to change the base?

Test: improve PW tests flakiness #452

Conversation

dominikvagner commented Feb 24, 2025

Summary

marusak Feb 27, 2025

Choose a reason for hiding this comment

dominikvagner Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

Andrewgdewar Mar 5, 2025

Choose a reason for hiding this comment

marusak Feb 27, 2025

Choose a reason for hiding this comment

dominikvagner Feb 27, 2025

Choose a reason for hiding this comment

marusak commented Feb 27, 2025

dominikvagner commented Feb 27, 2025

Andrewgdewar Mar 5, 2025

Choose a reason for hiding this comment

Andrewgdewar Mar 5, 2025

Choose a reason for hiding this comment

Andrewgdewar Mar 5, 2025 • edited Loading

Choose a reason for hiding this comment

dominikvagner Feb 27, 2025 •

edited

Loading

Andrewgdewar Mar 5, 2025 •

edited

Loading