feat: add global async query playwright tests

[drivaspreset]

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:

• Forced dashboard refresh going through the full 202 → poll → done cycle
• Reloading an already-cached dashboard being served synchronously (never touching the async channel)
• A broken chart surfacing a clean error under GAQ instead of hanging, and recovering once fixed
• Rapidly switching a filter value without a superseded (stale) response clobbering the screen
• A dashboard with many charts resolving every chart independently and correctly
• Losing the async-token cookie bouncing chart refresh to /login even though the underlying session stays valid
• Navigating away mid-load and back producing no console errors and re-rendering cleanly
• A native filter's value dropdown populating through the same async pipeline as chart data

A ninth test (global-async-query-sqllab.spec.ts) runs under the separate chromium-sqllab Playwright project and asserts that a simple SELECT in SQL Lab still runs synchronously with GLOBAL_ASYNC_QUERIES enabled, i.e. that the flag doesn't accidentally route SQL Lab traffic through the GAQ polling endpoint.

dashboard-test-helpers.ts gains an optional chartWidth on createDashboardWithCharts, 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_QUERIES enabled, 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).

cd superset-frontend

# dashboard GAQ suite
npx playwright test tests/dashboard/global-async-query.spec.ts

# SQL Lab smoke test (separate project)
npx playwright test tests/sqllab/global-async-query-sqllab.spec.ts --project=chromium-sqllab

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags: GLOBAL_ASYNC_QUERIES
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[bito-code-review]

Bito Review Skipped - Source Branch Not Found

Bito didn’t review this change because the pull request is no longer valid. It may have been merged, or the source/target branch may no longer exist.

[netlify]

✅ Deploy Preview for superset-docs-preview ready!

Name	Link
🔨 Latest commit	d237f3d
🔍 Latest deploy log	https://app.netlify.com/projects/superset-docs-preview/deploys/6abffb21c882a500093eede1
😎 Deploy Preview	https://deploy-preview-43004--superset-docs-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.28%. Comparing base (bc3698b) to head (0658907).
⚠️ Report is 46 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #43004      +/-   ##
==========================================
+ Coverage   82.22%   82.28%   +0.06%     
==========================================
  Files        2995     2995              
  Lines      184993   185098     +105     
  Branches    42818    42832      +14     
==========================================
+ Hits       152107   152307     +200     
+ Misses      30127    30031      -96     
- Partials     2759     2760       +1     

Flag	Coverage Δ
hive	36.30% <ø> (-0.01%)	⬇️
javascript	77.88% <ø> (+<0.01%)	⬆️
mysql	55.32% <ø> (-0.02%)	⬇️
postgres	55.33% <ø> (-0.03%)	⬇️
presto	38.17% <ø> (-0.01%)	⬇️
python	86.24% <ø> (+0.11%)	⬆️
sqlite	55.06% <ø> (-0.02%)	⬇️
unit	79.23% <ø> (+0.14%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[rusackas]

Heya @drivaspreset, nice coverage here on GAQ. @sadpandajoe's review comments are still open though, mostly pointing at duplicated code that could reuse existing helpers (apiPutChart, apiPostChart, the combo box filter component, the button component) - worth folding those in, this file's big enough that the reuse would help.

Codeant's race-condition threads look legit too: the girl/boy race assertion around line 583 doesn't actually confirm the fast request completed before checking for staleness, and the native-filter dropdown test around line 1172 arms its response listener before navigation, so it could pass without the click ever hitting GAQ. Worth tightening those up before merging.

[drivaspreset]

Heya @drivaspreset, nice coverage here on GAQ. @sadpandajoe's review comments are still open though, mostly pointing at duplicated code that could reuse existing helpers (apiPutChart, apiPostChart, the combo box filter component, the button component) - worth folding those in, this file's big enough that the reuse would help.

Codeant's race-condition threads look legit too: the girl/boy race assertion around line 583 doesn't actually confirm the fast request completed before checking for staleness, and the native-filter dropdown test around line 1172 arms its response listener before navigation, so it could pass without the click ever hitting GAQ. Worth tightening those up before merging.

Thanks for the flag! Joe's comments are now folded in — apiPutChart/apiPostChart replace the raw calls, and the setup/response-listener duplication is extracted into helpers (setupDashboardWithBigNumberCharts, trackChartAsyncSignals/trackMultiChartAsyncSignals) in dashboard-test-helpers.ts. The combo-box and apply-button reuse now goes through the existing DashboardFilterBar/Button components instead of hand-rolled locators.

Codeant's two threads were legit — fixed both:

• Girl/boy race assertion: the chart actually keeps showing the stale value the entire time a query is loading (confirmed via trace) rather than blanking, so the old assertion could pass without girl's second request ever completing. Reworked it to anchor on a fresh network-response tracker scoped to the race window instead of display text — which also surfaced that a repeated identical filter selection can hit GAQ's cache-hit shortcut (200, not 202); the fix accepts either as valid proof of completion.
• Native-filter dropdown test: moved the GAQ-signal assertion to run before the dropdown is touched at all, so it's explicit in code (not just a comment) that the async fetch happens during filter-panel init, and opening the dropdown is a separate, decoupled render check.

[sadpandajoe]

The required Playwright job currently times out here because the forced refresh never produces the expected 202; the same failure repeats in the resilience and multi-chart cases, so this PR leaves the required CI check red. Could we provision the GAQ flag/worker for this suite (or keep these specs out of the required matrix) before relying on these assertions?

[drivaspreset]

Taking your second option: the GAQ specs now stand down via the existing
isFeatureEnabled helper when GLOBAL_ASYNC_QUERIES is off, so the required run
collects and skips them instead of timing out — the same pattern the
recently-archived specs use for SOFT_DELETE. The specs themselves are
unchanged, so they start running for real the moment a flag-on server exists.

Your first option is tracked in
https://app.shortcut.com/preset/story/113559: the flag via
GLOBAL_ASYNC_QUERIES

playwright-tests (chromium) and pre-commit are both passing now

[drivaspreset]

Status update after merging master up — all threads addressed and CI is green.

Merge with master

#44659 ("collapse Playwright experimental workflow into E2E playwright-tests")
deleted .github/workflows/superset-playwright.yml, which is where the GAQ job
lived, and removed the INCLUDE_EXPERIMENTAL machinery from
playwright.config.ts and bashlib.sh.

playwright-tests-gaq therefore moved into superset-e2e.yml. I kept it as its
own job rather than folding it in as a step of playwright-tests, because it
needs a Celery worker and a 60s startup window — putting that on the required
path would let a GAQ flake block unrelated PRs. It stays continue-on-error
and outside playwright-tests-required, which still gates only on
playwright-tests.

Review fixes (c9dcafb)

1. Did-the-suite-run gate now counts expected + unexpected + flaky as
   "executed" instead of expected alone, so a run where every test genuinely
   fails reports the real failure rather than blaming an inactive
   GLOBAL_ASYNC_QUERIES.

2. Suite selection is now --project=chromium-gaq instead of a
   hand-maintained three-file list. playwright.config.ts's testMatch — the
   same glob that excludes these specs from the default and sqllab projects —
   is now the single definition of what the suite contains, so a new matching
   spec can't end up running nowhere.

3. DISTRIBUTED_COORDINATION_CONFIG moved out of the shared
   superset_test_config into a new superset_test_config_gaq, following the
   existing _thumbnails / _sqllab_backend_persist_off pattern. It was
   leaking a real Redis backend into every unit-test run via conftest.py,
   because _init_distributed_coordination never clears
   _distributed_coordination. Only the GAQ job points at the new config.

4. zizmor: the GAQ job's six ./.github/actions/cached-dependencies uses
   now carry the # zizmor: ignore[self-repository] suppression the other
   sixteen in the file already had.

Correction on the change-detector thread

The playwright token I added to the frontend pattern is no longer in the
branch. It existed so superset-playwright.yml would trigger its own jobs, and
that file no longer exists — GAQ now lives in superset-e2e.yml, which the
pre-existing e2e token already matches. I kept ^\.github/actions/ and its
test, since composite actions are still matched by nothing else.

CI

All checks pass, including both GAQ legs:

Running with no explicit paths (project selection comes from PLAYWRIGHT_EXTRA_ARGS, if set)
Running 10 tests using 1 worker
GAQ suite: 10 passed, 0 failed, 0 flaky, 0 skipped

0 flaky is the meaningful part: this suite previously reached green via a
retry after tag_name_key / Chart could not be created, so that confirms the
race window is closed rather than simply not hit.

[sadpandajoe]

This job pulls postgres:17-alpine and redis:7-alpine (line 493) straight from Docker Hub, but the sibling playwright-tests job in this same file uses the GHCR mirror instead (ghcr.io/apache/superset/ci/postgres:17-alpine / .../redis:7-alpine) — that switch (#40882) was specifically to avoid Docker Hub's anonymous-pull rate limit hitting shared runner IPs. This job runs on every PR, not just merges, continue-on-error: true, and isn't part of the required check, so a rate-limit hit here fails both matrix legs at container init silently, with no signal to anyone. Should this use the same GHCR-mirrored images as the rest of the workflow?

[drivaspreset]

Good catch — fixed in d56f007. Both services now use the GHCR mirrors, matching every other job in this file:

image: ghcr.io/apache/superset/ci/postgres:17-alpine
image: ghcr.io/apache/superset/ci/redis:7-alpine

Your point about the failure mode being invisible is the part that made this worth fixing rather than leaving: on a continue-on-error job outside the required check, a rate-limit hit at container init produces no signal to anyone, so the suite would silently stop providing coverage while the PR still looked green.

While in this job I also fixed two smaller inconsistencies against the sibling playwright-tests: the step comment still referred to playwright-tests-experimental (deleted by #44659), and "Set safe app root" interpolated ${{ matrix.app_root }} directly into the script where the sibling passes it via env:.

[sadpandajoe]

setupDashboardWithSelectFilter() always builds its chart from BIG_NUMBER_COUNT_SPEC (the saved count metric), but its only caller with a virtual dataset — the filter-dropdown test in global-async-query.spec.ts — creates that dataset via apiPostVirtualDataset, which carries no saved metrics. The chart-data query for that dashboard would fail ("Metric 'count' does not exist"), and the test wouldn't catch it since it only asserts on the filter dropdown's async signals and options, never on the chart itself. Should this take an explicit chart spec (or an ad-hoc metric, like the cold-first-load test does) instead of hardcoding BIG_NUMBER_COUNT_SPEC?

[drivaspreset]

You're right, and it was worse than "would fail" — it was failing, silently. I confirmed the mechanism: CreateDatasetCommand calls fetch_metadata, which only discovers columns and seeds no metrics, so a saved-metric reference against an API-created dataset raises Metric '%(metric)s' does not exist (superset/models/helpers.py:4748). That chart has been erroring on every run of this test, and because the test only ever looked at the filter dropdown, nothing caught it.

Fixed in d56f007:

• setupDashboardWithSelectFilter now takes an optional chartSpec (default unchanged: BIG_NUMBER_COUNT_SPEC), and the doc on that constant now says plainly that its count is the saved metric and will not resolve on an API-created dataset.
• The filter-dropdown test passes BIG_NUMBER_ADHOC_COUNT_SPEC — an ad-hoc COUNT(name), shared as ADHOC_COUNT_NAME_METRIC, which also replaces the inline copy the cold-first-load test had.
• The test now asserts on the chart too, so this can't regress unnoticed: it renders a digit, has no .ant-alert-error, and completes its own 202 -> 200 round trip.

Green on the current run (test 9, both matrix legs).

[sadpandajoe]

This compares each chart's post-refresh text to its own pre-refresh value, so it passes whether the refresh genuinely re-rendered fresh data or the DOM simply never updated at all — birth_names is static, so a correct refresh reproduces the same number as before. The signal-based block above already proves every chart completed its 202→200 network round trip; nothing here proves the DOM actually re-painted with that response. Could this assert against something that must change on a correct refresh, to actually prove the render happened?

[drivaspreset]

Agreed — as written that assertion could not distinguish a real re-render from a DOM that never updated, since birth_names is static and a correct refresh reproduces the same number.

Fixed in d56f007 by giving the test something that must change. The charts now sit on a per-run virtual dataset that stamps each query with the server clock:

SELECT name, CAST(EXTRACT(EPOCH FROM CURRENT_TIMESTAMP) * 1000 AS BIGINT) AS queried_at_ms FROM birth_names

A ninth chart shows MAX(queried_at_ms), and the refresh must make it strictly increase:

await expect
  .poll(readClock, { timeout: TIMEOUT.CHART_RENDER })
  .toBeGreaterThan(clockBefore);

That is the render-level proof the network block can't give: a stale DOM fails it. The chart carries y_axis_format: ',d' because the default SMART_NUMBER rounds two stamps seconds apart to the same 1.76T.

The eight per-name identity checks stay exactly as they were — with the repaint now established independently, "each chart still shows its own count" becomes a real anti-shuffling check rather than something that passes by default.

Passing on the current run (test 8, both legs).

[sadpandajoe]

This can capture the pre-filter value as expectedGirlText: the chart already shows a digit (the previous/unfiltered value) while girl's query is still in flight, and toHaveText(/\d/) — unlike the signal-based checks added later in this same test — doesn't wait for girl's request to actually resolve before the text is read. If girl's query is slow here, this baseline ends up wrong, and the race assertion below is then checked against it. Should this wait for a network signal (as the rest of the test does) before capturing the baseline?

[drivaspreset]

Correct — and it's the same trap the later part of this test already documents: the chart keeps displaying the previous value while a query is in flight, so toHaveText(/\d/) is satisfied by the unfiltered total and the baseline silently becomes wrong whenever girl's query is slow.

Fixed in d56f007 by gating the baseline on the network, the way the race assertion below it already does:

const baselineSignals = trackGaqSignals(page);
await filterBar.selectOption('girl');
await filterBar.apply();
await expect(() => {
  const statuses = baselineSignals.submitStatusesFor(chartId);
  expect(statuses).not.toHaveLength(0);
  expect(statuses.every(s => s === 200 || s === 202)).toBe(true);
  expect(statuses[statuses.length - 1]).toBe(200);
}).toPass({ timeout: TIMEOUT.CHART_RENDER });

Ending on a 200 covers both legitimate shapes — a straight cache hit, or 202 then 200 — while rejecting a 4xx/5xx that would otherwise leave stale pixels on screen for the text read to pick up. Only then is expectedGirlText captured.

Passing on the current run (test 2, both legs).

[sadpandajoe]

This says the SQL Lab smoke check needs the chromium-sqllab project, but chromium-sqllab's testIgnore deliberately excludes it (see playwright.config.ts) — it only runs under chromium-gaq, same as these dashboard specs. A maintainer following this comment to run the smoke test with --project=chromium-sqllab would collect zero tests. Can this reference chromium-gaq instead?

[drivaspreset]

You're right — the comment said the opposite of what the config does. Fixed in d56f007:

 * SQL Lab's smoke check lives in tests/sqllab/ but runs under the same
 * `chromium-gaq` project as these specs -- `chromium-sqllab` deliberately
 * excludes it via `testIgnore`, since it needs the flag only the GAQ job sets.

I also updated the header of global-async-query-sqllab.spec.ts itself, which described how that project gets selected in terms that predated the switch to --project=chromium-gaq.

[sadpandajoe]

On a loaded CI runner this can pass without actually proving a race. submitStatusFor(chartId) returns whichever chart-data response arrives first for this slice, not specifically "girl"'s -- "boy"'s request only reaches the server once its route-intercepted delay elapses, and that delay starts as soon as boy is applied, before girl's own select/apply cycle even runs. If that UI cycle plus network latency eats more than the 500ms margin left over from RACE_DELAY_MS, boy's response can land inside this window and satisfy the check instead of girl's, so the race this asserts isn't reliably what gets observed. Could this correlate the response with the filter that produced it (e.g. the request params) instead of relying on first-response ordering?

[drivaspreset]

Good catch, and it's worse than the window being tight — the check could pass on boy's response outright. submitStatusFor(chartId) keys only on slice id, and both requests are for that slice, so it returns whichever landed first.

The arithmetic: boy unparks at boyApplied + 3000, while the window ran from girlApplied for 3000 - 500. Boy lands inside it whenever girl's select/apply cycle exceeds 500ms — easy on a loaded runner — and the test goes green without girl ever completing.

Fixed in 686ba1e by correlating on the payload, as you suggested. A dashboard merges its native filters in as { col, op: 'IN', val: [...] } clauses (getSelectExtraFormData, src/filters/utils.ts), so the request identifies itself:

const statusesFor = (value: string) =>
  signals.submitStatusesWhere(chartId, body =>
    nativeFilterValuesIn(body, FILTER_COLUMN).includes(value),
  );

trackGaqSignals now keeps each chart-data request body alongside its status and exposes submitStatusesWhere; nativeFilterValuesIn reads those clauses back. submitStatusesFor is unchanged, so no other caller is affected.

Two supporting changes, about the window rather than the correlation:

• It's now measured — RACE_DELAY_MS - (Date.now() - boyAppliedAt) — rather than a hard-coded RACE_DELAY_MS - 500 counted from the wrong event, so it cannot silently shrink to nothing, and the test fails loudly if girl was applied after boy's delay had already elapsed.
• RACE_DELAY_MS 3000 -> 8000, so that UI cycle plus latency has room.

The assertion also now requires boy to still be parked while girl completes, so a run where the delay failed to hold boy back reports itself instead of quietly testing nothing.

Passing on the current run (test 2, both matrix legs, 0 flaky).

[sadpandajoe]

This final wait can't actually prove the anti-clobber behavior it's testing. When "girl" is applied, the app aborts the in-flight "boy" request client-side (chartAction.ts's setTimeout(() => prevController.abort(), 0), fired as soon as the new query starts) rather than letting it run to completion and discarding a late response. That abort happens almost immediately, well before this route's artificial RACE_DELAY_MS delay would release "boy"'s response -- so the browser cancels that request outright, and no late "boy" response is ever possible through this path for the code below to wait out. The waitForTimeout(RACE_DELAY_MS + 2000) + re-assert would pass the same way whether or not the app's supersession logic is correct.

Could the route handler hold "boy" open without letting the click complete (e.g. delay before route.continue() on the first interception, keeping the request itself in flight rather than resolved-then-aborted), or is there another way to get a genuine late response into this window?

[drivaspreset]

You're right, and thanks for chasing it to chartAction.ts — I confirmed the abort at line 813:

setTimeout(() => prevController.abort(), 0);

So the browser cancels "boy" as soon as "girl"'s query starts, long before the route delay would release anything. No late response is reachable through this path, which makes both the final waitForTimeout + re-assert and the statusesFor('boy') emptiness check I added vacuous — they pass whether or not supersession works.

Fixed in 50d2bf7 by asserting the mechanism that actually protects the screen: the cancellation itself.

expect(
  boyCancelled,
  '"boy" should have been cancelled client-side when "girl" superseded it',
).toBe(true);

boyCancelled is set from either signal, since which one fires depends on when the abort lands relative to the parked route: page.on('requestfailed') for the aborted request, or the route.continue() rejection that follows an already-aborted one (previously an unhandled rejection, now caught — that rejection is the expected path here, not a failure).

I kept the wait rather than deleting it, but for a narrower, honest reason now stated in the comment: it is the window a regressed cancellation would let a stale result land in, with "boy" asserted to have produced no response at all.

I could not verify locally which of the two signals fires — this stack can't run the suite — so it mattered that CI exercised it: test 2 passes on both matrix legs, 10 passed, 0 flaky.

[sadpandajoe]

boyAppliedAt is captured after filterBar.apply()'s UI promise resolves, not when the intercepted request actually starts the RACE_DELAY_MS timer in the route handler above. If that UI cycle is slow on a loaded runner, remainingParkMs (line 233) overstates how much of the delay is actually left, and the statusesFor('boy')).toHaveLength(0) check at line 268 can fail on a perfectly healthy run because "boy" is legitimately released before the computed deadline. Could the delay's start be measured from the route interception itself (e.g. a timestamp set inside the page.route handler) rather than from the client-side apply() return?

[drivaspreset]

Correct — and the consequence was worse than an overstated deadline: statusesFor('boy').toHaveLength(0) could fail on a perfectly healthy run, exactly as you describe.

Fixed in 50d2bf7. The timestamp is now stamped where the delay actually starts, inside the route handler:

await page.route(raceRoute, async route => {
  matchCount += 1;
  const isBoy = matchCount === 1;
  if (isBoy) {
    boyParkedAt = Date.now();
    await new Promise(resolve => { setTimeout(resolve, RACE_DELAY_MS); });
  }
  ...

boyAppliedAt and the derived remainingParkMs are gone; every remaining use measures from boyParkedAt. The brittle check you flagged is gone too — per the thread above, the test now asserts the cancellation rather than "boy" having produced no response within a computed window.

[sadpandajoe]

This can still fail a healthy run: girl may start at 1s and correctly cancel boy, but a response at 9s makes the 8s assertion fail because the deadline is checked after the response wait. Could this compare girl's request-start time with boyParkedAt instead?

[drivaspreset]

Right again — the deadline was checked after the block that waits for girl's response, so Date.now() there was response-arrival time rather than when girl's request started. Your 1s/9s case fails a run that did exactly the right thing.

Fixed in e9205bd. Both moments are stamped at interception and compared stamp to stamp, so response latency no longer enters it:

expect(
  girlRequestedAt - boyParkedAt,
  '"girl" should have started while "boy" was still parked',
).toBeLessThan(RACE_DELAY_MS);

The route handler also picks its request by payload rather than arrival order now, consistent with how the rest of the test tells the two apart, so matchCount is gone. The only surviving Date.now() is inside a waitForTimeout floored at Math.max(..., 0), so it cannot fail a run.

Since this was the third round on this test, I went back over the whole thing rather than just the reported line, and found something worse in 1ba7472:

the test could not detect the clobber it is named after. The baseline applied "girl", and applying "boy" parks its request while the chart keeps rendering its previous value — so the screen showed girl's number from the baseline onward and toHaveText(expectedGirlText) was already true before the race began. It still worked as a clobber detector, since a stale "boy" result would change the number, but nothing established that "boy" and "girl" render different numbers. Had they matched, a clobber would have been invisible and every assertion would still have passed. The multi-chart test guards this explicitly; this one did not.

Both counts are now read up front through the same payload-correlated round-trip wait, and asserted to differ before the race runs:

expect(
  girlText,
  '"boy" and "girl" should render different counts, or a clobber would be invisible',
).not.toBe(boyText);

The final check is then a real invariance check — still "girl", and necessarily not "boy" — with the mechanism (cancellation, and "boy" having produced no response) asserted separately rather than one standing in for the other. Also dropped raceResultText, which was captured right after asserting the value equalled expectedGirlText and so could only ever hold that same string.

Green on both matrix legs, 10 passed, 0 failed, 0 flaky, 0 skipped.

[sadpandajoe]

errorAlert is the same locator before and after forceRefresh(), and the refreshed metric is identical to the one that already failed on initial load -- so the checks at lines 100-102 (visible, "Data error", BAD_COLUMN) would pass identically whether the refresh's own query ever completed or the DOM simply never changed. Only the signals check at lines 104-113 (202 + a status-changes poll) proves a fresh submission happened, and even that only proves the task was queued and polled, not that its failure is what's actually rendered. Would asserting on something that changes across the refresh (e.g. a fresh task id in the poll response, or clearing/re-showing the alert element around the forceRefresh() call) close that gap?

[drivaspreset]

Agreed on both halves: the alert is already on screen from the initial load, and the refresh re-runs the identical broken query, so those three assertions hold up whether the refresh completed or the DOM never changed.

Fixed in 50d2bf7 by requiring the failure to come back over the wire on the refresh's own traffic, before the alert is looked at:

expect(
  responseBodies.filter(body => body.includes(BAD_COLUMN)),
  "the refresh's own chart-data response should carry the failure, rather than the alert being left over from the initial load",
).not.toHaveLength(0);

responseBodies is captured from a listener attached alongside trackGaqSignals, i.e. after the initial load has settled, so only the refresh's responses can satisfy it. That is stronger than the fresh-task-id option you suggested: it is not just that a new task was queued and polled, but that the rendered failure is the one this cycle produced.

The alert assertions still run, moved below so they read as confirming the rendering rather than carrying the proof.

[sadpandajoe]

This header says the file covers "a lost channel token" scenario, but none of the four tests below exercises losing the async-token cookie (that case is described in the PR body as living elsewhere). Also, the comment at lines 213-215 ("any chart-data response seen inside the race window belongs to 'girl'") is stale against the code that follows it: lines 240-243 and 265-268 specifically handle the case where "boy"'s response can also land in that window, via payload-correlated statusesFor rather than raw arrival order. Worth trimming the header to what this file actually covers, and updating/removing the now-inaccurate "belongs to girl" line?

[drivaspreset]

Both correct. Fixed in 50d2bf7 — the header now names what the four tests here actually cover:

 * Global Async Queries (GAQ) under stress: a query that fails, a superseded
 * query racing a newer one, a programmatic request that must stay synchronous,
 * and a page torn down mid-flight.

The "belongs to 'girl'" line is gone; it had been true of the original arrival-order check and stopped being true the moment the payload-correlated statusesFor landed. Both strings are now at zero occurrences in the file.

While here I also cut the comment blocks that had drifted into restating the code rather than explaining it, across this file, dashboard-test-helpers.ts and the SQL Lab spec (comment lines 100 -> 92, 242 -> 220, 43 -> 34).

[sadpandajoe]

This .github/actions/ pattern (and its own comment, which says these composite actions are "every Playwright job's" shared setup, explicitly including setup-backend) is added only under "frontend", not "python". setup-backend is a plain backend/Python setup action, not a frontend one -- a PR that changes only .github/actions/setup-backend/ would set frontend=true but leave python=false, so the Python unit/integration jobs gated on that output would skip re-running despite their own setup changing. Should this pattern (or at least the setup-backend/change-detector actions) also be added under "python"?

[drivaspreset]

Good catch — setup-backend is a backend action, and gating it on "frontend" alone meant a PR touching only .github/actions/setup-backend/ set frontend=true, python=false, and skipped the Python jobs whose own setup had changed.

Fixed in 50d2bf7: the pattern is now in both groups, and the comment no longer claims these are only the Playwright jobs' setup.

.github/actions/setup-backend/action.yml  -> python=True  frontend=True
.github/actions/cached-dependencies       -> python=True  frontend=True

The test is parametrized over both groups rather than asserting only frontend, so the same gap can't reopen on one side:

@pytest.mark.parametrize("group", ["frontend", "python"])
def test_composite_action_changes_trigger_tests(group: str) -> None:

[drivaspreset]

@sadpandajoe — before you re-review, a note on something I did between rounds, so you aren't re-deriving it.

Rather than keep fixing reported lines one at a time, I went back over all of your review comments on this PR and grouped them. They fall into four themes, and one of them is 11 of the last 12:

Theme	Status
Reuse / DRY — use the existing API helpers, extract duplicated fixture setup, split the file	resolved
CI plumbing — required matrix, if: success(), change detector, zizmor, GHCR mirrors, shared test config	resolved
Assertions that cannot fail	the recurring one
Comments that contradict the code	resolved

The third theme is one defect in four shapes:

1. Asserting state that was already true before the action — the multi-chart text comparison; errorAlert before and after forceRefresh(); the pre-filter expectedGirlText baseline.
2. Not identifying which of several in-flight requests is being asserted — submitStatusFor returning whichever responded first; toBeDefined() accepting a 4xx.
3. Asserting something the implementation makes unreachable — the anti-clobber wait, since chartAction.ts aborts the superseded request.
4. Leaving a component that can fail silently unasserted — the chart in the filter-dropdown test, which had been erroring on every run unnoticed.

So I audited every test in the suite for the same shapes instead of waiting for the next report. Three sites had it and were not yet reported:

• forced dashboard refresh goes through the GAQ 202 -> poll -> done cycle — asserted the chart was visible and showed a digit after refreshing. Both were already true beforehand, on static data that reproduces the same number: the same defect you reported for the multi-chart test. Its chart now renders the query clock, so the refresh has to advance a value that cannot advance on its own.
• reloading an already-cached dashboard... — asserted only "a digit" after reloading. A cache hit has to reproduce the cached result, so it now asserts that exact value.
• navigating away mid-load and back... — same: returning must show the value it had before leaving, not merely some number.

The query-clock dataset and chart spec were inline in the multi-chart test; they are now shared helpers (createQueryClockDataset, BIG_NUMBER_QUERY_CLOCK_SPEC, readQueryClock) used by both, rather than duplicating fixture setup the way the August comments called out.

I also audited the four surviving toHaveText(/\d/) uses and deliberately kept them: two capture a baseline for a later exact-value assertion, one proves a cold first load rendered from nothing, and one proves recovery from an error alert to a value. All can fail.

Everything above is green — 80 checks, and the suite reports 10 passed, 0 failed, 0 flaky, 0 skipped on both matrix legs.

[sadpandajoe]

Parking boy before its POST reaches Superset only tests cancellation of the submission, so dropping the AbortSignal forwarded to waitForAsyncData would leave accepted, superseded jobs polling/running without failing this test. Could we also switch filters after boy returns 202 and assert cancellation of its task IDs, or cover that signal forwarding at the chartAction boundary?

[drivaspreset]

You're right, and this one was a coverage gap rather than a weak assertion — thanks for pushing on it.

Supersession has two halves, and the test only covered the first. chartAction.ts forwards its abort signal into the wait:

// The optional signal lets a caller abort the wait (Stop pressed, chart
// superseded or unmounted), cancelling the outstanding tasks.
return waitForAsyncData(json as AsyncJob, refetch, signal);

and asyncEvent.ts is what turns that into actual cancellation:

waiter.onAbort = () => {
  unregister(waiter);
  cancelUnwaitedTasks(taskIds, submitTabId);   // -> POST /api/v1/task/<id>/cancel
  reject(new DOMException('Aborted', 'AbortError'));
};

Parking "boy" before its POST reaches Superset means it never reaches 202, so waitForAsyncData is never entered and none of that runs. Dropping the signal argument would leave accepted, superseded jobs polling and running with nobody waiting, and the test would stay green — exactly as you describe.

Added the other half in 0658907, as a second test rather than changing the existing one, since cancelling the submission is also a real path worth keeping:

• A per-run cache-cold dataset, so "boy" is guaranteed to be accepted (202) rather than served synchronously from a warm cache.
• The task-status poll is stalled rather than the submission. "boy" is accepted normally, but the client cannot observe its tasks finishing, so its waiter is still registered when "girl" aborts it. Without that the tasks could complete first and leave nothing to cancel — the test would pass while asserting nothing.
• The assertion is on the cancel calls for "boy"'s own task ids, read out of its 202 response:

await expect
  .poll(() => cancelledTaskIds, { ... })
  .toEqual(expect.arrayContaining(boyTaskIds));

I took your first suggestion over the second deliberately: asserting the /cancel calls tests the observable consequence, so it still holds if the signal-forwarding is ever refactored, whereas asserting at the chartAction boundary would pin the current wiring.

Suite is 11 tests now. Both matrix legs pass on this commit.

[bito-code-review]

Code Review Agent Run #2bcbf3

Actionable Suggestions - 0

Additional Suggestions - 3

• superset-frontend/playwright/tests/dashboard/dashboard-test-helpers.ts - 2

  • NaN on missing text · Line 767-767
    `Locator.textContent()` resolves to `string | null`; on `null`, `null?.replace(...)` yields `undefined` and `Number(undefined)` returns `NaN`, so `readQueryClock` can return `NaN` when the element detaches mid-`expect.poll` (see `global-async-query.spec.ts` polls). Assertions then fail as 'NaN to be greater than...' instead of a real cause. Throw on null text before converting. ([CWE-20](https://cwe.mitre.org/data/definitions/20.html))
  • Dialect-locked SQL stamp · Line 761-762
    `EXTRACT(EPOCH FROM CURRENT_TIMESTAMP)` is PostgreSQL/Oracle syntax; on SQLite or MySQL examples backends the dataset query would fail at execution. The repo does not pin the `examples` database backend for the Playwright environment, so this looks backend-dependent. If PostgreSQL-only is guaranteed, documenting that constraint in the docblock would make it explicit.
• superset-frontend/playwright/tests/dashboard/global-async-query.spec.ts - 1

  • Nullish fallback weakens assertion · Line 225-225
    `value.textContent()` is typed `string | null`, so `cachedText ?? ''` turns a failed capture into an assertion that the reloaded chart is empty — a vacuous pass with a misleading message instead of a clear one. Line 212's `toHaveText(/\d/)` makes null unlikely today, but the fallback still degrades diagnostics if the DOM changes; assert the capture explicitly before using it.

Filtered by Review Rules

Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.

• scripts/change_detector.py - 1

  • Over-broad action pattern · Line 57-57

Review Details

• Files reviewed - 9 · Commit Range: f5ffe36..0658907

  • superset-frontend/playwright/tests/dashboard/dashboard-test-helpers.ts
  • superset-frontend/playwright/tests/dashboard/global-async-query-resilience.spec.ts
  • superset-frontend/playwright/tests/dashboard/global-async-query.spec.ts
  • superset-frontend/playwright/tests/sqllab/global-async-query-sqllab.spec.ts
  • superset-frontend/playwright/utils/constants.ts
  • tests/unit_tests/utils/oauth2_tests.py
  • tests/unit_tests/utils/screenshot_test.py
  • scripts/change_detector.py
  • tests/unit_tests/scripts/change_detector_test.py
• Files skipped - 1

  • .github/workflows/superset-e2e.yml - Reason: Filter setting
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • 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 an incremental AI Review.

• /review full - 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 evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by