drivaskbk01 commented on code in PR #43004:
URL: https://github.com/apache/superset/pull/43004#discussion_r4124244643
##########
.github/workflows/bashlib.sh:
##########
@@ -368,6 +383,108 @@ playwright-run() {
return $status
}
+playwright-run-gaq() {
+ # Global Async Queries needs more than a feature flag: submissions are handed
+ # to Celery, so without a worker consuming the queue the API returns 202 and
+ # no job ever runs -- the specs would time out rather than fail usefully.
+ # `playwright-run` boots gunicorn with this step's environment, so the flag
+ # set on the step reaches both the web server and the worker started here.
+ local APP_ROOT=$1
+ shift || true
+ local TEST_PATHS=("$@")
+
+ cd "$GITHUB_WORKSPACE"
+
+ if [ "${SUPERSET_FEATURE_GLOBAL_ASYNC_QUERIES:-}" != "true" ]; then
+ echo "::error::SUPERSET_FEATURE_GLOBAL_ASYNC_QUERIES must be \"true\" for
this step; the specs would skip themselves and report nothing."
+ return 1
+ fi
+
+ local workerlog="${HOME}/superset-gaq-worker.log"
+ say "::group::Start Celery worker for GAQ"
+ # Mirrors docker/docker-bootstrap.sh's worker invocation.
+ nohup celery --app=superset.tasks.celery_app:app worker \
+ -O fair \
+ --loglevel=INFO \
+ --concurrency=2 \
+ >"$workerlog" 2>&1 </dev/null &
+ local workerPid=$!
+
+ # Fail fast on a worker that never comes up: a dead worker is
+ # indistinguishable from a slow one once the specs start timing out.
+ local timeout=60
+ while [ $timeout -gt 0 ]; do
+ if ! kill -0 "$workerPid" 2>/dev/null; then
+ echo "::error::Celery worker exited during startup"
+ cat "$workerlog" || true
+ say "::endgroup::"
+ return 1
+ fi
+ if grep -q "celery@.*ready" "$workerlog" 2>/dev/null; then
+ say "Celery worker is ready"
+ break
+ fi
+ sleep 1
+ timeout=$((timeout - 1))
+ done
+ if [ $timeout -eq 0 ]; then
+ echo "::error::Celery worker failed to become ready within 60 seconds"
+ cat "$workerlog" || true
+ kill "$workerPid" 2>/dev/null || true
+ say "::endgroup::"
+ return 1
+ fi
+ say "::endgroup::"
+
+ # The GAQ specs are excluded from every other project, so this is what makes
+ # them loadable at all -- see the chromium-gaq project in
playwright.config.ts.
+ export INCLUDE_GAQ=true
+
+ local
report="${GITHUB_WORKSPACE}/superset-frontend/playwright-gaq-report.json"
+ rm -f "$report"
+ export PLAYWRIGHT_JSON_OUTPUT_NAME="$report"
+ # --workers=1: the fixtures all create charts as the same admin user, and
+ # Superset's tag listener races on the shared `editor:<id>` tag (see the
+ # chromium-gaq project in playwright.config.ts). That project's
+ # `fullyParallel: false` only orders tests within one file -- Playwright
+ # still runs separate files concurrently -- and this suite spans three, so
+ # one worker is what actually serializes it.
+ export PLAYWRIGHT_EXTRA_ARGS="--reporter=list,json --workers=1"
+
+ # `set -e` is on: without the guard a failing run would exit before the
+ # worker log is emitted and before the did-it-actually-run check below.
+ local status=0
+ playwright-run "$APP_ROOT" "${TEST_PATHS[@]}" || status=$?
+
+ unset PLAYWRIGHT_EXTRA_ARGS PLAYWRIGHT_JSON_OUTPUT_NAME INCLUDE_GAQ
+
+ say "::group::Celery worker log"
+ cat "$workerlog" || true
+ say "::endgroup::"
+ kill "$workerPid" 2>/dev/null || true
+
+ # A suite that skips itself still exits 0. That is the failure mode this step
+ # exists to prevent, so assert that tests actually ran.
+ if [ ! -f "$report" ]; then
+ echo "::error::No Playwright JSON report produced; cannot confirm the GAQ
specs ran."
+ return 1
+ fi
+ local expected skipped
+ expected=$(jq '.stats.expected // 0' "$report")
+ skipped=$(jq '.stats.skipped // 0' "$report")
+ say "GAQ suite: ${expected} passed, ${skipped} skipped"
+ if [ "$expected" -eq 0 ]; then
Review Comment:
Good catch — fixed in c9dcafb8af.
The gate now counts `expected + unexpected + flaky` as "executed" and logs
the breakdown, so a run where the worker loses Redis reports the real failure
instead of accusing GLOBAL_ASYNC_QUERIES of being inactive. The zero-skip
assertion is unchanged.
The line reads like this on the current green run:
```
GAQ suite: 10 passed, 0 failed, 0 flaky, 0 skipped
```
##########
.github/workflows/superset-playwright.yml:
##########
@@ -210,8 +210,142 @@ jobs:
${{ github.workspace }}/superset-frontend/test-results/
name: playwright-experimental-artifact-${{ github.run_id }}-${{
github.job }}-${{ matrix.browser }}--${{
steps.set-safe-app-root.outputs.safe_app_root }}
+ # GAQ runs in its own job rather than as a step in
playwright-tests-experimental
+ # above. A step with no explicit `if:` implicitly inherits `if: success()`,
so
+ # when GAQ was a step after Experimental/Mobile in that job, a failure in
+ # either of those unrelated suites skipped GAQ entirely rather than failing
it
+ # -- silently leaving that commit with zero GAQ coverage instead of a visible
+ # red check. A separate job can't share that fate: it either runs and reports
+ # for itself, or it doesn't start (e.g. the environment itself never came
up),
+ # which is the only case where "no GAQ result" is actually the right outcome.
+ playwright-tests-gaq:
+ needs: changes
+ if: needs.changes.outputs.python == 'true' ||
needs.changes.outputs.frontend == 'true'
+ runs-on: ubuntu-26.04
+ timeout-minutes: 30
+ continue-on-error: true
+ permissions:
+ contents: read
+ pull-requests: read
+ strategy:
+ fail-fast: false
+ matrix:
+ browser: ["chromium"]
+ app_root: ["", "/app/prefix"]
+ env:
+ SUPERSET_ENV: development
+ SUPERSET_CONFIG: tests.integration_tests.superset_test_config
+ SUPERSET__SQLALCHEMY_DATABASE_URI:
postgresql+psycopg2://superset:[email protected]:15432/superset
+ PYTHONPATH: ${{ github.workspace }}
+ REDIS_PORT: 16379
+ GITHUB_TOKEN: ${{ github.token }}
+ services:
+ postgres:
+ image: postgres:17-alpine
+ env:
+ POSTGRES_USER: superset
+ POSTGRES_PASSWORD: superset
+ ports:
+ - 15432:5432
+ redis:
+ image: redis:7-alpine
+ ports:
+ - 16379:6379
+ steps:
+ # -------------------------------------------------------
+ # Conditional checkout based on context (same as Cypress workflow)
+ - name: Checkout for push or pull_request event
+ if: github.event_name == 'push' || github.event_name == 'pull_request'
+ uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 #
v7.0.1
+ with:
+ persist-credentials: false
+ submodules: recursive
+ ref: ${{ github.event_name == 'pull_request' &&
github.event.pull_request.head.sha || github.sha }}
+ - name: Checkout using ref (workflow_dispatch)
+ if: github.event_name == 'workflow_dispatch' &&
github.event.inputs.ref != ''
+ uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 #
v7.0.1
+ with:
+ persist-credentials: false
+ ref: ${{ github.event.inputs.ref }}
+ submodules: recursive
+ - name: Checkout using PR ID (workflow_dispatch)
+ if: github.event_name == 'workflow_dispatch' &&
github.event.inputs.pr_id != ''
+ uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 #
v7.0.1
+ with:
+ persist-credentials: false
+ ref: refs/pull/${{ github.event.inputs.pr_id }}/merge
+ submodules: recursive
+ # -------------------------------------------------------
+ - name: Setup Python
+ uses: $/.github/actions/setup-backend/
+ - name: Setup postgres
+ uses: ./.github/actions/cached-dependencies
+ with:
+ run: setup-postgres
+ - name: Import test data
+ uses: ./.github/actions/cached-dependencies
+ with:
+ run: playwright_testdata
+ - name: Setup Node.js
+ uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 #
v7.0.0
+ with:
+ node-version-file: "./superset-frontend/.nvmrc"
+ cache: "npm"
+ cache-dependency-path: "superset-frontend/package-lock.json"
+ - name: Install npm dependencies
+ uses: ./.github/actions/cached-dependencies
+ with:
+ run: npm-install
+ - name: Build javascript packages
+ uses: ./.github/actions/cached-dependencies
+ with:
+ run: build-instrumented-assets
+ - name: Install Playwright
+ uses: ./.github/actions/cached-dependencies
+ with:
+ run: playwright-install
+ - name: Run Playwright (Global Async Queries Tests)
+ uses: ./.github/actions/cached-dependencies
+ env:
+ NODE_OPTIONS: "--max-old-space-size=4096"
+ # Scoped to this step for the same reason as the embedded and mobile
+ # flags in playwright-tests-experimental: GLOBAL_ASYNC_QUERIES
changes
+ # chart-data behaviour for every spec sharing a server.
+ #
+ # This suite has its own job, and is not required, because unlike the
+ # other flag-gated suites it needs a Celery worker: submissions are
+ # handed to Celery, so without one the API returns 202 and no job
ever
+ # executes. Putting a background worker and its startup window on the
+ # required path would let a GAQ flake block PRs that have nothing to
+ # do with async queries.
+ #
+ # `playwright-run-gaq` starts the worker and sets INCLUDE_GAQ, which
+ # is what makes the chromium-gaq project exist at all.
+ SUPERSET_FEATURE_GLOBAL_ASYNC_QUERIES: "true"
+ with:
+ run: >-
+ playwright-run-gaq "${{ matrix.app_root }}"
+ dashboard/global-async-query.spec.ts
Review Comment:
Fixed in c9dcafb8af. `playwright-run-gaq` now passes
`--project=chromium-gaq` and no spec paths, so the project's `testMatch` — the
same glob that excludes these specs from the default and sqllab projects — is
the single definition of what the suite contains. A new matching spec gets
picked up automatically instead of running nowhere.
`playwright-run`'s no-path branch now forwards `PLAYWRIGHT_EXTRA_ARGS` so
that works; it is empty for every other caller.
From the current green run:
```
Running with no explicit paths (project selection comes from
PLAYWRIGHT_EXTRA_ARGS, if set)
Running 10 tests using 1 worker
```
Note this thread shows as outdated because #44659 deleted
`superset-playwright.yml` while this branch was being merged up. The job moved
into `superset-e2e.yml` carrying this defect with it, so the fix landed there
rather than the thread being stale.
##########
tests/integration_tests/superset_test_config.py:
##########
@@ -156,6 +156,27 @@ def GET_FEATURE_FLAGS_FUNC(ff): # noqa: N802
}
+# Async chart data runs on the Global Task Framework, which reaches Redis
through
+# the coordination service rather than a GAQ-specific cache backend
+# (`GLOBAL_ASYNC_QUERIES_CACHE_BACKEND` was removed with that migration). The
+# config default is `None`, so without this there is no coordinator: task
+# completion is never signalled, submissions return 202 and the client waits
+# forever. Built from the same environment variables as `CACHE_CONFIG` above,
+# with its own DB index so coordination streams stay out of the query cache.
+#
+# Note the discrete host/port/db keys rather than a CACHE_REDIS_URL: the Redis
+# backends read CACHE_REDIS_HOST/PORT/DB and ignore a URL entirely, so
supplying
+# one silently leaves the connection on its localhost:6379 defaults.
+COORDINATION_REDIS_DB = os.environ.get("COORDINATION_REDIS_DB", 5) # noqa:
F405
+DISTRIBUTED_COORDINATION_CONFIG = {
Review Comment:
You're right, and I confirmed the mechanism before changing anything:
`_init_distributed_coordination` does `if not config: return` and never clears
`_distributed_coordination`, so once conftest imports the shared config the
process-wide `cache_manager` singleton keeps a real Redis backend for unit
tests too.
Fixed in c9dcafb8af by moving `DISTRIBUTED_COORDINATION_CONFIG` (and
`COORDINATION_REDIS_DB`) into a new
`tests/integration_tests/superset_test_config_gaq.py`, following the existing
`superset_test_config_thumbnails` /
`superset_test_config_sqllab_backend_persist_off` pattern. Only the GAQ job
points at it:
| job | SUPERSET_CONFIG |
|---|---|
| cypress-matrix | superset_test_config |
| playwright-tests | superset_test_config |
| playwright-tests-gaq | superset_test_config_gaq |
The shared config no longer mentions either name, and both GAQ legs still
pass with the coordinator reachable.
##########
.github/workflows/superset-playwright.yml:
##########
@@ -210,8 +210,142 @@ jobs:
${{ github.workspace }}/superset-frontend/test-results/
name: playwright-experimental-artifact-${{ github.run_id }}-${{
github.job }}-${{ matrix.browser }}--${{
steps.set-safe-app-root.outputs.safe_app_root }}
+ # GAQ runs in its own job rather than as a step in
playwright-tests-experimental
+ # above. A step with no explicit `if:` implicitly inherits `if: success()`,
so
+ # when GAQ was a step after Experimental/Mobile in that job, a failure in
+ # either of those unrelated suites skipped GAQ entirely rather than failing
it
+ # -- silently leaving that commit with zero GAQ coverage instead of a visible
+ # red check. A separate job can't share that fate: it either runs and reports
+ # for itself, or it doesn't start (e.g. the environment itself never came
up),
+ # which is the only case where "no GAQ result" is actually the right outcome.
+ playwright-tests-gaq:
+ needs: changes
+ if: needs.changes.outputs.python == 'true' ||
needs.changes.outputs.frontend == 'true'
+ runs-on: ubuntu-26.04
+ timeout-minutes: 30
+ continue-on-error: true
+ permissions:
+ contents: read
+ pull-requests: read
+ strategy:
+ fail-fast: false
+ matrix:
+ browser: ["chromium"]
+ app_root: ["", "/app/prefix"]
+ env:
+ SUPERSET_ENV: development
+ SUPERSET_CONFIG: tests.integration_tests.superset_test_config
+ SUPERSET__SQLALCHEMY_DATABASE_URI:
postgresql+psycopg2://superset:[email protected]:15432/superset
+ PYTHONPATH: ${{ github.workspace }}
+ REDIS_PORT: 16379
+ GITHUB_TOKEN: ${{ github.token }}
+ services:
+ postgres:
+ image: postgres:17-alpine
+ env:
+ POSTGRES_USER: superset
+ POSTGRES_PASSWORD: superset
+ ports:
+ - 15432:5432
+ redis:
+ image: redis:7-alpine
+ ports:
+ - 16379:6379
+ steps:
+ # -------------------------------------------------------
+ # Conditional checkout based on context (same as Cypress workflow)
+ - name: Checkout for push or pull_request event
+ if: github.event_name == 'push' || github.event_name == 'pull_request'
+ uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 #
v7.0.1
+ with:
+ persist-credentials: false
+ submodules: recursive
+ ref: ${{ github.event_name == 'pull_request' &&
github.event.pull_request.head.sha || github.sha }}
+ - name: Checkout using ref (workflow_dispatch)
+ if: github.event_name == 'workflow_dispatch' &&
github.event.inputs.ref != ''
+ uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 #
v7.0.1
+ with:
+ persist-credentials: false
+ ref: ${{ github.event.inputs.ref }}
+ submodules: recursive
+ - name: Checkout using PR ID (workflow_dispatch)
+ if: github.event_name == 'workflow_dispatch' &&
github.event.inputs.pr_id != ''
+ uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 #
v7.0.1
+ with:
+ persist-credentials: false
+ ref: refs/pull/${{ github.event.inputs.pr_id }}/merge
+ submodules: recursive
+ # -------------------------------------------------------
+ - name: Setup Python
+ uses: $/.github/actions/setup-backend/
+ - name: Setup postgres
+ uses: ./.github/actions/cached-dependencies
Review Comment:
Fixed in c9dcafb8af — your count was exact: six uses missing the
suppression, against sixteen elsewhere in the file that carry it. All six now
have it plus the same rationale comment the others use, and `zizmor (GHA
security audit)` passes on the current run.
Same note as the thread above: this shows as outdated because #44659 deleted
`superset-playwright.yml`, but the six uses moved into `superset-e2e.yml` with
the job, so that is where they were fixed.
##########
.github/workflows/superset-playwright.yml:
##########
@@ -210,8 +210,142 @@ jobs:
${{ github.workspace }}/superset-frontend/test-results/
name: playwright-experimental-artifact-${{ github.run_id }}-${{
github.job }}-${{ matrix.browser }}--${{
steps.set-safe-app-root.outputs.safe_app_root }}
+ # GAQ runs in its own job rather than as a step in
playwright-tests-experimental
+ # above. A step with no explicit `if:` implicitly inherits `if: success()`,
so
+ # when GAQ was a step after Experimental/Mobile in that job, a failure in
+ # either of those unrelated suites skipped GAQ entirely rather than failing
it
+ # -- silently leaving that commit with zero GAQ coverage instead of a visible
+ # red check. A separate job can't share that fate: it either runs and reports
+ # for itself, or it doesn't start (e.g. the environment itself never came
up),
+ # which is the only case where "no GAQ result" is actually the right outcome.
+ playwright-tests-gaq:
+ needs: changes
+ if: needs.changes.outputs.python == 'true' ||
needs.changes.outputs.frontend == 'true'
Review Comment:
Correction to my earlier reply here — that change is no longer in the
branch, and I'd rather flag it than leave the thread reading as resolved on
something that got reverted.
#44659 deleted `superset-playwright.yml` and folded its jobs into
`superset-e2e.yml`. The `playwright` token I had added to the frontend pattern
therefore guarded a file that no longer exists, so I reverted it and its test
while merging master up. GAQ now lives in `superset-e2e.yml`, which the
pre-existing `e2e` token already matches — the gap you found is closed by the
restructure rather than by my change.
What I kept is `^\.github/actions/` and its test: composite actions are
still matched by nothing else, and that half of your point stands on its own.
--
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]
