Add E2E tests for Override Policy/Namespace level in the dashboard #268

SunsetB612 · 2025-09-12T02:59:23Z

No description provided.

karmada-bot · 2025-09-12T02:59:29Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign samzong for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

gemini-code-assist

Summary of Changes

Hello @SunsetB612, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request streamlines the dashboard's E2E test suite by removing several specific namespace-related tests and repurposing a general list test to focus on CronJobs. It also introduces the Kubernetes client for Node.js, indicating a potential shift towards more robust or direct interaction with Kubernetes resources in the dashboard's testing infrastructure or application logic. The changes appear to be a refactoring and update of the E2E testing framework rather than adding new tests for "Override Policy/Namespace level" as suggested by the title.

Highlights

Removed Namespace E2E Tests: Several existing Playwright E2E tests for namespace creation, deletion, and network error handling have been removed from the dashboard's test suite.
Refactored List E2E Test: The namespace-list.spec.ts file has been renamed to cronjob-list.spec.ts and its content updated to test the display of CronJob lists instead of namespace lists.
Dependency Addition: The @kubernetes/client-node package has been added as a dependency, along with its transitive dependencies, suggesting preparation for more direct Kubernetes API interactions within the dashboard's testing or functionality.
Dependency Updates: Various other dependencies in pnpm-lock.yaml have been updated, including @types/node.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request has a misleading title. It states 'Add E2E tests for Override Policy/Namespace level in the dashboard', but the changes primarily involve removing several E2E tests for namespaces and adding a new test for CronJob listing. Removing tests for existing functionality is a significant regression in test coverage. Please clarify the intent of this PR and update the title to accurately reflect the changes. If the namespace tests are no longer needed, an explanation would be beneficial.

I have also left a comment on the new CronJob test file to improve its maintainability.

gemini-code-assist · 2025-09-12T03:01:24Z

ui/apps/dashboard/e2e/workload/cronjob/cronjob-list.spec.ts

+    // 点击可见的 Statefulset tab
+    const statefulsetTab = page.locator('role=option[name="Cronjob"]');
+    await statefulsetTab.waitFor({ state: 'visible', timeout: 30000 });
+    await statefulsetTab.click();
+
+    // 验证选中状态
+    await expect(statefulsetTab).toHaveAttribute('aria-selected', 'true');
+
+
+    // 验证 StatefulSet 列表表格可见
    const table = page.locator('table');
    await expect(table).toBeVisible({ timeout: 30000 });


The variable names and comments in this test are inconsistent with the functionality being tested. The test is for 'Cronjob', but the variable is named statefulsetTab and comments refer to 'StatefulSet'. This can be confusing for future maintenance. Please update them to refer to 'Cronjob'.

Suggested change

// 点击可见的 Statefulset tab

const statefulsetTab = page.locator('role=option[name="Cronjob"]');

await statefulsetTab.waitFor({ state: 'visible', timeout: 30000 });

await statefulsetTab.click();

// 验证选中状态

await expect(statefulsetTab).toHaveAttribute('aria-selected', 'true');

// 验证 StatefulSet 列表表格可见

const table = page.locator('table');

await expect(table).toBeVisible({ timeout: 30000 });

// 点击可见的 Cronjob tab

const cronjobTab = page.locator('role=option[name="Cronjob"]');

await cronjobTab.waitFor({ state: 'visible', timeout: 30000 });

await cronjobTab.click();

// 验证选中状态

await expect(cronjobTab).toHaveAttribute('aria-selected', 'true');

// 验证 Cronjob 列表表格可见

const table = page.locator('table');

await expect(table).toBeVisible({ timeout: 30000 });

SunsetB612 · 2025-09-12T09:13:28Z

/hold

SunsetB612 · 2025-09-19T12:12:04Z

/unhold

XiShanYongYe-Chang · 2025-10-21T13:13:31Z

/gemini-review

gemini-code-assist · 2025-10-21T13:13:43Z

It looks like you tried to invoke a command, but /gemini-review is not a recognized command. If you intended to request a code review, please use the command /gemini review.

XiShanYongYe-Chang · 2025-10-21T13:14:03Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a comprehensive suite of E2E tests for the namespace-level Override Policy feature, covering create, list, view, edit, and delete functionalities. The implementation is solid, particularly the use of utility functions in test-utils.ts for managing test data and authentication. However, I've identified several opportunities to enhance the tests' robustness, correctness, and maintainability. My feedback focuses on improving test assertions, removing hardcoded values, and streamlining test logic for better reliability and clarity.

gemini-code-assist · 2025-10-21T13:15:59Z

ui/apps/dashboard/e2e/policy/overridepolicy/namespace/overridepolicy-namespace-create.spec.ts

+    try {
+        await expect(page.locator('table').locator(`text=${overridePolicyName}`)).toBeVisible({
+            timeout: 15000
+        });
+    } catch {
+        // If not shown immediately in list, may be due to cache or refresh delay
+        // But API success indicates overridepolicy was created
+    }


The try...catch block to verify the new policy's appearance in the list is problematic as it swallows assertion failures. This could result in a test passing even if the UI doesn't update correctly. A more reliable approach is to ensure the UI is in a consistent state (e.g., by reloading the page) and then assert the element's visibility directly.

// To ensure the list is updated, we can reload the page. await page.reload({ waitUntil: 'networkidle' }); await expect(page.locator('table').locator(`text=${overridePolicyName}`)).toBeVisible({ timeout: 15000 });

gemini-code-assist · 2025-10-21T13:15:59Z

ui/apps/dashboard/e2e/policy/overridepolicy/namespace/overridepolicy-namespace-edit.spec.ts

+    // Wait for edit success message or dialog to close
+    try {
+        // Try waiting for success message
+        await expect(page.locator('text=Updated')).toBeVisible({ timeout: 3000 });
+    } catch (e) {
+        try {
+            // If no success message, wait for dialog to close
+            await page.waitForSelector('[role="dialog"]', { state: 'detached', timeout: 3000 });
+        } catch (e2) {
+            // If dialog close also failed, check if page still exists
+            try {
+                const isPageActive = await page.evaluate(() => document.readyState);
+
+                if (isPageActive === 'complete') {
+                    // Edit operation may have succeeded
+                }
+            } catch (e3) {
+                // Page appears to be closed or crashed
+            }
+        }
+    }


This test successfully submits an edit, but it doesn't verify that the changes were actually persisted. The complex try-catch block only confirms that the submission process completes without error. To make the test more valuable, it should include a final step to verify the update, for example, by re-opening the 'View' or 'Edit' dialog and asserting that the content reflects the modifications.

gemini-code-assist · 2025-10-21T13:15:59Z

ui/apps/dashboard/e2e/policy/overridepolicy/namespace/overridepolicy-namespace-view.spec.ts

+    await viewButton.click();
+
+    // Verify details page is displayed
+    await page.waitForLoadState('networkidle');


The test navigates to the details page but only waits for the network to be idle. It doesn't verify that the correct policy's details are displayed. You should add an assertion to confirm that content specific to the test policy (like its name) is visible on the page.

await page.waitForLoadState('networkidle'); // Verify that the details page for the correct policy is displayed by checking for its name. await expect(page.locator(`text=${overridePolicyName}`).first()).toBeVisible();

gemini-code-assist · 2025-10-21T13:15:59Z

ui/apps/dashboard/e2e/policy/overridepolicy/namespace/overridepolicy-namespace-create.spec.ts

+    await expect(page.locator('[role="dialog"] button:has-text("确 定")')).toBeEnabled();
+    await page.click('[role="dialog"] button:has-text("确 定")');


The selector for the submit button uses a hardcoded Chinese string 确定, which appears to contain a typo (an extra space) and makes the test brittle against language changes. The actual button text is 确定.

It's best practice to use a more flexible selector, such as a regular expression that can match the text in different languages.

Suggested change

await expect(page.locator('[role="dialog"] button:has-text("确定")')).toBeEnabled();

await page.click('[role="dialog"] button:has-text("确定")');

await expect(page.locator('[role="dialog"] button').filter({ hasText: /^(确定|Confirm|OK)$/ })).toBeEnabled();

await page.locator('[role="dialog"] button').filter({ hasText: /^(确定|Confirm|OK)$/ }).click();

gemini-code-assist · 2025-10-21T13:15:59Z

ui/apps/dashboard/e2e/policy/overridepolicy/namespace/overridepolicy-namespace-delete.spec.ts

+    await page.reload();
+    await page.click('text=Policies');


After reloading the page, the test should still be on the override policy list. The subsequent page.click('text=Policies') is unnecessary and makes the test's intent less clear. It's better to remove it and add a waitUntil: 'networkidle' to the reload for added robustness.

Suggested change

await page.reload();

await page.click('text=Policies');

await page.reload({ waitUntil: 'networkidle' });

gemini-code-assist · 2025-10-21T13:15:59Z

ui/apps/dashboard/e2e/policy/overridepolicy/namespace/overridepolicy-namespace-edit.spec.ts

+    await expect(page.locator('[role="dialog"] button:has-text("确 定")')).toBeEnabled();
+    await page.click('[role="dialog"] button:has-text("确 定")');


Similar to the create test, the selector for the submit button uses a hardcoded Chinese string 确定 which has a typo and is not resilient to language changes. The correct text is 确定.

Using a regular expression is a more robust approach.

Suggested change

await expect(page.locator('[role="dialog"] button:has-text("确定")')).toBeEnabled();

await page.click('[role="dialog"] button:has-text("确定")');

await expect(page.locator('[role="dialog"] button').filter({ hasText: /^(确定|Confirm|OK)$/ })).toBeEnabled();

await page.locator('[role="dialog"] button').filter({ hasText: /^(确定|Confirm|OK)$/ }).click();

gemini-code-assist · 2025-10-21T13:16:00Z

ui/apps/dashboard/e2e/policy/overridepolicy/namespace/overridepolicy-namespace-list.spec.ts

+limitations under the License.
+*/
+
+// apps/dashboard/e2e/overridepolicy-namespace-list.spec.ts


The file path in this comment is incorrect. Updating it will improve code clarity and maintainability.

Suggested change

// apps/dashboard/e2e/overridepolicy-namespace-list.spec.ts

// ui/apps/dashboard/e2e/policy/overridepolicy/namespace/overridepolicy-namespace-list.spec.ts

gemini-code-assist · 2025-10-21T13:16:00Z

ui/apps/dashboard/e2e/policy/overridepolicy/namespace/test-utils.ts

+    await page.goto(`${baseURL}/login`, { waitUntil: 'networkidle' });
+    await page.evaluate((t) => localStorage.setItem('token', t), token);
+    await page.goto(`${baseURL}${basePath}`, { waitUntil: 'networkidle' });
+    await page.evaluate((t) => localStorage.setItem('token', t), token);


The token is being set in localStorage twice. The first call on line 87 is sufficient after navigating to the login page to establish the origin. This second call is redundant and can be removed to simplify the code.

Signed-off-by: SunsetB612 <[email protected]>

karmada-bot requested review from devadapter and jhnine September 12, 2025 02:59

karmada-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 12, 2025

gemini-code-assist bot reviewed Sep 12, 2025

View reviewed changes

karmada-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 12, 2025

SunsetB612 force-pushed the add-e2e-overridepolicy-namespace branch from cb9b1b9 to 0151a88 Compare September 19, 2025 09:45

karmada-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 19, 2025

SunsetB612 force-pushed the add-e2e-overridepolicy-namespace branch 3 times, most recently from a4cde2e to 2a9d52b Compare September 19, 2025 10:40

karmada-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 19, 2025

SunsetB612 force-pushed the add-e2e-overridepolicy-namespace branch 5 times, most recently from c253a80 to c9a61da Compare September 19, 2025 12:11

karmada-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 19, 2025

XiShanYongYe-Chang mentioned this pull request Oct 21, 2025

[Umbrella][Summer OSPP 2025] Introduce E2E for the Karmada Dashboard #261

Open

18 tasks

gemini-code-assist bot reviewed Oct 21, 2025

View reviewed changes

Add E2E tests for Override Policy/Namespace level in the dashboard

42f616d

Signed-off-by: SunsetB612 <[email protected]>

SunsetB612 force-pushed the add-e2e-overridepolicy-namespace branch from c9a61da to 42f616d Compare October 23, 2025 18:15

karmada-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 23, 2025

		await expect(page.locator('[role="dialog"] button:has-text("确定")')).toBeEnabled();
		await page.click('[role="dialog"] button:has-text("确定")');

	await page.reload();
	await page.click('text=Policies');
	await page.reload({ waitUntil: 'networkidle' });

	// apps/dashboard/e2e/overridepolicy-namespace-list.spec.ts
	// ui/apps/dashboard/e2e/policy/overridepolicy/namespace/overridepolicy-namespace-list.spec.ts

Add E2E tests for Override Policy/Namespace level in the dashboard #268

Are you sure you want to change the base?

Add E2E tests for Override Policy/Namespace level in the dashboard #268

Conversation

SunsetB612 commented Sep 12, 2025

Uh oh!

karmada-bot commented Sep 12, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

SunsetB612 commented Sep 12, 2025

Uh oh!

SunsetB612 commented Sep 19, 2025

Uh oh!

XiShanYongYe-Chang commented Oct 21, 2025

Uh oh!

gemini-code-assist bot commented Oct 21, 2025

Uh oh!

XiShanYongYe-Chang commented Oct 21, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Oct 21, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Oct 21, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Oct 21, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Oct 21, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Oct 21, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Oct 21, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Oct 21, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Oct 21, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants