rusackas opened a new pull request, #43502: URL: https://github.com/apache/superset/pull/43502
### SUMMARY Pilot for real-container testing of `db_engine_specs` against actual databases, using [testcontainers-python](https://testcontainers-python.readthedocs.io/). Existing `tests/unit_tests/db_engine_specs/` tests mock the driver/dialect layer entirely, which structurally cannot catch bugs in how SQLAlchemy actually compiles and executes SQL against a real server — e.g. #42899, where Trino emitted `OFFSET` before `LIMIT` for paginated queries. Only a real round-trip against a live instance catches that class of bug. **Runs nightly, not on every merge to master** (`.github/workflows/nightly-testcontainers.yml`, `schedule` + `workflow_dispatch`, no `push`/`pull_request` trigger). Spinning up real containers is slower and more prone to transient flakiness (image pulls, startup races) than the existing mocked unit tests, and this is explicitly a cost/signal measurement before considering wider adoption — not something to put on the merge-blocking path yet. **Pilot scope: 3 dialects** — `cockroachdb`, `crate`, `trino` — chosen because they're self-hostable via an official Docker image, weren't already covered by real-container CI, and testcontainers-python ships native container classes for all three (`testcontainers.community.{cockroachdb,cratedb,trino}`), keeping the pilot's own code simple. ### Two real bugs found building this - **CockroachDB was completely broken under SQLAlchemy 2.0.** The `cockroachdb` extra installs an abandoned PyPI package whose dialect can't even import under 2.0. Nothing caught it because the driver isn't in the default CI install and the existing mock-only test never constructs a real engine. Fixed separately in #43501 (merge that first; this PR's `cockroachdb` test depends on it). - **testcontainers-python's own `TrinoContainer.get_connection_url()` is broken** — it returns the container's *internal* port instead of the Docker-mapped host port, so the URL it builds can't connect to anything. Worked around locally in `test_trino.py` by building the URL manually via `get_exposed_port()`; worth filing upstream separately. ### Verification - `cockroachdb`, `trino`: ran locally against real containers, all 4 tests pass. - `crate`: **could not run locally** — `crate/crate` only publishes an `amd64` image, and CrateDB's binary requires a host CPU supporting `x86-64-v3`, which QEMU emulation on Apple Silicon can't satisfy even with `docker pull --platform linux/amd64`. Verified the test collects cleanly (imports resolve, no syntax errors); actual execution is CI-only-verifiable on GitHub's x86_64 runners. Flagged in the module docstring so this isn't a silent gap for the next person debugging it. ### Full dialect survey (as requested — enumerating everywhere this could go next) Superset's existing real-container CI coverage today: **Postgres, MySQL, Presto, Redis** (GH Actions `services:`) + **Hive** (custom setup step). Everything below is currently mock-only or completely untested. | Dialect | Current tests | Official Docker image | testcontainers-python module | Status | |---|---|---|---|---| | **cockroachdb** | 0 (real engine untested) | Yes, trivial single-node start | Native (`community.cockroachdb`) | **In this PR** | | **crate** | 4, mock-only | Docker *Official Image* tier | Native (`community.cratedb`) | **In this PR** — amd64-only, see above | | **trino** | 0 real-container (only Presto is covered) | Yes, multi-arch | Native (`community.trino`) | **In this PR** | | databend | 12, mock-only | Yes, explicitly built for CI use | None — needs generic `DockerContainer` | Good next candidate | | risingwave | 0 | Yes, single-container "playground" mode | None — needs generic `DockerContainer` | Good next candidate | | mssql | 12, mock-only | Yes, official Microsoft image | Native (`testcontainers.mssql`) | Good next candidate — native module, low effort | | oracle | 7, mock-only | Yes, official (`oracle`/`oracle-free`) | Native (`testcontainers.oracle`) | Good next candidate — native module, low effort | | db2 | 7, mock-only | Yes, official IBM image | Native (`testcontainers.db2`) | Good next candidate — native module, low effort | | doris | 6, mock-only | Yes, all-in-one | None | Needs AVX2 CPU — flaky on shared runners, lower priority | | starrocks | 12, mock-only | Likely (unverified) | None | Unverified — needs a follow-up check | | elasticsearch, solr, druid, dremio, drill, kylin, impala, pinot, ydb, tdengine, singlestore | Varies (0–14), mock-only | Not verified this pass | elasticsearch has a native module; rest unverified | Worth a dedicated follow-up sweep | | vertica | 0 | Community image only | None | **Excluded** — the community image requires a 2-day license verification and expires after 1 year; unsuitable for public CI | | snowflake, bigquery, athena, redshift, firebolt, databricks, aurora-data-api, d1, denodo, kusto | n/a | Cloud-only / SaaS, no local equivalent | n/a | **Excluded permanently** — nothing to containerize | To add a dialect: drop a `tests/testcontainers/db_engine_specs/test_<dialect>.py` following the pattern in this PR (module-scoped container fixture, a paginated LIMIT/OFFSET round-trip, a `get_columns`/`get_column_spec` introspection check against real server metadata) — no other wiring needed, the nightly workflow already runs everything under `tests/testcontainers/`. ### TESTING INSTRUCTIONS ``` pip install -r requirements/development.txt # now includes testcontainers[cockroachdb,cratedb,trino] pytest tests/testcontainers/db_engine_specs/test_cockroachdb.py tests/testcontainers/db_engine_specs/test_trino.py -v # 4 passed (requires a local Docker daemon) ``` Or trigger the new workflow directly via `workflow_dispatch` from the Actions tab. ### ADDITIONAL INFORMATION - [ ] Has associated issue: - [ ] Required feature flags: - [ ] Changes UI - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351)) - [ ] Migration is atomic, supports rollback & is backwards-compatible - [ ] Confirm DB migration upgrade and downgrade tested - [ ] Runtime estimates and downtime expectations provided - [x] Introduces new feature or API (new nightly CI workflow + `tests/testcontainers/` test category) - [ ] Removes existing feature or API -- 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]
