Copilot commented on code in PR #36684: URL: https://github.com/apache/superset/pull/36684#discussion_r2710584398
########## superset-frontend/playwright/components/core/AceEditor.ts: ########## @@ -0,0 +1,152 @@ +/** + * 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'; + +const ACE_EDITOR_SELECTORS = { + TEXT_INPUT: '.ace_text-input', + TEXT_LAYER: '.ace_text-layer', + CONTENT: '.ace_content', + SCROLLER: '.ace_scroller', +} as const; + +/** + * AceEditor component for interacting with Ace Editor instances in Playwright. + * Uses the ace editor API directly for reliable text manipulation. + */ +export class AceEditor { + readonly page: Page; + private readonly selector: string; + private readonly locator: Locator; + + constructor(page: Page, selector: string) { + this.page = page; + this.selector = selector; + this.locator = page.locator(selector); + } + + /** + * Gets the editor element locator + */ + get element(): Locator { + return this.locator; + } + + /** + * Waits for the ace editor to be fully loaded and ready for interaction. + */ + async waitForReady(): Promise<void> { + await this.locator.waitFor({ state: 'visible' }); + await this.locator.locator(ACE_EDITOR_SELECTORS.CONTENT).waitFor(); + await this.locator.locator(ACE_EDITOR_SELECTORS.TEXT_LAYER).waitFor(); + } + + /** + * Sets text in the ace editor using the ace API. + * @param text - The text to set + */ + async setText(text: string): Promise<void> { + await this.waitForReady(); + const editorId = this.extractEditorId(); + await this.page.evaluate( + ({ id, value }) => { + const editor = (window as any).ace.edit(id); + editor.setValue(value, 1); + editor.session.getUndoManager().reset(); + }, + { id: editorId, value: text }, + ); + } + + /** + * Gets the text content from the ace editor. + * @returns The text content + */ + async getText(): Promise<string> { + await this.waitForReady(); + const editorId = this.extractEditorId(); + return this.page.evaluate(id => { + const editor = (window as any).ace.edit(id); Review Comment: The code accesses `(window as any).ace.edit(id)` without checking if the `ace` global exists. If the Ace Editor library fails to load or the editor isn't fully initialized, this will throw a runtime error that may be hard to debug. Consider adding a null check or try-catch with a descriptive error message. ########## superset-frontend/playwright/components/core/AceEditor.ts: ########## @@ -0,0 +1,152 @@ +/** + * 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'; + +const ACE_EDITOR_SELECTORS = { + TEXT_INPUT: '.ace_text-input', + TEXT_LAYER: '.ace_text-layer', + CONTENT: '.ace_content', + SCROLLER: '.ace_scroller', +} as const; + +/** + * AceEditor component for interacting with Ace Editor instances in Playwright. + * Uses the ace editor API directly for reliable text manipulation. + */ +export class AceEditor { + readonly page: Page; + private readonly selector: string; + private readonly locator: Locator; + + constructor(page: Page, selector: string) { + this.page = page; + this.selector = selector; + this.locator = page.locator(selector); + } + + /** + * Gets the editor element locator + */ + get element(): Locator { + return this.locator; + } + + /** + * Waits for the ace editor to be fully loaded and ready for interaction. + */ + async waitForReady(): Promise<void> { + await this.locator.waitFor({ state: 'visible' }); + await this.locator.locator(ACE_EDITOR_SELECTORS.CONTENT).waitFor(); + await this.locator.locator(ACE_EDITOR_SELECTORS.TEXT_LAYER).waitFor(); + } + + /** + * Sets text in the ace editor using the ace API. + * @param text - The text to set + */ + async setText(text: string): Promise<void> { + await this.waitForReady(); + const editorId = this.extractEditorId(); + await this.page.evaluate( + ({ id, value }) => { + const editor = (window as any).ace.edit(id); + editor.setValue(value, 1); + editor.session.getUndoManager().reset(); + }, + { id: editorId, value: text }, + ); + } + + /** + * Gets the text content from the ace editor. + * @returns The text content + */ + async getText(): Promise<string> { + await this.waitForReady(); + const editorId = this.extractEditorId(); + return this.page.evaluate(id => { + const editor = (window as any).ace.edit(id); + return editor.getValue(); + }, editorId); + } + + /** + * Clears the text in the ace editor. + */ + async clear(): Promise<void> { + await this.setText(''); + } + + /** + * Appends text to the existing content in the ace editor. + * @param text - The text to append + */ + async appendText(text: string): Promise<void> { + await this.waitForReady(); + const editorId = this.extractEditorId(); + await this.page.evaluate( + ({ id, value }) => { + const editor = (window as any).ace.edit(id); Review Comment: The code accesses `(window as any).ace.edit(id)` without checking if the `ace` global exists. If the Ace Editor library fails to load or the editor isn't fully initialized, this will throw a runtime error that may be hard to debug. Consider adding a null check or try-catch with a descriptive error message. ########## superset-frontend/playwright/helpers/fixtures/testAssets.ts: ########## @@ -0,0 +1,61 @@ +/** + * 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 base } from '@playwright/test'; +import { apiDeleteDataset } from '../api/dataset'; +import { apiDeleteDatabase } from '../api/database'; + +/** + * Test asset tracker for automatic cleanup after each test. + * Inspired by Cypress's cleanDashboards/cleanCharts pattern. + */ +export interface TestAssets { + trackDataset(id: number): void; + trackDatabase(id: number): void; +} + +export const test = base.extend<{ testAssets: TestAssets }>({ + testAssets: async ({ page }, use) => { + // Use Set to de-dupe IDs (same resource may be tracked multiple times) + const datasetIds = new Set<number>(); + const databaseIds = new Set<number>(); + + await use({ + trackDataset: id => datasetIds.add(id), + trackDatabase: id => databaseIds.add(id), + }); + + // Cleanup: Delete datasets FIRST (they reference databases) + // Then delete databases. Use failOnStatusCode: false for tolerance. + await Promise.all( + [...datasetIds].map(id => + apiDeleteDataset(page, id, { failOnStatusCode: false }).catch(() => {}), + ), + ); + await Promise.all( + [...databaseIds].map(id => + apiDeleteDatabase(page, id, { failOnStatusCode: false }).catch( + () => {}, Review Comment: The error handling here catches all errors and silently ignores them with an empty arrow function. This makes debugging test failures extremely difficult as cleanup failures are swallowed without any logging. Consider at least logging the error to console.warn or test.info() to help diagnose issues during test runs. ########## superset-frontend/playwright/components/core/AceEditor.ts: ########## @@ -0,0 +1,152 @@ +/** + * 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'; + +const ACE_EDITOR_SELECTORS = { + TEXT_INPUT: '.ace_text-input', + TEXT_LAYER: '.ace_text-layer', + CONTENT: '.ace_content', + SCROLLER: '.ace_scroller', +} as const; + +/** + * AceEditor component for interacting with Ace Editor instances in Playwright. + * Uses the ace editor API directly for reliable text manipulation. + */ +export class AceEditor { + readonly page: Page; + private readonly selector: string; + private readonly locator: Locator; + + constructor(page: Page, selector: string) { + this.page = page; + this.selector = selector; + this.locator = page.locator(selector); + } + + /** + * Gets the editor element locator + */ + get element(): Locator { + return this.locator; + } + + /** + * Waits for the ace editor to be fully loaded and ready for interaction. + */ + async waitForReady(): Promise<void> { + await this.locator.waitFor({ state: 'visible' }); + await this.locator.locator(ACE_EDITOR_SELECTORS.CONTENT).waitFor(); + await this.locator.locator(ACE_EDITOR_SELECTORS.TEXT_LAYER).waitFor(); + } + + /** + * Sets text in the ace editor using the ace API. + * @param text - The text to set + */ + async setText(text: string): Promise<void> { + await this.waitForReady(); + const editorId = this.extractEditorId(); + await this.page.evaluate( + ({ id, value }) => { + const editor = (window as any).ace.edit(id); + editor.setValue(value, 1); + editor.session.getUndoManager().reset(); + }, + { id: editorId, value: text }, + ); + } + + /** + * Gets the text content from the ace editor. + * @returns The text content + */ + async getText(): Promise<string> { + await this.waitForReady(); + const editorId = this.extractEditorId(); + return this.page.evaluate(id => { + const editor = (window as any).ace.edit(id); + return editor.getValue(); + }, editorId); + } + + /** + * Clears the text in the ace editor. + */ + async clear(): Promise<void> { + await this.setText(''); + } + + /** + * Appends text to the existing content in the ace editor. + * @param text - The text to append + */ + async appendText(text: string): Promise<void> { + await this.waitForReady(); + const editorId = this.extractEditorId(); + await this.page.evaluate( + ({ id, value }) => { + const editor = (window as any).ace.edit(id); + const currentText = editor.getValue(); + const newText = currentText + (currentText ? '\n' : '') + value; + editor.setValue(newText, 1); + }, + { id: editorId, value: text }, + ); + } + + /** + * Focuses the ace editor. + */ + async focus(): Promise<void> { + await this.waitForReady(); + const editorId = this.extractEditorId(); + await this.page.evaluate(id => { + const editor = (window as any).ace.edit(id); Review Comment: The code accesses `(window as any).ace.edit(id)` without checking if the `ace` global exists. If the Ace Editor library fails to load or the editor isn't fully initialized, this will throw a runtime error that may be hard to debug. Consider adding a null check or try-catch with a descriptive error message. ########## superset-frontend/playwright/components/modals/DuplicateDatasetModal.ts: ########## @@ -55,7 +55,10 @@ export class DuplicateDatasetModal extends Modal { datasetName: string, options?: { timeout?: number; force?: boolean }, ): Promise<void> { - await this.nameInput.fill(datasetName, options); + const input = this.nameInput.element; + // Focus and select all existing text, then fill with new value + await input.click({ clickCount: 3 }); Review Comment: The implementation of `fillDatasetName` now uses triple-click to select all text, which may not work reliably across all browsers and platforms. Triple-click behavior can vary (e.g., it might select the entire line or paragraph in some contexts). Consider using `input.clear()` followed by `input.fill()`, or use `input.selectText()` if available, for more reliable cross-browser behavior. ```suggestion // Clear any existing text, then fill with the new value await input.clear(); ``` ########## superset-frontend/playwright/components/core/AceEditor.ts: ########## @@ -0,0 +1,152 @@ +/** + * 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'; + +const ACE_EDITOR_SELECTORS = { + TEXT_INPUT: '.ace_text-input', + TEXT_LAYER: '.ace_text-layer', + CONTENT: '.ace_content', + SCROLLER: '.ace_scroller', +} as const; + +/** + * AceEditor component for interacting with Ace Editor instances in Playwright. + * Uses the ace editor API directly for reliable text manipulation. + */ +export class AceEditor { + readonly page: Page; + private readonly selector: string; + private readonly locator: Locator; + + constructor(page: Page, selector: string) { + this.page = page; + this.selector = selector; + this.locator = page.locator(selector); + } + + /** + * Gets the editor element locator + */ + get element(): Locator { + return this.locator; + } + + /** + * Waits for the ace editor to be fully loaded and ready for interaction. + */ + async waitForReady(): Promise<void> { + await this.locator.waitFor({ state: 'visible' }); + await this.locator.locator(ACE_EDITOR_SELECTORS.CONTENT).waitFor(); + await this.locator.locator(ACE_EDITOR_SELECTORS.TEXT_LAYER).waitFor(); + } + + /** + * Sets text in the ace editor using the ace API. + * @param text - The text to set + */ + async setText(text: string): Promise<void> { + await this.waitForReady(); + const editorId = this.extractEditorId(); + await this.page.evaluate( + ({ id, value }) => { + const editor = (window as any).ace.edit(id); Review Comment: The code accesses `(window as any).ace.edit(id)` without checking if the `ace` global exists. If the Ace Editor library fails to load or the editor isn't fully initialized, this will throw a runtime error that may be hard to debug. Consider adding a null check or try-catch with a descriptive error message. ########## superset-frontend/playwright/components/core/Select.ts: ########## @@ -0,0 +1,174 @@ +/** + * 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'; + +/** + * Ant Design Select component selectors + */ +const SELECT_SELECTORS = { + DROPDOWN: '.ant-select-dropdown', + OPTION: '.ant-select-item-option', + SEARCH_INPUT: '.ant-select-selection-search-input', + CLEAR: '.ant-select-clear', +} as const; + +/** + * Select component for Ant Design Select/Combobox interactions. + */ +export class Select { + readonly page: Page; + private readonly locator: Locator; + + constructor(page: Page, selector: string); + constructor(page: Page, locator: Locator); + constructor(page: Page, selectorOrLocator: string | Locator) { + this.page = page; + if (typeof selectorOrLocator === 'string') { + this.locator = page.locator(selectorOrLocator); + } else { + this.locator = selectorOrLocator; + } + } + + /** + * Creates a Select from a combobox role with the given accessible name + * @param page - The Playwright page + * @param name - The accessible name (aria-label or placeholder text) + */ + static fromRole(page: Page, name: string): Select { + const locator = page.getByRole('combobox', { name }); + return new Select(page, locator); + } + + /** + * Gets the select element locator + */ + get element(): Locator { + return this.locator; + } + + /** + * Opens the dropdown, types to filter, and selects an option. + * Handles cases where the option may not be initially visible in the dropdown. + * Waits for dropdown to close after selection to avoid stale dropdowns. + * @param optionText - The text of the option to select + */ + async selectOption(optionText: string): Promise<void> { + await this.open(); + await this.type(optionText); + await this.clickOption(optionText); + // Wait for dropdown to close to avoid multiple visible dropdowns + await this.waitForDropdownClose(); + } + + /** + * Waits for dropdown to close after selection + * This prevents strict mode violations when multiple selects are used sequentially + */ + private async waitForDropdownClose(): Promise<void> { + // Wait for dropdown animation to complete + await this.page.waitForTimeout(300); Review Comment: Using arbitrary timeout values like `waitForTimeout(300)` is an anti-pattern in end-to-end testing. This introduces artificial delays and makes tests slower. Consider using Playwright's `waitFor` with a state check or waiting for the dropdown element to be detached/hidden instead of a fixed delay. ```suggestion // Wait for the Ant Design dropdown to actually close (become hidden) await this.page.locator(SELECT_SELECTORS.DROPDOWN).first().waitFor({ state: 'hidden' }); ``` ########## superset-frontend/playwright/components/modals/ConfirmDialog.ts: ########## @@ -0,0 +1,61 @@ +/** + * 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, Locator } 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. + * Uses getByRole with name to target specific confirm dialogs when multiple are open. + */ +export class ConfirmDialog extends Modal { + private readonly specificLocator: Locator; + + constructor(page: Page, dialogName = 'Confirm save') { + super(page); + // Use getByRole with specific name to avoid strict mode violations + // when multiple dialogs are open (e.g., Edit Dataset modal + Confirm save dialog) + this.specificLocator = page.getByRole('dialog', { name: dialogName }); + } + + /** + * Override element getter to use specific locator + */ + override get element(): Locator { + return this.specificLocator; + } + + /** + * Clicks the OK button to confirm + * Waits for element to be stable (animation complete) before clicking + */ + async clickOk(): Promise<void> { + // Wait for modal animation to complete before clicking + await this.page.waitForTimeout(200); Review Comment: Using arbitrary timeout values like `waitForTimeout(200)` is an anti-pattern in end-to-end testing. This creates flakiness and doesn't actually guarantee the modal animation is complete. Consider using Playwright's built-in wait mechanisms like `waitFor({ state: 'attached' })` or waiting for a specific element state instead of a fixed delay. ```suggestion // Wait for modal to be visible (animation complete) before clicking await this.element.waitFor({ state: 'visible' }); ``` ########## superset-frontend/playwright/pages/CreateDatasetPage.ts: ########## @@ -0,0 +1,136 @@ +/** + * 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 { Button, Select } from '../components/core'; + +/** + * Create Dataset Page object for the dataset creation wizard. + */ +export class CreateDatasetPage { + readonly page: Page; + + /** + * Data-test selectors for the create dataset form elements. + * Using data-test attributes avoids strict mode violations with multiple selects. + */ + private static readonly SELECTORS = { + DATABASE: '[data-test="select-database"]', + SCHEMA: '[data-test="Select schema or type to search schemas"]', + TABLE: '[data-test="Select table or type to search tables"]', + } as const; + + constructor(page: Page) { + this.page = page; + } + + /** + * Gets the database selector using data-test attribute + */ + getDatabaseSelect(): Select { + return new Select(this.page, CreateDatasetPage.SELECTORS.DATABASE); + } + + /** + * Gets the schema selector using data-test attribute + */ + getSchemaSelect(): Select { + return new Select(this.page, CreateDatasetPage.SELECTORS.SCHEMA); + } + + /** + * Gets the table selector using data-test attribute + */ + getTableSelect(): Select { + return new Select(this.page, CreateDatasetPage.SELECTORS.TABLE); + } + + /** + * Gets the create and explore button + */ + getCreateAndExploreButton(): Button { + return new Button( + this.page, + this.page.getByRole('button', { name: /Create and explore dataset/i }), + ); + } + + /** + * Navigate to the create dataset page + */ + async goto(): Promise<void> { + await this.page.goto('dataset/add/'); + } + + /** + * Select a database from the dropdown + * @param databaseName - The name of the database to select + */ + async selectDatabase(databaseName: string): Promise<void> { + await this.getDatabaseSelect().selectOption(databaseName); + } + + /** + * Select a schema from the dropdown + * @param schemaName - The name of the schema to select + */ + async selectSchema(schemaName: string): Promise<void> { + await this.getSchemaSelect().selectOption(schemaName); + } + + /** + * Select a table from the dropdown + * @param tableName - The name of the table to select + */ + async selectTable(tableName: string): Promise<void> { + await this.getTableSelect().selectOption(tableName); + } + + /** + * Click the "Create dataset" button (without exploring) + * Uses the dropdown menu to select "Create dataset" option + */ + async clickCreateDataset(): Promise<void> { + // Click the dropdown arrow to open the menu + const dropdownButton = new Button( + this.page, + this.page.locator( + '.ant-dropdown-trigger, .ant-btn-group .ant-btn:last-child', + ), Review Comment: This selector uses a CSS class-based selector `.ant-dropdown-trigger, .ant-btn-group .ant-btn:last-child` which is fragile and may break if Ant Design's class names change in future versions. Consider using more stable selectors like data-test attributes or ARIA roles for better maintainability and resistance to UI framework updates. ```suggestion // Click the dropdown button to open the "Create dataset" menu const dropdownButton = new Button( this.page, this.page.getByRole('button', { name: /Create dataset/i }), ``` ########## superset-frontend/playwright/helpers/fixtures/testAssets.ts: ########## @@ -0,0 +1,61 @@ +/** + * 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 base } from '@playwright/test'; +import { apiDeleteDataset } from '../api/dataset'; +import { apiDeleteDatabase } from '../api/database'; + +/** + * Test asset tracker for automatic cleanup after each test. + * Inspired by Cypress's cleanDashboards/cleanCharts pattern. + */ +export interface TestAssets { + trackDataset(id: number): void; + trackDatabase(id: number): void; +} + +export const test = base.extend<{ testAssets: TestAssets }>({ + testAssets: async ({ page }, use) => { + // Use Set to de-dupe IDs (same resource may be tracked multiple times) + const datasetIds = new Set<number>(); + const databaseIds = new Set<number>(); + + await use({ + trackDataset: id => datasetIds.add(id), + trackDatabase: id => databaseIds.add(id), + }); + + // Cleanup: Delete datasets FIRST (they reference databases) + // Then delete databases. Use failOnStatusCode: false for tolerance. + await Promise.all( + [...datasetIds].map(id => + apiDeleteDataset(page, id, { failOnStatusCode: false }).catch(() => {}), + ), + ); + await Promise.all( + [...databaseIds].map(id => + apiDeleteDatabase(page, id, { failOnStatusCode: false }).catch( + () => {}, + ), Review Comment: The error handling here catches all errors and silently ignores them with an empty arrow function. This makes debugging test failures extremely difficult as cleanup failures are swallowed without any logging. Consider at least logging the error to console.warn or test.info() to help diagnose issues during test runs. ```suggestion apiDeleteDataset(page, id, { failOnStatusCode: false }).catch(error => { console.warn('Failed to delete dataset during test cleanup', { id, error, }); }), ), ); await Promise.all( [...databaseIds].map(id => apiDeleteDatabase(page, id, { failOnStatusCode: false }).catch(error => { console.warn('Failed to delete database during test cleanup', { id, error, }); }), ``` -- 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]
