-
Notifications
You must be signed in to change notification settings - Fork 12
ci: e2e #507
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
ci: e2e #507
Conversation
f484eaa to
9eb5881
Compare
3920fc6 to
897d645
Compare
📝 WalkthroughWalkthroughA new GitHub Actions workflow is added for end-to-end testing, triggered by PR labels. The workflow builds a Podman Desktop extension plugin and runs Playwright-based tests on multiple operating systems with dependency caching. README documentation is updated to describe the CI testing process. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Pre-merge checks❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Nitpick comments (6)
tests/playwright/tsconfig.json (2)
8-8: Consider enablingskipLibCheckfor faster compilation.Setting
skipLibCheck: falseis unusually strict and may cause type-checking issues with third-party libraries like@playwright/test. The standard practice is to set this totrueto skip type-checking of declaration files, which speeds up compilation without affecting your code's type safety.- "skipLibCheck": false, + "skipLibCheck": true,
12-12: Clarify or remove thetests/**exclusion.Excluding
tests/**in a configuration file that is itself located intests/playwright/is confusing. If you intend to exclude nested test directories (e.g.,tests/playwright/tests/), this should be clarified. Otherwise, this exclusion may not have the intended effect.If no nested
tests/directory exists, consider removing this exclusion:- "exclude": ["node_modules/**", "output/**", "tests/**"] + "exclude": ["node_modules/**", "output/**"]tests/playwright/playwright.config.ts (1)
21-23: Consider adding timeout and retry configuration.The configuration is missing important test execution settings that could improve reliability:
- No
timeoutsetting could allow tests to hang indefinitely- No
retriessetting means flaky tests won't be automatically retriedexport default defineConfig({ outputDir: './output/', workers: 1, + timeout: 60000, // 60 seconds per test + retries: process.env.CI ? 2 : 0, // Retry twice on CI, never locally reporter: [tests/playwright/src/extension.spec.ts (1)
96-111: Inconsistent use of constant vs string literal.Line 98 uses the hardcoded string
'kubernetes-dashboard'while line 92 usesCATALOG_EXTENSION_NAME. Consider using the constant for consistency.- const extensionCard = await extensions.getInstalledExtension('kubernetes-dashboard', CATALOG_EXTENSION_LABEL); + const extensionCard = await extensions.getInstalledExtension(CATALOG_EXTENSION_NAME, CATALOG_EXTENSION_LABEL);.github/workflows/e2e-tests.yaml (2)
83-87: Minor: Inconsistent indentation.Line 85 has a leading space before
podman --version.run: | echo "Adding Podman to PATH" $env:PATH += ";C:\Program Files\RedHat\Podman" - podman --version + podman --version podman machine init --now
89-109: Update deprecatedapt-keyusage and consider Ubuntu version alignment.Two points worth addressing:
apt-key addis deprecated and will be removed after Ubuntu 24.04. Use the modern approach: download the key withcurlandgpg --dearmor, save it to/etc/apt/keyrings/<repo>.gpg, then reference it viasigned-by=in the sources.list entry.Using
ubuntu_version='23.10'repos onubuntu-24.04runner works but is fragile. Consider updating to24.04to align with the runner OS and ensure repository stability.
📜 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 (11)
.github/workflows/e2e-tests.yaml(1 hunks).github/workflows/release.yaml(2 hunks)README.md(1 hunks)build/Containerfile(2 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). (2)
- GitHub Check: Build Extension Image
- GitHub Check: Build extension OCI image
🔇 Additional comments (10)
pnpm-workspace.yaml (1)
3-3: LGTM!Correctly expands the workspace to include the new test packages.
tests/playwright/playwright.config.ts (1)
32-39: LGTM! Single browser configuration is appropriate for extension testing.The chromium-only configuration is suitable for Podman Desktop extension e2e tests, as the application runs in Electron (which is Chromium-based).
.github/workflows/release.yaml (2)
72-72: LGTM! Correctly includes test package in version tagging.The release workflow now properly updates the version in
tests/playwright/package.jsonduring tagging, maintaining version consistency across all packages.
106-107: LGTM! Test package included in version bump flow.The version bump logic correctly includes the test package when creating the PR to bump versions after a release.
build/Containerfile (2)
24-24: Explicit path for workspace config is clear.Making the destination path explicit improves readability, even though this doesn't change the copy behavior.
57-57: LGTM! The--workspace-rootflag is necessary.Adding
--workspace-rootensures that pnpm correctly resolves workspace dependencies when the workspace configuration now includestests/*. This change aligns with the expanded workspace structure.tests/playwright/src/model/kd-details-page.ts (1)
19-26: LGTM!Clean implementation of the page object pattern. The class correctly extends
ExtensionDetailsPageand passes the appropriate extension name to the parent constructor.tests/playwright/src/extension.spec.ts (2)
56-67: LGTM!Good test setup with appropriate timeouts. The beforeAll hook properly initializes the test environment, and afterAll ensures cleanup.
103-110: Nice error capture pattern.Good approach to capture the error stack trace before asserting the Error tab is not visible. This provides better debugging information when the test fails.
.github/workflows/e2e-tests.yaml (1)
24-45: LGTM!The build-container job correctly builds the extension image, extracts the plugin files, and uploads them as an artifact. Good use of conditional execution with the
ci/e2elabel check.
|
suggestion(non-blocking): split in multiple commits for :
|
e167440 to
c3e8433
Compare
90cf858 to
2538555
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (2)
package.json (1)
26-27: Align e2e scripts with pnpm workspace usage (duplicate of earlier feedback)The repo otherwise standardizes on
pnpm; these scripts switch tonpmand manualcd, which is harder to maintain in a workspace.Consider instead:
- "test:e2e": "cd tests/playwright && npm run test:e2e", - "test:e2e:smoke": "cd tests/playwright && npm run test:e2e:smoke", + "test:e2e": "pnpm --filter kubernetes-dashboard-tests-playwright test:e2e", + "test:e2e:smoke": "pnpm --filter kubernetes-dashboard-tests-playwright test:e2e:smoke",(or
pnpm -C tests/playwright test:e2eif you prefer directory-based addressing).tests/playwright/package.json (1)
12-18: Update@podman-desktop/tests-playwrightto a published version –^1.23.1does not existVersion 1.23.1 is not published on npm; the latest available version is 1.21.0. The
pnpm installwill fail during CI. Update to^1.21.0or use a published version known to exist at merge time.- "@podman-desktop/tests-playwright": "^1.23.1", + "@podman-desktop/tests-playwright": "^1.21.0",
📜 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 (11)
.github/workflows/e2e-tests.yaml(1 hunks).github/workflows/release.yaml(2 hunks)README.md(1 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 due to trivial changes (1)
- tests/playwright/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/playwright/playwright.config.ts
- build/Containerfile
- .github/workflows/e2e-tests.yaml
- tests/playwright/src/extension.spec.ts
- README.md
- .github/workflows/release.yaml
⏰ 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). (5)
- GitHub Check: Build / macos-14
- GitHub Check: Build Extension Image
- GitHub Check: Build / ubuntu-24.04
- GitHub Check: Build extension OCI image
- GitHub Check: Build / windows-2022
🔇 Additional comments (3)
pnpm-workspace.yaml (2)
3-3: Good—correctly expands workspace scope for test packages.Adding
tests/*appropriately includes the new test directory in the workspace.
5-8: Clarify why svelte-preprocess and unrs-resolver are added to onlyBuiltDependencies.The addition of these Svelte-related dependencies to
onlyBuiltDependenciesis unclear. While the AI summary mentions alignment with test tooling, the connection to the Playwright E2E test setup is not apparent. Please clarify:
- Why do these dependencies need to prevent hoisting?
- Is this change directly related to the E2E test setup, or is it addressing a separate dependency resolution issue?
- Was this necessary to resolve build or test execution issues?
tests/playwright/src/model/kd-details-page.ts (1)
19-25: KubernetesDashboardDetailsPage wiring looks goodThe page object cleanly extends
ExtensionDetailsPageand scopes to the expected title; no issues from a POM / Playwright perspective.
Signed-off-by: Philippe Martin <[email protected]>
Signed-off-by: Philippe Martin <[email protected]>
|
This PR now only contains the CI workflow to run the tests, the tests themselves have been merged as part of another PR. It's ready for review again |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/e2e-tests.yaml (1)
142-146: Fix the artifact download path (previously flagged).The path contains nested duplication:
tests/playwright/tests/playwright/output/kubernetes-dashboard-tests/plugins/. This was flagged in the previous review and remains unresolved. Remove the extratests/playwright/segment.- name: Download Extension Plugin uses: actions/download-artifact@v6 with: name: extension-plugin - path: tests/playwright/tests/playwright/output/kubernetes-dashboard-tests/plugins/ + path: tests/playwright/output/kubernetes-dashboard-tests/plugins/
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/e2e-tests.yaml(1 hunks)README.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- README.md
⏰ 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). (2)
- GitHub Check: Build Extension Image
- GitHub Check: Build extension OCI image
🔇 Additional comments (1)
.github/workflows/e2e-tests.yaml (1)
96-104: Validate the @podman-desktop/api version extraction.The version extraction using
pnpm listwith jq parsing looks correct, but verify that:
- The command reliably extracts the resolved version from the workspace
- The output path
packages/extension/is correct for your repository structure
Add a workflow to run the e2e tests on CI, when the
ci/e2elabel is setPart of #76