feat(test): Migrate Common.robot to Playwright#22630
feat(test): Migrate Common.robot to Playwright#22630vg006 wants to merge 39 commits intogoharbor:mainfrom
Conversation
Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com> fix: playwright github action Signed-off-by: bupd <bupdprasanth@gmail.com> fix: playwright github action Signed-off-by: bupd <bupdprasanth@gmail.com> fix: rename login logout test Signed-off-by: bupd <bupdprasanth@gmail.com> fix: playwright testing Signed-off-by: bupd <bupdprasanth@gmail.com> feat: correct test Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: vg006 <devvg006@gmail.com>
Signed-off-by: vg006 <devvg006@gmail.com>
Signed-off-by: vg006 <devvg006@gmail.com>
Signed-off-by: vg006 <devvg006@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #22630 +/- ##
===========================================
+ Coverage 45.36% 65.85% +20.48%
===========================================
Files 244 1073 +829
Lines 13333 116095 +102762
Branches 2719 2931 +212
===========================================
+ Hits 6049 76451 +70402
- Misses 6983 35407 +28424
- Partials 301 4237 +3936
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Well done |
There was a problem hiding this comment.
I believe we dont need group1-nightly anymore. since all of tests we have would be just UI. We might not need this one. just /src/portal/e2e/common.spec.ts is enough
There was a problem hiding this comment.
@bupd Okay I will update it directory structure accordingly after successfully migrating all the test cases!
Signed-off-by: vg006 <devvg006@gmail.com>
Signed-off-by: vg006 <devvg006@gmail.com>
Signed-off-by: vg006 <devvg006@gmail.com>
Signed-off-by: vg006 <devvg006@gmail.com>
Signed-off-by: vg006 <devvg006@gmail.com>
Signed-off-by: vg006 <devvg006@gmail.com>
Signed-off-by: vg006 <devvg006@gmail.com>
Signed-off-by: vg006 <devvg006@gmail.com>
Signed-off-by: vg006 <devvg006@gmail.com>
Signed-off-by: vg006 <devvg006@gmail.com>
Signed-off-by: vg006 <devvg006@gmail.com>
Signed-off-by: vg006 <devvg006@gmail.com>
|
Hey @bupd! As described in this test Also I witnessed that this test case was last updated 4 years back during |
|
@vg006 feel free to remove it. its just for trusting the registry's self signed certificate. |
Signed-off-by: vg006 <devvg006@gmail.com>
Signed-off-by: vg006 <devvg006@gmail.com>
Signed-off-by: vg006 <devvg006@gmail.com>
Signed-off-by: vg006 <devvg006@gmail.com>
Signed-off-by: vg006 <devvg006@gmail.com>
Signed-off-by: vg006 <devvg006@gmail.com>
Signed-off-by: vg006 <devvg006@gmail.com>
Signed-off-by: vg006 <devvg006@gmail.com>
Signed-off-by: vg006 <devvg006@gmail.com>
Signed-off-by: vg006 <devvg006@gmail.com>
Signed-off-by: vg006 <devvg006@gmail.com>
Signed-off-by: vg006 <devvg006@gmail.com>
Signed-off-by: vg006 <devvg006@gmail.com>
Signed-off-by: vg006 <devvg006@gmail.com>
Signed-off-by: vg006 <devvg006@gmail.com>
Signed-off-by: vg006 <devvg006@gmail.com>
Signed-off-by: vg006 <devvg006@gmail.com>
Signed-off-by: vg006 <devvg006@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces a Playwright-based E2E test framework for Harbor as a first step in migrating tests from Robot Framework, targeting the Common.robot test suite. It establishes the Playwright infrastructure including configuration, fixtures, utilities, and migrates ~28 test cases.
Changes:
- Sets up Playwright test infrastructure (config, fixtures, shared utils) in
src/portal/e2e/ - Migrates a substantial portion of
Common.robottest cases tocommon.spec.tswith Playwright - Adds a GitHub Actions workflow to run the Playwright tests on pull requests
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/playwright.yml |
New CI workflow to run Playwright tests; uses actions/checkout@v5 (inconsistent with repo's @v6); sets BASE_URL but tests expect HARBOR_BASE_URL |
.gitignore |
Adds Playwright artifact exclusions, but paths are root-anchored and won't match artifacts generated under src/portal/; also has redundant node_modules/ entry |
src/portal/.gitignore |
Correctly excludes Playwright-generated artifacts at the portal directory level |
src/portal/.env.sample |
Provides sample environment variables; missing LOCAL_REGISTRY and LOCAL_REGISTRY_NAMESPACE used extensively in tests |
src/portal/package.json |
Adds dotenv and playwright as runtime dependencies; both should be in devDependencies |
src/portal/playwright.config.ts |
Standard Playwright config using BASE_URL environment variable |
src/portal/e2e/fixtures/harbor.ts |
Defines a Harbor-specific test fixture with login/logout helpers; default admin password hardcoded |
src/portal/e2e/utils.ts |
Shared utility functions for Docker operations and UI interactions; has an unused variable in pushImage and a timeout-bypass bug in waitForProjectInList |
src/portal/e2e/login-logout.spec.ts |
Basic login/logout test with hardcoded credentials |
src/portal/e2e/group1-nightly/common.spec.ts |
Main migration file with 28+ test cases; several tests that modify global settings (read-only mode, project creation policy, system admin) lack cleanup on failure |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "dotenv": "^17.2.3", | ||
| "echarts": "^5.5.1", | ||
| "js-yaml": "^4.1.0", | ||
| "ngx-clipboard": "^15.1.0", | ||
| "ngx-cookie": "^6.0.1", | ||
| "ngx-markdown": "16.0.0", | ||
| "playwright": "^1.54.1", |
There was a problem hiding this comment.
Both dotenv and playwright are placed in dependencies (production dependencies) but they are only used for testing, not for the Angular application itself. They should be moved to devDependencies alongside @playwright/test. Keeping test-only tools in production dependencies unnecessarily increases the installed package size for production deployments.
| @@ -0,0 +1,3 @@ | |||
| HARBOR_USERNAME="<your_admin_username>" # e.g., admin | |||
| HARBOR_PASSWORD="<your_admin_password>" # e.g., Harbor12345 | |||
| HARBOR_BASE_URL="<your_harbor_instance_url>" # e.g., https://harbor.mycompany.com | |||
There was a problem hiding this comment.
The LOCAL_REGISTRY and LOCAL_REGISTRY_NAMESPACE environment variables are used extensively in the tests (referenced over 30 times in common.spec.ts) to pull images from a local registry before pushing to Harbor, but they are not documented in .env.sample. Developers setting up the environment will not know these variables exist or what values to use. Please add them to the sample file (e.g., LOCAL_REGISTRY="docker.io" and LOCAL_REGISTRY_NAMESPACE="library").
| HARBOR_BASE_URL="<your_harbor_instance_url>" # e.g., https://harbor.mycompany.com | |
| HARBOR_BASE_URL="<your_harbor_instance_url>" # e.g., https://harbor.mycompany.com | |
| LOCAL_REGISTRY="docker.io" # e.g., docker.io for the default Docker Hub registry | |
| LOCAL_REGISTRY_NAMESPACE="library" # e.g., library for official Docker Hub images |
| // No more pages, wait a bit and check one more time | ||
| await harborPage.waitForTimeout(500); | ||
| if (await projectLink.isVisible()) { | ||
| if (goto) { | ||
| await projectLink.click(); | ||
| } | ||
| return; | ||
| } | ||
| throw new Error(`Project "${projectName}" not found in project list after checking all pages`); |
There was a problem hiding this comment.
The waitForProjectInList function has a logic bug: when no "Next Page" button is available (i.e., the entire list fits on one page), the function only waits 500ms before throwing an error, completely ignoring the timeout parameter. This is problematic for a newly created project that may take longer to appear in the list, even though no pagination is needed.
The function should implement a retry loop within the single-page case too, continuing to check until the timeout elapses rather than throwing after just one 500ms wait.
| // No more pages, wait a bit and check one more time | |
| await harborPage.waitForTimeout(500); | |
| if (await projectLink.isVisible()) { | |
| if (goto) { | |
| await projectLink.click(); | |
| } | |
| return; | |
| } | |
| throw new Error(`Project "${projectName}" not found in project list after checking all pages`); | |
| // No more pages; wait a bit before retrying within the overall timeout loop | |
| await harborPage.waitForTimeout(500); |
| test('sign-out', async ({ harborPage, harborUser }) => { | ||
| // Sign-out if already signed in | ||
| await harborPage.getByRole('button', { name: harborUser.username, exact: true }).click(); | ||
| await harborPage.getByRole('menuitem', { name: 'Log Out' }).click(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
This test only verifies that a sign-out can be performed, but the harborPage fixture already logs in before the test body executes, and logoutIfPossible automatically logs out after each test. So this test is essentially logging in (via fixture) and then immediately logging out, with no meaningful assertion being validated. It appears to be a carry-over from the Robot Framework test structure which required explicit state management between sequential tests.
This test adds no unique test coverage beyond what the fixture already handles, and could be confusing to future maintainers.
| test('sign-out', async ({ harborPage, harborUser }) => { | |
| // Sign-out if already signed in | |
| await harborPage.getByRole('button', { name: harborUser.username, exact: true }).click(); | |
| await harborPage.getByRole('menuitem', { name: 'Log Out' }).click(); | |
| }); |
| await harborPage.getByRole('button', { name: 'SAVE' }).click(); | ||
| await expect(harborPage.getByText(/system is in read only mode/i)).not.toBeVisible({ timeout: 5000 }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
The cannot copy image in readonly mode test also enables system-wide read-only mode without guaranteed cleanup on failure. If the test fails after enabling read-only mode (line 1404-1406) but before disabling it (line 1435-1437), all subsequent tests that attempt to push images will fail.
This is especially concerning because read-only mode is a global Harbor setting that affects the entire system, not just this test's isolated resources. Consider using test.afterEach or a try/finally block to ensure read-only mode is always disabled.
| test.afterEach(async ({ harborPage }) => { | |
| // Ensure Repository Read Only mode is disabled after each test | |
| await harborPage.getByRole('link', { name: 'Configuration' }).click(); | |
| await harborPage.getByRole('button', { name: 'System Settings' }).click(); | |
| const readOnlyGroup = harborPage | |
| .getByRole('group') | |
| .filter({ hasText: 'Repository Read Only' }); | |
| const readOnlyCheckbox = readOnlyGroup.locator('input[type="checkbox"]'); | |
| if (await readOnlyCheckbox.isChecked()) { | |
| await readOnlyGroup.locator('clr-checkbox-wrapper label').click(); | |
| await harborPage.getByRole('button', { name: 'SAVE' }).click(); | |
| await expect(harborPage.getByText(/system is in read only mode/i)).not.toBeVisible({ timeout: 5000 }); | |
| } | |
| }); |
| const harborTest = base.extend<HarborFixtures>({ | ||
| harborUser: async ({}, use) => { | ||
| await use({ | ||
| username: process.env.HARBOR_USERNAME || 'admin', | ||
| password: process.env.HARBOR_PASSWORD || 'Harbor12345', |
There was a problem hiding this comment.
The default password 'Harbor12345' for the admin user is hardcoded as a fallback in harborUser fixture. While having a default for local development is understandable, the same default password 'Harbor12345' is also hardcoded throughout common.spec.ts for user1 and user2 test accounts (e.g., lines 77, 130, 143, 252, 628, 1016, 1715) without any environment variable fallback for these test accounts. Consider adding HARBOR_TEST_USER_PASSWORD or similar to allow overriding test user passwords per environment.
| const harborTest = base.extend<HarborFixtures>({ | |
| harborUser: async ({}, use) => { | |
| await use({ | |
| username: process.env.HARBOR_USERNAME || 'admin', | |
| password: process.env.HARBOR_PASSWORD || 'Harbor12345', | |
| const harborPassword = | |
| process.env.HARBOR_TEST_USER_PASSWORD || | |
| process.env.HARBOR_PASSWORD || | |
| ''; | |
| const harborTest = base.extend<HarborFixtures>({ | |
| harborUser: async ({}, use) => { | |
| await use({ | |
| username: process.env.HARBOR_USERNAME || 'admin', | |
| password: harborPassword, |
| // Wait for and verify the repo size is displayed (alpine 2.6 is approximately 3.68MiB) | ||
| await expect(harborPage.getByText(/3\.6[0-9]MiB/)).toBeVisible({ timeout: 10000 }); |
There was a problem hiding this comment.
The repo size test asserts that the pushed Alpine image size matches the regex /3\.6[0-9]MiB/ (line 1467). This hardcodes an expected image size for a specific version of Alpine (alpine 2.6 as the comment says, but the code pushes alpine without specifying a version). Different Alpine versions have different sizes, and even the latest tag size changes as the image is updated over time. This assertion will be brittle and may fail spuriously when the Alpine image is updated upstream.
| // Wait for and verify the repo size is displayed (alpine 2.6 is approximately 3.68MiB) | |
| await expect(harborPage.getByText(/3\.6[0-9]MiB/)).toBeVisible({ timeout: 10000 }); | |
| // Wait for and verify that a repository size in MiB is displayed | |
| await expect(harborPage.getByText(/\d+(\.\d+)?MiB/)).toBeVisible({ timeout: 10000 }); |
| echo "TOKEN_PRIVATE_KEY_PATH=${GITHUB_WORKSPACE}/src/github.com/goharbor/harbor/tests/private_key.pem" >> $GITHUB_ENV | ||
| IP=`hostname -I | awk '{print $1}'` | ||
| echo "IP=$IP" >> $GITHUB_ENV | ||
| echo "BASE_URL=https://$IP" >> $GITHUB_ENV |
There was a problem hiding this comment.
The workflow sets BASE_URL (line 63: echo "BASE_URL=https://$IP" >> $GITHUB_ENV), which is what playwright.config.ts uses for the Playwright baseURL configuration. However, the test fixtures (e2e/fixtures/harbor.ts) and many tests in common.spec.ts read from process.env.HARBOR_BASE_URL to construct the Harbor IP address used for Docker commands (e.g., docker login, docker push, docker pull).
Since HARBOR_BASE_URL is never set in the workflow, all those environment variable reads will return undefined, and the Docker commands will fall back to 'localhost' instead of the actual Harbor IP ($IP). This means all tests that push or pull Docker images will fail in CI because Docker commands target localhost instead of the running Harbor instance.
Either rename BASE_URL to HARBOR_BASE_URL in the workflow (and update playwright.config.ts accordingly), or add a separate step to also set HARBOR_BASE_URL=https://$IP.
| echo "BASE_URL=https://$IP" >> $GITHUB_ENV | |
| echo "BASE_URL=https://$IP" >> $GITHUB_ENV | |
| echo "HARBOR_BASE_URL=https://$IP" >> $GITHUB_ENV |
| // login | ||
| await page.goto('/'); | ||
| await page.getByRole('textbox', { name: 'Username' }).click(); | ||
| await page.getByRole('textbox', { name: 'Username' }).fill('admin'); | ||
| await page.getByRole('textbox', { name: 'Password' }).click(); | ||
| await page.getByRole('textbox', { name: 'Password' }).fill('Harbor12345'); | ||
| await page.getByRole('button', { name: 'LOG IN' }).click(); | ||
|
|
||
| // logout | ||
| await page.getByRole('button', { name: 'admin', exact: true }).waitFor(); | ||
| await page.getByRole('button', { name: 'admin', exact: true }).click(); |
There was a problem hiding this comment.
The login-logout.spec.ts test has the admin credentials ('admin' and 'Harbor12345') hardcoded directly in the test. The same credentials are configured via environment variables (HARBOR_USERNAME, HARBOR_PASSWORD, HARBOR_BASE_URL) in the fixture. This test should use the fixture's approach (reading from environment variables) or use the harborPage / harborUser fixture for consistency, rather than hardcoding values that may differ across environments.
| // login | |
| await page.goto('/'); | |
| await page.getByRole('textbox', { name: 'Username' }).click(); | |
| await page.getByRole('textbox', { name: 'Username' }).fill('admin'); | |
| await page.getByRole('textbox', { name: 'Password' }).click(); | |
| await page.getByRole('textbox', { name: 'Password' }).fill('Harbor12345'); | |
| await page.getByRole('button', { name: 'LOG IN' }).click(); | |
| // logout | |
| await page.getByRole('button', { name: 'admin', exact: true }).waitFor(); | |
| await page.getByRole('button', { name: 'admin', exact: true }).click(); | |
| const username = process.env.HARBOR_USERNAME; | |
| const password = process.env.HARBOR_PASSWORD; | |
| const baseUrl = process.env.HARBOR_BASE_URL ?? '/'; | |
| if (!username || !password) { | |
| throw new Error('HARBOR_USERNAME and HARBOR_PASSWORD must be set for login-logout test.'); | |
| } | |
| // login | |
| await page.goto(baseUrl); | |
| await page.getByRole('textbox', { name: 'Username' }).click(); | |
| await page.getByRole('textbox', { name: 'Username' }).fill(username); | |
| await page.getByRole('textbox', { name: 'Password' }).click(); | |
| await page.getByRole('textbox', { name: 'Password' }).fill(password); | |
| await page.getByRole('button', { name: 'LOG IN' }).click(); | |
| // logout | |
| await page.getByRole('button', { name: username, exact: true }).waitFor(); | |
| await page.getByRole('button', { name: username, exact: true }).click(); |
| test('read only mode', async ({ harborPage, harborUser }) => { | ||
| const d = Date.now(); | ||
| const projectName = `project${d}`; | ||
| const image = 'busybox'; | ||
| const tag = 'latest'; | ||
|
|
||
| // Create a new project | ||
| await createProject(harborPage, projectName); | ||
|
|
||
| // Enable Read Only Mode | ||
| await harborPage.getByRole('link', { name: 'Configuration' }).click(); | ||
| await harborPage.getByRole('button', { name: 'System Settings' }).click(); | ||
| await harborPage.getByRole('group').filter({ hasText: 'Repository Read Only' }).locator('clr-checkbox-wrapper label').click(); | ||
| await harborPage.getByRole('button', { name: 'SAVE' }).click(); | ||
|
|
||
| // Wait for save to complete | ||
| await harborPage.waitForTimeout(1000); | ||
|
|
||
| // Try to push image - should fail in read-only mode | ||
| const harborIp = process.env.HARBOR_BASE_URL?.replace(/^https?:\/\//, '') || 'localhost'; | ||
| await cannotPushImage({ | ||
| ip: harborIp, | ||
| user: harborUser.username, | ||
| pwd: harborUser.password, | ||
| project: projectName, | ||
| image: `${image}:${tag}`, | ||
| expectedError: 'denied: The system is in read only mode. Any modification is prohibited.', | ||
| localRegistry: process.env.LOCAL_REGISTRY || 'docker.io', | ||
| localRegistryNamespace: process.env.LOCAL_REGISTRY_NAMESPACE || 'library', | ||
| }); | ||
|
|
||
| // Disable Read Only Mode | ||
| await harborPage.getByRole('link', { name: 'Configuration' }).click(); | ||
| await harborPage.getByRole('button', { name: 'System Settings' }).click(); | ||
| await harborPage.getByRole('group').filter({ hasText: 'Repository Read Only' }).locator('clr-checkbox-wrapper label').click(); | ||
| await harborPage.getByRole('button', { name: 'SAVE' }).click(); | ||
|
|
||
| // Wait for save to complete | ||
| await harborPage.waitForTimeout(1000); | ||
|
|
||
| // Now push should succeed | ||
| await pushImage({ | ||
| ip: harborIp, | ||
| user: harborUser.username, | ||
| pwd: harborUser.password, | ||
| project: projectName, | ||
| imageWithOrWithoutTag: `${image}:${tag}`, | ||
| needPullFirst: true, | ||
| localRegistry: process.env.LOCAL_REGISTRY || 'docker.io', | ||
| localRegistryNamespace: process.env.LOCAL_REGISTRY_NAMESPACE || 'library', | ||
| }); | ||
|
|
||
| // Verify image was pushed successfully | ||
| await harborPage.getByRole('link', { name: 'Projects' }).click(); | ||
| await waitForProjectInList(harborPage, projectName, 15000, true); | ||
| await expect(harborPage.getByRole('link', { name: new RegExp(`${projectName}/${image}`) })).toBeVisible({ timeout: 10000 }); | ||
| }); |
There was a problem hiding this comment.
The read only mode test enables the Harbor system-wide "Repository Read Only" setting but has no guaranteed cleanup if the test fails midway. If the test fails after enabling read-only mode (e.g., at cannotPushImage, pushImage, or the navigation steps), the system will remain in read-only mode, which will cause all subsequent tests that push images to fail.
Similarly for cannot copy image in readonly mode - if it fails after enabling read-only mode (line 1401-1406) but before disabling it (line 1432-1436), subsequent tests will be broken.
Consider using Playwright's test.afterEach hook or a try/finally block to always disable read-only mode, even if the test fails.
Comprehensive Summary of your change
Issue being fixed
Fixes #22134
Summary
This PR aims to migrate tests in
Common.robot. As a first step, few basic testcases are migrated and to track the progress the testcases are listed below.Sign with AdminPush ORAS and DisplayPush CNAB Bundle and DisplayCreate A New ProjectDelete A ProjectRepo SizeStaticsinfoPush ImageProject Level Policy PublicVerify Download Ca LinkEdit Token ExpireCreate A New LabelsUpdate LabelDelete LabelUser View ProjectsUser View LogsManage Project MemberManage project publicityAssign Sys AdminEdit Project CreationEdit Repo InfoDelete Multi ProjectDelete Multi RepoDelete Multi ArtifactsDelete Repo on CardViewDelete Multi MemberProject Admin Operate LabelsProject Admin Add Labels To RepoDeveloper Operate LabelsCopy A ImageCopy A Image And AccessoryCreate An New Project With Quotas SetProject Storage Quotas Dispaly And ControlProject Quotas Control Under CopyTag CRUDTag RetentionTag ImmutabilityProject Level Robot AccountPush Docker Manifest Index and DisplayCan Not Copy Image In ReadOnly ModeRead Only ModeSystem Robot Account Cover All ProjectsSystem Robot AccountGo To Harbor Api PageWASM Push And Pull To HarborCarvel Imgpkg Push And Pull To HarborCosign And Cosign Deployment Security PolicyNotation And Notation Deployment Security PolicyAudit Log And PurgeAudit Log ForwardExport CVEHelm CLI Push And Pull In HarborJob Service Dashboard Job QueuesJob Service Dashboard SchedulesJob Service Dashboard WorkersRetain Image Last Pull TimeBanner MessagePlease indicate you've done the following: