bito-code-review[bot] commented on code in PR #43004:
URL: https://github.com/apache/superset/pull/43004#discussion_r4041559625
##########
superset-frontend/playwright.config.ts:
##########
@@ -119,6 +124,8 @@ export default defineConfig({
// via API with unique names — no shared mutable state between tests.
name: 'chromium-sqllab',
testMatch: '**/tests/sqllab/**/*.spec.ts',
+ // See the chromium-gaq project below.
+ testIgnore: '**/global-async-query*.spec.ts',
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>GAQ spec moved out of sqllab project</b></div>
<div id="fix">
`testIgnore: '**/global-async-query*.spec.ts'` on `chromium-sqllab` also
excludes `tests/sqllab/global-async-query-sqllab.spec.ts`, whose header says it
lives in `tests/sqllab/` to run under `chromium-sqllab` and needs only the
flag, not Redis/Celery. It now only runs under `chromium-gaq` (requires
`INCLUDE_GAQ` full infra, `fullyParallel` default true), contradicting that
documented design. Reconcile the ignore pattern or update the spec comment.
</div>
</div>
<small><i>Code Review Run #66870c</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/playwright/tests/dashboard/global-async-query.spec.ts:
##########
@@ -0,0 +1,419 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * Global Async Queries (GAQ): the pipeline works for each of its consumers --
+ * a cold first load, a forced refresh, the cache-hit shortcut that bypasses
the
+ * cycle entirely, many charts at once, and native filter value lookups.
+ *
+ * Failure and edge-case behavior lives in
global-async-query-resilience.spec.ts.
+ * SQL Lab's smoke check lives in tests/sqllab/, which needs the
+ * `chromium-sqllab` project rather than this directory's default one.
+ *
+ * Requires the `GLOBAL_ASYNC_QUERIES` feature flag, Redis, and a running
+ * Celery worker -- without a worker, submissions still return 202 but no job
+ * ever executes and these tests time out. The cache-hit test is the exception:
+ * it is served synchronously and needs only the flag.
+ */
+import { testWithAssets, expect } from '../../helpers/fixtures';
+import { apiPostVirtualDataset } from '../../helpers/api/dataset';
+import { getDatabaseByName } from '../../helpers/api/database';
+import { extractIdFromResponse } from '../../helpers/api/assertions';
+import { TIMEOUT } from '../../utils/constants';
+import {
+ BIG_NUMBER_COUNT_SPEC,
+ bigNumberValueLocator,
+ createDashboardWithCharts,
+ setupDashboardWithBigNumberCharts,
+ setupDashboardWithSelectFilter,
+ trackGaqSignals,
+} from './dashboard-test-helpers';
+import { DashboardPage } from '../../pages/DashboardPage';
+import { isFeatureEnabled } from '../../helpers/featureFlags';
+
+testWithAssets.beforeEach(async ({ page }) => {
+ await page.goto('chart/list/');
+ testWithAssets.skip(
+ !(await isFeatureEnabled(page, 'GLOBAL_ASYNC_QUERIES')),
+ 'GLOBAL_ASYNC_QUERIES is not enabled on this instance',
+ );
+});
+
+testWithAssets(
+ 'forced dashboard refresh goes through the GAQ 202 -> poll -> done cycle',
+ async ({ page, testAssets }) => {
+ const { dashboard, charts, valueLocators } =
+ await setupDashboardWithBigNumberCharts(
+ page,
+ testAssets,
+ testWithAssets.info(),
+ {
+ datasetName: 'birth_names',
+ chartNamePrefix: 'gaq_tc1_cold_cache',
+ chartSpecs: [BIG_NUMBER_COUNT_SPEC],
+ },
+ );
+ const [chart] = charts;
+ const [value] = valueLocators;
+ await expect(value).toBeVisible({ timeout: TIMEOUT.CHART_RENDER });
+
+ // Track only after the initial load settles, so these signals describe the
+ // forced refresh rather than the load that preceded it.
+ const signals = trackGaqSignals(page);
+
+ // A fresh chart's query can still collide with an identical one another
+ // suite already cached, so force the refresh: forced requests take the
+ // async path regardless of cache state.
+ await dashboard.forceRefresh();
+ await expect(value).toBeVisible({ timeout: TIMEOUT.CHART_RENDER });
+ await expect(value).toHaveText(/\d/);
+
+ await expect(() => {
+ expect(
+ signals.submitStatusFor(chart.id),
+ 'forced chart-data submission should be accepted (202) onto the async
path',
+ ).toBe(202);
+ expect(
+ signals.sawTaskStatusPoll,
+ 'the client should have polled /api/v1/task/status_changes while the
tasks ran',
+ ).toBe(true);
+ expect(
+ signals.submitStatusesFor(chart.id),
+ 'the client should re-issue chart-data once the tasks finish and be
served 200 from the warmed cache',
+ ).toEqual([202, 200]);
+ }).toPass({ timeout: TIMEOUT.CHART_RENDER });
+ },
+);
+
+testWithAssets(
+ 'a cold first load resolves through the GAQ cycle with no manual refresh',
+ async ({ page, testAssets }) => {
+ testWithAssets.setTimeout(TIMEOUT.SLOW_TEST);
+
+ const examplesDb = await getDatabaseByName(page, 'examples');
+ if (!examplesDb) {
+ throw new Error('examples database not found');
+ }
+
+ // The test above forces a refresh, because a fresh chart over a shared
+ // physical table can collide with a query another suite already cached and
+ // a forced request takes the async path regardless of cache state. That
+ // leaves the unforced first load -- the one a real user gets --
unasserted.
+ //
+ // Same fix as the native-filter test below: a per-run SQL comment keeps
the
+ // query text, and so its cache key, unique. This load is therefore
+ // guaranteed cold, and the async cycle can be asserted on the initial
+ // render itself rather than on a refresh that follows it.
+ const uniqueSuffix =
`${Date.now()}_${testWithAssets.info().parallelIndex}`;
+ const datasetResp = await apiPostVirtualDataset(page, {
+ database: examplesDb.id,
+ schema: '',
+ table_name: `gaq_cold_first_load_${uniqueSuffix}`,
+ sql: `SELECT name FROM birth_names /* run:${uniqueSuffix} */`,
+ editors: [],
+ });
+ expect(datasetResp.ok()).toBe(true);
+ const datasetId = await extractIdFromResponse(datasetResp);
+ testAssets.trackDataset(datasetId);
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicated dataset setup block</b></div>
<div id="fix">
This recipe — `getDatabaseByName` guard, per-run `uniqueSuffix`,
`apiPostVirtualDataset`, `ok()` assert, `extractIdFromResponse`, `trackDataset`
— repeats nearly verbatim at lines 346-366 for the filter test, and the comment
at line 119 already hand-syncs the two ('Same fix as the native-filter test
below'). Extract a shared helper (e.g. in `dashboard-test-helpers.ts`) so the
next fix lands once.
</div>
</div>
<small><i>Code Review Run #66870c</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/playwright/tests/sqllab/global-async-query-sqllab.spec.ts:
##########
@@ -0,0 +1,78 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * Global Async Queries (GAQ): SQL Lab keeps working with the flag turned on.
+ *
+ * SQL Lab's own async execution (`ASynchronousSqlJsonExecutor`) is a separate,
+ * older Celery mechanism that shares no code with GAQ. The only thing worth
+ * confirming is that enabling GLOBAL_ASYNC_QUERIES instance-wide does not leak
+ * into or break the unrelated system next to it -- so this is deliberately one
+ * shallow smoke check, not a SQL Lab suite (see sqllab.spec.ts for that).
+ *
+ * Requires only the `GLOBAL_ASYNC_QUERIES` feature flag; no Redis/Celery,
+ * since nothing here should reach GAQ's pipeline at all.
+ *
+ * Lives in tests/sqllab/ rather than alongside the other GAQ specs so it runs
+ * under the `chromium-sqllab` project: SQL Lab's tab state is server-side per
+ * user and needs sequential execution.
+ */
+import { test, expect } from '../../helpers/fixtures/testAssets';
+import { SqlLabPage } from '../../pages/SqlLabPage';
+import { expectStatus } from '../../helpers/api/assertions';
+import { GAQ, TIMEOUT } from '../../utils/constants';
+import { isFeatureEnabled } from '../../helpers/featureFlags';
+
+let sqlLabPage: SqlLabPage;
+
+test.beforeEach(async ({ page }) => {
+ test.setTimeout(TIMEOUT.SLOW_TEST);
+ sqlLabPage = new SqlLabPage(page);
+ await sqlLabPage.gotoAndReady();
+ test.skip(
+ !(await isFeatureEnabled(page, 'GLOBAL_ASYNC_QUERIES')),
+ 'GLOBAL_ASYNC_QUERIES is not enabled on this instance',
+ );
+});
+
+test('runs a simple SELECT normally with GLOBAL_ASYNC_QUERIES enabled, never
touching the GAQ task-status endpoint', async ({
+ page,
+}) => {
+ let sawTaskStatusPoll = false;
+ page.on('response', response => {
+ if (
+ response.request().method() === 'GET' &&
+ response.url().includes(GAQ.TASK_STATUS_CHANGES_PATH)
+ ) {
+ sawTaskStatusPoll = true;
+ }
+ });
+
+ const response = await sqlLabPage.executeQuery('SELECT 1 AS test_col');
+ expectStatus(response, 200);
+
+ await sqlLabPage.waitForQueryResults('test_col');
+ const headers = await sqlLabPage.resultsGrid.getHeaderTexts();
+ expect(headers.some(h => h.includes('test_col'))).toBe(true);
+
+ expect(
+ sawTaskStatusPoll,
+ "SQL Lab execution should never touch GAQ's task-status polling endpoint
-- it has its own, separate async mechanism",
+ ).toBe(false);
+});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Response listener attached too late</b></div>
<div id="fix">
The `page.on('response')` listener is registered inside the test body, but
`beforeEach` already ran `gotoAndReady()`, which loads the full SQL Lab bundle.
Any GAQ task-status poll fired during that load is missed, so
`sawTaskStatusPoll` can only observe responses after this line — the
never-polls assertion at line 74 can pass for the wrong reason. Attaching the
listener in `beforeEach` before navigation covers the whole page lifecycle.
</div>
</div>
<small><i>Code Review Run #66870c</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
