codeant-ai-for-open-source[bot] commented on code in PR #36684: URL: https://github.com/apache/superset/pull/36684#discussion_r2761260188
########## superset-frontend/playwright/components/modals/EditDatasetModal.ts: ########## @@ -0,0 +1,145 @@ +/** + * 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'; +import { AceEditor, Input, Modal, Tabs } from '../core'; + +/** + * Edit Dataset Modal component (DatasourceModal). + * Used for editing dataset properties like description, metrics, columns, etc. + * Uses specific dialog name to avoid strict mode violations when multiple dialogs are open. + */ +export class EditDatasetModal extends Modal { + private static readonly SELECTORS = { + NAME_INPUT: '[data-test="inline-name"]', + LOCK_ICON: '[data-test="lock"]', + UNLOCK_ICON: '[data-test="unlock"]', + }; + + private readonly tabs: Tabs; + private readonly specificLocator: Locator; + + constructor(page: Page) { + super(page); + this.tabs = new Tabs(page); + // Use getByRole with specific name to target Edit Dataset dialog + // The dialog has aria-labelledby that resolves to "edit Edit Dataset" + this.specificLocator = page.getByRole('dialog', { name: /edit.*dataset/i }); Review Comment: **Suggestion:** The generic tab-click helper is backed by a `Tabs` instance created against the whole page, so `clickTab` can accidentally click a tab with the same name outside the dataset modal, making tests flaky whenever another tab list (for example, on the underlying page) exposes a tab with the same label. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Modal tab navigation tests click wrong tab. - ⚠️ Settings tab interactions become flaky. - ⚠️ Tests relying on modal-scoped editors break. ``` </details> ```suggestion // Use getByRole with specific name to target Edit Dataset dialog // The dialog has aria-labelledby that resolves to "edit Edit Dataset" this.specificLocator = page.getByRole('dialog', { name: /edit.*dataset/i }); this.tabs = new Tabs(page, this.element.getByRole('tablist')); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open an application page where a global tablist is present beneath the modal (common in dataset list pages). Playwright test opens Edit modal and instantiates EditDatasetModal from `superset-frontend/playwright/components/modals/EditDatasetModal.ts` which sets `this.tabs = new Tabs(page)` at `...:35-44`. 2. Run a test step that calls `editModal.clickTab('Settings')` (calls Tabs.clickTab backed by an instance created with the whole page). If another tablist with a "Settings" tab exists outside the modal (for example the underlying page or a different component), `Tabs.clickTab` may find and click that external tab instead of the modal's tab; the click target will be outside the modal and the test will not reach the modal Settings panel. 3. Reproduce concretely by creating a Playwright test that first renders a page-level tablist with a "Settings" tab, opens the Edit modal, then calls `editModal.clickSettingsTab()` / `editModal.clickTab('Settings')`. The test will either switch the wrong tab or fail to reveal the modal's Settings panel; traceable to `EditDatasetModal.ts:35-44` where Tabs was created unscoped. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/playwright/components/modals/EditDatasetModal.ts **Line:** 40:43 **Comment:** *Possible Bug: The generic tab-click helper is backed by a `Tabs` instance created against the whole page, so `clickTab` can accidentally click a tab with the same name outside the dataset modal, making tests flaky whenever another tab list (for example, on the underlying page) exposes a tab with the same label. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset-frontend/playwright/components/core/AceEditor.ts: ########## @@ -0,0 +1,144 @@ +/** + * 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 locator: Locator; + + constructor(page: Page, selectorOrLocator: string | Locator) { + this.page = page; + if (typeof selectorOrLocator === 'string') { + this.locator = page.locator(selectorOrLocator); + } else { + this.locator = selectorOrLocator; + } + } + + /** + * Gets the editor element locator + */ + get element(): Locator { + return this.locator; + } + + /** + * Waits for the ace editor to be fully loaded and ready for interaction. + * Also waits for the ace library to be available on window. + */ + 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(); + // Wait for ace library to be loaded + await this.page.waitForFunction( + () => + typeof (window as any).ace !== 'undefined' && + typeof (window as any).ace.edit === 'function', + ); + } + + /** + * Sets text in the ace editor using the ace API. + * Uses element handle to target the specific editor (avoids global ID lookup bug). + * @param text - The text to set + */ + async setText(text: string): Promise<void> { + await this.waitForReady(); + const elementHandle = await this.locator.elementHandle(); + await this.page.evaluate( + ({ element, value }) => { + const editor = (window as any).ace.edit(element); + editor.setValue(value, 1); + editor.session.getUndoManager().reset(); + }, + { element: elementHandle, value: text }, Review Comment: **Suggestion:** In `setText`, the element handle is passed to `page.evaluate` nested inside an object, which Playwright does not support for JSHandles and will cause a runtime error about non-serializable handles; the element handle should be passed as a top-level argument instead. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Playwright tests using AceEditor.setText fail immediately. - ⚠️ Dataset/SQL editor E2E flows blocked by failing tests. ``` </details> ```suggestion (element, value) => { const editor = (window as any).ace.edit(element); editor.setValue(value, 1); editor.session.getUndoManager().reset(); }, elementHandle, text, ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run Playwright tests that use AceEditor.setText (file superset-frontend/playwright/components/core/AceEditor.ts). 2. The test calls AceEditor.setText() which executes lines 74-85: it obtains an element handle at line 76 and calls page.evaluate(..., { element: elementHandle, value: text }) at line 77-84. 3. Playwright attempts to serialize the second argument (the object) to the browser; JSHandles (elementHandle) cannot be nested inside arbitrary objects for page.evaluate and will cause a runtime error like "JSHandle is not serializable" or "Argument is not transferable". 4. The evaluate callback is never executed in the browser and the test fails immediately with a Playwright serialization error referencing the evaluate call in AceEditor.setText at the lines above. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/playwright/components/core/AceEditor.ts **Line:** 78:83 **Comment:** *Logic Error: In `setText`, the element handle is passed to `page.evaluate` nested inside an object, which Playwright does not support for JSHandles and will cause a runtime error about non-serializable handles; the element handle should be passed as a top-level argument instead. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset-frontend/playwright/components/core/AceEditor.ts: ########## @@ -0,0 +1,144 @@ +/** + * 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 locator: Locator; + + constructor(page: Page, selectorOrLocator: string | Locator) { + this.page = page; + if (typeof selectorOrLocator === 'string') { + this.locator = page.locator(selectorOrLocator); + } else { + this.locator = selectorOrLocator; + } + } + + /** + * Gets the editor element locator + */ + get element(): Locator { + return this.locator; + } + + /** + * Waits for the ace editor to be fully loaded and ready for interaction. + * Also waits for the ace library to be available on window. + */ + 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(); + // Wait for ace library to be loaded + await this.page.waitForFunction( + () => + typeof (window as any).ace !== 'undefined' && + typeof (window as any).ace.edit === 'function', + ); + } + + /** + * Sets text in the ace editor using the ace API. + * Uses element handle to target the specific editor (avoids global ID lookup bug). + * @param text - The text to set + */ + async setText(text: string): Promise<void> { + await this.waitForReady(); + const elementHandle = await this.locator.elementHandle(); + await this.page.evaluate( + ({ element, value }) => { + const editor = (window as any).ace.edit(element); + editor.setValue(value, 1); + editor.session.getUndoManager().reset(); + }, + { element: elementHandle, value: text }, + ); + } + + /** + * Gets the text content from the ace editor. + * Uses element handle to target the specific editor. + * @returns The text content + */ + async getText(): Promise<string> { + await this.waitForReady(); + const elementHandle = await this.locator.elementHandle(); + return this.page.evaluate(element => { Review Comment: **Suggestion:** In `getText`, `locator.elementHandle()` can return `null` (for example if the editor gets detached between `waitForReady` and the call), which will result in `ace.edit(null)` and a hard-to-diagnose runtime error; explicitly checking for a missing element and failing with a clear error avoids this. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ E2E tests may fail with unclear editor null errors. - ⚠️ Debugging takes longer for intermittent flakiness. ``` </details> ```suggestion if (!elementHandle) { throw new Error('Ace editor element not found while getting text'); } ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run a Playwright test that calls AceEditor.getText (superset-frontend/playwright/components/core/AceEditor.ts). 2. getText awaits waitForReady() at line 93, then calls locator.elementHandle() at line 94 to obtain the element handle. 3. If the editor DOM is detached between waitForReady() and elementHandle() (a realistic race in E2E tests), elementHandle will be null and page.evaluate is invoked with null at line 95-98, causing ace.edit(null) in the browser and a confusing runtime failure. 4. Adding an explicit null check (as in improved_code) yields a clear error immediately and points to the failing locator, making test debugging straightforward. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/playwright/components/core/AceEditor.ts **Line:** 95:95 **Comment:** *Possible Bug: In `getText`, `locator.elementHandle()` can return `null` (for example if the editor gets detached between `waitForReady` and the call), which will result in `ace.edit(null)` and a hard-to-diagnose runtime error; explicitly checking for a missing element and failing with a clear error avoids this. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset-frontend/playwright/components/core/AceEditor.ts: ########## @@ -0,0 +1,144 @@ +/** + * 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 locator: Locator; + + constructor(page: Page, selectorOrLocator: string | Locator) { + this.page = page; + if (typeof selectorOrLocator === 'string') { + this.locator = page.locator(selectorOrLocator); + } else { + this.locator = selectorOrLocator; + } + } + + /** + * Gets the editor element locator + */ + get element(): Locator { + return this.locator; + } + + /** + * Waits for the ace editor to be fully loaded and ready for interaction. + * Also waits for the ace library to be available on window. + */ + 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(); + // Wait for ace library to be loaded + await this.page.waitForFunction( + () => + typeof (window as any).ace !== 'undefined' && + typeof (window as any).ace.edit === 'function', + ); + } + + /** + * Sets text in the ace editor using the ace API. + * Uses element handle to target the specific editor (avoids global ID lookup bug). + * @param text - The text to set + */ + async setText(text: string): Promise<void> { + await this.waitForReady(); + const elementHandle = await this.locator.elementHandle(); + await this.page.evaluate( + ({ element, value }) => { + const editor = (window as any).ace.edit(element); + editor.setValue(value, 1); + editor.session.getUndoManager().reset(); + }, + { element: elementHandle, value: text }, + ); + } + + /** + * Gets the text content from the ace editor. + * Uses element handle to target the specific editor. + * @returns The text content + */ + async getText(): Promise<string> { + await this.waitForReady(); + const elementHandle = await this.locator.elementHandle(); + return this.page.evaluate(element => { + const editor = (window as any).ace.edit(element); + return editor.getValue(); + }, elementHandle); + } + + /** + * 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 elementHandle = await this.locator.elementHandle(); + await this.page.evaluate( + ({ element, value }) => { + const editor = (window as any).ace.edit(element); + const currentText = editor.getValue(); + const newText = currentText + (currentText ? '\n' : '') + value; + editor.setValue(newText, 1); + }, + { element: elementHandle, value: text }, + ); + } + + /** + * Focuses the ace editor. + */ + async focus(): Promise<void> { + await this.waitForReady(); + const elementHandle = await this.locator.elementHandle(); + await this.page.evaluate(element => { Review Comment: **Suggestion:** In `focus`, the code assumes `locator.elementHandle()` always returns a handle, but if it returns `null` (e.g., due to a race where the editor is removed), `ace.edit(null).focus()` will throw, so it should explicitly guard against a missing element. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Tests that focus the editor can fail intermittently. - ⚠️ Flaky editor interactions increase test instability. ``` </details> ```suggestion if (!elementHandle) { throw new Error('Ace editor element not found while focusing'); } ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run a Playwright test that calls AceEditor.focus (superset-frontend/playwright/components/core/AceEditor.ts). 2. focus waits for readiness at line 130 then calls locator.elementHandle() at line 131 to get the element. 3. If the editor is detached concurrently, elementHandle can be null and page.evaluate at lines 132-135 runs with null, causing ace.edit(null).focus() in the browser and a cryptic failure. 4. The improved guard throws a clear error at line 132-134, surfacing the real cause (missing element) immediately. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/playwright/components/core/AceEditor.ts **Line:** 132:132 **Comment:** *Possible Bug: In `focus`, the code assumes `locator.elementHandle()` always returns a handle, but if it returns `null` (e.g., due to a race where the editor is removed), `ace.edit(null).focus()` will throw, so it should explicitly guard against a missing element. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset-frontend/playwright/components/core/AceEditor.ts: ########## @@ -0,0 +1,144 @@ +/** + * 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 locator: Locator; + + constructor(page: Page, selectorOrLocator: string | Locator) { + this.page = page; + if (typeof selectorOrLocator === 'string') { + this.locator = page.locator(selectorOrLocator); + } else { + this.locator = selectorOrLocator; + } + } + + /** + * Gets the editor element locator + */ + get element(): Locator { + return this.locator; + } + + /** + * Waits for the ace editor to be fully loaded and ready for interaction. + * Also waits for the ace library to be available on window. + */ + 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(); + // Wait for ace library to be loaded + await this.page.waitForFunction( + () => + typeof (window as any).ace !== 'undefined' && + typeof (window as any).ace.edit === 'function', + ); + } + + /** + * Sets text in the ace editor using the ace API. + * Uses element handle to target the specific editor (avoids global ID lookup bug). + * @param text - The text to set + */ + async setText(text: string): Promise<void> { + await this.waitForReady(); + const elementHandle = await this.locator.elementHandle(); + await this.page.evaluate( + ({ element, value }) => { + const editor = (window as any).ace.edit(element); + editor.setValue(value, 1); + editor.session.getUndoManager().reset(); + }, + { element: elementHandle, value: text }, + ); + } + + /** + * Gets the text content from the ace editor. + * Uses element handle to target the specific editor. + * @returns The text content + */ + async getText(): Promise<string> { + await this.waitForReady(); + const elementHandle = await this.locator.elementHandle(); + return this.page.evaluate(element => { + const editor = (window as any).ace.edit(element); + return editor.getValue(); + }, elementHandle); + } + + /** + * 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 elementHandle = await this.locator.elementHandle(); + await this.page.evaluate( + ({ element, value }) => { + const editor = (window as any).ace.edit(element); + const currentText = editor.getValue(); + const newText = currentText + (currentText ? '\n' : '') + value; + editor.setValue(newText, 1); + }, + { element: elementHandle, value: text }, Review Comment: **Suggestion:** In `appendText`, the element handle is again passed to `page.evaluate` inside an object, which Playwright cannot serialize for JSHandles, leading to a runtime error instead of executing the editor update. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Playwright tests using AceEditor.appendText fail. - ⚠️ Any test relying on incremental editor updates broken. ``` </details> ```suggestion (element, value) => { const editor = (window as any).ace.edit(element); const currentText = editor.getValue(); const newText = currentText + (currentText ? '\n' : '') + value; editor.setValue(newText, 1); }, elementHandle, text, ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Execute a Playwright test that calls AceEditor.appendText (superset-frontend/playwright/components/core/AceEditor.ts). 2. appendText obtains elementHandle at line 114 and calls page.evaluate with an object containing that handle at lines 115-123. 3. Playwright fails while serializing the argument (the object contains a JSHandle), emitting an error like "JSHandle is not serializable" and aborting the evaluate call. 4. The browser-side code that appends text (editor.setValue) never runs and the test fails where appendText was awaited. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/playwright/components/core/AceEditor.ts **Line:** 116:122 **Comment:** *Logic Error: In `appendText`, the element handle is again passed to `page.evaluate` inside an object, which Playwright cannot serialize for JSHandles, leading to a runtime error instead of executing the editor update. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset-frontend/playwright/components/modals/EditDatasetModal.ts: ########## @@ -0,0 +1,145 @@ +/** + * 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'; +import { AceEditor, Input, Modal, Tabs } from '../core'; + +/** + * Edit Dataset Modal component (DatasourceModal). + * Used for editing dataset properties like description, metrics, columns, etc. + * Uses specific dialog name to avoid strict mode violations when multiple dialogs are open. + */ +export class EditDatasetModal extends Modal { + private static readonly SELECTORS = { + NAME_INPUT: '[data-test="inline-name"]', + LOCK_ICON: '[data-test="lock"]', + UNLOCK_ICON: '[data-test="unlock"]', + }; + + private readonly tabs: Tabs; + private readonly specificLocator: Locator; + + constructor(page: Page) { + super(page); + this.tabs = new Tabs(page); + // Use getByRole with specific name to target Edit Dataset dialog + // The dialog has aria-labelledby that resolves to "edit Edit Dataset" + this.specificLocator = page.getByRole('dialog', { name: /edit.*dataset/i }); + } + + /** + * Override element getter to use specific locator + */ + override get element(): Locator { + return this.specificLocator; + } + + /** + * Click the Save button to save changes + */ + async clickSave(): Promise<void> { + await this.clickFooterButton('Save'); + } + + /** + * Click the Cancel button to discard changes + */ + async clickCancel(): Promise<void> { + await this.clickFooterButton('Cancel'); + } + + /** + * Click the lock icon to enable edit mode + * The modal starts in read-only mode and requires clicking the lock to edit + */ + async enableEditMode(): Promise<void> { + const lockButton = this.body.locator(EditDatasetModal.SELECTORS.LOCK_ICON); + await lockButton.click(); + } + + /** + * Gets the dataset name input component + */ + private get nameInput(): Input { + return new Input( + this.page, + this.body.locator(EditDatasetModal.SELECTORS.NAME_INPUT), + ); + } + + /** + * Fill in the dataset name field + * Note: Call enableEditMode() first if the modal is in read-only mode + * @param name - The new dataset name + */ + async fillName(name: string): Promise<void> { + await this.nameInput.fill(name); + } + + /** + * Navigate to a specific tab in the modal + * @param tabName - The name of the tab (e.g., 'Source', 'Metrics', 'Columns') + */ + async clickTab(tabName: string): Promise<void> { + await this.tabs.clickTab(tabName); + } + + /** + * Navigate to the Settings tab + */ + async clickSettingsTab(): Promise<void> { + await this.element.getByRole('tab', { name: 'Settings' }).click(); + } + + /** + * Gets the description editor locator in the Settings tab + * The description is an AceEditor in the Settings panel + */ + private getDescriptionEditor(): AceEditor { + const settingsPanel = this.element.locator( + '[id="table-tabs-panel-SETTINGS"]', + ); Review Comment: **Suggestion:** The description editor lookup relies on a hard-coded panel id selector, which couples the test to internal implementation details; if the id changes (for example, due to refactoring the tab system), these tests will start failing even though the visible "Settings" tab is still present, whereas using the tab panel located via the tab name is more robust. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Description editor lookup failures break Settings tests. - ⚠️ Import/edit dataset E2E scenarios affected. - ⚠️ Tests brittle to UI refactors and id changes. ``` </details> ```suggestion const settingsPanel = this.tabs.getTabPanel('Settings'); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open Edit modal and switch to Settings tab using `EditDatasetModal.clickSettingsTab()` which targets `EditDatasetModal` at `superset-frontend/playwright/components/modals/EditDatasetModal.ts:106-108`. 2. Call `editModal.fillDescription('text')` which invokes `getDescriptionEditor()` at `EditDatasetModal.ts:114-124`. The current implementation looks for a hard-coded panel id selector `'[id="table-tabs-panel-SETTINGS"]'`. 3. If the tab system implementation changes (IDs renamed, dynamic ids introduced, or Tabs refactor), the hard-coded id will no longer match; Playwright will not find the settings panel or ace-editor and the call will fail with a missing element error originating from `getDescriptionEditor()` lines 114-124. 4. Concrete reproduction: modify the application to change the tab panel id (or simulate a refactor) so the DOM no longer contains `id="table-tabs-panel-SETTINGS"`, then run the E2E that calls `fillDescription()` and observe failure at `EditDatasetModal.ts:114-124`. Using `this.tabs.getTabPanel('Settings')` scopes lookup to the accessible tab panel and is more robust. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/playwright/components/modals/EditDatasetModal.ts **Line:** 115:117 **Comment:** *Possible Bug: The description editor lookup relies on a hard-coded panel id selector, which couples the test to internal implementation details; if the id changes (for example, due to refactoring the tab system), these tests will start failing even though the visible "Settings" tab is still present, whereas using the tab panel located via the tab name is more robust. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset-frontend/playwright/tests/experimental/dataset/dataset-list.spec.ts: ########## @@ -85,10 +85,18 @@ test.afterEach(async ({ page }) => { test('should navigate to Explore when dataset name is clicked', async ({ page, }) => { + const explorePage = new ExplorePage(page); + // Use existing physical dataset (loaded in CI via --load-examples) const datasetName = TEST_DATASETS.PHYSICAL_DATASET; const dataset = await getDatasetByName(page, datasetName); - expect(dataset).not.toBeNull(); + + // Guard: Verify example dataset exists (required for all tests in this file) + // This makes tests fail fast with clear message if examples aren't loaded correctly + expect( + dataset, + `Example dataset '${datasetName}' not found. Ensure --load-examples was run.`, + ).not.toBeNull(); // Verify dataset is visible in list (uses page object + Playwright auto-wait) await expect(datasetListPage.getDatasetRow(datasetName)).toBeVisible(); Review Comment: **Suggestion:** The guard that checks the example dataset's existence only asserts that the value is not null, but if `getDatasetByName` ever returns `undefined` (a common "not found" sentinel), this check will still pass and the test will proceed with a missing dataset, failing later with a less-informative error instead of the intended explicit message. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Tests in dataset-list.spec fail with non-actionable errors. - ⚠️ CI run diagnostics for missing examples become noisy. ``` </details> ```suggestion ).toBeTruthy(); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open the test file at superset-frontend/playwright/tests/experimental/dataset/dataset-list.spec.ts and locate the dataset-existence guard (lines 88-100 surrounding the expect call; the expect starts at line 94 in the PR diff and ends at 99). 2. Run the test 'should navigate to Explore when dataset name is clicked' (test harness invokes this file's tests). The test calls getDatasetByName(page, 'birth_names') at the call on the line immediately before the expect (dataset variable creation at line ~92). 3. If the example datasets were not loaded in the CI job, getDatasetByName may return undefined. The current assertion uses .not.toBeNull() (lines 94-99) which passes for undefined, so the guard will not fail here. 4. The test then continues to call datasetListPage.getDatasetRow(datasetName) (line ~101) and later clickDatasetName(datasetName) (line ~105). Those later calls will fail with a less-informative Playwright error like "row not found" or a timeout, masking the original cause (missing examples). 5. Replacing .not.toBeNull() with .toBeTruthy() (as in the improved code) will fail fast when dataset is undefined or null, producing the explicit message at the expect call and making CI diagnostics clear. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/playwright/tests/experimental/dataset/dataset-list.spec.ts **Line:** 102:102 **Comment:** *Possible Bug: The guard that checks the example dataset's existence only asserts that the value is not null, but if `getDatasetByName` ever returns `undefined` (a common "not found" sentinel), this check will still pass and the test will proceed with a missing dataset, failing later with a less-informative error instead of the intended explicit message. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset-frontend/playwright/tests/experimental/dataset/dataset-list.spec.ts: ########## @@ -224,3 +232,136 @@ 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.PHYSICAL_DATASET; + + // Verify dataset is visible in list + await expect(datasetListPage.getDatasetRow(datasetName)).toBeVisible(); + + // Set up API response intercept for export endpoint + // Note: We intercept the API response instead of relying on download events because + // Superset uses blob downloads (createObjectURL) which don't trigger Playwright's + // download event consistently, especially in app-prefix configurations. + const exportResponsePromise = page.waitForResponse( + response => + response.url().includes('/api/v1/dataset/export/') && + response.status() === 200, + ); + + // Click export action button + await datasetListPage.clickExportAction(datasetName); + + // Wait for export API response + const exportResponse = await exportResponsePromise; + + // Verify response is a zip file + const contentType = exportResponse.headers()['content-type']; + expect(contentType).toMatch(/application\/(zip|x-zip-compressed)/); + + // Verify content-disposition header indicates file download + const contentDisposition = exportResponse.headers()['content-disposition']; + expect(contentDisposition).toContain('attachment'); + expect(contentDisposition).toMatch(/\.zip/); +}); + +test('should export dataset via bulk select action', async ({ page }) => { + // Tests the bulk select + export workflow (uses single dataset for simplicity) + const datasetName = TEST_DATASETS.PHYSICAL_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.bulkSelect.getControls()).toBeVisible(); + + // Select the dataset checkbox + await datasetListPage.selectDatasetCheckbox(datasetName); Review Comment: **Suggestion:** The bulk export test is intended to validate multi-row bulk export but currently selects only a single dataset, so it never actually exercises the behavior of exporting multiple datasets from the bulk action, leaving that core path untested and potentially allowing regressions in bulk selection handling. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Bulk-export multi-selection logic remains untested. - ❌ Potential regressions in multi-dataset export go undetected. ``` </details> ```suggestion // Create two virtual datasets for this bulk export test (hermetic - no dependency on examples) const datasetName1 = `test_bulk_export_1_${Date.now()}`; const datasetName2 = `test_bulk_export_2_${Date.now()}`; const datasetId1 = await createTestVirtualDataset(page, datasetName1); const datasetId2 = await createTestVirtualDataset(page, datasetName2); expect(datasetId1).not.toBeNull(); expect(datasetId2).not.toBeNull(); // Track for cleanup in case test fails partway through testResources = { datasetIds: [datasetId1!, datasetId2!] }; // Refresh page to see new datasets await datasetListPage.goto(); await datasetListPage.waitForTableLoad(); // Verify both datasets are visible in list await expect(datasetListPage.getDatasetRow(datasetName1)).toBeVisible(); await expect(datasetListPage.getDatasetRow(datasetName2)).toBeVisible(); // Enable bulk select mode await datasetListPage.clickBulkSelectButton(); // Verify bulk select controls appear await expect(datasetListPage.bulkSelect.getControls()).toBeVisible(); // Select both dataset checkboxes await datasetListPage.selectDatasetCheckbox(datasetName1); await datasetListPage.selectDatasetCheckbox(datasetName2); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open superset-frontend/playwright/tests/experimental/dataset/dataset-list.spec.ts and find the bulk export test starting at line ~269 (test title 'should export dataset via bulk select action'). 2. Run only that test (Playwright command targeting this file). The test enables bulk select (datasetListPage.clickBulkSelectButton at ~276) and selects a single dataset checkbox via datasetListPage.selectDatasetCheckbox(datasetName) at ~283. 3. Because only one dataset is selected, the bulk export action exercised is effectively identical to exporting a single dataset; the code path that aggregates multiple selected dataset IDs and forms a multi-dataset export request is not exercised. 4. A regression in the multi-selection aggregation logic (for example, batching multiple IDs into the export payload) would not be detected by this single-selection test; only by creating/selecting multiple datasets (as in the improved code) will the multi-dataset export request path be observed by the test's page.waitForResponse predicate. 5. To reproduce the missing coverage locally: create two virtual datasets (use createTestVirtualDataset helper), refresh the list (datasetListPage.goto()/waitForTableLoad), enable bulk select, select both checkboxes, click bulk Export, and confirm the export API request contains multiple dataset identifiers in the request URL/body (this exercise is added in the improved code). ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/playwright/tests/experimental/dataset/dataset-list.spec.ts **Line:** 270:283 **Comment:** *Logic Error: The bulk export test is intended to validate multi-row bulk export but currently selects only a single dataset, so it never actually exercises the behavior of exporting multiple datasets from the bulk action, leaving that core path untested and potentially allowing regressions in bulk selection handling. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset-frontend/playwright/components/modals/EditDatasetModal.ts: ########## @@ -0,0 +1,145 @@ +/** + * 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'; +import { AceEditor, Input, Modal, Tabs } from '../core'; + +/** + * Edit Dataset Modal component (DatasourceModal). + * Used for editing dataset properties like description, metrics, columns, etc. + * Uses specific dialog name to avoid strict mode violations when multiple dialogs are open. + */ +export class EditDatasetModal extends Modal { + private static readonly SELECTORS = { + NAME_INPUT: '[data-test="inline-name"]', + LOCK_ICON: '[data-test="lock"]', + UNLOCK_ICON: '[data-test="unlock"]', + }; + + private readonly tabs: Tabs; + private readonly specificLocator: Locator; + + constructor(page: Page) { + super(page); + this.tabs = new Tabs(page); + // Use getByRole with specific name to target Edit Dataset dialog + // The dialog has aria-labelledby that resolves to "edit Edit Dataset" + this.specificLocator = page.getByRole('dialog', { name: /edit.*dataset/i }); + } + + /** + * Override element getter to use specific locator + */ + override get element(): Locator { + return this.specificLocator; + } + + /** + * Click the Save button to save changes + */ + async clickSave(): Promise<void> { + await this.clickFooterButton('Save'); + } + + /** + * Click the Cancel button to discard changes + */ + async clickCancel(): Promise<void> { + await this.clickFooterButton('Cancel'); + } + + /** + * Click the lock icon to enable edit mode + * The modal starts in read-only mode and requires clicking the lock to edit + */ + async enableEditMode(): Promise<void> { + const lockButton = this.body.locator(EditDatasetModal.SELECTORS.LOCK_ICON); Review Comment: **Suggestion:** The edit-mode helper is not idempotent: if the modal is already unlocked and the lock icon is no longer rendered, calling the method again will fail because it always looks up and clicks the lock icon, causing tests to break when `enableEditMode` is called more than once for the same modal instance. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Edit dataset modal tests fail intermittently. - ⚠️ Name/description edit E2E scenarios affected. - ⚠️ Test suite flakiness during modal reuse. ``` </details> ```suggestion const unlockButton = this.body.locator( EditDatasetModal.SELECTORS.UNLOCK_ICON, ); if (await unlockButton.isVisible()) { // Already in edit mode return; } ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open Dataset List and trigger Edit modal for a dataset (modal opens via UI, Playwright test uses EditDatasetModal in `superset-frontend/playwright/.../EditDatasetModal.ts`). 2. At `EditDatasetModal.ts:71-74`, the test calls `enableEditMode()` which executes the current implementation and clicks the lock icon to enter edit mode. 3. If the same test (or a subsequent step in the same test) calls `enableEditMode()` again on the same modal instance, the existing code will look up the lock icon selector at `EditDatasetModal.SELECTORS.LOCK_ICON` and attempt to click it even though the modal is already unlocked and only the unlock icon is present; Playwright will fail to find/click the absent element causing the test to error (stack shows failing click on locator in `EditDatasetModal.ts:72-74`). 4. Reproduction is concrete: update or add a Playwright test that opens the edit modal, calls `await editModal.enableEditMode()` twice in a row and observe the second call fail with a locator/click error originating from `superset-frontend/playwright/components/modals/EditDatasetModal.ts:71-74`. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/playwright/components/modals/EditDatasetModal.ts **Line:** 72:72 **Comment:** *Possible Bug: The edit-mode helper is not idempotent: if the modal is already unlocked and the lock icon is no longer rendered, calling the method again will fail because it always looks up and clicks the lock icon, causing tests to break when `enableEditMode` is called more than once for the same modal instance. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </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: [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]
