From ce94f79a090d5b4302cb5d3f7bddb1fd28a059b3 Mon Sep 17 00:00:00 2001 From: rusackas Date: Tue, 25 Aug 2026 16:23:12 -0700 Subject: [PATCH] fix(testcontainers): address reviewer feedback on cockroachdb DBAPI, 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 --- .github/workflows/testcontainers.yml | 13 ++++- UPDATING.md | 2 +- pyproject.toml | 7 ++- pytest.ini | 8 +++- .../testcontainers/db_engine_specs/_driver.py | 47 +++++++++++++++++++ .../db_engine_specs/test_cockroachdb.py | 13 ++++- .../db_engine_specs/test_crate.py | 10 +++- .../db_engine_specs/test_db2.py | 10 +++- .../db_engine_specs/test_elasticsearch.py | 6 ++- .../db_engine_specs/test_mssql.py | 10 +++- .../db_engine_specs/test_oracle.py | 10 +++- .../db_engine_specs/test_trino.py | 10 +++- 12 files changed, 125 insertions(+), 21 deletions(-) create mode 100644 tests/testcontainers/db_engine_specs/_driver.py diff --git a/.github/workflows/testcontainers.yml b/.github/workflows/testcontainers.yml index deb9519b8db..a9611dbd1d3 100644 --- a/.github/workflows/testcontainers.yml +++ b/.github/workflows/testcontainers.yml @@ -22,7 +22,10 @@ on: - "tests/testcontainers/**" concurrency: - group: ${{ github.workflow }} + # Scoped by ref, not just workflow name -- otherwise every PR run and the + # nightly cron share one group, and starting the workflow on another PR + # (or the nightly firing mid-PR-run) cancels an unrelated in-progress run. + group: ${{ github.workflow }}-${{ github.ref }} cancel-in-progress: true jobs: @@ -60,6 +63,11 @@ jobs: PYTHONPATH: ${{ github.workspace }} SUPERSET_TESTENV: true SUPERSET_SECRET_KEY: not-a-secret + # This job's matrix installs exactly one dialect's testcontainers + # driver for exactly this job, so treat that driver as required: a + # broken/missing import should fail the job, not silently skip to a + # misleadingly green, zero-tests-run result. See _driver.py. + SUPERSET_TESTCONTAINERS_STRICT: true steps: - name: Checkout uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -71,7 +79,8 @@ jobs: python-version: current - name: Run testcontainers db_engine_specs tests (${{ matrix.dialect }}) run: | - pytest --durations-min=2 -v ./tests/testcontainers/db_engine_specs/test_${{ matrix.dialect }}.py \ + pytest --durations-min=2 -v -m testcontainers \ + ./tests/testcontainers/db_engine_specs/test_${{ matrix.dialect }}.py \ --junit-xml=test-results/junit-testcontainers-${{ matrix.dialect }}.xml - name: Upload JUnit test results if: always() diff --git a/UPDATING.md b/UPDATING.md index 98e19351255..3cc8ad42136 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -25,7 +25,7 @@ assists people when migrating to a new version. ## Next - `SAMPLES_ROW_LIMIT` is now the default for `/datasource/samples` requests without a valid explicit `per_page`, rather than a hard per-request ceiling; explicit limits are honored up to the existing global row-limit ceiling, matching `/chart/data` SAMPLES requests. -- The `cockroachdb` extra (`pip install apache-superset[cockroachdb]`) now installs `sqlalchemy-cockroachdb` instead of the abandoned `cockroachdb` package, whose SQLAlchemy dialect could not be imported under SQLAlchemy 2.0. Existing environments with the old package installed should `pip uninstall cockroachdb && pip install sqlalchemy-cockroachdb` (or simply reinstall the extra) to restore CockroachDB connectivity. +- The `cockroachdb` extra (`pip install apache-superset[cockroachdb]`) now installs `sqlalchemy-cockroachdb` instead of the abandoned `cockroachdb` package, whose SQLAlchemy dialect could not be imported under SQLAlchemy 2.0. Existing environments with the old package installed must `pip uninstall cockroachdb` before reinstalling the extra -- both packages register the same `cockroachdb` SQLAlchemy dialect entry point, so leaving the old one in place can still load the abandoned implementation. ### MCP tool results preserve stored string values diff --git a/pyproject.toml b/pyproject.toml index 7396d6195fc..da2d9f33221 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -148,10 +148,9 @@ clickhouse = ["clickhouse-connect>=1.7.2, <2.0"] # 2.0). sqlalchemy-cockroachdb is the actively maintained replacement, # already linked from CockroachDbEngineSpec.metadata's docs_url, and # registers the same `cockroachdb` SQLAlchemy dialect entry point. -# sqlalchemy-cockroachdb depends only on SQLAlchemy itself, not on a DBAPI -# driver, so psycopg2-binary is pinned alongside it (matching the `postgres` -# extra) to keep this extra self-contained -- CockroachDB speaks the -# PostgreSQL wire protocol, so psycopg2 is what actually opens connections. +# sqlalchemy-cockroachdb itself declares no DBAPI dependency (its own docs +# require picking one), so pull in the same psycopg2-binary pin as the +# `postgres` extra -- CockroachDB speaks the Postgres wire protocol. cockroachdb = ["sqlalchemy-cockroachdb>=2.0.0, <3", "psycopg2-binary==2.9.12"] crate = ["sqlalchemy-cratedb>=0.43.1, <1"] # sqlalchemy-d1's only release (0.1.0, Nov 2025) pins sqlalchemy<2,>=1.4, diff --git a/pytest.ini b/pytest.ini index f3833da5d53..d2cf551aee4 100644 --- a/pytest.ini +++ b/pytest.ini @@ -19,7 +19,13 @@ testpaths = tests python_files = *_test.py test_*.py *_tests.py *viz/utils.py # `-p no:warnings` temporarily disabled in favor of more finely tuned `filterwarnings`. -#addopts = -p no:warnings +# `not testcontainers` excludes tests/testcontainers/ by default: those spin up +# real Docker containers, and `testpaths = tests` would otherwise pull them into +# every plain `pytest` run. The dedicated CI job (testcontainers.yml) overrides +# this with an explicit `-m testcontainers` to run them. +addopts = -m "not testcontainers" +markers = + testcontainers: exercises a real database via testcontainers-python (needs Docker); excluded by default, see .github/workflows/testcontainers.yml asyncio_mode = auto # `ignore` is effectively equivalent to `-p no:warnings`. diff --git a/tests/testcontainers/db_engine_specs/_driver.py b/tests/testcontainers/db_engine_specs/_driver.py new file mode 100644 index 00000000000..b5d8bca5cb1 --- /dev/null +++ b/tests/testcontainers/db_engine_specs/_driver.py @@ -0,0 +1,47 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +""" +Shared import guard for the per-dialect testcontainers modules +(tests/testcontainers/db_engine_specs/test_*.py), each of which needs its +own optional `testcontainers[...]` driver submodule to even import. +""" + +import importlib +import os + + +def require_driver(module_name: str) -> None: + """ + Import `module_name`, a dialect's `testcontainers` driver submodule. + + Most environments treat that driver as optional: a bare local `pytest` + run, or another CI job that never installed the `testcontainers` extras, + should skip the module rather than fail collection outright. + + The dedicated per-dialect CI job (.github/workflows/testcontainers.yml) + sets SUPERSET_TESTCONTAINERS_STRICT, because there the driver is not + optional -- that job's matrix installs exactly this one driver for + exactly this one module. A broken or missing import there means the job + is misconfigured, and should fail loudly instead of silently reporting + a misleadingly green, zero-tests-run result. + """ + if os.environ.get("SUPERSET_TESTCONTAINERS_STRICT"): + importlib.import_module(module_name) + else: + import pytest + + pytest.importorskip(module_name) diff --git a/tests/testcontainers/db_engine_specs/test_cockroachdb.py b/tests/testcontainers/db_engine_specs/test_cockroachdb.py index 8238ed9bdff..b4f5a45d99b 100644 --- a/tests/testcontainers/db_engine_specs/test_cockroachdb.py +++ b/tests/testcontainers/db_engine_specs/test_cockroachdb.py @@ -37,12 +37,19 @@ from sqlalchemy.engine import Engine from superset.db_engine_specs.cockroachdb import CockroachDbEngineSpec from superset.sql.parse import Table +from superset.utils.core import GenericDataType -pytest.importorskip("testcontainers.community.cockroachdb") +pytestmark = pytest.mark.testcontainers + +from ._driver import require_driver # noqa: E402 + +require_driver("testcontainers.community.cockroachdb") from testcontainers.community.cockroachdb import CockroachDBContainer # noqa: E402 -from ._pagination import assert_paginated_query_returns_correct_rows_in_order +from ._pagination import ( # noqa: E402 + assert_paginated_query_returns_correct_rows_in_order, +) @pytest.fixture(scope="module") @@ -87,3 +94,5 @@ def test_get_columns_maps_native_types(engine: Engine) -> None: for col in by_name.values(): spec = CockroachDbEngineSpec.get_column_spec(str(col["type"])) assert spec is not None + assert spec.generic_type == GenericDataType.NUMERIC + assert isinstance(spec.sqla_type, Integer) diff --git a/tests/testcontainers/db_engine_specs/test_crate.py b/tests/testcontainers/db_engine_specs/test_crate.py index 19c308febeb..1849e7e067d 100644 --- a/tests/testcontainers/db_engine_specs/test_crate.py +++ b/tests/testcontainers/db_engine_specs/test_crate.py @@ -42,11 +42,17 @@ from sqlalchemy.engine import Connection, Engine from superset.db_engine_specs.crate import CrateEngineSpec from superset.sql.parse import Table -pytest.importorskip("testcontainers.community.cratedb") +pytestmark = pytest.mark.testcontainers + +from ._driver import require_driver # noqa: E402 + +require_driver("testcontainers.community.cratedb") from testcontainers.community.cratedb import CrateDBContainer # noqa: E402 -from ._pagination import assert_paginated_query_returns_correct_rows_in_order +from ._pagination import ( # noqa: E402 + assert_paginated_query_returns_correct_rows_in_order, +) @pytest.fixture(scope="module") diff --git a/tests/testcontainers/db_engine_specs/test_db2.py b/tests/testcontainers/db_engine_specs/test_db2.py index bda895019fa..7fbf4ec1b64 100644 --- a/tests/testcontainers/db_engine_specs/test_db2.py +++ b/tests/testcontainers/db_engine_specs/test_db2.py @@ -41,11 +41,17 @@ from sqlalchemy.engine import Engine from superset.db_engine_specs.db2 import Db2EngineSpec from superset.sql.parse import Table -pytest.importorskip("testcontainers.community.db2") +pytestmark = pytest.mark.testcontainers + +from ._driver import require_driver # noqa: E402 + +require_driver("testcontainers.community.db2") from testcontainers.community.db2 import Db2Container # noqa: E402 -from ._pagination import assert_paginated_query_returns_correct_rows_in_order +from ._pagination import ( # noqa: E402 + assert_paginated_query_returns_correct_rows_in_order, +) @pytest.fixture(scope="module") diff --git a/tests/testcontainers/db_engine_specs/test_elasticsearch.py b/tests/testcontainers/db_engine_specs/test_elasticsearch.py index 1e73b73c81a..3182bd5d692 100644 --- a/tests/testcontainers/db_engine_specs/test_elasticsearch.py +++ b/tests/testcontainers/db_engine_specs/test_elasticsearch.py @@ -37,7 +37,11 @@ from sqlalchemy.engine import Engine from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec from superset.sql.parse import Table -pytest.importorskip("testcontainers.community.elasticsearch") +pytestmark = pytest.mark.testcontainers + +from ._driver import require_driver # noqa: E402 + +require_driver("testcontainers.community.elasticsearch") from testcontainers.community.elasticsearch import ElasticSearchContainer # noqa: E402 diff --git a/tests/testcontainers/db_engine_specs/test_mssql.py b/tests/testcontainers/db_engine_specs/test_mssql.py index ae583e0bd0a..417b0d0d7b6 100644 --- a/tests/testcontainers/db_engine_specs/test_mssql.py +++ b/tests/testcontainers/db_engine_specs/test_mssql.py @@ -39,11 +39,17 @@ from sqlalchemy.engine import Engine from superset.db_engine_specs.mssql import MssqlEngineSpec from superset.sql.parse import Table -pytest.importorskip("testcontainers.community.mssql") +pytestmark = pytest.mark.testcontainers + +from ._driver import require_driver # noqa: E402 + +require_driver("testcontainers.community.mssql") from testcontainers.community.mssql import SqlServerContainer # noqa: E402 -from ._pagination import assert_paginated_query_returns_correct_rows_in_order +from ._pagination import ( # noqa: E402 + assert_paginated_query_returns_correct_rows_in_order, +) @pytest.fixture(scope="module") diff --git a/tests/testcontainers/db_engine_specs/test_oracle.py b/tests/testcontainers/db_engine_specs/test_oracle.py index fce7d0fdd28..93020613c56 100644 --- a/tests/testcontainers/db_engine_specs/test_oracle.py +++ b/tests/testcontainers/db_engine_specs/test_oracle.py @@ -41,11 +41,17 @@ from sqlalchemy.engine import Engine from superset.db_engine_specs.oracle import OracleEngineSpec from superset.sql.parse import Table -pytest.importorskip("testcontainers.community.oracle") +pytestmark = pytest.mark.testcontainers + +from ._driver import require_driver # noqa: E402 + +require_driver("testcontainers.community.oracle") from testcontainers.community.oracle import OracleDbContainer # noqa: E402 -from ._pagination import assert_paginated_query_returns_correct_rows_in_order +from ._pagination import ( # noqa: E402 + assert_paginated_query_returns_correct_rows_in_order, +) @pytest.fixture(scope="module") diff --git a/tests/testcontainers/db_engine_specs/test_trino.py b/tests/testcontainers/db_engine_specs/test_trino.py index aa5e367304c..88bf2872b55 100644 --- a/tests/testcontainers/db_engine_specs/test_trino.py +++ b/tests/testcontainers/db_engine_specs/test_trino.py @@ -39,11 +39,17 @@ from superset.db_engine_specs.trino import TrinoEngineSpec from superset.sql.parse import Table from superset.utils.core import GenericDataType -pytest.importorskip("testcontainers.community.trino") +pytestmark = pytest.mark.testcontainers + +from ._driver import require_driver # noqa: E402 + +require_driver("testcontainers.community.trino") from testcontainers.community.trino import TrinoContainer # noqa: E402 -from ._pagination import assert_paginated_query_returns_correct_rows_in_order +from ._pagination import ( # noqa: E402 + assert_paginated_query_returns_correct_rows_in_order, +) @pytest.fixture(scope="module")