Copilot commented on code in PR #36684: URL: https://github.com/apache/superset/pull/36684#discussion_r2624992390
########## superset-frontend/playwright/components/modals/ConfirmDialog.ts: ########## @@ -0,0 +1,46 @@ +/** + * 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/Modal'; + +/** + * Confirm Dialog component for Ant Design Modal.confirm dialogs. + * These are the "OK" / "Cancel" confirmation dialogs used throughout Superset. + */ +export class ConfirmDialog extends Modal { + constructor(page: Page) { + // Modal.confirm uses the same [role="dialog"] selector Review Comment: The ConfirmDialog constructor comment states it uses the same [role="dialog"] selector, but the super() call doesn't pass any parameters, which means it will use the default '[role="dialog"]' selector from the Modal base class. While this works correctly, the comment could be clearer. Consider rephrasing to: "Uses the default Modal selector ([role="dialog"]) which works for Modal.confirm dialogs." ```suggestion // Uses the default Modal selector ([role="dialog"]) which works for Modal.confirm dialogs. ``` ########## superset-frontend/playwright/components/core/Textarea.ts: ########## @@ -0,0 +1,86 @@ +/** + * 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 { Locator, Page } from '@playwright/test'; + +/** + * Textarea component for multi-line text input interactions. + */ +export class Textarea { + readonly page: Page; + private readonly locator: Locator; + + constructor(page: Page, locator: Locator) { + this.page = page; + this.locator = locator; Review Comment: The Textarea component constructor only accepts a Locator, unlike Button and Input which support both string selector and Locator overloads for flexibility. Consider adding constructor overload support to accept either a string selector or Locator, similar to the pattern used in Button and Input components. This would provide consistency across the component library. ```suggestion constructor(page: Page, locatorOrSelector: Locator | string) { this.page = page; if (typeof locatorOrSelector === 'string') { this.locator = page.locator(locatorOrSelector); } else { this.locator = locatorOrSelector; } ``` ########## superset-frontend/playwright/components/core/Select.ts: ########## @@ -0,0 +1,112 @@ +/** + * 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 { Locator, Page } from '@playwright/test'; + +/** + * Select component for Ant Design Select/Combobox interactions. + */ +export class Select { + readonly page: Page; + private readonly locator: Locator; + + constructor(page: Page, locator: Locator) { + this.page = page; + this.locator = locator; Review Comment: The Select component constructor only accepts a Locator, unlike Button and Input which support both string selector and Locator overloads for flexibility. Consider adding constructor overload support to accept either a string selector or Locator, similar to the pattern used in Button and Input components. This would provide consistency across the component library. ```suggestion constructor(page: Page, locatorOrSelector: Locator | string) { this.page = page; if (typeof locatorOrSelector === 'string') { this.locator = page.locator(locatorOrSelector); } else { this.locator = locatorOrSelector; } ``` ########## superset-frontend/playwright/tests/experimental/dataset/dataset-list.spec.ts: ########## @@ -252,3 +255,159 @@ test('should duplicate a dataset with new name', async ({ page }) => { // Name should be different (the duplicate name) expect(duplicateDataFull.result.table_name).toBe(duplicateName); }); + +test('should export a single dataset', async ({ page }) => { + // Use existing example dataset + const datasetName = TEST_DATASETS.EXAMPLE_DATASET; + + // Verify dataset is visible in list + await expect(datasetListPage.getDatasetRow(datasetName)).toBeVisible(); + + // Set up download handler before triggering export + const downloadPromise = page.waitForEvent('download'); + + // Click export action button + await datasetListPage.clickExportAction(datasetName); + + // Wait for download to complete + const download = await downloadPromise; + + // Verify downloaded file is a zip + const fileName = download.suggestedFilename(); + expect(fileName).toMatch(/\.zip$/); + expect(fileName).toContain('dataset'); + + // Verify download completed successfully (no failure) + const failure = await download.failure(); + expect(failure).toBeNull(); +}); + +test('should bulk export multiple datasets', async ({ page }) => { + // Use existing example dataset for bulk export + const datasetName = TEST_DATASETS.EXAMPLE_DATASET; + + // Verify dataset is visible in list + await expect(datasetListPage.getDatasetRow(datasetName)).toBeVisible(); + + // Enable bulk select mode + await datasetListPage.clickBulkSelectButton(); + + // Verify bulk select controls appear + await expect(datasetListPage.getBulkSelectControls()).toBeVisible(); + + // Select the dataset checkbox + await datasetListPage.selectDatasetCheckbox(datasetName); + + // Set up download handler before triggering bulk export + const downloadPromise = page.waitForEvent('download'); + + // Click Export bulk action + await datasetListPage.clickBulkAction('Export'); + + // Wait for download to complete + const download = await downloadPromise; + + // Verify downloaded file is a zip + const fileName = download.suggestedFilename(); + expect(fileName).toMatch(/\.zip$/); + expect(fileName).toContain('dataset'); + + // Verify download completed successfully (no failure) + const failure = await download.failure(); + expect(failure).toBeNull(); +}); + +test('should navigate the create dataset wizard', async ({ page }) => { + const createDatasetPage = new CreateDatasetPage(page); + + // Click the "+ Dataset" button to navigate to create page + await page.getByRole('button', { name: 'Dataset' }).click(); + + // Wait for create dataset page to load + await createDatasetPage.waitForPageLoad(); + + // Verify we're on the create dataset page + await expect(page).toHaveURL(/dataset\/add/); + + // Verify database select is visible and functional + const databaseSelect = createDatasetPage.getDatabaseSelect(); + await expect(databaseSelect.element).toBeVisible(); + + // Select the examples database + await createDatasetPage.selectDatabase('examples'); + + // Verify schema select appears after database selection + const schemaSelect = createDatasetPage.getSchemaSelect(); + await expect(schemaSelect.element).toBeVisible(); + + // Select the main schema + await createDatasetPage.selectSchema('main'); + + // Verify table select appears after schema selection + const tableSelect = createDatasetPage.getTableSelect(); + await expect(tableSelect.element).toBeVisible(); + + // Open table dropdown and verify options are available + await tableSelect.open(); + const tableOptions = page.locator('.ant-select-item-option'); + await expect(tableOptions.first()).toBeVisible(); + + // Close dropdown + await tableSelect.close(); + + // Verify the create and explore button is visible + await expect( + createDatasetPage.getCreateAndExploreButton().element, + ).toBeVisible(); +}); + +test('should edit a dataset description', async ({ page }) => { + const editModal = new EditDatasetModal(page); + const confirmDialog = new ConfirmDialog(page); + const toast = new Toast(page); + + // Use existing example dataset + const datasetName = TEST_DATASETS.EXAMPLE_DATASET; + + // Verify dataset is visible in list + await expect(datasetListPage.getDatasetRow(datasetName)).toBeVisible(); + + // Click edit action button + await datasetListPage.clickEditAction(datasetName); + + // Edit modal should appear + await editModal.waitForVisible(); + + // Generate unique description to verify edit works + const newDescription = `Test description updated at ${Date.now()}`; + + // Fill in new description + await editModal.fillDescription(newDescription); + + // Click Save button + await editModal.clickSave(); + + // Confirm the save in the confirmation dialog + await confirmDialog.waitForVisible(); + await confirmDialog.clickOk(); + + // Modal should close + await editModal.waitForHidden(); + + // Verify success toast appears + const successToast = toast.getSuccess(); + await expect(successToast).toBeVisible(); + + // Verify the description was saved by re-opening the edit modal + await datasetListPage.clickEditAction(datasetName); + await editModal.waitForVisible(); + + // Verify description textarea contains our new description + await expect(editModal.getDescriptionTextarea().element).toHaveValue( + newDescription, + ); + + // Close modal without saving + await editModal.clickCancel(); + await editModal.waitForHidden(); +}); Review Comment: The edit dataset test modifies the description of an example dataset (members_channels_2) which is shared across all tests. This could cause test flakiness or failures in parallel execution since other tests might depend on the original state. Consider creating a dedicated test dataset in the beforeEach hook for this test or reverting the description change after verification to ensure test isolation. ########## superset-frontend/playwright/components/core/Checkbox.ts: ########## @@ -0,0 +1,80 @@ +/** + * 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 { Locator } from '@playwright/test'; + +/** + * Checkbox component for checkbox interactions. + */ +export class Checkbox { + private readonly locator: Locator; + + constructor(locator: Locator) { Review Comment: The Checkbox component should store a reference to the Page object for consistency with other core components (Button, Input, Form, etc.). While not strictly necessary for the current implementation, storing the page reference maintains consistency and allows for future extensibility. Consider adding `readonly page: Page` as a class property and accepting it in the constructor. ```suggestion import { Locator, Page } from '@playwright/test'; /** * Checkbox component for checkbox interactions. */ export class Checkbox { readonly page: Page; private readonly locator: Locator; constructor(page: Page, locator: Locator) { this.page = page; ``` ########## superset-frontend/playwright/components/modals/EditDatasetModal.ts: ########## @@ -0,0 +1,71 @@ +/** + * 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, Tabs, Textarea } from '../core'; + +/** + * Edit Dataset Modal component (DatasourceModal). + * Used for editing dataset properties like description, metrics, columns, etc. + */ +export class EditDatasetModal extends Modal { + private readonly tabs: Tabs; + + constructor(page: Page) { + super(page, 'Edit Dataset'); Review Comment: The EditDatasetModal constructor is passing 'Edit Dataset' as the modalSelector parameter to the base Modal class. However, the Modal base class expects a CSS selector (default: '[role="dialog"]'), not a title string. This will cause the modal element selection to fail because there is no HTML element with selector 'Edit Dataset'. The constructor should either pass no parameter to use the default '[role="dialog"]' selector, or pass a valid CSS selector that identifies this specific modal. ```suggestion super(page); ``` -- 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]
