Skip to content

Conversation

@JessicaJHee
Copy link
Member

@JessicaJHee JessicaJHee commented Jan 14, 2026

Description

E2E tests added under the auth provider tests, tests passed locally

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 14, 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 gustavolira 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:

🎫 Ticket compliance analysis 🔶

RHIDP-9704 - Partially compliant

Compliant requirements:

  • Add E2E tests in the auth provider nightly tests (in this PR).
  • Trigger auto logout popup and ensure the popup appears.
  • Wait for user to idle and timeout to expire, then ensure that the user is signed out.

Non-compliant requirements:

Requires further human verification:

  • Verify these tests are included in the intended “auth provider nightly” CI suite and run reliably in GitHub Actions (not only locally).
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🔒 No security concerns identified
⚡ Recommended focus areas for review

Flaky Timing

The tests rely on hard sleeps and short timing windows (idle timeout vs prompt timing), which can be flaky under CI load. Consider replacing fixed waits with deterministic event/selector-based waits tied to the popup appearance/disappearance and/or sign-out redirect/state.

await uiHelper.verifyTextVisible(
  "Logging out due to inactivity",
  false,
  60000,
);
await page.waitForTimeout(5000);

await page.reload();
Cookie Assumption

Sign-out verification depends on the oidc-refresh-token cookie name and absence after reload. This is tightly coupled to the current auth implementation and may fail if cookie naming/storage changes (or if the token is stored elsewhere). Consider also validating a user-visible logged-out state (e.g., redirected to login / session-expired screen) in addition to cookie checks.

  const cookies = await context.cookies();
  const authCookie = cookies.find(
    (cookie) => cookie.name === "oidc-refresh-token",
  );
  expect(authCookie).toBeUndefined();
});
Test Isolation

The tests mutate deployment config and restart the deployment inside each test. Ensure there is no config leakage between tests (especially since both enable autologout but with different prompt timings) and that cleanup restores defaults if subsequent tests in the suite assume autologout disabled.

test(`Enable autologout and user is logged out after inactivity`, async () => {
  deployment.setAppConfigProperty("auth.autologout.enabled", "true");
  deployment.setAppConfigProperty(
    "auth.autologout.idleTimeoutMinutes",
    "0.5", // minimum allowed value is 0.5 minutes
  );
  deployment.setAppConfigProperty(
    "auth.autologout.promptBeforeIdleSeconds",
    "3",
  );
  await deployment.updateAllConfigs();
  await deployment.restartLocalDeployment();
  await deployment.waitForDeploymentReady();
  await deployment.waitForSynced();

  const login = await common.keycloakLogin(
    "zeus",
    process.env.DEFAULT_USER_PASSWORD,
  );
  expect(login).toBe("Login successful");

  await uiHelper.verifyTextVisible(
    "Logging out due to inactivity",
    false,
    60000,
  );
  await page.waitForTimeout(5000);

  await page.reload();

  const cookies = await context.cookies();
  const authCookie = cookies.find(
    (cookie) => cookie.name === "oidc-refresh-token",
  );
  expect(authCookie).toBeUndefined();
});

test(`Enable autologout and user stays logged in after clicking "Don't log me out"`, async () => {
  deployment.setAppConfigProperty("auth.autologout.enabled", "true");
  deployment.setAppConfigProperty(
    "auth.autologout.idleTimeoutMinutes",
    "0.5", // minimum allowed value is 0.5 minutes
  );
  deployment.setAppConfigProperty(
    "auth.autologout.promptBeforeIdleSeconds",
    "5",
  );
  await deployment.updateAllConfigs();
  await deployment.restartLocalDeployment();
  await deployment.waitForDeploymentReady();
  await deployment.waitForSynced();
📄 References
  1. No matching references available

@rhdh-qodo-merge
Copy link

PR Type

Tests


Description

  • Add two E2E tests for autologout feature functionality

  • Test user automatic logout after inactivity period

  • Test user session persistence when dismissing logout prompt

  • Configure and validate autologout settings via deployment config


File Walkthrough

Relevant files
Tests
oidc.spec.ts
Add autologout E2E tests for OIDC provider                             

e2e-tests/playwright/e2e/auth-providers/oidc.spec.ts

  • Added test for automatic user logout after inactivity timeout
  • Added test for user remaining logged in after dismissing logout prompt
  • Both tests configure autologout settings, restart deployment, and
    verify expected behavior
  • Tests validate cookie removal and UI interactions related to
    autologout feature
+65/-0   

@JessicaJHee
Copy link
Member Author

/test e2e-ocp-operator-auth-providers-nightly

@rhdh-qodo-merge
Copy link

rhdh-qodo-merge bot commented Jan 14, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Fix visibility assertion

Correct the verifyTextVisible call by changing the visibility flag from false to
true to correctly assert that the logout prompt appears.

e2e-tests/playwright/e2e/auth-providers/oidc.spec.ts [470-474]

 await uiHelper.verifyTextVisible(
   "Logging out due to inactivity",
-  false,
+  true,
   60000,
 );
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: This suggestion correctly identifies a critical logical error in the test where it asserts the opposite of the intended behavior, which would cause the test to fail incorrectly or pass for the wrong reasons.

Medium
Wait for prompt before clicking

Add a verifyTextVisible call to wait for the "Don't log me out" button to appear
before clicking it, preventing a race condition.

e2e-tests/playwright/e2e/auth-providers/oidc.spec.ts [507]

-await uiHelper.clickButtonByText("Don't log me out", { timeout: 60000 });
+await uiHelper.verifyTextVisible("Don't log me out", true, 60000);
+await uiHelper.clickButtonByText("Don't log me out");
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: This suggestion correctly identifies and fixes a race condition in the test, making it more reliable by explicitly waiting for the prompt to be visible before attempting to interact with it.

Medium
Replace hard timeout with explicit wait

Replace the fixed page.waitForTimeout(5000) with an explicit wait for the URL to
change to the login page, ensuring the user has been logged out.

e2e-tests/playwright/e2e/auth-providers/oidc.spec.ts [475]

-await page.waitForTimeout(5000);
+await expect(page).toHaveURL(/login/);
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly points out that using waitForTimeout is a bad practice and proposes a more robust alternative by waiting for a specific application state, which improves test reliability.

Medium
  • Update

@github-actions
Copy link
Contributor

The image is available at:

/test e2e-ocp-helm

@github-actions
Copy link
Contributor

This PR is stale because it has been open 7 days with no activity. Remove stale label or comment or this will be closed in 21 days.

@github-actions github-actions bot added the Stale label Jan 22, 2026
Signed-off-by: Jessica He <jhe@redhat.com>
@JessicaJHee
Copy link
Member Author

/test e2e-ocp-operator-auth-providers-nightly

@sonarqubecloud
Copy link

@github-actions
Copy link
Contributor

@openshift-ci
Copy link

openshift-ci bot commented Jan 22, 2026

@JessicaJHee: 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 1a2df2c link true /test e2e-ocp-helm
ci/prow/e2e-ocp-operator-auth-providers-nightly 1a2df2c link false /test e2e-ocp-operator-auth-providers-nightly

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.

@github-actions github-actions bot removed the Stale label Jan 23, 2026
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.

1 participant