try and make tests more stable - #50995

Pull request overview

Improves Admin UI Playwright test isolation, selectors, readiness checks, environment configuration, and feature handling.

Changes:

• Isolates group and SAML test data.
• Hardens table, alert, switch, and drag-and-drop interactions.
• Adds configurable server credentials and OID4VCI gating.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 10 comments.

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.

  const exactButton = toolbar.getByRole("button", {
    name: itemName,
    exact: true,
  });
  if ((await exactButton.count()) > 0) {

export const SERVER_URL = normalizeServerUrl(
  process.env.KEYCLOAK_SERVER_URL ?? DEFAULT_SERVER_URL,
);

  try {
    await switchOff(page, "#attributes\\.saml🍺force🍺post🍺binding");
  } catch {
    // Some generated SAML clients expose this setting as read-only.
  }

    test.skip(
      !moved,
      "Drag-and-drop reorder could not be triggered deterministically in this runtime.",
    );

Thanks for digging into this — a lot of it is clearly right, and a couple of the fixes address real bugs rather than just noise.

Worth keeping as-is

• Dropping waitForLoadState("domcontentloaded") from the OID4VCI specs. That call does nothing for SPA state transitions; replacing it with real visibility/toContainText assertions is exactly right.
• The SAML client fix in clients/saml.spec.ts. The suite created saml-settings-${uuid()} but then clicked clickTableRowItem(page, "saml") — substring-matched with .first(), so it clicked whatever row happened to sort first. That's a genuine flake source, now fixed.
• getRowByCellText returning an actual row locator. I checked all five call sites (clients/main.spec.ts:119, clients/authorization.ts:26, autentication/flows.spec.ts:51-52) — none break.
• assertRowExists(..., false) moving to toHaveCount(0). toBeHidden() on a zero-match locator was passing for the wrong reason.
• The per-realm test-bed isolation in groups/list.spec.ts and the realm parameters on AdminClient.
• getTableData dropping elementHandles() — removes the handle leak and the stale-handle race.

Main concern

Several changes buy stability by removing the suite's ability to fail. A few examples, detailed inline:

1. switchOff silently lost its toBeChecked() precondition, so it now no-ops instead of failing when a default flips.
2. assertNotificationMessage went from toHaveText to toContainText, which weakens roughly every assertion in the suite. The toBeVisible gate you added is the right fix for the race on its own.
3. The OID4VC_VCI test.skip() means ~15 tests go silently green if CI ever stops enabling the feature.
4. clickClientSignature became a blind toggle, which breaks the modal-title assertions on retry.

Fallback ladders

dragExecutionAboveExecution, clickAddClientScope, clickTableRowItem, clickTableToolbarItem and setSwitchState all follow a "try 4-6 strategies, swallow every exception" shape. Three problems: the retries are not idempotent (strategy 1 partially succeeding means strategies 2-5 apply more mutations), diagnostics get worse rather than better (a /^Add$/ fallback will happily click the wrong button and fail three steps later), and several of them use page.waitForTimeout, which eslint-plugin-playwright's recommended config flags — it's applied to apps/admin-ui/test/** in js/eslint.config.js:142-143.

If the real cause is something concrete like "dnd-kit doesn't fire drag events in Firefox CI", an explicit test.skip(browserName === "firefox") with a linked issue is more honest and much easier to retire later than five speculative strategies.

One request

Could you list which tests were actually flaking, and how often? Right now it's hard to tell which of these changes are load-bearing and which are speculative — and the speculative ones are the ones costing coverage. That'd also make it much easier to review the fallback ladders on their merits.

No security concerns; test-only changes, and making the admin credentials env-overridable is a small improvement. One perf note inline on getTableData.

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated no new comments.

  if ((await candidate.count()) === 0) {
    return false;
  }

const TABLE_LOAD_TIMEOUT_MS = 5_000;

Pull request overview

Copilot reviewed 27 out of 27 changed files in this pull request and generated no new comments.

  if ((await createScopeButton.count()) > 0) {
    await createScopeButton.first().click();

    if (await waitForSwitchState(switchElement, checked)) {
      return;
    }

    await clickSwitchElement(switchElement);

export const SERVER_URL = normalizeServerUrl(
  process.env.KEYCLOAK_SERVER_URL ?? DEFAULT_SERVER_URL,
);

Pull request overview

Copilot reviewed 29 out of 29 changed files in this pull request and generated no new comments.

    await adminClient.deleteUser("testuser8@gmail.com", realmName, true);

  if ((await candidate.count()) === 0) {
    return false;

- `KEYCLOAK_REQUIRE_OID4VCI=true` to fail OID4VCI tests when the server feature is missing instead of skipping them

Pull request overview

Copilot reviewed 29 out of 29 changed files in this pull request and generated no new comments.

  if ((await candidate.count()) === 0) {
    return false;

  const usernameField = page.getByTestId("username");
  if (await usernameField.isVisible()) {
    await usernameField.fill(username);

  await expect(page).toHaveURL(/.*mappers(\/[^/]+)?$/);

Solid, well-motivated stability work for the admin-ui Playwright suite. No product code is touched, so risk is limited to the test suite's own reliability. Several changes fix genuine bugs: clickTableToolbarItem's kebab lookup used to search the whole page (could hit the wrong kebab) and is now scoped to the specific toolbar; getRowByCellText used to match text anywhere on the page and is now scoped to table tbody tr; the new escapeRegex() in clickTableRowItem prevents item names with regex metacharacters from building a broken RegExp; AdminClient.deleteClient re-authenticates before deleting; and moving groups/saml tests onto per-test createTestBed()/per-test client creation removes real cross-test state coupling.

A few things worth a look before merge (left as inline comments): the drag-and-drop helper's success check only verifies relative row order rather than that a move actually happened, and its return value isn't asserted at the call site; one of the two touched saml.ts files bypasses the new clickSwitch helper it's otherwise standardizing on; and the multi-strategy switch-toggle retry logic swallows failures silently, which will make future flakiness harder to diagnose. None of these block the overall direction — just flagging for consideration.

Pull request overview

Copilot reviewed 29 out of 29 changed files in this pull request and generated no new comments.

  if ((await candidate.count()) === 0) {
    return false;

  await expect(page).toHaveURL(/.*mappers(\/[^/]+)?$/);

  await switchOff(page, providerEnabledSwitch);

Re-reviewed the current head. Good news: all 4 issues from my earlier review are genuinely fixed (verified against code, not just commit messages) — the drag helper now has a real precondition + move-detection, the return value is asserted, identity-providers/saml.ts uses the shared switch helpers, and the switch-toggle fallbacks now log which strategy fired. Most of the other review passes on this PR (an earlier round from me, plus the automated Copilot review) are also fixed: the switchOff/ensureSwitchOff split, the masthead.ts assertion strictness, the per-test SAML client isolation, groups/list.spec.ts cleanup, table.ts timeouts/locators, SERVER_URL now actually being used everywhere, the OID4VCI constant de-duplication, and the PR-specific flaky-matrix workflow being removed.

Four concrete issues from those same review passes are still open in the current code — left as inline comments below. None of these were touched by either "PR review" fixup commit.