sadpandajoe commented on code in PR #43004:
URL: https://github.com/apache/superset/pull/43004#discussion_r4109339024
##########
.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:
The six new `./.github/actions/cached-dependencies` uses here (this line
plus 286, 296, 300, 304, 308) are missing the `# zizmor:
ignore[self-repository]` suppression every other occurrence of this exact
pattern carries, including the sibling job seven lines above. Once this merges,
that's six new self-repository code-scanning alerts on a file this PR itself
just touched. Can these get the same suppression comment as the rest of the
file?
##########
.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:
chromium-gaq in playwright.config.ts picks up any file matching
`**/global-async-query*.spec.ts`, and that same glob is what excludes those
files from chromium/chromium-sqllab — but this step runs a hardcoded 3-file
list instead of the project itself. A new file matching the glob would be
dropped from the required run by the config but never added here, so it runs
nowhere, and the zero-skip check below only validates the paths it was given,
so nothing catches the gap. Would running `--project=chromium-gaq` here instead
of listing files keep both sides in sync?
##########
.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:
This only checks `.stats.expected` (first-attempt passes) to decide whether
the suite ran. When every GAQ test actually fails — say the worker can't reach
Redis mid-run — `expected` is also 0, so the error says GLOBAL_ASYNC_QUERIES
was inactive instead of naming the real failure; the job still goes red, but
points whoever's debugging it at the wrong layer. Could this also treat
`.stats.unexpected`/`.stats.flaky` > 0 as "the suite ran"?
##########
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:
This lands in the shared `superset_test_config`, which
`tests/integration_tests/test_app.py` builds a module-level app from at import
time, and `tests/conftest.py` imports that module for every test run, not just
this job. `CacheManager._init_distributed_coordination` only sets
`_distributed_coordination` when a config is present and never clears it
otherwise, so once that import runs, the process-wide `cache_manager` singleton
keeps pointing at a real Redis backend even for unit tests that build their app
from `superset.config` (where this defaults to None). Any unit test that
reaches `DistributedLock`/`CoordinationService` without mocking it now silently
takes the Redis path instead of the documented no-op default, and fails
outright wherever `localhost:6379` isn't available. Should this move to a
config scoped to just this job instead of the shared test config?
--
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]
