Skip to content

Conversation

@PatAKnight
Copy link
Member

Description

Re-enables and hopefully fixes the failing RBAC tests that we were seeing

Which issue(s) does this PR fix

PR acceptance criteria

Please make sure that the following steps are complete:

  • GitHub Actions are completed and successful
  • Unit Tests are updated and passing
  • E2E Tests are updated and passing
  • Documentation is updated if necessary (requirement for new features)
  • Add a screenshot if the change is UX/UI related

How to test changes / Special notes to the reviewer

@openshift-ci
Copy link

openshift-ci bot commented Jan 20, 2026

[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 assign josephca for approval. For more information see the Code Review Process.

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

Details Needs approval from an approver in each of these files:

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

@rhdh-qodo-merge
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🔒 No security concerns identified
⚡ Recommended focus areas for review

Test Flakiness

Two RBAC tests were re-enabled (previously skipped). Validate they are now stable across CI environments (timing, eventual consistency, and async UI updates) and that they don’t intermittently fail due to reliance on UI state, ordering, or background RBAC propagation delays.

test("Create and edit a role from the roles list page", async ({
  page,
}) => {
  const uiHelper = new UIhelper(page);

  await uiHelper.clickButton("Create");
  await uiHelper.verifyHeading("Create role");
  await uiHelper.fillTextInputByLabel("name", "sample-role-1");
  await uiHelper.fillTextInputByLabel(
    "description",
    "Test Description data",
  );

  await uiHelper.clickButton("Next");
  // Wait for the users and groups step to be visible
  await expect(
    page.getByTestId("users-and-groups-text-field"),
  ).toBeVisible();
  await uiHelper.fillTextInputByLabel(
    "Select users and groups",
    "sample-role-1",
  );
  await page
    .getByTestId("users-and-groups-text-field")
    .getByLabel("clear search")
    .click();
  await expect(
    page.getByTestId("users-and-groups-text-field").getByRole("combobox"),
  ).toBeEmpty();
  await uiHelper.verifyHeading("No users and groups selected");
  await uiHelper.clickButton("Cancel");
  await uiHelper.verifyText("Exit role creation?");
  await uiHelper.clickButton("Discard");
  await expect(page.getByRole("alert")).toHaveCount(0);

  const rbacPo = new RbacPo(page);
  const testUser = "Jonathon Page";
  await rbacPo.createRole(
    "test-role",
    [RbacPo.rbacTestUsers.guest, RbacPo.rbacTestUsers.tara],
    [RbacPo.rbacTestUsers.backstage],
    [{ permission: "catalog.entity.delete" }],
  );
  await page.click(
    ROLES_PAGE_COMPONENTS.editRole("role:default/test-role"),
  );
  await uiHelper.verifyHeading("Edit Role");
  await uiHelper.clickButton("Next");
  // Wait for users and groups step to be ready
  await expect(page.getByLabel("Select users and groups")).toBeVisible();
  await rbacPo.addUsersAndGroups(testUser);
  await page.click(rbacPo.selectMember(testUser));
  await uiHelper.verifyHeading(rbacPo.regexpShortUsersAndGroups(3, 1));
  await uiHelper.clickButton("Next");
  // Wait for permissions step to be ready (use .first() to handle multiple Next buttons)
  const nextButton = page.getByTestId("nextButton-2").first();
  await expect(nextButton).toBeVisible();
  await expect(nextButton).toBeEnabled();
  await nextButton.click();
  // Wait for review step to be ready
  const saveButton = page.getByRole("button", { name: "Save" });
  await expect(saveButton).toBeVisible();
  await expect(saveButton).toBeEnabled();
  await saveButton.click();
  await uiHelper.verifyText(
    "Role role:default/test-role updated successfully",
  );

  await page.getByPlaceholder("Filter").waitFor({
    state: "visible",
  });
  await page.getByPlaceholder("Filter").fill("test-role");
  await uiHelper.verifyHeading("All roles (1)");
  // Use semantic selector for table cell
  const usersAndGroupsLocator = page
    .getByRole("cell")
    .filter({ hasText: rbacPo.regexpShortUsersAndGroups(3, 1) });
  await expect(usersAndGroupsLocator).toBeVisible();

  await rbacPo.deleteRole("role:default/test-role");
});

//FIXME https://issues.redhat.com/browse/RHDHBUGS-2483
test("Edit users and groups and update policies of a role from the overview page", async ({
  page,
}) => {
  const uiHelper = new UIhelper(page);
  const rbacPo = new RbacPo(page);
Test Isolation

Confirm the re-enabled tests reliably clean up any created roles/users/groups and are idempotent when re-run (including after partial failures). Ensure no shared naming/state causes cross-test interference when the suite runs in parallel or retries.

test("Create and edit a role from the roles list page", async ({
  page,
}) => {
  const uiHelper = new UIhelper(page);

  await uiHelper.clickButton("Create");
  await uiHelper.verifyHeading("Create role");
  await uiHelper.fillTextInputByLabel("name", "sample-role-1");
  await uiHelper.fillTextInputByLabel(
    "description",
    "Test Description data",
  );

  await uiHelper.clickButton("Next");
  // Wait for the users and groups step to be visible
  await expect(
    page.getByTestId("users-and-groups-text-field"),
  ).toBeVisible();
  await uiHelper.fillTextInputByLabel(
    "Select users and groups",
    "sample-role-1",
  );
  await page
    .getByTestId("users-and-groups-text-field")
    .getByLabel("clear search")
    .click();
  await expect(
    page.getByTestId("users-and-groups-text-field").getByRole("combobox"),
  ).toBeEmpty();
  await uiHelper.verifyHeading("No users and groups selected");
  await uiHelper.clickButton("Cancel");
  await uiHelper.verifyText("Exit role creation?");
  await uiHelper.clickButton("Discard");
  await expect(page.getByRole("alert")).toHaveCount(0);

  const rbacPo = new RbacPo(page);
  const testUser = "Jonathon Page";
  await rbacPo.createRole(
    "test-role",
    [RbacPo.rbacTestUsers.guest, RbacPo.rbacTestUsers.tara],
    [RbacPo.rbacTestUsers.backstage],
    [{ permission: "catalog.entity.delete" }],
  );
  await page.click(
    ROLES_PAGE_COMPONENTS.editRole("role:default/test-role"),
  );
  await uiHelper.verifyHeading("Edit Role");
  await uiHelper.clickButton("Next");
  // Wait for users and groups step to be ready
  await expect(page.getByLabel("Select users and groups")).toBeVisible();
  await rbacPo.addUsersAndGroups(testUser);
  await page.click(rbacPo.selectMember(testUser));
  await uiHelper.verifyHeading(rbacPo.regexpShortUsersAndGroups(3, 1));
  await uiHelper.clickButton("Next");
  // Wait for permissions step to be ready (use .first() to handle multiple Next buttons)
  const nextButton = page.getByTestId("nextButton-2").first();
  await expect(nextButton).toBeVisible();
  await expect(nextButton).toBeEnabled();
  await nextButton.click();
  // Wait for review step to be ready
  const saveButton = page.getByRole("button", { name: "Save" });
  await expect(saveButton).toBeVisible();
  await expect(saveButton).toBeEnabled();
  await saveButton.click();
  await uiHelper.verifyText(
    "Role role:default/test-role updated successfully",
  );

  await page.getByPlaceholder("Filter").waitFor({
    state: "visible",
  });
  await page.getByPlaceholder("Filter").fill("test-role");
  await uiHelper.verifyHeading("All roles (1)");
  // Use semantic selector for table cell
  const usersAndGroupsLocator = page
    .getByRole("cell")
    .filter({ hasText: rbacPo.regexpShortUsersAndGroups(3, 1) });
  await expect(usersAndGroupsLocator).toBeVisible();

  await rbacPo.deleteRole("role:default/test-role");
});

//FIXME https://issues.redhat.com/browse/RHDHBUGS-2483
test("Edit users and groups and update policies of a role from the overview page", async ({
  page,
}) => {
  const uiHelper = new UIhelper(page);
  const rbacPo = new RbacPo(page);
📄 References
  1. No matching references available

@rhdh-qodo-merge
Copy link

rhdh-qodo-merge bot commented Jan 20, 2026

PR Type

Tests


Description

  • Re-enable two previously skipped RBAC e2e tests

  • Remove test.skip() calls from role creation and editing tests

  • Tests address RHDHBUGS-2483 issue resolution


File Walkthrough

Relevant files
Tests
rbac.spec.ts
Re-enable two skipped RBAC role management tests                 

e2e-tests/playwright/e2e/plugins/rbac/rbac.spec.ts

  • Changed test.skip() to test() for "Create and edit a role from the
    roles list page" test
  • Changed test.skip() to test() for "Edit users and groups and update
    policies of a role from the overview page" test
  • Removed FIXME comments indicating tests were previously disabled due
    to [RHDHBUGS-2483](https://issues.redhat.com//browse/RHDHBUGS-2483)
+2/-2     

@rhdh-qodo-merge
Copy link

rhdh-qodo-merge bot commented Jan 20, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Generate unique role names

Use a dynamically generated unique role name instead of the hardcoded
"sample-role-1" to prevent test collisions.

e2e-tests/playwright/e2e/plugins/rbac/rbac.spec.ts [331]

-await uiHelper.fillTextInputByLabel("name", "sample-role-1");
+const roleName = `sample-role-${Date.now()}`;
+await uiHelper.fillTextInputByLabel("name", roleName);

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 7

__

Why: The suggestion improves test robustness by proposing to use a dynamic role name, which is a best practice to prevent flaky tests caused by name collisions across test runs.

Medium
Use dynamic test role names

Use a dynamically generated unique role name instead of the hardcoded
"test-role1" to prevent test collisions.

e2e-tests/playwright/e2e/plugins/rbac/rbac.spec.ts [412-417]

+const roleName = `test-role1-${Date.now()}`;
 await rbacPo.createRole(
-  "test-role1",
+  roleName,
   [RbacPo.rbacTestUsers.guest, RbacPo.rbacTestUsers.tara],
   [RbacPo.rbacTestUsers.backstage],
   [{ permission: "catalog.entity.delete" }],
 );

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 7

__

Why: The suggestion improves test robustness by proposing to use a dynamic role name, which is a best practice to prevent flaky tests caused by name collisions across test runs.

Medium
Remove outdated FIXME comment

Remove the outdated FIXME comment above the re-enabled test.

e2e-tests/playwright/e2e/plugins/rbac/rbac.spec.ts [406-409]

-//FIXME https://issues.redhat.com/browse/RHDHBUGS-2483
 test("Edit users and groups and update policies of a role from the overview page", async ({
   page,
 }) => {
  • Apply / Chat
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly identifies an outdated FIXME comment that should be removed as the test is being re-enabled, which improves code clarity and maintainability.

Low
  • Update

@github-actions
Copy link
Contributor

The image is available at:

/test e2e-ocp-helm

@PatAKnight
Copy link
Member Author

/retest

@github-actions
Copy link
Contributor

The image is available at:

/test e2e-ocp-helm

@gustavolira
Copy link
Member

/test ?

@openshift-ci
Copy link

openshift-ci bot commented Jan 21, 2026

@gustavolira: The following commands are available to trigger required jobs:

/test e2e-ocp-helm

The following commands are available to trigger optional jobs:

/test cleanup-mapt-destroy-orphaned-aks-clusters
/test cleanup-mapt-destroy-orphaned-eks-clusters
/test e2e-aks-helm-nightly
/test e2e-aks-operator-nightly
/test e2e-eks-helm-nightly
/test e2e-eks-operator-nightly
/test e2e-gke-helm-nightly
/test e2e-gke-operator-nightly
/test e2e-ocp-helm-nightly
/test e2e-ocp-helm-upgrade-nightly
/test e2e-ocp-operator-auth-providers-nightly
/test e2e-ocp-operator-nightly
/test e2e-ocp-v4-17-helm-nightly
/test e2e-ocp-v4-19-helm-nightly
/test e2e-ocp-v4-20-helm-nightly
/test e2e-osd-gcp-helm-nightly
/test e2e-osd-gcp-operator-nightly

Use /test all to run the following jobs that were automatically triggered:

pull-ci-redhat-developer-rhdh-main-e2e-ocp-helm
Details

In response to this:

/test ?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@gustavolira
Copy link
Member

/test e2e-ocp-helm-nightly

@gustavolira
Copy link
Member

/test ?

@openshift-ci
Copy link

openshift-ci bot commented Jan 21, 2026

@gustavolira: The following commands are available to trigger required jobs:

/test e2e-ocp-helm

The following commands are available to trigger optional jobs:

/test cleanup-mapt-destroy-orphaned-aks-clusters
/test cleanup-mapt-destroy-orphaned-eks-clusters
/test e2e-aks-helm-nightly
/test e2e-aks-operator-nightly
/test e2e-eks-helm-nightly
/test e2e-eks-operator-nightly
/test e2e-gke-helm-nightly
/test e2e-gke-operator-nightly
/test e2e-ocp-helm-nightly
/test e2e-ocp-helm-upgrade-nightly
/test e2e-ocp-operator-auth-providers-nightly
/test e2e-ocp-operator-nightly
/test e2e-ocp-v4-17-helm-nightly
/test e2e-ocp-v4-19-helm-nightly
/test e2e-ocp-v4-20-helm-nightly
/test e2e-osd-gcp-helm-nightly
/test e2e-osd-gcp-operator-nightly

Use /test all to run the following jobs that were automatically triggered:

pull-ci-redhat-developer-rhdh-main-e2e-ocp-helm
Details

In response to this:

/test ?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Signed-off-by: Patrick Knight <pknight@redhat.com>
Signed-off-by: Patrick Knight <pknight@redhat.com>
@sonarqubecloud
Copy link

@github-actions
Copy link
Contributor

The image is available at:

/test e2e-ocp-helm

@openshift-ci
Copy link

openshift-ci bot commented Jan 21, 2026

@PatAKnight: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-ocp-helm-nightly 643ff36 link false /test e2e-ocp-helm-nightly
ci/prow/e2e-ocp-helm dbde019 link true /test e2e-ocp-helm

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

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.

2 participants