feat: add global async query playwright tests - #43004
Conversation
|
Bito Review Skipped - Source Branch Not Found |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| await expect(value).toHaveText(/\d/, { | ||
| timeout: RACE_DELAY_MS - 500, | ||
| }); | ||
| const raceResultText = await value.textContent(); |
There was a problem hiding this comment.
Suggestion: The race assertion does not establish that the fast girl request completed. If that request fails or never starts, the pre-existing unfiltered chart value can still satisfy toHaveText(/\d/), remain unchanged during the wait, and differ from the later boy result, causing the test to pass without exercising the intended superseded-response scenario. Capture and assert the expected girl count or correlate the second request and its completion before checking for stale overwrites. [incorrect condition logic]
Severity Level: Major ⚠️
- ⚠️ Race regression test may pass without testing `girl` results.
- ⚠️ Failures in the second filter request can be masked.
- ⚠️ GAQ stale-response coverage becomes unreliable.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/playwright/tests/dashboard/global-async-query.spec.ts
**Line:** 583:586
**Comment:**
*Incorrect Condition Logic: The race assertion does not establish that the fast `girl` request completed. If that request fails or never starts, the pre-existing unfiltered chart value can still satisfy `toHaveText(/\d/)`, remain unchanged during the wait, and differ from the later `boy` result, causing the test to pass without exercising the intended superseded-response scenario. Capture and assert the expected `girl` count or correlate the second request and its completion before checking for stale overwrites.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| expect( | ||
| new Set(displayedValues).size, | ||
| 'each chart filters on a different name, so their counts should not all collapse to the same number (a sign of misrouted/cross-contaminated results)', | ||
| ).toBeGreaterThan(1); | ||
| }, |
There was a problem hiding this comment.
Suggestion: Checking only that the set of displayed values has more than one entry does not verify that each chart displays the result for its own name. Several charts can show the wrong chart's result, or multiple charts can be blank/duplicated while at least two unrelated values remain, and this assertion will still pass. Compare every locator with the expected count for the corresponding NAMES entry, or correlate each chart's final response to its slice. [incorrect condition logic]
Severity Level: Major ⚠️
- ⚠️ GAQ multi-chart test can miss swapped chart results.
- ⚠️ Incorrect dashboard values may appear individually plausible.
- ⚠️ Concurrent response-routing coverage remains incomplete.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/playwright/tests/dashboard/global-async-query.spec.ts
**Line:** 745:749
**Comment:**
*Incorrect Condition Logic: Checking only that the set of displayed values has more than one entry does not verify that each chart displays the result for its own name. Several charts can show the wrong chart's result, or multiple charts can be blank/duplicated while at least two unrelated values remain, and this assertion will still pass. Compare every locator with the expected count for the corresponding `NAMES` entry, or correlate each chart's final response to its slice.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| await dashboard.forceRefresh(); | ||
| await page.goto('/superset/welcome/'); |
There was a problem hiding this comment.
Suggestion: forceRefresh() only waits for the menu click to finish; it does not wait for a chart-data request or loading state. The following navigation can therefore tear down the page before the refresh request is submitted, so the test may pass without testing navigation during an in-flight GAQ request. Arm a request/loading-state promise before triggering the refresh and await that signal before navigating away. [race condition]
Severity Level: Major ⚠️
- ⚠️ Navigation test may skip the in-flight refresh scenario.
- ⚠️ GAQ cancellation and teardown behavior may remain untested.
- ⚠️ Passing results can provide false lifecycle confidence.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/playwright/tests/dashboard/global-async-query.spec.ts
**Line:** 956:957
**Comment:**
*Race Condition: `forceRefresh()` only waits for the menu click to finish; it does not wait for a chart-data request or loading state. The following navigation can therefore tear down the page before the refresh request is submitted, so the test may pass without testing navigation during an in-flight GAQ request. Arm a request/loading-state promise before triggering the refresh and await that signal before navigating away.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| let filterValueSubmitStatus: number | undefined; | ||
| let sawAsyncEventPoll = false; | ||
| page.on('response', response => { | ||
| const request = response.request(); | ||
| const url = response.url(); | ||
|
|
||
| if ( | ||
| request.method() === 'POST' && | ||
| url.includes('/api/v1/chart/data') && | ||
| sliceIdFromChartDataUrl(url) === undefined | ||
| ) { | ||
| filterValueSubmitStatus = response.status(); | ||
| return; | ||
| } | ||
| if (request.method() === 'GET' && url.includes('/api/v1/async_event/')) { | ||
| sawAsyncEventPoll = true; | ||
| } |
There was a problem hiding this comment.
Suggestion: The response listener is installed before dashboard navigation, while FilterValue fetches native-filter options during dashboard initialization. That initialization request has no slice_id and can set filterValueSubmitStatus and sawAsyncEventPoll before the dropdown is opened, allowing the final assertions to pass even if the click does not use GAQ. Reset or attach the listener only after the initial dashboard/filter loading has completed, then correlate the request to the dropdown action. [incorrect condition logic]
Severity Level: Major ⚠️
- ⚠️ Dropdown test may pass without exercising its click path.
- ⚠️ Native filter GAQ coverage can be falsely attributed.
- ⚠️ Regressions in dropdown-triggered fetching may go undetected.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/playwright/tests/dashboard/global-async-query.spec.ts
**Line:** 1121:1137
**Comment:**
*Incorrect Condition Logic: The response listener is installed before dashboard navigation, while `FilterValue` fetches native-filter options during dashboard initialization. That initialization request has no `slice_id` and can set `filterValueSubmitStatus` and `sawAsyncEventPoll` before the dropdown is opened, allowing the final assertions to pass even if the click does not use GAQ. Reset or attach the listener only after the initial dashboard/filter loading has completed, then correlate the request to the dropdown action.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43004 +/- ##
==========================================
+ Coverage 65.60% 66.41% +0.80%
==========================================
Files 2859 2859
Lines 161460 161460
Branches 37191 37191
==========================================
+ Hits 105929 107232 +1303
+ Misses 53499 52190 -1309
- Partials 2032 2038 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
drivaspreset
changed the title
| const { dashboardId, charts } = await createDashboardWithCharts( | ||
| page, | ||
| testAssets, | ||
| testWithAssets.info(), | ||
| { | ||
| datasetName: 'birth_names', | ||
| chartNamePrefix: 'gaq_tc1_cold_cache', | ||
| dashboardTitlePrefix: 'gaq_tc1_cold_cache', | ||
| chartSpecs: [ | ||
| { | ||
| viz_type: 'big_number_total', | ||
| params: { metric: 'count' }, | ||
| }, | ||
| ], | ||
| }, | ||
| ); | ||
| const [chart] = charts; | ||
|
|
||
| const dashboard = new DashboardPage(page); | ||
| await dashboard.gotoById(dashboardId); | ||
| await dashboard.waitForLoad(); | ||
|
|
||
| const value = dashboard | ||
| .getChart(chart.id) | ||
| .locator('.superset-legacy-chart-big-number .header-line'); | ||
| await expect(value).toBeVisible({ timeout: TIMEOUT.CHART_RENDER }); |
There was a problem hiding this comment.
Seeing this in multiple places. Can this be placed as a function or in a before method so it can be reused?
| page.on('response', response => { | ||
| const request = response.request(); | ||
| const url = response.url(); | ||
|
|
||
| if ( | ||
| request.method() === 'POST' && | ||
| url.includes('/api/v1/chart/data') && | ||
| sliceIdFromChartDataUrl(url) === chart.id | ||
| ) { | ||
| chartDataSubmitStatus = response.status(); | ||
| return; | ||
| } | ||
| if (request.method() === 'GET' && url.includes('/api/v1/async_event/')) { | ||
| sawAsyncEventPoll = true; | ||
| return; | ||
| } | ||
| if ( | ||
| request.method() === 'GET' && | ||
| /\/api\/v1\/chart\/data\/qc-/.test(url) | ||
| ) { | ||
| sawFinalCachedFetch = true; | ||
| } | ||
| }); |
There was a problem hiding this comment.
This block of code seems to be repeated elsewhere, can we create a helper function within the test to do this?
| const filterCombobox = page | ||
| .locator('[data-test="form-item-value"]') | ||
| .first() | ||
| .locator('[role="combobox"]'); |
There was a problem hiding this comment.
Do we not have a combo box filter component we can reuse?
| const applyBtn = page.locator( | ||
| '[data-test="filter-bar__apply-button"], [data-test="filterbar-action-buttons"] button[type="submit"]', | ||
| ); |
There was a problem hiding this comment.
probably can use the button component here
| const dashboardId: number = dashBody.result?.id ?? dashBody.id; | ||
| testAssets.trackDashboard(dashboardId); | ||
|
|
||
| const linkResp = await apiPut(page, `api/v1/chart/${chartId}`, { |
There was a problem hiding this comment.
Can we not use apiPutChart?
| const dashboardId: number = dashBody.result?.id ?? dashBody.id; | ||
| testAssets.trackDashboard(dashboardId); | ||
|
|
||
| const linkResp = await apiPut(page, `api/v1/chart/${chartId}`, { |
There was a problem hiding this comment.
Can we not use apiPutChart?
| metric: 'count', | ||
| adhoc_filters: [], | ||
| }; | ||
| const chartResp = await apiPost(page, 'api/v1/chart/', { |
There was a problem hiding this comment.
Can we not use apiPostChart?
| metric: 'count', | ||
| adhoc_filters: [], | ||
| }; | ||
| const chartResp = await apiPost(page, 'api/v1/chart/', { |
There was a problem hiding this comment.
Can we not use apiPostChart?
2 participants
SUMMARY
Adds Playwright E2E coverage for Global Async Queries (GAQ) on dashboards and SQL Lab.
Eight dashboard-level tests cover the GAQ request/response lifecycle end to end:
/logineven though the underlying session stays validA ninth test (
global-async-query-sqllab.spec.ts) runs under the separatechromium-sqllabPlaywright project and asserts that a simpleSELECTin SQL Lab still runs synchronously withGLOBAL_ASYNC_QUERIESenabled, i.e. that the flag doesn't accidentally route SQL Lab traffic through the GAQ polling endpoint.dashboard-test-helpers.tsgains an optionalchartWidthoncreateDashboardWithCharts, so tests can lay out more charts per row than the default single-row 12-column grid would otherwise allow (needed for the many-charts test).BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A - test-only change, no UI behavior modified.
TESTING INSTRUCTIONS
Requires
GLOBAL_ASYNC_QUERIESenabled, plus Redis and a running Celery worker (all tests except the cache-hit-reload case, which is served synchronously and only needs the feature flag).ADDITIONAL INFORMATION
GLOBAL_ASYNC_QUERIES