NO-ISSUE: add Playwright e2e harness for manual verification against a live cluster - #104
Merged
openshift-merge-bot[bot] merged 18 commits intoAug 3, 2026
Conversation
|
@batzionb: This pull request explicitly references no jira issue. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: batzionb The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesThe PR adds a private Playwright harness for manual verification of deployed or local Playwright E2E harness
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
actor Developer
participant Playwright
participant Application
participant Keycloak
participant Chromium
Developer->>Playwright: Run manual verification
Playwright->>Application: Open application root
Application->>Keycloak: Redirect to sign-in
Playwright->>Keycloak: Submit credentials
Keycloak-->>Application: Establish authenticated session
Playwright->>Chromium: Run authenticated smoke or scratch specs
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/e2e/playwright.config.ts`:
- Around line 5-8: Update the E2E_BASE_URL validation in playwright.config.ts to
parse the configured value at startup and accept only valid absolute URLs using
the http: or https: protocols. Reject missing, malformed, and unsupported-scheme
values before passing the URL to Playwright.
- Around line 22-25: Update the Playwright configuration’s ignoreHTTPSErrors
setting to remain false by default and enable only when the explicit
E2E_IGNORE_HTTPS_ERRORS environment variable is set to true. Preserve the opt-in
behavior for dev/lab environments without disabling TLS verification for normal
Keycloak redirect and password-submission flows.
In `@apps/e2e/README.md`:
- Around line 8-10: Correct the documentation wording in apps/e2e/README.md
lines 8-10 by changing “to manually confirming” to “to manually confirm”; also
update AGENTS.md lines 188-191 to change “for manually confirming a change
works” to “for manually confirming that a change works.”
- Around line 99-102: Update the temporary-spec guidance in apps/e2e/README.md
lines 99-102 and AGENTS.md lines 193-196 to require deleting the spec after
verification; remove any wording that permits leaving it in place, while
preserving the existing execution instructions.
In `@apps/e2e/src/auth.setup.ts`:
- Around line 21-24: Update the password field lookup in the authentication
setup login flow to target the native password input using a scoped input
locator, rather than getByRole('textbox'). Keep the existing password value and
Sign In button interaction unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ea36c539-9471-4346-962e-b28ac846b4b7
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
.gitignoreAGENTS.mdapps/e2e/README.mdapps/e2e/package.jsonapps/e2e/playwright.config.tsapps/e2e/src/auth-file.tsapps/e2e/src/auth.setup.tsapps/e2e/tsconfig.jsonpackage.json
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/e2e/playwright.config.ts`:
- Around line 13-22: Update the baseURL validation around new URL(baseURL) to
reject URLs with either url.username or url.password, while retaining the
existing http/https protocol check. Change the catch diagnostic to a generic
message that does not interpolate or otherwise expose the raw E2E_BASE_URL
value.
- Around line 40-43: Update the scratch directory setup around scratchDir to use
a process-unique temporary directory instead of the fixed .e2e-run path, while
preserving recursive creation and cleanup before use. Register cleanup for that
directory after the Playwright run completes so concurrent e2e processes cannot
remove or overwrite each other’s copied specs.
- Around line 31-34: Update the resolved E2E_SPEC_FILE validation before the
copy operation to require that the path is a regular file, using
fs.statSync(resolvedSpecFile).isFile() in addition to existence handling. Keep
rejecting missing paths and, if the existing configuration does not already
constrain it, apply the established allow-list for permitted test-file locations
or extensions.
- Around line 40-45: Update the temporary spec handling around scratchDir and
scratchSpecFile so the E2E runner executes the original resolvedSpecFile with
its original filesystem/module context preserved. Replace the direct
fs.copyFileSync approach with a symlink, import trampoline, or equivalent
mechanism that keeps relative imports and fixture paths anchored to the source
spec directory.
In `@apps/e2e/README.md`:
- Around line 39-40: Replace the inline E2E_PASSWORD values in the README
command examples with a safer read -s and export pattern, while retaining the
existing E2E_BASE_URL, E2E_USERNAME, E2E_SPEC_FILE, and pnpm e2e usage. Apply
the same update to all referenced examples.
- Around line 88-94: Update the documentation describing E2E_SPEC_FILE and
copied specs to address relative imports: either document that the harness
preserves the original module-resolution context, or explicitly require specs to
be self-contained or use package imports rather than adjacent relative files.
Ensure the “specs can live anywhere” guidance matches the behavior implemented
by playwright.config.ts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4b0b3b77-a56f-413c-8575-596b82597e32
📒 Files selected for processing (5)
.gitignoreAGENTS.mdapps/e2e/README.mdapps/e2e/playwright.config.tsapps/e2e/src/auth.setup.ts
New pnpm workspace package for manually verifying osac-ui against a live deployed cluster (not CI, not hermetic — the app requires real Keycloak OIDC login and there is no mock server). Config splits tests into a "setup" project and a "chromium" project that depends on it and reuses the setup project's saved browser storage state, so the login flow runs once per test run instead of per test. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: batzionb <brotman@redhat.com>
Drives the app's real login flow (it auto-redirects to Keycloak on load when unauthenticated, per apps/app-frontend/src/hooks/oidc-login.tsx) with credentials from E2E_USERNAME/E2E_PASSWORD, then saves the resulting browser session to a storage-state file for the "chromium" project to reuse — so subsequent tests start already authenticated instead of repeating the login flow each time. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: batzionb <brotman@redhat.com>
Loads the app and asserts the logged-in account menu is visible, proving the full chain works: build, deploy, auto-redirect to Keycloak (handled by the setup project), and an authenticated page load. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: batzionb <brotman@redhat.com>
apps/e2e/.auth/ holds a real Keycloak session (live access/refresh cookies) written by the setup project — must never be committed. test-results/, playwright-report/, and blob-report/ are Playwright's own run output. Also adds a root "e2e" script for discoverability. Deliberately not added to the root "test"/"lint" aggregate scripts: this package targets a live external cluster with real credentials, so it must stay an explicit, opt-in command rather than something that runs (and fails) during a plain pnpm test/lint or CI run. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: batzionb <brotman@redhat.com>
Documents prerequisites, setup, required env vars, the storageState auth pattern and why it's gitignored, and that this targets live deployments manually rather than running in CI or replacing osac-test-infra's e2e suite. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: batzionb <brotman@redhat.com>
Verified live against a real deployment. Two issues surfaced that no
amount of code reading would have caught:
- Chromium doesn't trust the dev cluster's Route certificate (the same
reason manual testing against these environments always needs
curl -k) — added ignoreHTTPSErrors to the Playwright context.
- This realm's Keycloak theme uses a two-step identifier-first login
(username + Sign In, then a separate password screen also submitted
via Sign In), not a single combined form. Also, getByLabel('Password')
is ambiguous against this theme's "Show password" toggle button;
disambiguated with getByRole('textbox', { name: 'Password' }).
Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: batzionb <brotman@redhat.com>
Shortcut for E2E_BASE_URL=http://localhost:5173 pnpm e2e, matching the existing dev:proxy script's inline env var pattern. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: batzionb <brotman@redhat.com>
Removes the committed smoke.spec.ts and gitignores apps/e2e/src/*.spec.ts. This package now only commits the harness (playwright.config.ts, auth.setup.ts) — actual test specs are written ad hoc for a specific manual-verification task (typically by an AI agent working through a change against a live cluster), run once, and never committed. Updated the README's "Writing a test" section accordingly. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: batzionb <brotman@redhat.com>
Ignore ad hoc spec files under apps/e2e/src, rework the README's testing section around that, and add AGENTS.md guidance distinguishing this Playwright harness from osac-test-infra's gRPC-level E2E suites so agents don't mistake either for persisted UI test coverage. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: batzionb <brotman@redhat.com>
- Validate E2E_BASE_URL as an absolute http(s) URL instead of just non-empty
- Default ignoreHTTPSErrors to false, opt-in via E2E_IGNORE_HTTPS_ERRORS
- Point E2E_SPEC_FILE at a spec file anywhere on disk, copied into a
gitignored scratch dir, so ad hoc specs never need to live under apps/e2e
- Fix auth.setup.ts password locator: a native input[type=password] has no
ARIA role, so getByRole('textbox') never matched it
- Wording fixes in README.md and AGENTS.md
Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: batzionb <brotman@redhat.com>
…ith real e2e tests Renames apps/e2e -> apps/playwright and @osac/e2e -> @osac/playwright since this is a manual, throwaway verification harness against a live cluster, not persisted e2e test coverage (that's osac-test-infra). Also: - Moves ad hoc specs to a fixed, gitignored apps/playwright/scratch/ directory instead of an arbitrary E2E_SPEC_FILE path, and renames env vars (E2E_BASE_URL -> OSAC_UI_BASE_URL, E2E_USERNAME -> OSAC_USERNAME, E2E_PASSWORD -> OSAC_PASSWORD, E2E_IGNORE_HTTPS_ERRORS -> IGNORE_HTTPS_ERRORS) to drop the e2e naming entirely. - Ignores apps/playwright/*.png and *.jpg — ad hoc specs that call page.screenshot() with a bare filename write relative to cwd, not testDir, so they can land outside scratch/. - Adds a root playwright:report script since `pnpm exec playwright show-report` without --filter recurses across every workspace package. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: batzionb <brotman@redhat.com>
Drop the /e2e-skill comparison — that skill lives in the osac-workspace meta-repo, not here, so referencing it in this repo's AGENTS.md is confusing context that doesn't resolve for anyone reading osac-ui in isolation. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: batzionb <brotman@redhat.com>
batzionb
force-pushed
the
feature/e2e-playwright-setup
branch
from
July 28, 2026 19:18
53bb6b7 to
422e6fc
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/playwright/package.json`:
- Around line 11-13: Update the dependency entries in
apps/playwright/package.json to use exact versions without ^ or ~, and
regenerate both lockfiles so they record and preserve those pinned versions.
In `@apps/playwright/README.md`:
- Around line 49-67: Update the pnpm playwright:dev documentation and its
referenced configuration so IGNORE_HTTPS_ERRORS is not enabled automatically.
Require users to explicitly opt in when testing against a trusted local or lab
cluster, and revise the surrounding instructions to remain consistent with the
stated off-by-default behavior.
In `@apps/playwright/src/auth.setup.ts`:
- Line 30: Update the authentication setup around storageState to create or
enforce the .auth directory with 0700 permissions and save user.json with 0600
permissions. Ensure these permissions are applied after storageState writes the
file, preserving the existing authentication-state generation flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: baca2048-970b-4c63-9f29-2546b3eabf8e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
.gitignoreAGENTS.mdapps/playwright/README.mdapps/playwright/package.jsonapps/playwright/playwright.config.tsapps/playwright/src/auth-file.tsapps/playwright/src/auth.setup.tsapps/playwright/tsconfig.jsonpackage.json
- Drop the automatic IGNORE_HTTPS_ERRORS=true from `pnpm playwright:dev` — the login redirect goes to a real Keycloak on the backing dev/lab cluster, not just localhost, so TLS bypass must stay an explicit opt-in even for the dev workflow. - Restrict .auth/user.json to 0600 and the .auth directory to 0700 right after storageState() writes it, since the file holds a live session cookie that shouldn't be readable by other local accounts. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: batzionb <brotman@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/playwright/src/auth.setup.ts`:
- Around line 33-36: Update the auth setup flow around storageState to create
the parent directory and apply 0700 permissions before writing the session
state, preventing a newly created or permissive .auth directory from exposing
the live cookie; retain the existing 0600 chmod on AUTH_FILE after storageState
completes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f0074d4d-67ec-495b-9279-e61fa3e38608
📒 Files selected for processing (3)
apps/playwright/README.mdapps/playwright/src/auth.setup.tspackage.json
Add a persisted smoke spec plus a dedicated Playwright project to sanity-check the harness itself, drop the redundant playwright:dev script in favor of setting OSAC_UI_BASE_URL directly, and simplify the local-dev docs accordingly. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: batzionb <brotman@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/playwright/README.md`:
- Around line 143-148: Update the README guidance around scratch specifications
to clarify that gitignore only prevents them from being committed; users must
delete a scratch spec when they no longer want it to run, since stale specs are
executed again. Preserve the existing distinction between scratch specs and
persisted regression coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0e18eac5-5a61-41cb-adaf-a3d1d3bd91d3
📒 Files selected for processing (6)
AGENTS.mdapps/playwright/README.mdapps/playwright/package.jsonapps/playwright/playwright.config.tsapps/playwright/src/smoke.spec.tspackage.json
Clarify that gitignore only keeps scratch/ specs out of git, not out of the run — pnpm playwright reruns every spec still in the directory, so stale specs must be deleted, not just left gitignored. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: batzionb <brotman@redhat.com>
Self-review on the round-3 fix found a second, more direct instance of the same "gitignored, so nothing to clean up" contradiction a few lines below, plus a dangling "(see the example below)" pointer to an example that never showed a deletion step. Both now consistently say gitignore keeps a spec out of git, not out of Playwright's next run. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: batzionb <brotman@redhat.com>
User feedback: the "regression coverage" callout doesn't need to name osac-test-infra specifically — state the rule (no CI, nothing under scratch/ is committed) without pointing at another repo's suite. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: batzionb <brotman@redhat.com>
storageState() previously ran before the 0700/0600 chmod calls, leaving a window where a newly created or permissively existing .auth directory could briefly expose the live Keycloak session cookie. Create the directory with 0700 permissions first, then write the session state, then chmod as a defensive re-assertion. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: batzionb <brotman@redhat.com>
Contributor
|
/lgtm |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
apps/e2e(@osac/e2e), a Playwright harness for manually verifyingosac-uiagainst a live, already-deployed cluster (real proxy, SPA, Keycloak realm, and fulfillment-service backend) — not a CI suite, no mock server, cannot run hermetically.auth.setup.ts) logs in once via Keycloak's real OIDC login form and reuses the saved session across tests; addpnpm e2e/pnpm e2e:devscripts.apps/e2e/src/*.spec.tsare gitignored on purpose — they're throwaway, written ad hoc per manual-verification task (typically by an AI agent) and not persisted; only the harness itself (config, auth setup) is committed.osac-ui/AGENTS.mdto document this harness and explicitly clarify it is not persisted UI-level E2E coverage, and thatosac-test-infra's CI-tracked E2E suites are gRPC/REST-API-level only (no browser, nothing UI-related) — to prevent agents from conflating the two.Jira
N/A
Test plan
pnpm lintpassespnpm testpasses (412 tests)proxy:make lintandmake testpasspnpm e2e:devagainst a local dev server with a real Keycloak login🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Chores