drivaskbk01 commented on code in PR #43004:
URL: https://github.com/apache/superset/pull/43004#discussion_r4124248550
##########
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.
--
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]
