test(playwright): additional dataset list playwright tests #36684
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive Playwright end-to-end tests for the dataset list page, expanding test coverage for export, bulk operations, dataset creation wizard navigation, and dataset editing functionality. The changes introduce new page object models and core component wrappers to support these additional test scenarios.
- Adds 4 new test cases covering export (single and bulk), create wizard navigation, and dataset editing
- Introduces new page objects (CreateDatasetPage, EditDatasetModal, ConfirmDialog) for better test organization
- Adds reusable core components (Select, Checkbox, Tabs, Textarea) to the component library
- Refactors test file to remove unused file-scoped variables
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| superset-frontend/playwright/tests/experimental/dataset/dataset-list.spec.ts | Adds 4 new tests for export, bulk export, create wizard, and edit functionality; removes unused explorePage variable |
| superset-frontend/playwright/pages/DatasetListPage.ts | Adds methods for bulk selection, edit action, and checkbox interactions |
| superset-frontend/playwright/pages/CreateDatasetPage.ts | New page object for dataset creation wizard with database/schema/table selection |
| superset-frontend/playwright/components/modals/EditDatasetModal.ts | New modal component for dataset editing with description and tab navigation support |
| superset-frontend/playwright/components/modals/ConfirmDialog.ts | New component for Ant Design confirmation dialogs with OK/Cancel actions |
| superset-frontend/playwright/components/core/Checkbox.ts | New core component wrapper for checkbox interactions |
| superset-frontend/playwright/components/core/Select.ts | New core component wrapper for Ant Design Select/Combobox with filtering support |
| superset-frontend/playwright/components/core/Tabs.ts | New core component wrapper for Ant Design tab navigation |
| superset-frontend/playwright/components/core/Textarea.ts | New core component wrapper for multi-line text input |
| superset-frontend/playwright/components/core/index.ts | Exports new core components for use throughout the test suite |
superset-frontend/playwright/tests/experimental/dataset/dataset-list.spec.ts
Show resolved
Hide resolved
superset-frontend/playwright/components/modals/ConfirmDialog.ts
Outdated
Show resolved
Hide resolved
superset-frontend/playwright/components/modals/EditDatasetModal.ts
Outdated
Show resolved
Hide resolved
sadpandajoe
force-pushed
the
feat-additional-dataset-playwright-tests
branch
from
2b82b04 to
a94eaff
Compare
sadpandajoe
force-pushed
the
feat-additional-dataset-playwright-tests
branch
from
314176b to
017c171
Compare
sadpandajoe
force-pushed
the
feat-additional-dataset-playwright-tests
branch
from
e257bdf to
8775c2a
Compare
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
sadpandajoe
force-pushed
the
feat-additional-dataset-playwright-tests
branch
from
8775c2a to
1d21092
Compare
dosubot
bot
added
the
listview
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Agent Run #0ee456
Actionable Suggestions - 2
-
superset-frontend/playwright/components/core/AceEditor.ts - 1
- Incorrect ID Extraction · Line 138-151
-
superset-frontend/playwright/helpers/api/database.ts - 1
- Interface Mismatch with API Response · Line 31-45
Additional Suggestions - 6
-
superset-frontend/playwright/components/modals/ConfirmDialog.ts - 1
-
Discouraged waitForTimeout usage · Line 51-51The waitForTimeout here is discouraged in Playwright as it introduces fixed delays that can make tests unreliable. Playwright's click() method already waits for elements to be stable, including during animations, so this explicit wait is unnecessary and potentially harmful.
Code suggestion
@@ -49,4 +49,2 @@ - async clickOk(): Promise<void> { - // Wait for modal animation to complete before clicking - await this.page.waitForTimeout(200); - await this.clickFooterButton('OK'); + async clickOk(): Promise<void> { + await this.clickFooterButton('OK'); }
-
-
superset-frontend/playwright/components/core/Select.ts - 1
-
Flaky test timing · Line 87-87The use of page.waitForTimeout here can make tests unreliable in slower environments or CI. It appears better to wait for the actual dropdown state change instead of a fixed delay.
-
-
superset-frontend/playwright/components/core/Checkbox.ts - 1
-
Inconsistent constructor API · Line 29-32The constructor only accepts Locator, unlike other Playwright components (e.g., `Button` and `Input`) that accept string | Locator for flexibility. This creates inconsistency in the API.
Code suggestion
@@ -28,5 +28,5 @@ - - constructor(page: Page, locator: Locator) { - this.page = page; - this.locator = locator; - } + constructor(page: Page, locator: string); + constructor(page: Page, locator: string | Locator) { + this.page = page; + this.locator = typeof locator === 'string' ? page.locator(locator) : locator; + }
-
-
superset-frontend/playwright/components/core/AceEditor.ts - 2
-
Type Safety Violation · Line 69-69The code uses (window as any).ace.edit(id) in multiple methods, which violates the project's strict rule against 'any' types. This undermines TypeScript type safety efforts. A global declaration can provide proper typing without 'any'.
Code suggestion
@@ -20,2 +20,6 @@ - import { Locator, Page } from '@playwright/test'; - + import { Locator, Page } from '@playwright/test'; - + + declare global { + interface Window { + ace: any; + } + }
-
Extra Newlines in Append · Line 108-108The appendText logic adds a newline even if the current text already ends with one, leading to extra blank lines. This can cause inconsistent editor content.
Code suggestion
@@ -108,1 +108,1 @@ - const newText = currentText + (currentText ? '\\n' : '') + value; + const newText = currentText + (currentText && !currentText.endsWith('\n') ? '\n' : '') + value;
-
-
superset-frontend/playwright/pages/ChartCreationPage.ts - 1
-
Unused selector dead code · Line 32-32The VIZ_TYPE_ITEM selector is defined but never used in the class methods. This creates dead code that should be removed to keep the codebase clean.
-
Review Details
-
Files reviewed - 23 · Commit Range:
2dcebac..a5303ba- superset-frontend/playwright.config.ts
- superset-frontend/playwright/components/core/AceEditor.ts
- superset-frontend/playwright/components/core/Checkbox.ts
- superset-frontend/playwright/components/core/Select.ts
- superset-frontend/playwright/components/core/Tabs.ts
- superset-frontend/playwright/components/core/Textarea.ts
- superset-frontend/playwright/components/core/index.ts
- superset-frontend/playwright/components/modals/ConfirmDialog.ts
- superset-frontend/playwright/components/modals/DuplicateDatasetModal.ts
- superset-frontend/playwright/components/modals/EditDatasetModal.ts
- superset-frontend/playwright/components/modals/ImportDatasetModal.ts
- superset-frontend/playwright/components/modals/index.ts
- superset-frontend/playwright/helpers/api/assertions.ts
- superset-frontend/playwright/helpers/api/database.ts
- superset-frontend/playwright/helpers/api/dataset.ts
- superset-frontend/playwright/helpers/api/intercepts.ts
- superset-frontend/playwright/helpers/fixtures/index.ts
- superset-frontend/playwright/helpers/fixtures/testAssets.ts
- superset-frontend/playwright/pages/ChartCreationPage.ts
- superset-frontend/playwright/pages/CreateDatasetPage.ts
- superset-frontend/playwright/pages/DatasetListPage.ts
- superset-frontend/playwright/tests/experimental/dataset/create-dataset.spec.ts
- superset-frontend/playwright/tests/experimental/dataset/dataset-list.spec.ts
-
Files skipped - 2
- superset-frontend/package-lock.json - Reason: Filter setting
- superset-frontend/package.json - Reason: Filter setting
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at [email protected].
Documentation & Help
AI Code Review powered by
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| // Extract origin (scheme + host + port) for health check | ||
| const healthUrl = new URL('health', baseUrl).href; | ||
| return { | ||
| command: `curl -f ${healthUrl}`, |
Contributor
There was a problem hiding this comment.
Suggestion: The shell command string interpolates healthUrl directly into curl -f ${healthUrl}; because healthUrl is ultimately derived from the PLAYWRIGHT_BASE_URL environment variable, a value containing shell metacharacters like ; can break out of the command and execute arbitrary shell instructions when Playwright starts the webServer, creating a command injection risk even though it is primarily a test configuration. [security]
Severity Level: Critical 🚨
- ❌ Arbitrary shell command execution possible via env var.
- ⚠️ Local development environment security risk.
- ⚠️ CI secrets misuse could escalate risk.
Suggested change
| command: `curl -f ${healthUrl}`, | |
| command: `curl -f "${healthUrl}"`, |
Steps of Reproduction ✅
1. Start tests locally where Playwright uses the webServer config in
superset-frontend/playwright.config.ts (webServer closure around lines 120-136).
2. Export a malicious PLAYWRIGHT_BASE_URL such as:
PLAYWRIGHT_BASE_URL='http://localhost:8088/health; echo hacked > /tmp/pwn' and run
Playwright. The code constructs healthUrl from the env var and then places it unquoted
into the shell command at line 130: `command: \`curl -f ${healthUrl}\``.
3. When Playwright executes the webServer command, the unquoted interpolation allows the
shell to interpret the semicolon and run the appended command, performing arbitrary
actions (e.g., creating /tmp/pwn). Observe the side-effect on the filesystem or process
list to confirm command execution.
4. Note: In typical CI the env var is controlled and this is unlikely, but locally or in a
compromised environment this creates a real command-injection vector originating from this
config line (superset-frontend/playwright.config.ts:130).Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/playwright.config.ts
**Line:** 130:130
**Comment:**
*Security: The shell command string interpolates `healthUrl` directly into `curl -f ${healthUrl}`; because `healthUrl` is ultimately derived from the `PLAYWRIGHT_BASE_URL` environment variable, a value containing shell metacharacters like `;` can break out of the command and execute arbitrary shell instructions when Playwright starts the webServer, creating a command injection risk even though it is primarily a test configuration.
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.
Contributor
Code Review Agent Run #7c3fe6Actionable Suggestions - 0Additional Suggestions - 4
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
This comment was marked as outdated.
This comment was marked as outdated.
Comment on lines
+154
to
+150
| } catch { | ||
| // Fallback: locator might be the input itself (e.g., from getByRole('combobox')) | ||
| await this.locator.fill(text); |
There was a problem hiding this comment.
The type method has a try-catch block that falls back to filling the locator directly if the searchInput is not found. However, the catch block is too broad and will catch any error (including network errors, JavaScript errors, etc.). Consider catching more specific errors or at least checking the error type before falling back to the alternative approach.
Suggested change
| } catch { | |
| // Fallback: locator might be the input itself (e.g., from getByRole('combobox')) | |
| await this.locator.fill(text); | |
| } catch (error) { | |
| // Only fall back when we fail to find/attach the search input (e.g. timeout) | |
| if (error instanceof Error && /timeout/i.test(error.message)) { | |
| // Fallback: locator might be the input itself (e.g., from getByRole('combobox')) | |
| await this.locator.fill(text); | |
| } else { | |
| // Unexpected error - rethrow so tests fail visibly | |
| throw error; | |
| } |
| async expandColumn(columnName: string): Promise<Locator> { | ||
| // Find cell with exact column name text, then derive row from that cell | ||
| const cell = this.body.getByRole('cell', { name: columnName, exact: true }); | ||
| const row = cell.locator('xpath=ancestor::tr[1]'); |
There was a problem hiding this comment.
The EditDatasetModal.expandColumn method uses an XPath expression 'xpath=ancestor::tr[1]' to find the row element from a cell (line 157). XPath selectors are generally more fragile than modern Playwright locators and can break if the DOM structure changes. Consider using a more robust selector strategy, such as cell.locator('..').locator('xpath=ancestor::tr[1]') or refactoring to use getByRole with table row relationships.
Suggested change
| const row = cell.locator('xpath=ancestor::tr[1]'); | |
| const row = this.body.getByRole('row', { has: cell }); |
Comment on lines
72
to
75
| } catch (error) { | ||
| if (!(error instanceof Error) || error.name !== 'TimeoutError') { | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
The error handling pattern here only catches TimeoutError but re-throws all other errors. However, the catch block checks if the error is an instance of Error before accessing the name property. If a non-Error object is thrown (e.g., a string or null), this check will succeed but error.name will be undefined, potentially causing unexpected behavior. Consider adding a more defensive check like: if (error?.name !== 'TimeoutError') throw error;
Comment on lines
40
to
41
| trackDataset: id => datasetIds.add(id), | ||
| trackDatabase: id => databaseIds.add(id), |
There was a problem hiding this comment.
The testAssets fixture uses Set to de-duplicate IDs (line 36-37), which is good. However, if a test tracks the same ID multiple times (e.g., due to a bug or retry logic), the cleanup will only attempt deletion once. While this is generally fine, consider logging when duplicate IDs are tracked to help identify potential test issues during development.
Suggested change
| trackDataset: id => datasetIds.add(id), | |
| trackDatabase: id => databaseIds.add(id), | |
| trackDataset: id => { | |
| if (datasetIds.has(id)) { | |
| console.warn( | |
| `[testAssets] Dataset ID ${id} is being tracked multiple times in the same test.`, | |
| ); | |
| } | |
| datasetIds.add(id); | |
| }, | |
| trackDatabase: id => { | |
| if (databaseIds.has(id)) { | |
| console.warn( | |
| `[testAssets] Database ID ${id} is being tracked multiple times in the same test.`, | |
| ); | |
| } | |
| databaseIds.add(id); | |
| }, |
| await this.locator.waitFor({ state: 'attached' }); | ||
| await this.locator | ||
| .locator(ACE_EDITOR_SELECTORS.CONTENT) | ||
| .waitFor({ state: 'attached' }); |
There was a problem hiding this comment.
The AceEditor methods (setText, getText, appendText, focus) all use page.evaluate to directly access the ace global object. If the ace library is not loaded or takes time to load, these methods will fail with 'Ace editor library not loaded' error. While waitForReady() checks for DOM elements, it doesn't verify that the ace JavaScript library is actually loaded. Consider adding a check in waitForReady() to poll for window.ace availability before proceeding with editor operations.
Suggested change
| .waitFor({ state: 'attached' }); | |
| .waitFor({ state: 'attached' }); | |
| // Additionally ensure the Ace library is loaded in the page before proceeding. | |
| await this.page.waitForFunction(() => { | |
| const win = window as unknown as { | |
| ace?: { edit?: unknown }; | |
| }; | |
| return !!(win.ace && typeof win.ace.edit === 'function'); | |
| }); |
Comment on lines
58
to
72
| function matchUrl( | ||
| url: string, | ||
| pattern: string | RegExp, | ||
| pathMatch?: boolean, | ||
| ): boolean { | ||
| if (typeof pattern === 'string') { | ||
| if (pathMatch) { | ||
| const pathname = normalizePath(new URL(url).pathname); | ||
| const normalizedPattern = normalizePath(pattern); | ||
| return pathname.endsWith(normalizedPattern); | ||
| } | ||
| return url.includes(pattern); | ||
| } | ||
| return pattern.test(url); | ||
| } |
There was a problem hiding this comment.
The matchUrl function uses new URL(url) to parse URLs when pathMatch is enabled (line 65). If the url parameter is not a valid URL (e.g., a relative path), this will throw an error. While this is likely intentional to catch malformed URLs, consider adding error handling or validation to provide a more helpful error message when an invalid URL is encountered.
Comment on lines
+115
to
+118
| // The dropdown trigger is in the same button group, find it relative to main button | ||
| const dropdownTrigger = mainButton | ||
| .locator('xpath=following-sibling::button') | ||
| .first(); |
There was a problem hiding this comment.
The CreateDatasetPage.clickCreateDataset method uses an XPath expression 'xpath=following-sibling::button' to find the dropdown trigger button (line 117). XPath selectors are generally more fragile than modern Playwright locators and can break if the DOM structure changes. Consider using a more robust selector strategy, such as data-test attributes or getByRole with more specific criteria, to make this locator more maintainable.
Suggested change
| // The dropdown trigger is in the same button group, find it relative to main button | |
| const dropdownTrigger = mainButton | |
| .locator('xpath=following-sibling::button') | |
| .first(); | |
| // The dropdown trigger is in the same button group: go to the parent container | |
| // and select the second button in that group (index 1). | |
| const dropdownTrigger = mainButton | |
| .locator('..') | |
| .getByRole('button') | |
| .nth(1); |
|
|
||
| const createResponse = await apiPostVirtualDataset(page, { | ||
| database: baseDataset!.database.id, | ||
| schema: baseDataset!.schema ?? '', |
There was a problem hiding this comment.
The schema field in VirtualDatasetCreatePayload is defined as 'string | null', but in the apiPostVirtualDataset call on line 583, it uses 'baseDataset!.schema ?? '''. This means if schema is null, it will be converted to an empty string. However, the type definition allows null. Consider either: (1) passing null directly when schema is null to match the type signature, or (2) documenting why an empty string is preferred over null for the schema field.
Suggested change
| schema: baseDataset!.schema ?? '', | |
| schema: baseDataset!.schema ?? null, |
sadpandajoe
added
the
🎪 ⚡ showtime-trigger-start
Contributor
Code Review Agent Run #777ddeActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Add test for exporting a single dataset via the action button. The test verifies: - Download is triggered when clicking export action - Downloaded file is a zip file - Download completes without failure 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add test for bulk exporting datasets via bulk select mode. The test verifies: - Bulk select mode can be enabled - Datasets can be selected via checkboxes - Export bulk action triggers download - Downloaded file is a zip file Add Checkbox component to components/core for checkbox interactions. Add bulk select methods to DatasetListPage: - getBulkSelectButton() / clickBulkSelectButton() - getDatasetCheckbox() / selectDatasetCheckbox() - getBulkActionButton() / clickBulkAction() - getBulkSelectControls() 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add E2E test for the create dataset wizard flow: - Navigate to create dataset page via "+ Dataset" button - Verify cascading select dropdowns (database → schema → table) - Verify create and explore button availability Add supporting components: - Select component for Ant Design combobox interactions (with type-to-filter) - CreateDatasetPage page object with component-based selectors Refactor test file to initialize page objects only in tests that use them. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add E2E test for editing a dataset description: - Click edit action to open DatasourceModal - Update description field - Save with confirmation dialog - Verify toast and persisted change Add supporting components: - EditDatasetModal using Modal base class and component abstractions - ConfirmDialog for reusable OK/Cancel confirmation dialogs - Textarea component for multi-line text input - Tabs component for tab navigation - clickEditAction method in DatasetListPage 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Address Copilot review comments: - Fix EditDatasetModal passing wrong modalSelector parameter to Modal - Add constructor overloads to Textarea and Select for consistency with Button/Input - Add Page parameter to Checkbox for consistency with other components - Clarify ConfirmDialog constructor comment - Refactor edit test to use throwaway dataset for test isolation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
The create wizard test was failing with strict mode violation because
getByRole('button', { name: 'Dataset' }) matched both the "Datasets"
nav link and the "+ Dataset" button. Added clickAddDataset() method
to DatasetListPage with a regex pattern that specifically targets the
"+ Dataset" button.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <[email protected]>
1. Select.clickOption: Changed from getByTitle() to Ant Design's
.ant-select-item-option class selector since dropdown options
don't have title attributes.
2. Export tests: Changed from waitForEvent('download') to API
response interception. The export uses blob download via fetch
(downloadBlob with createObjectURL), which doesn't trigger
Playwright's download event consistently. Now verifies the
export API response headers (content-type and content-disposition).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <[email protected]>
1. Renamed "bulk export multiple datasets" to "export dataset via bulk select action" - more accurately describes test scope (tests bulk select mechanism, not multi-item export). 2. Added virtual dataset guard in first test - fails fast with clear message if example dataset is missing or not virtual, which is required for duplicate functionality used in other tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add SELECT_SELECTORS constants matching Cypress patterns - Use .getByText() with exact match for option selection - Wait for dropdown visibility before finding options - Remove invalid dataset_type guard (field not in list API response) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add acceptDownloads: true to Playwright config for export tests - Create AceEditor component for Ace Editor interactions - Fix Select.clickOption to handle multiple dropdowns using :not(.ant-select-dropdown-hidden) - Update CreateDatasetPage to use data-test selectors instead of fromRole - Update EditDatasetModal to use AceEditor instead of Textarea for description - Update test to use getDescription() for AceEditor value verification 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Select: Add waitForDropdownClose after selection to avoid stale dropdowns - Select: Support partial text matching for filtered dropdown options - Select: Fix type() to target actual search input inside component - ConfirmDialog: Use specific dialog name to avoid strict mode violations - ConfirmDialog: Add animation wait before clicking OK button - EditDatasetModal: Use specific dialog name selector for reliability - DuplicateDatasetModal: Triple-click to select existing text before fill - DatasetListPage: Use role-based selectors for action buttons 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add duplicateDataset() helper for API-based dataset duplication - Add unzipper dependency for export file verification in tests - Restore test.describe wrapper in dataset-list tests - Simplify test resource tracking and cleanup 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Change sql/schema types from `string?` to `string | null` in DatasetResult (API can return null for these fields) - Flatten test structure: remove test.describe wrapper per project convention (matches "avoid nesting when testing" guidance) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Normalize duplicateDataset() return: handle id at top level or in result - Accept both 200/201 status in response intercept (varies by environment) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add guard in duplicateDataset() to throw if resolvedId is falsy - Handle both response shapes in test (result.id vs top-level id) - Assert duplicateId is truthy before using for cleanup 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
…efix support
- Change duplicateDataset() return type from DatasetResult to { id: number }
to accurately reflect API response (may only return id without full details)
- Use download.saveAs(tempPath) instead of download.path() which can return
null in certain Playwright runners
- Wrap saveAs + unzip in try/finally for reliable temp directory cleanup
- Fix webServer health check to respect PLAYWRIGHT_BASE_URL for app prefix
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Replace page.waitForEvent('download') with API response interception
for the export dataset test. Superset's export uses blob downloads
(createObjectURL + anchor click) which don't trigger Playwright's
download event consistently in app-prefix configurations.
The new approach:
- Intercepts /api/v1/dataset/export/ response directly
- Verifies content-type and content-disposition headers
- Parses zip contents from response body using unzipper.Open.buffer()
- Eliminates temp file handling (simpler, faster)
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Refactor EditDatasetModal to use data-test selectors and Input component - Add SELECTORS for inline-name, lock, and unlock icons - Replace AceEditor description methods with name input methods - Add enableEditMode() to click lock icon before editing - Update edit test to modify dataset name instead of description - Add ConfirmDialog handling for save confirmation - Simplify dataset wizard test to verify UI states without creating Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add E2E test for dataset creation via wizard using Google Sheets database - Create ChartCreationPage page object for the "Create a new chart" wizard - Add database API helpers (apiPostDatabase, apiDeleteDatabase, getDatabaseByName) - Use public Netflix dataset Google Sheet (no auth required) - Test flow: Dataset List → Create Dataset → Chart Creation → Explore Co-Authored-By: Claude Opus 4.5 <[email protected]>
…dation - Fix Select.type() to handle both container and input locators by waiting for nested search input with timeout, falling back to direct fill - Extract expectValidExportZip() helper to reduce duplication in export tests - Add Response type to Playwright imports Co-Authored-By: Claude Opus 4.5 <[email protected]>
Use sibling relationship to main button instead of generic class selector for more reliable dropdown trigger identification. Co-Authored-By: Claude Opus 4.5 <[email protected]>
…ucture - Fix command injection risk: quote URL in curl health check - Fix health check URL: use origin instead of preserving app prefix path - Select.ts: only catch TimeoutError in waitForDropdownClose() and type() - dataset.ts: handle both response shapes in createTestVirtualDataset() - dataset-list.spec.ts: add explicit timeout to expect.poll() - dataset-list.spec.ts: use null instead of empty string for schema Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-authored-by: Copilot <[email protected]>
- Create BulkSelect component for ListView bulk operations - Encapsulates enable(), selectRow(), clickAction() pattern - Update DatasetListPage to use BulkSelect via composition - Reusable for future ChartListPage, DashboardListPage, etc. Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Move BulkSelect from core/ to ListView/ (not an Ant Design wrapper) - Create ListView/index.ts for ListView-specific components - Update imports in DatasetListPage Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Consolidate clickOk() and clickOkIfVisible() into single method with optional timeout parameter (no timeout = strict, with timeout = conditional) - Look up 'examples' database by name instead of hardcoding databaseId=1 - Add API 404 verification to single-delete test (matches bulk-delete) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add TIMEOUT.CONFIRM_DIALOG (2s) for confirmation dialogs - Add TIMEOUT.FILE_IMPORT (30s) for import operations - Replace hardcoded timeout values in dataset-list.spec.ts Co-Authored-By: Claude Opus 4.5 <[email protected]>
sadpandajoe
force-pushed
the
feat-additional-dataset-playwright-tests
branch
from
b1b16ac to
d11046e
Compare
Contributor
There was a problem hiding this comment.
Code Review Agent Run #819970
Actionable Suggestions - 3
-
superset-frontend/playwright/helpers/api/dataset.ts - 1
- Missing API response validation · Line 218-219
-
superset-frontend/playwright.config.ts - 1
- Shell injection risk in webServer command · Line 130-134
-
superset-frontend/playwright/components/core/Textarea.ts - 1
- Selector Escaping Issue · Line 63-63
Additional Suggestions - 3
-
superset-frontend/playwright/pages/CreateDatasetPage.ts - 2
-
Property visibility inconsistency · Line 27-27The page property should be private to match the pattern in other page objects like AuthPage, improving encapsulation.
Code suggestion
@@ -26,1 +26,1 @@ - readonly page: Page; + private readonly page: Page;
-
Hardcoded URL instead of constant · Line 78-78Consider adding a URL constant for 'dataset/add/' to match the pattern in other page objects like AuthPage, which improves maintainability.
-
-
superset-frontend/playwright/components/core/Checkbox.ts - 1
-
Constructor pattern mismatch · Line 39-45The constructor should be overloaded to accept either a string selector or Locator, like other core components (Button, Input), and the unused page field removed for consistency.
-
Review Details
-
Files reviewed - 19 · Commit Range:
0c95457..d11046e- superset-frontend/playwright.config.ts
- superset-frontend/playwright/components/ListView/BulkSelect.ts
- superset-frontend/playwright/components/ListView/index.ts
- superset-frontend/playwright/components/core/AceEditor.ts
- superset-frontend/playwright/components/core/Checkbox.ts
- superset-frontend/playwright/components/core/Select.ts
- superset-frontend/playwright/components/core/Tabs.ts
- superset-frontend/playwright/components/core/Textarea.ts
- superset-frontend/playwright/components/core/index.ts
- superset-frontend/playwright/components/modals/ConfirmDialog.ts
- superset-frontend/playwright/components/modals/DuplicateDatasetModal.ts
- superset-frontend/playwright/components/modals/EditDatasetModal.ts
- superset-frontend/playwright/helpers/api/database.ts
- superset-frontend/playwright/helpers/api/dataset.ts
- superset-frontend/playwright/pages/ChartCreationPage.ts
- superset-frontend/playwright/pages/CreateDatasetPage.ts
- superset-frontend/playwright/pages/DatasetListPage.ts
- superset-frontend/playwright/tests/experimental/dataset/dataset-list.spec.ts
- superset-frontend/playwright/utils/constants.ts
-
Files skipped - 2
- superset-frontend/package-lock.json - Reason: Filter setting
- superset-frontend/package.json - Reason: Filter setting
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at [email protected].
Documentation & Help
AI Code Review powered by
Comment on lines
+218
to
+219
| }); | ||
| const body = await response.json(); |
Contributor
There was a problem hiding this comment.
Missing API response validation
The duplicateDataset function lacks a check for response.ok() before parsing JSON, which could cause exceptions if the API fails. Other functions in this file, like createTestVirtualDataset, include this check for consistency.
Code suggestion
Check the AI-generated fix before applying
Suggested change
| }); | |
| const body = await response.json(); | |
| }); | |
| if (!response.ok()) { | |
| throw new Error(`Duplicate dataset API failed: ${response.status()} ${response.statusText()}`); | |
| } | |
| const body = await response.json(); |
Code Review Run #819970
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Comment on lines
+130
to
+134
| return { | ||
| // Quote URL to prevent shell injection via PLAYWRIGHT_BASE_URL | ||
| command: `curl -f '${healthUrl}'`, | ||
| url: healthUrl, | ||
| reuseExistingServer: true, |
Contributor
There was a problem hiding this comment.
Shell injection risk in webServer command
The curl command uses single quotes around the healthUrl, but if PLAYWRIGHT_BASE_URL contains single quotes, it could lead to shell injection. Switching to double quotes prevents this since URLs typically don't contain double quotes.
Code suggestion
Check the AI-generated fix before applying
Suggested change
| return { | |
| // Quote URL to prevent shell injection via PLAYWRIGHT_BASE_URL | |
| command: `curl -f '${healthUrl}'`, | |
| url: healthUrl, | |
| reuseExistingServer: true, | |
| return { | |
| // Quote URL to prevent shell injection via PLAYWRIGHT_BASE_URL | |
| command: `curl -f "${healthUrl}"`, | |
| url: healthUrl, | |
| reuseExistingServer: true, |
Code Review Run #819970
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| * @param name - The name attribute value | ||
| */ | ||
| static fromName(page: Page, name: string): Textarea { | ||
| const locator = page.locator(`textarea[name="${name}"]`); |
Contributor
There was a problem hiding this comment.
Selector Escaping Issue
The selector construction in fromName could fail if the name parameter includes double quotes, as it directly interpolates without escaping. This might cause locator errors in tests with such names.
Code suggestion
Check the AI-generated fix before applying
- const locator = page.locator(`textarea[name=\"${name}\"]`);
+ const locator = page.locator(`textarea[name=${JSON.stringify(name)}]`);
Code Review Run #819970
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
Adds comprehensive Playwright E2E test coverage for the Dataset List page, covering all major CRUD operations accessible from the list view.
Tests Added (12 total):
Infrastructure Added:
testAssetsfixture for automatic cleanup (datasets, databases)createTestDataset()helper using virtual datasets (duplicate API only works with virtual)datasetListPagefixture for navigation (Playwright-idiomatic beforeEach)waitForGet,waitForPost, etc.)expectStatus,expectStatusOneOf)ENDPOINTSconstants for API path discoverabilityImportDatasetModalpage objectComponents Added:
Select- Ant Design combobox with type-to-filterCheckbox- Checkbox interactions for bulk selectConfirmDialog- OK/Cancel confirmation dialogsEditDatasetModal- Dataset edit modal with lock/unlock, tabs, description editorCreateDatasetPage- Create dataset wizard page objectChartCreationPage- Chart creation wizard (post-dataset creation)BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A - Test-only changes
TESTING INSTRUCTIONS
Note: Tests require Google Sheets connector (
shillelagh[gsheetsapi]) for create/import tests. Tests will skip gracefully if unavailable.ADDITIONAL INFORMATION
Related PRs: