Repository navigation
feat(ci): add testcontainers-based db_engine_specs tests (7 dialects) - #43502
Merged
Merged
Conversation
rusackas
requested review from
betodealmeida,
dpgaspar,
eschutho,
geido,
hainenber,
kgabryje,
mistercrunch,
nytai,
sadpandajoe,
sha174n and
villebro
as code owners
August 25, 2026 05:55
dosubot
Bot
added
data:connect:cockroachdb
Contributor
Code Review Agent Run #723c20Actionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
github-actions
Bot
added
github_actions
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Contributor
|
The flagged issue is correct. The current test likely only verifies the existence of a column specification without validating that the specific native type (e.g., Since I do not have access to the file content of Regarding other comments on this PR, I have checked the available review data and found no other comments to address. If you have additional comments you would like me to review, please provide them. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43502 +/- ##
=======================================
Coverage 79.89% 79.89%
=======================================
Files 2916 2916
Lines 170755 170761 +6
Branches 39569 39570 +1
=======================================
+ Hits 136429 136437 +8
+ Misses 31808 31806 -2
Partials 2518 2518
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
rusackas
added a commit
that referenced
this pull request
Aug 25, 2026
Address bot review feedback on #43502: the integration test only checked that a ColumnSpec existed, not that INTEGER columns actually mapped to the numeric generic type and an Integer sqla type. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Code Review Agent Run #fb7d57Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
rusackas
pushed a commit
that referenced
this pull request
Aug 25, 2026
docker==7.0.0 (the previously-unpinned floor) raises `docker.errors.DockerException: ... Not supported URL scheme http+docker` against the requests/urllib3 versions pinned elsewhere in this file -- confirmed on a real GHA run of #43502's nightly-testcontainers workflow, where every test errored before any container was even pulled. 7.2.0 is confirmed working, both locally and against a live container.
rusackas
changed the title
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Agent Run #23364e
Actionable Suggestions - 1
-
tests/testcontainers/db_engine_specs/test_db2.py - 1
- Duplicate paginated query test function · Line 53-53
Additional Suggestions - 1
-
tests/testcontainers/db_engine_specs/test_elasticsearch.py - 1
-
Magic number timeout value · Line 50-50Extract the repeated magic number `10` (seconds) to a named module-level constant `_REQUEST_TIMEOUT_SECONDS` to improve maintainability and make timeout adjustments easier to reason about.
-
Review Details
-
Files reviewed - 9 · Commit Range:
8b2f317..4233992- pyproject.toml
- requirements/development.txt
- requirements/development.in
- tests/testcontainers/db_engine_specs/test_db2.py
- tests/testcontainers/db_engine_specs/test_elasticsearch.py
- tests/testcontainers/db_engine_specs/test_mssql.py
- tests/testcontainers/db_engine_specs/test_oracle.py
- superset/db_engine_specs/cockroachdb.py
- tests/unit_tests/db_engine_specs/test_crdb.py
-
Files skipped - 3
- .github/workflows/nightly-testcontainers.yml - Reason: Filter setting
- .github/workflows/testcontainers.yml - Reason: Filter setting
- UPDATING.md - Reason: Filter setting
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers an incremental AI Review. -
/review full- Manually triggers a full AI Review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
rusackas
pushed a commit
that referenced
this pull request
Sep 6, 2026
docker==7.0.0 (the previously-unpinned floor) raises `docker.errors.DockerException: ... Not supported URL scheme http+docker` against the requests/urllib3 versions pinned elsewhere in this file -- confirmed on a real GHA run of #43502's nightly-testcontainers workflow, where every test errored before any container was even pulled. 7.2.0 is confirmed working, both locally and against a live container.
rusackas
added a commit
that referenced
this pull request
Sep 6, 2026
The "paginated query returns correct rows in order" test body was duplicated verbatim across cockroachdb/crate/db2/mssql/oracle/trino's testcontainers suites. Factor the shared table-setup/assert logic into _pagination.py, with an optional post-insert hook for CrateDB's eventual-consistency REFRESH TABLE step; each call site keeps its own test function and dialect-specific docstring. Addresses bito-code-review feedback on #43502. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
rusackas
force-pushed
the
feat/testcontainers-nightly-pilot
branch
from
September 6, 2026 05:28
1eadf35 to
3d8ba4e
Compare
Contributor
Code Review Agent Run #abae72Actionable Suggestions - 0Additional Suggestions - 7
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
github-actions
Bot
added
the
requires:rebase
Pilot for real-container testing of db_engine_specs against actual databases (CockroachDB, CrateDB, Trino), via testcontainers-python. Existing unit tests mock the driver/dialect layer entirely, which cannot catch real SQL-compilation or type-mapping bugs -- e.g. #42899, where Trino emitted OFFSET before LIMIT for paginated queries. Runs nightly (.github/workflows/nightly-testcontainers.yml), not on every merge: container pulls and startup are slower and more flake-prone than the existing mocked unit tests, and this measures that cost/signal tradeoff before considering wider adoption. Building this surfaced two real bugs, fixed/documented separately: - The cockroachdb dialect was completely broken under SQLAlchemy 2.0 due to a dead upstream package -- fixed in #43501. - testcontainers-python's TrinoContainer.get_connection_url() returns the container-internal port instead of the Docker-mapped host port; worked around locally, filed upstream.
Address bot review feedback on #43502: the integration test only checked that a ColumnSpec existed, not that INTEGER columns actually mapped to the numeric generic type and an Integer sqla type. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds a scoped pull_request trigger so this workflow runs on this PR itself, to get real GHA runner timing before deciding whether/how to adopt this pattern more broadly. Also adds the actions-timeline job (same pattern as superset-python-presto-hive.yml) to visualize per-step duration. Revert the pull_request trigger before merge -- it's here to measure cost, not to become a permanent merge-blocking check.
docker==7.0.0 (the previously-unpinned floor) raises `docker.errors.DockerException: ... Not supported URL scheme http+docker` against the requests/urllib3 versions pinned elsewhere in this file -- confirmed on a real GHA run of #43502's nightly-testcontainers workflow, where every test errored before any container was even pulled. 7.2.0 is confirmed working, both locally and against a live container.
…icsearch Adds four more dialects to the testcontainers suite (cockroachdb, crate, trino from the initial pilot): mssql, oracle, db2, elasticsearch. All four have native testcontainers-python container classes. Restructures the workflow from one job running the whole suite to a matrix, one job per dialect, running in parallel. A single slow container would otherwise inflate wall-clock time for every dialect, not just its own -- matrixing bounds total suite time by the slowest dialect instead of the sum of all of them. Also renames the workflow file/name from "Nightly-Testcontainers" to "Testcontainers" now that it runs on pull_request (scoped via `paths`) in addition to the nightly cron. Elasticsearch needed a different data-setup approach than the SQL-native dialects: indices/documents get created via its REST API, not SQL INSERT, matching how Superset actually encounters Elasticsearch in practice. Confirmed empirically that Elasticsearch's SQL layer has no OFFSET support at all (a real protocol limitation, already correctly documented via ElasticSearchEngineSpec.supports_offset = False) and adjusted that dialect's pagination test accordingly -- LIMIT/ORDER BY only, no OFFSET. mssql and db2 could not be verified locally (no arm64 images for either; this environment is Apple Silicon), same situation as crate's amd64-only image from the initial pilot. Both are written against verified library source (dialect names, connection URL construction) and will get their first real execution on CI.
…my-cockroachdb The `cockroachdb` PyPI package (last released 2021) is abandoned, and its SQLAlchemy dialect references sqlalchemy.dialects.postgresql.psycopg2's PGCompiler_psycopg2, which SQLAlchemy 2.0 removed. Constructing a cockroachdb:// engine has raised ImportError since the SQLAlchemy 2.0 bump (#42803) landed, with nothing catching it: the driver isn't part of the default dev/CI install, and the existing test_crdb.py only exercises convert_dttm(), never a real engine. Switches the `cockroachdb` extra to sqlalchemy-cockroachdb, the actively maintained replacement already linked from CockroachDbEngineSpec's own docs_url, adds it to the default dev install so this keeps getting exercised, and adds a regression test that constructs a real engine (verified to fail against the old package, pass against the new one).
…m mssql extra Adding the mssql extra to development.in for testcontainers coverage installs pymssql in the dev/test env, so the existing mssql+pymssql://url probe in test_test_connection_failed no longer hits the "driver not found" path it was meant to exercise -- it now attempts a real (failing) connection instead. Swap it for a dialect+driver URI whose dialect will never resolve to an installed extra. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The "paginated query returns correct rows in order" test body was duplicated verbatim across cockroachdb/crate/db2/mssql/oracle/trino's testcontainers suites. Factor the shared table-setup/assert logic into _pagination.py, with an optional post-insert hook for CrateDB's eventual-consistency REFRESH TABLE step; each call site keeps its own test function and dialect-specific docstring. Addresses bito-code-review feedback on #43502. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…CI isolation, and test rigor - pyproject.toml: pin psycopg2-binary alongside sqlalchemy-cockroachdb -- the latter declares no DBAPI dependency of its own, so the documented `apache-superset[cockroachdb]` install couldn't actually connect. - testcontainers.yml: scope the concurrency group by ref so a PR run and the nightly cron (or two different PRs) no longer cancel each other. - test_cockroachdb.py: assert the actual generic/SQLAlchemy type, matching the Trino test, instead of only checking a column spec was found. - pytest.ini + new `testcontainers` marker + _driver.py: exclude tests/testcontainers/ from a plain `pytest` run by default (it needs Docker), while the dedicated CI job now sets SUPERSET_TESTCONTAINERS_STRICT so a broken/missing driver import fails that job instead of silently skipping to a green, zero-tests-run result. - UPDATING.md: the migration note now says to uninstall the old `cockroachdb` package outright, since reinstalling the extra alone can leave both packages registering the same dialect entry point. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…cockroachdb DBAPI docs, stale workflow refs) - requirements/development.in + testcontainers.yml: scope the `db2` extra (`ibm-db-sa`/`ibm-db`) out of the baseline dev install. `ibm-db` ships no Linux arm64 wheel, so bundling it there broke the multi-platform dev Docker image build on push. The testcontainers CI job now installs it directly, only for its own db2 matrix leg. - cockroachdb.py: list `psycopg2-binary` alongside `sqlalchemy-cockroachdb` in the `pypi_packages` metadata, since a plain `cockroachdb://` URL can't connect without a DBAPI and sqlalchemy-cockroachdb doesn't install one. - test_cockroachdb.py/test_crate.py/test_trino.py: fix stale docstring references to the old `nightly-testcontainers.yml` filename. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The db2 testcontainers extra pulls in ibm-db/ibm-db-sa transitively; the pinned lockfile was missing those entries. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…abytedb, monetdb, mongodb (#43640) Co-authored-by: Superset Dev <dev@superset.apache.org> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…43662) Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: hainenber <dotronghai96@gmail.com> Signed-off-by: Gabriel Torres Ruiz <gabo2595@gmail.com> Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Superset Dev <dev@superset.apache.org> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Milad Rashidikhah <mrashidikhah32@gmail.com> Co-authored-by: bucketbase26 <singhayush062006@gmail.com> Co-authored-by: Joe Li <joe@preset.io> Co-authored-by: Viktor Högberg <119532259+vhogberg@users.noreply.github.com> Co-authored-by: joey <97154801+chkang83@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Chandan P <95340276+NoiceHax@users.noreply.github.com> Co-authored-by: Mafi <matt.fitzgerald@gmail.com> Co-authored-by: Matt Fitzgerald <matt.fitzgerald@preset.io> Co-authored-by: Enzo Martellucci <52219496+EnxDev@users.noreply.github.com> Co-authored-by: Alexandru Soare <37236580+alexandrusoare@users.noreply.github.com> Co-authored-by: Mehmet Salih Yavuz <salih.yavuz@proton.me> Co-authored-by: Sanmitra Nagaraj <48400413+s1ny1998@users.noreply.github.com> Co-authored-by: Lalith Kothuru <lalith.kothuru@gmail.com> Co-authored-by: shaurya <shauryajaiswal.dev@gmail.com> Co-authored-by: Shaurya <19599684+no-hup@users.noreply.github.com> Co-authored-by: Đỗ Trọng Hải <41283691+hainenber@users.noreply.github.com> Co-authored-by: rlei <242280117+rlei-odes@users.noreply.github.com> Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com> Co-authored-by: Gabriel Torres Ruiz <gabo2595@gmail.com> Co-authored-by: hainenber <dotronghai96@gmail.com> Co-authored-by: ʈᵃᵢ <tai@apache.org> Co-authored-by: Mike Bridge <michael.bridge@preset.io> Co-authored-by: Elizabeth Thompson <eschutho@gmail.com> Co-authored-by: Francesco.Castaldi <info@francescocastaldi.it> Co-authored-by: FrancescoCastaldi <francesco.castaldi@mapsgroup.it> Co-authored-by: Abdul Rehman <76230556+Abdulrehman-PIAIC80387@users.noreply.github.com> Co-authored-by: Sepuri Sai Krishna <saik20533@gmail.com> Co-authored-by: Mallikarjuna Reddy Nimmakayala <mallikarjunareddy.nimmakayala@gmail.com> Co-authored-by: PRATHAMESH HUKKERI <prathamhukkeri04@gmail.com> Co-authored-by: Prathamesh Hukkeri <prathamesh04@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: SBIN2010 <Sbin2010@mail.ru>
…ase extra as unresolvable - docs/src/data/databases.json: the rendered CockroachDB setup docs still listed the abandoned `cockroachdb` PyPI package instead of the `sqlalchemy-cockroachdb`/`psycopg2-binary` pair pyproject.toml actually installs. - pyproject.toml: document that `pip install apache-superset[oceanbase]` cannot resolve (oceanbase_py pins sqlalchemy-utils<0.39, conflicting with Superset's own >=0.42.1 pin); CI installs the driver standalone with --no-deps instead. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The `oceanbase` extra pinned oceanbase_py, whose sqlalchemy-utils range conflicts outright with Superset's own pin, so `pip install apache-superset[oceanbase]` could never resolve. CI already installs oceanbase_py standalone with --no-deps, so drop the unusable extra and keep the rationale as a comment instead. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| with: | ||
| persist-credentials: false | ||
| - name: Setup Python | ||
| uses: ./.github/actions/setup-backend/ |
Contributor
Code Review Agent Run #0ff7caActionable Suggestions - 0Additional Suggestions - 2
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
villebro
approved these changes
Sep 9, 2026
1 of 9 tasks
1 of 9 tasks
This was referenced Oct 13, 2026
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
Real-container testing of
db_engine_specsagainst actual databases, using testcontainers-python. Existingtests/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 emittedOFFSETbeforeLIMITfor paginated queries. Only a real round-trip against a live instance catches that class of bug.7 dialects, one matrix job each:
cockroachdb,crate,trino,mssql,oracle,db2,elasticsearch. All chosen because they're self-hostable via an official Docker image, weren't already covered by real-container CI, and testcontainers-python ships a native container class for each — keeping this PR's own code simple and low-risk.Runs on a nightly cron and on
pull_request, the latter scoped viapathsto only PRs touchingtests/testcontainers/**or the workflow itself, so unrelated PRs across the repo are never affected. Real measured cost on GitHub's actual runners (not local estimates): the slowest dialect (db2) took 161s; everything else is under 140s. Since each dialect is its own parallel matrix job rather than one job running the whole suite sequentially, total wall-clock for the workflow is bounded by the slowest dialect (~3 minutes including setup), not the sum of all seven (~13 minutes) — a single slow container no longer inflates the cost of the rest.Three real bugs found building this
cockroachdbextra installed an abandoned PyPI package whose dialect couldn't even import under 2.0. Nothing caught it because the driver wasn't in the default CI install and the existing mock-only test never constructed a real engine. Fixed in fix(cockroachdb): replace abandoned cockroachdb package with sqlalchemy-cockroachdb #43501 (merge that first — this PR'scockroachdbjob depends on it).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 intest_trino.py; filed upstream: TrinoContainer.get_connection_url() returns the internal port, not the mapped host port testcontainers/testcontainers-python#1111.docker(the Python package) needed a floor bump to 7.2.0 — 7.0.0 raisesdocker.errors.DockerException: ... Not supported URL scheme http+dockeragainst the requests/urllib3 versions pinned elsewhere in this repo, breaking every testcontainers test before any container is even pulled. Confirmed directly on a real GHA run.Also confirmed one non-bug
Elasticsearch's SQL layer genuinely does not support
OFFSETat all — a real protocol limitation (parsing_exception: mismatched input 'OFFSET'), already correctly documented viaElasticSearchEngineSpec.supports_offset = False. Adjusted that dialect's pagination test toLIMIT/ORDER BYonly, matching what Elasticsearch's SQL translator actually supports.Verification
cockroachdb,trino,oracle,elasticsearch: ran locally against real containers, all pass.crate: could not run locally —crate/crateonly publishes anamd64image, and CrateDB's binary requires a host CPU supportingx86-64-v3, which QEMU emulation on Apple Silicon can't satisfy even withdocker pull --platform linux/amd64.mssql,db2: could not run locally either — neither publishes an arm64 image (SQL Server on Linux and Db2 Community Edition are both amd64-only). Written against verified library source (dialect names, connection URL construction) and got their first real execution on CI.db2for a transienticr.ioregistry timeout — genuine network flakiness, not a code issue, and exactly the kind of risk a nightly/PR-scoped cadence is meant to absorb rather than block merges on).Full dialect survey (enumerating everywhere this could go next)
Superset's existing real-container CI coverage before this PR: Postgres, MySQL, Presto, Redis (GH Actions
services:) + Hive (custom setup step). Everything below was mock-only or completely untested.community.cockroachdb)community.cratedb)community.trino)community.mssql)gvenzl/oracle-free(multi-arch)community.oracle)community.db2)community.elasticsearch, generic container)DockerContainerDockerContainerTo add a dialect: drop a
tests/testcontainers/db_engine_specs/test_<dialect>.pyfollowing the pattern in this PR (module-scoped container fixture, aLIMIT/OFFSETround-trip where the dialect supports it, aget_columns/get_column_specintrospection check against real server metadata), then add a{dialect, timeout}entry to the workflow's matrix.TESTING INSTRUCTIONS
Requires a local Docker daemon.
crate,mssql,db2need an x86_64 host (no arm64 images) — they'll only actually run on CI if you're on Apple Silicon, though they still collect cleanly.Or trigger the workflow directly via
workflow_dispatchfrom the Actions tab, or viagh workflow run testcontainers.yml.ADDITIONAL INFORMATION
tests/testcontainers/test category)