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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
