Skip to content

Optimize e2e parallel#562

Open
Anton-Fil wants to merge 3 commits into
Kuadrant:mainfrom
Anton-Fil:optimize-e2e-parallel
Open

Optimize e2e parallel#562
Anton-Fil wants to merge 3 commits into
Kuadrant:mainfrom
Anton-Fil:optimize-e2e-parallel

Conversation

@Anton-Fil

@Anton-Fil Anton-Fil commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Description

Closes #559

Reduces e2e CI time from ~30 minutes to ~11 minutes by enabling parallel test execution, fixing parallel conflicts, and splitting tests into smoke (PR) and nightly runs.

Type of change

Parallel test execution

  • Enable fullyParallel: true and increase workers from 1 to 3
  • Tests now run concurrently instead of sequentially

Fix parallel conflicts

  • apiproduct-apikeys-tab: use unique namespace names per test run
    (george-${uid}) instead of fixed names to avoid conflicts
  • apiproduct-crud: add mode: serial to prevent race condition
    with apiproduct-overview-tab reading shared test-edit-product
  • Refactor edit test to create its own unique APIProduct in
    beforeEach and clean up in afterEach

Smoke / Nightly split

  • PR runs only critical smoke tests (~58 tests, ~11 min)
  • Nightly runs all 96 tests every night at 2:00 UTC
  • Moved to nightly: filter tests, UI detail tests, tab content tests

CI optimization

  • Start yarn start in parallel with cluster setup to save ~40s

How to verify

Smoke tests (runs automatically on PR):

  1. Push any commit to this branch
  2. GitHub Actions will trigger E2E Tests workflow automatically
  3. Verify it runs only the 6 smoke test files (~58 tests)
  4. Check total CI time is ~11 minutes

Nightly tests (full suite):

  1. Go to Actions → E2E Tests (Nightly) in your fork
  2. Click Run workflowRun workflow to trigger manually
  3. Verify all 96 tests run and pass
  4. Or wait until 2:00 UTC for automatic trigger
Before After
CI time on PR ~30 min ~11 min
Full test coverage ✅ (nightly)

Summary by CodeRabbit

  • New Features
    • Added a reusable E2E GitHub Actions workflow to run smoke or full test suites, plus an automated nightly E2E run.
  • Bug Fixes
    • Improved E2E reliability by isolating test data with dynamically generated Kubernetes resources and updated cleanup.
  • Tests
    • Increased Playwright concurrency (more workers, full parallelism) while continuing to upload test reports and ensure teardown after runs.

@Anton-Fil Anton-Fil requested a review from R-Lawton June 29, 2026 14:41
@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@Anton-Fil, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 36 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: cf16e1c1-1613-401e-b29c-830f31b522d9

📥 Commits

Reviewing files that changed from the base of the PR and between c2116d6 and 3e3bba9.

📒 Files selected for processing (1)
  • e2e/tests/apikey-approvals.spec.ts
📝 Walkthrough

Walkthrough

Adds a reusable GitHub Actions E2E workflow with smoke and nightly full variants, wires it into PR and nightly workflows, increases Playwright concurrency, and updates three Playwright specs to create and clean up unique per-test Kubernetes resources.

Changes

E2E CI workflow

Layer / File(s) Summary
Reusable E2E workflow
.github/workflows/e2e-common.yaml
Defines a reusable workflow with a suite input, shared setup, dev-server startup, conditional smoke/full test execution, report upload, and teardown.
Smoke and nightly workflow wiring
.github/workflows/e2e.yaml, .github/workflows/e2e-nightly.yaml
Replaces the inline PR job with a reusable smoke workflow call and adds a nightly workflow that calls the same reusable workflow for the full suite.

E2E test execution and fixtures

Layer / File(s) Summary
Playwright parallelism
e2e/playwright.config.ts
Sets fullyParallel to true and increases workers from 1 to 3.
Dynamic fixtures in apikey-approvals spec
e2e/tests/apikey-approvals.spec.ts
Uses per-test generated namespaces, API key names, and emails across approve, reject, bulk approve, bulk reject, and deny flows, including readiness checks and cleanup.
Dynamic fixtures in apiproduct-apikeys-tab spec
e2e/tests/apiproduct-apikeys-tab.spec.ts
Switches the API Keys tab tests to timestamp-based resource names and updates row lookup and modal assertions accordingly.
Per-test APIProduct fixture in crud spec
e2e/tests/apiproduct-crud.spec.ts
Creates and deletes a unique APIProduct per test and updates edit/delete flows to use the generated name.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Trigger
  participant E2ECommon as e2e-common.yaml
  participant DevServer
  participant Playwright
  participant ArtifactStore

  Trigger->>E2ECommon: workflow_call (suite input)
  E2ECommon->>DevServer: start plugin dev server
  E2ECommon->>DevServer: poll http://localhost:9001
  DevServer-->>E2ECommon: ready
  E2ECommon->>Playwright: run smoke or full suite
  Playwright-->>E2ECommon: test results
  E2ECommon->>ArtifactStore: upload playwright-report-${suite}
  E2ECommon->>E2ECommon: run ./e2e/teardown.sh
Loading

Possibly related issues

  • #584: Adds reusable smoke/full and nightly E2E workflows, matching the split of PR and nightly test execution in this change.

Possibly related PRs

Suggested labels: tests

Suggested reviewers: emmaaroche, R-Lawton, eguzki

Poem

A rabbit hopped through CI light,
With smoke by day and full by night.
Three workers ran, and names grew new,
So tests could scurry straight on through.
No clash of keys, no tangled trail —
Just tidy hops in every run and rail. 🐇

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: speeding up e2e runs with parallel execution.
Linked Issues check ✅ Passed The PR addresses #559 by parallelising Playwright, isolating test resources, splitting smoke/nightly runs, and reducing CI setup time.
Out of Scope Changes check ✅ Passed The changes stay focused on e2e runtime reduction, test isolation, and workflow efficiency with no clear unrelated additions.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@R-Lawton

Copy link
Copy Markdown
Contributor

hey nice work on getting the CI time down! couple of thoughts:

reusable workflow instead of duplicating yaml

instead of having two separate workflow files with mostly the same setup steps, could we use a reusable workflow? something like making e2e.yaml accept a workflow_call with an input for which test files to run. then the smoke and nightly workflows become thin wrappers that just call it with different inputs. keeps the env setup, teardown, artifact upload etc in one place.

parallel collision risks

the uid fix for the "View and Actions" block looks good but i think the same treatment is needed in a few other places:

  • apiproduct-apikeys-tab.spec.ts — the "Approve Request" (ivan), "Reject Request" (judy), and "Deny Active Key" (kate) blocks still use hardcoded namespace names
  • apiproduct-crud.spec.ts — the delete test reuses the shared test-delete-product fixture instead of creating its own unique one like the edit test now does
  • apikey-approvals.spec.ts — all the test blocks use hardcoded namespaces (consumer-alice-approve, consumer-bob-reject etc) which could cause flakiness on retries

basically anything creating resources with fixed names could collide with fullyParallel: true and workers: 3

@Anton-Fil Anton-Fil marked this pull request as ready for review July 1, 2026 10:22

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (7)
.github/workflows/e2e-common.yaml (4)

42-45: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Backgrounded server start swallows failures until the readiness timeout.

yarn clean && yarn start & backgrounds immediately, so this step always exits 0 even if yarn clean/yarn start fail outright. Failures are only surfaced up to 60s later via the readiness loop, and stdout/stderr from the background process isn't captured to a log for debugging. Redirecting output to a file would ease triage on failures.

♻️ Suggested improvement
       - name: Start plugin dev server
-        run: yarn clean && yarn start &
+        run: (yarn clean && yarn start) > /tmp/plugin-dev-server.log 2>&1 &
🤖 Prompt for 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.

In @.github/workflows/e2e-common.yaml around lines 42 - 45, The Start plugin dev
server step backgrounds too early, so failures from yarn clean or yarn start are
hidden until the readiness loop times out. Update the workflow step in the Start
plugin dev server job to run the command in a way that preserves the exit status
and captures stdout/stderr to a log file for later inspection, so issues are
surfaced immediately and are easier to debug.

17-17: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Disable credential persistence on checkout.

actions/checkout@v4 persists the GITHUB_TOKEN credential in the local git config by default. Since this job uploads artifacts (playwright-report/) and doesn't need git write access afterwards, disable persistence to avoid inadvertent credential exposure.

🔒 Proposed fix
       - uses: actions/checkout@v4
+        with:
+          persist-credentials: false
🤖 Prompt for 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.

In @.github/workflows/e2e-common.yaml at line 17, The checkout step currently
uses actions/checkout@v4 with default credential persistence, which is
unnecessary for this artifact-only job. Update the checkout configuration in the
e2e-common workflow to disable persisted git credentials by setting the checkout
action’s credential persistence option off, so the job does not leave the
GITHUB_TOKEN in local git config.

Source: Linters/SAST tools


26-31: 🔒 Security & Privacy | 🔵 Trivial | ⚖️ Poor tradeoff

No integrity verification for the downloaded oinc binary.

The binary is fetched over HTTPS and executed directly without checksum/signature verification, which is a minor supply-chain hardening gap for CI tooling.

Please check whether the oinc GitHub releases publish checksums or signatures that could be verified before chmod +x/execution.

🤖 Prompt for 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.

In @.github/workflows/e2e-common.yaml around lines 26 - 31, The Install oinc
step downloads and installs a binary without verifying its integrity. Update the
workflow job around the oinc install commands to check whether the GitHub
release provides a checksum or signature, and verify the downloaded file before
chmod +x and sudo mv. If a verification artifact exists, add the corresponding
validation step in the same install block so the oinc binary is only installed
after a successful integrity check.

62-74: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Silent no-op if suite doesn't match smoke or full.

If inputs.suite is ever anything other than 'smoke' or 'full' (e.g. a typo in a future caller), neither test step's condition is satisfied, no tests run, and the job still reports success. Consider adding a fallback validation step to fail fast on an unrecognized suite value.

🛡️ Proposed fix
+      - name: Validate suite input
+        run: |
+          if [[ "${{ inputs.suite }}" != "smoke" && "${{ inputs.suite }}" != "full" ]]; then
+            echo "Invalid suite input: ${{ inputs.suite }}"
+            exit 1
+          fi
+
       - name: Run E2E tests (smoke)
🤖 Prompt for 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.

In @.github/workflows/e2e-common.yaml around lines 62 - 74, The E2E workflow
currently does nothing when inputs.suite is not exactly smoke or full, so add an
explicit validation/fallback in the same job to fail fast on unsupported values.
Update the logic around the Run E2E tests (smoke) and Run E2E tests (full) steps
in e2e-common.yaml so an unexpected inputs.suite value triggers a clear error
before either Playwright command is skipped.
.github/workflows/e2e.yaml (1)

15-18: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Add an explicit least-privilege permissions block.

No permissions: block is set, so the job inherits the repository's default GITHUB_TOKEN scope, which may be broader than required. This job only needs to check out code and upload artifacts (in the shared workflow).

🔒 Proposed fix
+permissions:
+  contents: read
+
 jobs:
   e2e-smoke:
     uses: ./.github/workflows/e2e-common.yaml
🤖 Prompt for 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.

In @.github/workflows/e2e.yaml around lines 15 - 18, The e2e-smoke workflow job
currently relies on the default inherited GITHUB_TOKEN scope, so add an explicit
least-privilege permissions block for the reusable job defined in e2e-smoke.
Update the workflow near the existing uses: ./.github/workflows/e2e-common.yaml
stanza so the job only has the token access needed for checkout and artifact
upload, and keep the scope as narrow as possible.

Source: Linters/SAST tools

e2e/tests/apikey-approvals.spec.ts (1)

92-147: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Namespace cleanup isn't guaranteed on test failure.

Unlike the other suites in this file (bob, bob2, carol/dave, ellen/frank, george all use test.beforeEach/test.afterEach), this test creates aliceNs and only deletes it at line 146, after all assertions. If any expect(...) in between throws (e.g. the approval modal or toast timeout), the namespace is never cleaned up, leaking test resources on the cluster.

♻️ Move fixture lifecycle into beforeEach/afterEach, matching sibling suites
-  test('approve request shows success toast', async ({ page }) => {
-    const uid = String(Date.now());
-    const aliceNs = `consumer-alice-approve-${uid}`;
+  let uid: string;
+  let aliceNs: string;
+  let aliceKey: string;
+  let aliceEmail: string;
+
+  test.beforeEach(async () => {
+    uid = String(Date.now());
+    aliceNs = `consumer-alice-approve-${uid}`;
     ...
+  });
+
+  test.afterEach(async () => {
+    execSync(`kubectl delete namespace ${aliceNs} --ignore-not-found=true`, { stdio: 'inherit' });
+  });
+
+  test('approve request shows success toast', async ({ page }) => {
     ...
-    execSync(`kubectl delete namespace ${aliceNs} --ignore-not-found=true`, { stdio: 'inherit' });
   });
🤖 Prompt for 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.

In `@e2e/tests/apikey-approvals.spec.ts` around lines 92 - 147, The alice approval
test creates aliceNs inline and only deletes it at the end, so failures skip
cleanup and leak cluster resources. Move the fixture setup/teardown for this
test into test.beforeEach and test.afterEach, following the same pattern used by
the sibling approval suites, and make sure aliceNs, aliceKey, and aliceEmail are
owned by that lifecycle so the namespace is always deleted even when an
assertion fails.
e2e/tests/apiproduct-crud.spec.ts (1)

37-84: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Scope this fixture to the edit/delete tests. test.beforeEach/test.afterEach currently wrap the whole APIProduct CRUD Operations suite, so the create/list/view tests also pay for an extra kubectl apply/kubectl delete on every run. Move this into a nested describe for the tests that use editProductName.

🤖 Prompt for 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.

In `@e2e/tests/apiproduct-crud.spec.ts` around lines 37 - 84, Move the edit
fixture setup/teardown out of the top-level APIProduct CRUD suite and into a
nested describe that only contains the edit/delete tests. The current
test.beforeEach and test.afterEach around editProductName make every test run
kubectl apply/delete unnecessarily; keep the existing beforeEach/afterEach logic
but scope it to the tests that actually use editProductName, preserving the
page.goto and dismissConsoleTour flow for those tests only.
🤖 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 `@e2e/tests/apikey-approvals.spec.ts`:
- Line 372: The concurrent polling in execSync uses backgrounded timeout loops
followed by a bare wait, which hides failures when either APIKeyRequest never
appears. Update the APIKey approvals test to capture and validate each
background job’s exit status instead of relying on plain wait, and apply the
same fix to both the carol/dave and ellen/frank polling blocks in
apikey-approvals.spec.ts so the test fails immediately if either resource does
not show up.
- Around line 92-95: The current `uid = String(Date.now())` pattern in this test
file can still collide across parallel workers, so update the shared test
identifier generation used for `aliceNs`, `aliceKey`, and `aliceEmail` to
include extra entropy such as the Playwright worker index or a random suffix.
Apply the same fix to the repeated `uid` creation in this file and the related
specs `apiproduct-apikeys-tab.spec.ts` and `apiproduct-crud.spec.ts` so all
generated namespaces, keys, and emails remain unique under `fullyParallel` with
multiple workers.

---

Nitpick comments:
In @.github/workflows/e2e-common.yaml:
- Around line 42-45: The Start plugin dev server step backgrounds too early, so
failures from yarn clean or yarn start are hidden until the readiness loop times
out. Update the workflow step in the Start plugin dev server job to run the
command in a way that preserves the exit status and captures stdout/stderr to a
log file for later inspection, so issues are surfaced immediately and are easier
to debug.
- Line 17: The checkout step currently uses actions/checkout@v4 with default
credential persistence, which is unnecessary for this artifact-only job. Update
the checkout configuration in the e2e-common workflow to disable persisted git
credentials by setting the checkout action’s credential persistence option off,
so the job does not leave the GITHUB_TOKEN in local git config.
- Around line 26-31: The Install oinc step downloads and installs a binary
without verifying its integrity. Update the workflow job around the oinc install
commands to check whether the GitHub release provides a checksum or signature,
and verify the downloaded file before chmod +x and sudo mv. If a verification
artifact exists, add the corresponding validation step in the same install block
so the oinc binary is only installed after a successful integrity check.
- Around line 62-74: The E2E workflow currently does nothing when inputs.suite
is not exactly smoke or full, so add an explicit validation/fallback in the same
job to fail fast on unsupported values. Update the logic around the Run E2E
tests (smoke) and Run E2E tests (full) steps in e2e-common.yaml so an unexpected
inputs.suite value triggers a clear error before either Playwright command is
skipped.

In @.github/workflows/e2e.yaml:
- Around line 15-18: The e2e-smoke workflow job currently relies on the default
inherited GITHUB_TOKEN scope, so add an explicit least-privilege permissions
block for the reusable job defined in e2e-smoke. Update the workflow near the
existing uses: ./.github/workflows/e2e-common.yaml stanza so the job only has
the token access needed for checkout and artifact upload, and keep the scope as
narrow as possible.

In `@e2e/tests/apikey-approvals.spec.ts`:
- Around line 92-147: The alice approval test creates aliceNs inline and only
deletes it at the end, so failures skip cleanup and leak cluster resources. Move
the fixture setup/teardown for this test into test.beforeEach and
test.afterEach, following the same pattern used by the sibling approval suites,
and make sure aliceNs, aliceKey, and aliceEmail are owned by that lifecycle so
the namespace is always deleted even when an assertion fails.

In `@e2e/tests/apiproduct-crud.spec.ts`:
- Around line 37-84: Move the edit fixture setup/teardown out of the top-level
APIProduct CRUD suite and into a nested describe that only contains the
edit/delete tests. The current test.beforeEach and test.afterEach around
editProductName make every test run kubectl apply/delete unnecessarily; keep the
existing beforeEach/afterEach logic but scope it to the tests that actually use
editProductName, preserving the page.goto and dismissConsoleTour flow for those
tests only.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: fbbe4f25-317a-4c87-9c76-6c39830c3452

📥 Commits

Reviewing files that changed from the base of the PR and between 5d6c47d and 5569d2c.

📒 Files selected for processing (7)
  • .github/workflows/e2e-common.yaml
  • .github/workflows/e2e-nightly.yaml
  • .github/workflows/e2e.yaml
  • e2e/playwright.config.ts
  • e2e/tests/apikey-approvals.spec.ts
  • e2e/tests/apiproduct-apikeys-tab.spec.ts
  • e2e/tests/apiproduct-crud.spec.ts

Comment thread e2e/tests/apikey-approvals.spec.ts Outdated
Comment thread e2e/tests/apikey-approvals.spec.ts Outdated
Anton-Fil added 2 commits July 2, 2026 16:06
  - Fix parallel conflicts by using unique resource names per test
  - Refactor edit test to use unique APIProduct instead of shared fixture

Signed-off-by: Anton-Fil <a.filkach@gmail.com>
…able workflow, retry flakiness fixes

Signed-off-by: Anton-Fil <a.filkach@gmail.com>
@Anton-Fil Anton-Fil force-pushed the optimize-e2e-parallel branch from 688a772 to c2116d6 Compare July 2, 2026 15:10

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
e2e/tests/apikey-approvals.spec.ts (1)

92-148: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Namespace cleanup isn't guaranteed on test failure.

kubectl delete namespace ${aliceNs} (line 146) only runs if every preceding assertion in the test body succeeds. If any expect(...) between lines 129-144 times out or fails, the test throws and this cleanup step never executes, leaking the consumer-alice-approve-* namespace. Every other suite in this file (bob, bob2, carol/dave, ellen/frank, george) avoids this by placing cleanup in test.afterEach, which Playwright always runs regardless of outcome. With fullyParallel: true and repeated nightly full-suite runs, failed/flaky runs of this specific test will accumulate orphaned namespaces in the cluster.

🛠️ Suggested fix: move setup/cleanup into beforeEach/afterEach like the other suites
-test.describe('APIKey Approvals - Approve single request', () => {
-  test('approve single request shows success toast', async ({ page }) => {
-    const uid = `${Date.now()}-${Math.random().toString(36).slice(2, 6)}`;
-    const aliceNs = `consumer-alice-approve-${uid}`;
-    ...
-    execSync(`kubectl delete namespace ${aliceNs} --ignore-not-found=true`, { stdio: 'inherit' });
-  });
-});
+test.describe('APIKey Approvals - Approve single request', () => {
+  let uid: string;
+  let aliceNs: string;
+  ...
+  test.beforeEach(async ({ page }) => {
+    uid = `${Date.now()}-${Math.random().toString(36).slice(2, 6)}`;
+    aliceNs = `consumer-alice-approve-${uid}`;
+    ...
+  });
+
+  test.afterEach(async () => {
+    execSync(`kubectl delete namespace ${aliceNs} --ignore-not-found=true`, { stdio: 'inherit' });
+  });
+
+  test('approve single request shows success toast', async ({ page }) => {
+    ...
+  });
+});
🤖 Prompt for 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.

In `@e2e/tests/apikey-approvals.spec.ts` around lines 92 - 148, The cleanup for
the alice approval test is only executed at the end of the test body, so it is
skipped on assertion failures and can leave orphaned namespaces. Move the
namespace creation and APIKey setup into a shared setup path and add the
`kubectl delete namespace ${aliceNs}` cleanup to a `test.afterEach` block,
matching the pattern used by the other approval suites in
`apikey-approvals.spec.ts`. Use the existing `aliceNs`, `aliceKey`, and
`aliceEmail` test variables to keep the teardown tied to the correct test
instance.
🧹 Nitpick comments (1)
.github/workflows/e2e-common.yaml (1)

36-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

No TypeScript compile-time check before running tests.

The coding guidelines require running TypeScript type checks throughout the codebase, but this reusable e2e workflow goes straight from yarn install to Playwright browser install/tests with no tsc --noEmit (or equivalent lint/type-check) step, so type errors in test specs or app code would only surface as opaque runtime failures during the e2e run.

As per coding guidelines, "**/*.ts?(x): Run TypeScript for type safety and compile-time checks throughout the codebase".

🤖 Prompt for 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.

In @.github/workflows/e2e-common.yaml around lines 36 - 41, The reusable e2e
workflow skips the required TypeScript safety check before browser setup and
tests. Update the workflow around the Install dependencies / Install Playwright
browsers steps to run a TypeScript compile-time check first (such as tsc
--noEmit or the repo’s equivalent type-check command) so failures in specs or
app code are caught early. Keep the change in the e2e-common workflow and
preserve the existing Playwright install/test flow after the new type-check
step.

Source: Coding guidelines

🤖 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.

Outside diff comments:
In `@e2e/tests/apikey-approvals.spec.ts`:
- Around line 92-148: The cleanup for the alice approval test is only executed
at the end of the test body, so it is skipped on assertion failures and can
leave orphaned namespaces. Move the namespace creation and APIKey setup into a
shared setup path and add the `kubectl delete namespace ${aliceNs}` cleanup to a
`test.afterEach` block, matching the pattern used by the other approval suites
in `apikey-approvals.spec.ts`. Use the existing `aliceNs`, `aliceKey`, and
`aliceEmail` test variables to keep the teardown tied to the correct test
instance.

---

Nitpick comments:
In @.github/workflows/e2e-common.yaml:
- Around line 36-41: The reusable e2e workflow skips the required TypeScript
safety check before browser setup and tests. Update the workflow around the
Install dependencies / Install Playwright browsers steps to run a TypeScript
compile-time check first (such as tsc --noEmit or the repo’s equivalent
type-check command) so failures in specs or app code are caught early. Keep the
change in the e2e-common workflow and preserve the existing Playwright
install/test flow after the new type-check step.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6a438a6c-cbbb-4045-a27c-61c19cd440d8

📥 Commits

Reviewing files that changed from the base of the PR and between 688a772 and c2116d6.

📒 Files selected for processing (7)
  • .github/workflows/e2e-common.yaml
  • .github/workflows/e2e-nightly.yaml
  • .github/workflows/e2e.yaml
  • e2e/playwright.config.ts
  • e2e/tests/apikey-approvals.spec.ts
  • e2e/tests/apiproduct-apikeys-tab.spec.ts
  • e2e/tests/apiproduct-crud.spec.ts
🚧 Files skipped from review as they are similar to previous changes (3)
  • .github/workflows/e2e-nightly.yaml
  • e2e/playwright.config.ts
  • e2e/tests/apiproduct-apikeys-tab.spec.ts

Signed-off-by: Anton-Fil <a.filkach@gmail.com>
@R-Lawton

R-Lawton commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

👀

@R-Lawton R-Lawton left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nice work on the ci speedup, couple things before merging

let editProductName = '';

test.beforeEach(async ({ page }) => {
// Create a unique APIProduct for the edit test to avoid conflicts with other parallel tests

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the beforeEach creates a unique APIProduct for every test but only edit and delete use editProductName. the other ~10 tests create and tear down a k8s resource for nothing. wrapping edit and delete in a nested test.describe with their own setup/teardown would fix this

await expect(page.locator('td:has-text("alice@example.com")')).toBeVisible();
});
test.beforeEach(async ({ page }) => {
const uid = `${Date.now()}-${Math.random().toString(36).slice(2, 6)}`;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same thing here. the beforeEach creates a dynamic namespace + APIKey + Secret but the "cancel approval modal" test never uses them, it just operates on the setup.sh alice@example.com. these should be moved into their own describe or the cancel test should use the dynamic resources

execSync('timeout 30 bash -c \'until kubectl get apikeyrequests -n kuadrant-test | grep -q carol-bulk; do sleep 1; done\'', { stdio: 'inherit' });
execSync('timeout 30 bash -c \'until kubectl get apikeyrequests -n kuadrant-test | grep -q dave-bulk; do sleep 1; done\'', { stdio: 'inherit' });
execSync(`bash -c 'timeout 30 bash -c "until kubectl get apikeyrequests -n kuadrant-test | grep -q ${carolKey}; do sleep 1; done" & PID1=$!; timeout 30 bash -c "until kubectl get apikeyrequests -n kuadrant-test | grep -q ${daveKey}; do sleep 1; done" & PID2=$!; wait $PID1 || exit 1; wait $PID2 || exit 1'`, { stdio: 'inherit' });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the parallel kubectl wait works but it's pretty dense. a short comment explaining what it's doing would help


- name: Start plugin dev server
run: yarn clean && yarn start &
env:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if yarn start dies in the background the wait loop sits there for 60s before failing with no useful output. since you're already improving ci reliability here, capturing the pid and checking it's still alive during the loop would make it fail fast

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SPIKE: What can we do to make our e2e tests quicker to run

2 participants