You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "sadpandajoe (via GitHub)" <gi...@apache.org> on 2026/02/10 22:55:33 UTC

[PR] test(e2e): add Playwright E2E tests for Chart List page [superset]

sadpandajoe opened a new pull request, #37866:
URL: https://github.com/apache/superset/pull/37866

   ### SUMMARY
   
   Adds Playwright E2E tests for the Chart List page, migrating coverage from Cypress. This PR introduces 5 test cases covering chart CRUD operations with both UI and backend API verification.
   
   **New files (5):**
   - `playwright/helpers/api/chart.ts` — Chart API helpers (CRUD + getByName), following the dataset.ts pattern
   - `playwright/pages/ChartListPage.ts` — Page object composing Table + BulkSelect components
   - `playwright/components/modals/ChartPropertiesModal.ts` — Properties edit modal (extends Modal base)
   - `playwright/tests/experimental/chart/chart-test-helpers.ts` — `createTestChart()` factory using members_channels_2 dataset
   - `playwright/tests/experimental/chart/chart-list.spec.ts` — 5 E2E tests
   
   **Modified files (1):**
   - `playwright/helpers/fixtures/testAssets.ts` — Added `trackChart()` with chart cleanup (respects FK order: charts → datasets → databases)
   
   **Tests:**
   | Test | Description |
   |------|-------------|
   | delete chart with confirmation | Single chart delete via action button + DELETE modal |
   | edit chart name via properties modal | Edit name through properties modal, verify via API |
   | export chart as zip file | Single chart export, validates zip structure |
   | bulk delete multiple charts | Bulk select 2 charts, delete, verify 404s |
   | bulk export multiple charts | Bulk select 2 charts, export, validate zip with 2+ charts |
   
   All tests create data via API, perform UI actions, and verify both UI state and backend API responses.
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   N/A — test-only changes
   
   ### TESTING INSTRUCTIONS
   ```bash
   cd superset-frontend
   INCLUDE_EXPERIMENTAL=true npx playwright test playwright/tests/experimental/chart/chart-list.spec.ts
   ```
   Requires a running Superset instance with `--load-examples`.
   
   ### ADDITIONAL INFORMATION
   - [ ] Has associated issue:
   - [ ] Required feature flags:
   - [ ] Changes UI
   - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
     - [ ] Migration is atomic, supports rollback & is backwards-compatible
     - [ ] Confirm DB migration upgrade and downgrade tested
     - [ ] Runtime estimates and downtime expectations provided
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   
   🤖 Generated with [Claude Code](https://claude.com/claude-code)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] test(e2e): add Playwright E2E tests for Chart List page [superset]

Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #37866:
URL: https://github.com/apache/superset/pull/37866#discussion_r2790774399


##########
superset-frontend/playwright/tests/experimental/chart/chart-list.spec.ts:
##########
@@ -0,0 +1,297 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import {
+  test as testWithAssets,
+  expect,
+} from '../../../helpers/fixtures/testAssets';
+import { ChartListPage } from '../../../pages/ChartListPage';
+import { ChartPropertiesModal } from '../../../components/modals/ChartPropertiesModal';
+import { DeleteConfirmationModal } from '../../../components/modals/DeleteConfirmationModal';
+import { Toast } from '../../../components/core/Toast';
+import { apiGetChart, ENDPOINTS } from '../../../helpers/api/chart';
+import { createTestChart } from './chart-test-helpers';
+import { waitForGet } from '../../../helpers/api/intercepts';
+import {
+  expectStatusOneOf,
+  expectValidExportZip,
+} from '../../../helpers/api/assertions';
+
+/**
+ * Extend testWithAssets with chartListPage navigation (beforeEach equivalent).
+ */
+const test = testWithAssets.extend<{ chartListPage: ChartListPage }>({
+  chartListPage: async ({ page }, use) => {
+    const chartListPage = new ChartListPage(page);
+    await chartListPage.goto();
+    await chartListPage.waitForTableLoad();
+    await use(chartListPage);
+  },
+});
+
+test('should delete a chart with confirmation', async ({
+  page,
+  chartListPage,
+  testAssets,
+}) => {
+  // Create throwaway chart for deletion
+  const { id: chartId, name: chartName } = await createTestChart(
+    page,
+    testAssets,
+    test.info(),
+    { prefix: 'test_delete' },
+  );
+
+  // Refresh to see the new chart (created via API)
+  await chartListPage.goto();
+  await chartListPage.waitForTableLoad();
+
+  // Verify chart is visible in list
+  await expect(chartListPage.getChartRow(chartName)).toBeVisible();
+
+  // Click delete action button
+  await chartListPage.clickDeleteAction(chartName);
+
+  // Delete confirmation modal should appear
+  const deleteModal = new DeleteConfirmationModal(page);
+  await deleteModal.waitForVisible();
+
+  // Type "DELETE" to confirm
+  await deleteModal.fillConfirmationInput('DELETE');
+
+  // Click the Delete button
+  await deleteModal.clickDelete();
+
+  // Modal should close
+  await deleteModal.waitForHidden();
+
+  // Verify success toast appears
+  const toast = new Toast(page);
+  await expect(toast.getSuccess()).toBeVisible();
+
+  // Verify chart is removed from list
+  await expect(chartListPage.getChartRow(chartName)).not.toBeVisible();
+
+  // Backend verification: API returns 404
+  await expect
+    .poll(
+      async () => {
+        const response = await apiGetChart(page, chartId, {
+          failOnStatusCode: false,
+        });
+        return response.status();
+      },
+      { timeout: 10000, message: `Chart ${chartId} should return 404` },
+    )
+    .toBe(404);
+});
+
+test('should edit chart name via properties modal', async ({
+  page,
+  chartListPage,
+  testAssets,
+}) => {
+  // Create throwaway chart for editing
+  const { id: chartId, name: chartName } = await createTestChart(
+    page,
+    testAssets,
+    test.info(),
+    { prefix: 'test_edit' },
+  );
+
+  // Refresh to see the new chart
+  await chartListPage.goto();
+  await chartListPage.waitForTableLoad();
+
+  // Verify chart is visible in list
+  await expect(chartListPage.getChartRow(chartName)).toBeVisible();
+
+  // Click edit action to open properties modal
+  await chartListPage.clickEditAction(chartName);
+
+  // Wait for properties modal to be ready
+  const propertiesModal = new ChartPropertiesModal(page);
+  await propertiesModal.waitForReady();
+
+  // Edit the chart name
+  const newName = `renamed_${Date.now()}_${test.info().parallelIndex}`;
+  await propertiesModal.fillName(newName);
+
+  // Click Save button
+  await propertiesModal.clickSave();
+
+  // Modal should close
+  await propertiesModal.waitForHidden();
+
+  // Verify success toast appears
+  const toast = new Toast(page);
+  await expect(toast.getSuccess()).toBeVisible();
+
+  // Backend verification: API returns updated name
+  const response = await apiGetChart(page, chartId);
+  const chart = (await response.json()).result;
+  expect(chart.slice_name).toBe(newName);

Review Comment:
   The chart rename test clicks Save and immediately fetches the chart via `apiGetChart()`. Unlike the dataset equivalent, it never waits for the update (PUT) request to finish, which can cause a race/flaky assertion if the API read happens before the write is committed. Consider adding a `waitForPut(page, `${ENDPOINTS.CHART}${chartId}`)` (or the specific update endpoint used by the modal) and asserting its status (e.g. via `expectStatusOneOf`) before closing the modal / verifying via API (or alternatively poll `apiGetChart()` until `slice_name` matches).



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] test(e2e): add Playwright E2E tests for Chart List page [superset]

Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #37866:
URL: https://github.com/apache/superset/pull/37866#discussion_r2790966369


##########
superset-frontend/playwright/helpers/api/chart.ts:
##########
@@ -0,0 +1,104 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { Page, APIResponse } from '@playwright/test';
+import {
+  apiGet,
+  apiPost,
+  apiDelete,
+  apiPut,
+  ApiRequestOptions,
+} from './requests';
+
+export const ENDPOINTS = {
+  CHART: 'api/v1/chart/',
+  CHART_EXPORT: 'api/v1/chart/export/',
+} as const;
+

Review Comment:
   Good catch — `getChartByName` was removed in round 2 (b273c2d) since it was unused. Updated the PR description to say "CRUD" instead of "CRUD + getByName".



##########
superset-frontend/playwright/helpers/api/assertions.ts:
##########
@@ -59,3 +60,60 @@ export function expectStatusOneOf<T extends ResponseLike>(
   ).toContain(response.status());
   return response;
 }
+
+interface ExportZipOptions {
+  /** Directory name containing resource yaml files (e.g. 'charts', 'datasets') */
+  resourceDir: string;
+  /** Minimum number of resource yaml files expected (default: 1) */
+  minCount?: number;
+  /** Regex to validate Content-Disposition header (skipped if omitted) */
+  contentDispositionPattern?: RegExp;
+  /** Resource names that must each appear in at least one YAML filepath */
+  expectedNames?: string[];
+}
+
+/**
+ * Validate an export zip response: content-type, zip structure, and resource yaml files.
+ * Shared across chart and dataset export tests.
+ */
+export async function expectValidExportZip(
+  response: Response,

Review Comment:
   Agreed — both `Response` and `APIResponse` have compatible `.headers()` and `.body()` signatures. Widened to `ResponseLike` for consistency with `expectStatus*` helpers in the same file.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] test(e2e): add Playwright E2E tests for Chart List page [superset]

Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #37866:
URL: https://github.com/apache/superset/pull/37866#discussion_r2790699389


##########
superset-frontend/playwright/components/modals/ChartPropertiesModal.ts:
##########
@@ -0,0 +1,55 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { Page } from '@playwright/test';
+import { Modal } from '../core';
+
+/**
+ * Chart properties edit modal.
+ * Opened by clicking the edit icon on a chart row in the chart list.
+ * General section is expanded by default (defaultActiveKey="general").
+ */
+export class ChartPropertiesModal extends Modal {
+  private static readonly SELECTORS = {
+    NAME_INPUT: '[data-test="properties-modal-name-input"]',
+  };
+
+  constructor(page: Page) {
+    super(page, '[data-test="properties-edit-modal"]');
+  }
+
+  /**
+   * Fills the chart name input field
+   * @param name - The new chart name
+   */
+  async fillName(name: string): Promise<void> {
+    const input = this.body.locator(
+      ChartPropertiesModal.SELECTORS.NAME_INPUT,
+    );
+    await input.clear();

Review Comment:
   The `.clear()` call is redundant. According to the Input class documentation and Playwright's `.fill()` behavior, `.fill()` already clears the input field before filling. The EditDatasetModal (line 93) and other modals in the codebase call `.fill()` directly without an explicit `.clear()` first.
   ```suggestion
   
   ```



##########
superset-frontend/playwright/tests/experimental/chart/chart-test-helpers.ts:
##########
@@ -0,0 +1,88 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import type { Page, TestInfo } from '@playwright/test';
+import type { TestAssets } from '../../../helpers/fixtures/testAssets';
+import { apiPostChart } from '../../../helpers/api/chart';
+import { getDatasetByName } from '../../../helpers/api/dataset';
+
+interface TestChartResult {
+  id: number;
+  name: string;
+}
+
+interface CreateTestChartOptions {
+  /** Prefix for generated name (default: 'test_chart') */
+  prefix?: string;
+}
+
+/**
+ * Creates a test chart via the API for E2E testing.
+ * Uses the members_channels_2 dataset (loaded via --load-examples).
+ *
+ * @example
+ * const { id, name } = await createTestChart(page, testAssets, test.info());
+ *
+ * @example
+ * const { id, name } = await createTestChart(page, testAssets, test.info(), {
+ *   prefix: 'test_delete',
+ * });
+ */
+export async function createTestChart(
+  page: Page,
+  testAssets: TestAssets,
+  testInfo: TestInfo,
+  options?: CreateTestChartOptions,
+): Promise<TestChartResult> {
+  const prefix = options?.prefix ?? 'test_chart';
+  const name = `${prefix}_${Date.now()}_${testInfo.parallelIndex}`;
+
+  // Look up the members_channels_2 dataset for chart creation
+  const dataset = await getDatasetByName(page, 'members_channels_2');
+  if (!dataset) {
+    throw new Error(
+      'members_channels_2 dataset not found — run Superset with --load-examples',
+    );
+  }
+
+  const response = await apiPostChart(page, {
+    slice_name: name,
+    datasource_id: dataset.id,
+    datasource_type: 'table',
+    viz_type: 'table',
+    params: '{}',
+  });
+
+  if (!response.ok()) {
+    throw new Error(`Failed to create test chart: ${response.status()}`);
+  }
+
+  const body = await response.json();
+  // Chart POST returns { id: number, result: <payload> } — ID is at top level

Review Comment:
   The comment is misleading. The code handles both `body.result.id` and `body.id`, but the comment states "ID is at top level". It should clarify that the ID can be in either location, similar to how dataset.ts line 136-137 documents this with "Handle both response shapes: { id } or { result: { id } }".
   ```suggestion
     // Handle both response shapes: { id } or { result: { id } }
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] test(e2e): add Playwright E2E tests for Chart List page [superset]

Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #37866:
URL: https://github.com/apache/superset/pull/37866#discussion_r2790847449


##########
superset-frontend/playwright/helpers/api/assertions.ts:
##########
@@ -59,3 +60,60 @@ export function expectStatusOneOf<T extends ResponseLike>(
   ).toContain(response.status());
   return response;
 }
+
+interface ExportZipOptions {
+  /** Directory name containing resource yaml files (e.g. 'charts', 'datasets') */
+  resourceDir: string;
+  /** Minimum number of resource yaml files expected (default: 1) */
+  minCount?: number;
+  /** Regex to validate Content-Disposition header (skipped if omitted) */
+  contentDispositionPattern?: RegExp;
+  /** Resource names that must each appear in at least one YAML filepath */
+  expectedNames?: string[];
+}
+
+/**
+ * Validate an export zip response: content-type, zip structure, and resource yaml files.
+ * Shared across chart and dataset export tests.
+ */
+export async function expectValidExportZip(
+  response: Response,

Review Comment:
   `expectValidExportZip` is in the same helper module as `expectStatus*`, which explicitly supports both Playwright `Response` and `APIResponse` (via `ResponseLike`). However, `expectValidExportZip` currently only accepts `Response`, even though it only uses methods that also exist on `APIResponse` (`headers()`/`body()`). Consider widening the parameter type to `ResponseLike` (or `Response | APIResponse`) for consistency and reuse in API-based export tests.
   ```suggestion
     response: ResponseLike,
   ```



##########
superset-frontend/playwright/helpers/api/chart.ts:
##########
@@ -0,0 +1,104 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { Page, APIResponse } from '@playwright/test';
+import {
+  apiGet,
+  apiPost,
+  apiDelete,
+  apiPut,
+  ApiRequestOptions,
+} from './requests';
+
+export const ENDPOINTS = {
+  CHART: 'api/v1/chart/',
+  CHART_EXPORT: 'api/v1/chart/export/',
+} as const;
+

Review Comment:
   PR description mentions the new Chart API helper includes a getByName helper, but this file only adds CRUD endpoints (post/get/delete/put). Either add a `getChartByName()` helper (to match the dataset helper pattern) or update the PR description to avoid misleading future readers.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] test(e2e): add Playwright E2E tests for Chart List page [superset]

Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37866:
URL: https://github.com/apache/superset/pull/37866#issuecomment-3881669334

   AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
   
   <details>
   <summary>
   Bito Usage Guide
   </summary>
   
   **Commands**
         
   Type the following command in the pull request comment and save the comment.
         
   - `/review` - Manually triggers a full AI review.
   
   - `/pause` - Pauses automatic reviews on this pull request.
   - `/resume` - Resumes automatic reviews.
   - `/resolve` - Marks all Bito-posted review comments as resolved.
   - `/abort` - Cancels all in-progress reviews.
   
         
   Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
         
   **Configuration**
   
   This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
         
   **Documentation & Help**
   - <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
   - <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
   - <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
   - <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
   
   </details>
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] test(e2e): add Playwright E2E tests for Chart List page [superset]

Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #37866:
URL: https://github.com/apache/superset/pull/37866#discussion_r2790836545


##########
superset-frontend/playwright/tests/experimental/chart/chart-list.spec.ts:
##########
@@ -0,0 +1,297 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import {
+  test as testWithAssets,
+  expect,
+} from '../../../helpers/fixtures/testAssets';
+import { ChartListPage } from '../../../pages/ChartListPage';
+import { ChartPropertiesModal } from '../../../components/modals/ChartPropertiesModal';
+import { DeleteConfirmationModal } from '../../../components/modals/DeleteConfirmationModal';
+import { Toast } from '../../../components/core/Toast';
+import { apiGetChart, ENDPOINTS } from '../../../helpers/api/chart';
+import { createTestChart } from './chart-test-helpers';
+import { waitForGet } from '../../../helpers/api/intercepts';
+import {
+  expectStatusOneOf,
+  expectValidExportZip,
+} from '../../../helpers/api/assertions';
+
+/**
+ * Extend testWithAssets with chartListPage navigation (beforeEach equivalent).
+ */
+const test = testWithAssets.extend<{ chartListPage: ChartListPage }>({
+  chartListPage: async ({ page }, use) => {
+    const chartListPage = new ChartListPage(page);
+    await chartListPage.goto();
+    await chartListPage.waitForTableLoad();
+    await use(chartListPage);
+  },
+});
+
+test('should delete a chart with confirmation', async ({
+  page,
+  chartListPage,
+  testAssets,
+}) => {
+  // Create throwaway chart for deletion
+  const { id: chartId, name: chartName } = await createTestChart(
+    page,
+    testAssets,
+    test.info(),
+    { prefix: 'test_delete' },
+  );
+
+  // Refresh to see the new chart (created via API)
+  await chartListPage.goto();
+  await chartListPage.waitForTableLoad();
+
+  // Verify chart is visible in list
+  await expect(chartListPage.getChartRow(chartName)).toBeVisible();
+
+  // Click delete action button
+  await chartListPage.clickDeleteAction(chartName);
+
+  // Delete confirmation modal should appear
+  const deleteModal = new DeleteConfirmationModal(page);
+  await deleteModal.waitForVisible();
+
+  // Type "DELETE" to confirm
+  await deleteModal.fillConfirmationInput('DELETE');
+
+  // Click the Delete button
+  await deleteModal.clickDelete();
+
+  // Modal should close
+  await deleteModal.waitForHidden();
+
+  // Verify success toast appears
+  const toast = new Toast(page);
+  await expect(toast.getSuccess()).toBeVisible();
+
+  // Verify chart is removed from list
+  await expect(chartListPage.getChartRow(chartName)).not.toBeVisible();
+
+  // Backend verification: API returns 404
+  await expect
+    .poll(
+      async () => {
+        const response = await apiGetChart(page, chartId, {
+          failOnStatusCode: false,
+        });
+        return response.status();
+      },
+      { timeout: 10000, message: `Chart ${chartId} should return 404` },
+    )
+    .toBe(404);
+});
+
+test('should edit chart name via properties modal', async ({
+  page,
+  chartListPage,
+  testAssets,
+}) => {
+  // Create throwaway chart for editing
+  const { id: chartId, name: chartName } = await createTestChart(
+    page,
+    testAssets,
+    test.info(),
+    { prefix: 'test_edit' },
+  );
+
+  // Refresh to see the new chart
+  await chartListPage.goto();
+  await chartListPage.waitForTableLoad();
+
+  // Verify chart is visible in list
+  await expect(chartListPage.getChartRow(chartName)).toBeVisible();
+
+  // Click edit action to open properties modal
+  await chartListPage.clickEditAction(chartName);
+
+  // Wait for properties modal to be ready
+  const propertiesModal = new ChartPropertiesModal(page);
+  await propertiesModal.waitForReady();
+
+  // Edit the chart name
+  const newName = `renamed_${Date.now()}_${test.info().parallelIndex}`;
+  await propertiesModal.fillName(newName);
+
+  // Click Save button
+  await propertiesModal.clickSave();
+
+  // Modal should close
+  await propertiesModal.waitForHidden();
+
+  // Verify success toast appears
+  const toast = new Toast(page);
+  await expect(toast.getSuccess()).toBeVisible();
+
+  // Backend verification: API returns updated name
+  const response = await apiGetChart(page, chartId);
+  const chart = (await response.json()).result;
+  expect(chart.slice_name).toBe(newName);

Review Comment:
   Addressed in 4e129fd — added `waitForPut` + `expectStatusOneOf` before the `apiGetChart` assertion, matching the dataset edit test pattern.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] test(e2e): add Playwright E2E tests for Chart List page [superset]

Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37866:
URL: https://github.com/apache/superset/pull/37866#issuecomment-3881170159

   AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] test(e2e): add Playwright E2E tests for Chart List page [superset]

Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe merged PR #37866:
URL: https://github.com/apache/superset/pull/37866


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org