test: e2e tests #517
test: e2e tests #517
Conversation
feloy
requested review from
SoniaSandler,
dgolovin and
simonrey1
and removed request for
a team
feloy
force-pushed
the
feat/playwright
branch
from
eac912d to
692782a
Compare
📝 WalkthroughWalkthroughAdds a Playwright end-to-end test package and tests for the Kubernetes Dashboard extension, updates workspace and TypeScript configs, adds root npm scripts, adjusts the build container pnpm command, and includes the new test package in release workflow version-bump and git-add steps. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks✅ Passed checks (3 passed)
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: 1
🧹 Nitpick comments (5)
tests/playwright/tsconfig.json (2)
8-8: Consider enabling skipLibCheck for faster compilation.Setting
skipLibCheck: falseforces TypeScript to check all declaration files, which slows down compilation. For test code, setting this totrueis typically preferred.Apply this diff:
- "skipLibCheck": false, + "skipLibCheck": true,
12-12: Review the "tests/" exclusion pattern.**Excluding
"tests/**"from a tsconfig.json located intests/playwright/may be confusing or could unintentionally exclude nested test directories if any are added later.Consider whether this exclusion is necessary, or clarify its intent with a comment.
tests/playwright/src/extension.spec.ts (3)
28-34: Defaulting to:latestimage may hurt CI determinismUsing
:latestas the fallback forEXTENSION_OCI_IMAGE(Line 28) is convenient locally but makes CI runs non‑reproducible when the env var isn’t set. If these tests are ever run in CI withoutEXTENSION_OCI_IMAGEdefined, you may see spurious breakage when the image is updated.Consider either:
- Making
EXTENSION_OCI_IMAGErequired in CI (and only defaulting for local runs), or- Pointing the default to a fixed tag or digest that you update intentionally.
70-83: Avoid cross‑test shared state forextensionsPage
extensionsPage(Line 71) is populated in one test and consumed in another:
- Line 73–78: initialized in
Open Settings -> Extensions page.- Line 80–83: used in
Install extensionwithout any local initialization.This has a couple of drawbacks:
- It relies on
test.describe.serialsemantics plus execution order; reordering tests or running a single test in isolation would break it.- Under stricter TypeScript settings, this pattern can trigger TS2454 (“Variable is used before being assigned”), since the compiler doesn’t see the assignment happening within the same function body.
A more robust pattern is to keep each test self‑contained, for example:
- test(`Install extension`, async () => { - test.skip(EXTENSION_PREINSTALLED, 'Extension is preinstalled'); - await extensionsPage.installExtensionFromOCIImage(EXTENSION_OCI_IMAGE); - }); + test(`Install extension`, async ({ navigationBar }) => { + test.skip(EXTENSION_PREINSTALLED, 'Extension is preinstalled'); + const extensionsPage = await navigationBar.openExtensions(); + await extensionsPage.installExtensionFromOCIImage(EXTENSION_OCI_IMAGE); + });This preserves serial semantics but removes cross‑test coupling and any definite‑assignment concerns.
103-111: Clarify error‑tab assertion to better express intentThe current pattern:
- Checks if an “Error” tab exists.
- If present, activates it and captures
stackTrace.- Then always runs
expect(errorTab, message).not.toBeVisible()(Line 110).Functionally this will:
- Pass when the Error tab is absent (no error).
- Fail when it’s present/visible, with a message that includes the captured stack trace.
That meets the intent but is a bit non‑obvious to a reader, since we deliberately make the tab visible in the error case and then assert that it’s not visible to force a failure.
You could make the intent clearer by asserting on the count instead of visibility, while still propagating the stack trace, for example:
- let stackTrace = ''; - if ((await errorTab.count()) > 0) { - await details.activateTab('Error'); - stackTrace = await details.errorStackTrace.innerText(); - } - await playExpect(errorTab, `Error Tab was present with stackTrace: ${stackTrace}`).not.toBeVisible(); + const errorTabCount = await errorTab.count(); + let stackTrace = ''; + if (errorTabCount > 0) { + await details.activateTab('Error'); + stackTrace = await details.errorStackTrace.innerText(); + } + await playExpect( + errorTabCount, + `Error Tab was present with stackTrace: ${stackTrace}`, + ).toBe(0);Same behavior, but the assertion more directly encodes “there must be no Error tab”.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
.github/workflows/release.yaml(2 hunks)build/Containerfile(1 hunks)package.json(1 hunks)pnpm-workspace.yaml(1 hunks)tests/playwright/package.json(1 hunks)tests/playwright/playwright.config.ts(1 hunks)tests/playwright/src/extension.spec.ts(1 hunks)tests/playwright/src/model/kd-details-page.ts(1 hunks)tests/playwright/tsconfig.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/playwright/src/extension.spec.ts (1)
tests/playwright/src/model/kd-details-page.ts (1)
KubernetesDashboardDetailsPage(22-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build / macos-14
- GitHub Check: Build / ubuntu-24.04
- GitHub Check: Build / windows-2022
- GitHub Check: Build extension OCI image
🔇 Additional comments (10)
.github/workflows/release.yaml (2)
74-77: LGTM!The tagging workflow correctly includes the new Playwright test package in version updates and git staging, maintaining consistency with existing package handling.
111-114: LGTM!The version bump workflow correctly includes the new test package, ensuring consistent versioning across the monorepo.
pnpm-workspace.yaml (1)
3-3: LGTM!Adding
tests/*to the workspace correctly integrates the new Playwright test package into the monorepo structure.build/Containerfile (1)
57-57: LGTM!Adding
--workspace-rootcorrectly aligns the pnpm installation with the workspace configuration now thattests/*has been added to the workspace. This ensures pnpm respects the workspace structure when installing dependencies in the extension directory.package.json (1)
26-27: LGTM!The E2E test scripts correctly delegate to the Playwright package, providing convenient entry points from the root of the monorepo.
tests/playwright/playwright.config.ts (1)
21-40: LGTM!The Playwright configuration is well-suited for bootstrapping E2E tests with:
- Serial execution (workers: 1) for stability
- Comprehensive reporting (junit, json, html)
- Chromium-only testing as a starting point
tests/playwright/src/model/kd-details-page.ts (1)
22-26: LGTM!The page object model follows best practices by extending the base
ExtensionDetailsPageclass and provides a clean abstraction for the Kubernetes Dashboard details page.tests/playwright/src/extension.spec.ts (3)
35-62: RunnerOptions and startup flow look consistent with the fixturesThe use of
RunnerOptionswith a dedicatedcustomFolderand disabled extensions plus thebeforeAllthat handles the welcome page and waits for the Podman machine to start are coherent and match typical Playwright patterns for this kind of setup. Timeout bump to 80s for startup is reasonable.
85-94: Polling for installation and status assertion look goodThe polling loop via
playExpect.poll(Lines 87–91) forextensionIsInstalledfollowed by reading the installed card and assertingACTIVEstatus is a solid way to avoid race conditions during extension installation. This pattern should be fairly robust in CI.
96-103: Inconsistent extension name passed togetInstalledExtensionIn line 98, you call:
const extensionCard = await extensions.getInstalledExtension('kubernetes-dashboard', CATALOG_EXTENSION_LABEL);but elsewhere in this file:
CATALOG_EXTENSION_NAMEis'Kubernetes Dashboard'(Line 32)- The earlier test uses
getInstalledExtension(CATALOG_EXTENSION_NAME, CATALOG_EXTENSION_LABEL)(Line 92)This inconsistency between
'kubernetes-dashboard'and'Kubernetes Dashboard'is likely a bug and may causegetInstalledExtensionto fail to locate the card (or behave differently than intended).Recommend aligning this call with the constant and the previous usage:
- const extensionCard = await extensions.getInstalledExtension('kubernetes-dashboard', CATALOG_EXTENSION_LABEL); + const extensionCard = await extensions.getInstalledExtension( + CATALOG_EXTENSION_NAME, + CATALOG_EXTENSION_LABEL, + );
feloy
force-pushed
the
feat/playwright
branch
from
692782a to
42a24e2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/playwright/src/extension.spec.ts (2)
56-67: Timeout management could be centralized or made more explicitCalling
test.setTimeoutinsidebeforeAllandafterAllworks, but it makes per-hook time budgets a bit implicit and duplicated with the explicit80_000passed intowaitForPodmanMachineStartup. Consider:
- Moving timeouts to Playwright config or a top-level
test.setTimeoutfor this file; or- Extracting timeout values into named constants so they’re easier to tune consistently.
This is purely for clarity/maintainability; behavior is fine as-is.
You may want to confirm in the Playwright docs that
test.setTimeoutbehaves as expected insideafterAllhooks for your version.
69-84: SharedextensionsPagestate makes tests order-dependent
extensionsPageis initialized intest("Open Settings -> Extensions page")and then used intest("Install extension"). This couples the second test to the first and assumes they always run together and in order.Even with
describe.serial, running a subset of tests (e.g., via a test name filter) can causeextensionsPageto beundefinedwhen only “Install extension” is executed.Consider refactoring, for example:
- Move navigation to Extensions into a
beforeEach/helper that returnsextensionsPage; or- Merge these two tests into a single scenario test that opens the page and performs installation in one flow.
This will make the suite more robust to selective test runs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
.github/workflows/release.yaml(2 hunks)build/Containerfile(1 hunks)package.json(1 hunks)pnpm-workspace.yaml(1 hunks)tests/playwright/package.json(1 hunks)tests/playwright/playwright.config.ts(1 hunks)tests/playwright/src/extension.spec.ts(1 hunks)tests/playwright/src/model/kd-details-page.ts(1 hunks)tests/playwright/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/playwright/tsconfig.json
- tests/playwright/src/model/kd-details-page.ts
- build/Containerfile
- package.json
- .github/workflows/release.yaml
- tests/playwright/package.json
- tests/playwright/playwright.config.ts
- pnpm-workspace.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
tests/playwright/src/extension.spec.ts (1)
tests/playwright/src/model/kd-details-page.ts (1)
KubernetesDashboardDetailsPage(22-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build / macos-14
- GitHub Check: Build / windows-2022
- GitHub Check: Build / ubuntu-24.04
- GitHub Check: Build extension OCI image
🔇 Additional comments (2)
tests/playwright/src/extension.spec.ts (2)
35-54: RunnerOptions and extension-disabling setup look solidCustom
runnerOptions(folder name andextensions.disabledlist) are a good fit for keeping this suite focused and faster by turning off unrelated extensions. No functional issues spotted here.If you want to double‑check IDs, you can grep for these extension identifiers in the repo or Podman Desktop settings schema to ensure they exactly match the installed extensions.
85-94: Polling for installation and card status looks appropriateThe use of
expect.pollwithextensionIsInstalled(CATALOG_EXTENSION_LABEL)and subsequentgetInstalledExtension(CATALOG_EXTENSION_NAME, CATALOG_EXTENSION_LABEL)plus status check againstCATALOG_STATUS_ACTIVEis clear and matches the intent of verifying successful installation.If you observe flaky installs, you might later consider:
- Slightly increasing the poll timeout; or
- Reusing a shared constant for this timeout across tests.
But as written, this block is sound.
Signed-off-by: Philippe Martin <[email protected]>
feloy
force-pushed
the
feat/playwright
branch
from
42a24e2 to
a5724c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
.github/workflows/release.yaml(2 hunks)build/Containerfile(1 hunks)package.json(1 hunks)pnpm-workspace.yaml(1 hunks)tests/playwright/package.json(1 hunks)tests/playwright/playwright.config.ts(1 hunks)tests/playwright/src/extension.spec.ts(1 hunks)tests/playwright/src/model/kd-details-page.ts(1 hunks)tests/playwright/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- .github/workflows/release.yaml
- tests/playwright/playwright.config.ts
- tests/playwright/tsconfig.json
- package.json
- tests/playwright/src/extension.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build / ubuntu-24.04
- GitHub Check: Build extension OCI image
- GitHub Check: Build / macos-14
- GitHub Check: Build / windows-2022
🔇 Additional comments (3)
pnpm-workspace.yaml (1)
1-8: Workspace update to include tests/ looks good*Including
tests/*in the workspace aligns with the new Playwright test package and should integrate cleanly with existing pnpm setup.build/Containerfile (1)
53-58: Use of--workspace-rootwith pnpm in the image build looks correctPointing
pnpm addat/opt/app-root/extensionwith--workspace-rootis consistent with the workspace setup and keepsisomorphic-wspinned at the intended root package level.tests/playwright/src/model/kd-details-page.ts (1)
1-26: Page object implementation is clean and idiomaticThe
KubernetesDashboardDetailsPagewrapper overExtensionDetailsPageis minimal, typed, and follows a clear pattern; constructor wiring via the extension display name looks appropriate.
3 participants
Bootstrap e2e tests, to be completed
Tests will be executed in CI, in follow-up PR
Part of #76