Skip to content

ci: enforce SQLAlchemy 2.0 deprecation warnings in unit-test CI - #41915

Merged
rusackas merged 1 commit into
masterfrom
ci/enable-sqlalchemy-2-warnings
Jul 12, 2026
Merged

ci: enforce SQLAlchemy 2.0 deprecation warnings in unit-test CI#41915
rusackas merged 1 commit into
masterfrom
ci/enable-sqlalchemy-2-warnings

Conversation

@rusackas

Copy link
Copy Markdown
Member

SUMMARY

Part of the SQLAlchemy 2.0 migration battleplan: #40273

pytest.ini's filterwarnings already promotes several SQLAlchemy 2.0 deprecation-warning categories from a bare warning to a hard error, as each gets fixed and locked in (see #41914 for 3 more). But SQLALCHEMY_WARN_20 was never actually set anywhere in CI — grepped the whole repo, it only appears in a comment in pytest.ini. That means those RemovedIn20Warning categories never actually fire during a normal CI run, so the "enforcement" was a no-op: a regression on any already-"fixed" category would pass CI silently, and a contributor would only catch it if they remembered to set the env var locally.

This sets SQLALCHEMY_WARN_20: "1" on the unit-tests job so the categories already promoted to error: in pytest.ini are actually exercised on every PR and push.

TESTING INSTRUCTIONS

Ran the exact CI command locally against master's current pytest.ini (3 promoted categories):

SQLALCHEMY_WARN_20=1 SUPERSET_TESTENV=true SUPERSET_SECRET_KEY=not-a-secret \
  pytest --durations-min=0.5 --cov-report= --cov=superset ./tests/common ./tests/unit_tests --cache-clear --maxfail=50

Only 2 pre-existing, unrelated flaky tests fail (test_get_time_filter ×6, test_previous_calendar_quarter), and both reproduce identically with no env vars set at all — confirmed unrelated to this change (date-boundary-sensitive).

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration
  • Introduces new feature or API
  • Removes existing feature or API

Part of the SQLAlchemy 2.0 migration battleplan
(#40273).

pytest.ini's filterwarnings already promotes several SQLAlchemy 2.0
deprecation-warning categories from a bare warning to a hard error, as
each gets fixed and locked in — but SQLALCHEMY_WARN_20 was never
actually set anywhere in CI, so those RemovedIn20Warning categories
never actually fire during a normal test run and the enforcement was a
no-op. A regression on any already-"fixed" category would pass CI
silently.

Set SQLALCHEMY_WARN_20=1 for the unit-tests job so the categories
already promoted to error in pytest.ini are actually exercised on every
PR and push, closing that gap.

Verified locally by running the exact CI command
(`pytest --durations-min=0.5 --cov-report= --cov=superset ./tests/common
./tests/unit_tests --cache-clear --maxfail=50`) with SQLALCHEMY_WARN_20=1
against master's current pytest.ini (3 promoted categories): only 2
pre-existing, unrelated date-boundary-sensitive flaky tests fail
(test_get_time_filter, test_previous_calendar_quarter), both of which
reproduce identically with no env vars set at all.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@dosubot dosubot Bot added the github_actions Pull requests that update GitHub Actions code label Jul 10, 2026
@bito-code-review

bito-code-review Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Bito Automatic Review Skipped - Files Excluded

Bito didn't auto-review this change because all changed files are in the exclusion list for automatic reviews. No action is needed if you didn't intend for the agent to review it. Otherwise, to manually trigger a review, type /review in a comment and save.
You can change the excluded files settings here, or contact your Bito workspace admin at evan@preset.io.

@codecov

codecov Bot commented Jul 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.56%. Comparing base (1fd43ff) to head (4299573).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #41915      +/-   ##
==========================================
- Coverage   64.57%   64.56%   -0.02%     
==========================================
  Files        2713     2718       +5     
  Lines      151269   151491     +222     
  Branches    34793    34802       +9     
==========================================
+ Hits        97684    97808     +124     
- Misses      51760    51854      +94     
- Partials     1825     1829       +4     
Flag Coverage Δ
hive 39.07% <ø> (+0.06%) ⬆️
mysql 57.44% <ø> (+<0.01%) ⬆️
postgres 57.50% <ø> (-0.01%) ⬇️
presto 40.58% <ø> (+0.05%) ⬆️
python 58.90% <ø> (-0.01%) ⬇️
sqlite 57.09% <ø> (+<0.01%) ⬆️
unit 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@hy144328

Copy link
Copy Markdown
Contributor

I think this would be great to catch regression in the migration.
I considered it in #40274 but I was reluctant to make the step at the time.
Currently, the unit tests would product 45131 lines locally with warnings turned on with 45218 warnings produced.
I was not sure whether this would overwhelm the CI, a log database, etc.
Other than that, my two cents is that this is not a controversial change.

==== 7 failed, 9756 passed, 5 skipped, 45218 warnings in 782.83s (0:13:02) =====

@rusackas

Copy link
Copy Markdown
Member Author

@hy144328 thanks for the numbers. CI ran green here so the volume did not seem to overwhelm anything, and filterwarnings only hard-errors the few categories we have actually migrated, so the rest stay print-only. Curious what your 7 local failures were though, if they fall outside that set?

@hainenber

Copy link
Copy Markdown
Contributor

LGTM. Let's do this!

@rusackas
rusackas merged commit a1d1d69 into master Jul 12, 2026
63 checks passed
@rusackas
rusackas deleted the ci/enable-sqlalchemy-2-warnings branch July 12, 2026 15:43
@hy144328

Copy link
Copy Markdown
Contributor

@hy144328 thanks for the numbers. CI ran green here so the volume did not seem to overwhelm anything, and filterwarnings only hard-errors the few categories we have actually migrated, so the rest stay print-only. Curious what your 7 local failures were though, if they fall outside that set?

@rusackas Sorry for the late reply.

I have been noticing errors locally here and there that did not show up in the CI.
So I mostly ignored them to keep things moving.
I have just noticed that they all go away when I upgrade my virtual environment from Python 3.10 to 3.11.
For instance, there is the LalitMaganti/syntaqlite dependency, which claims to be Python 3.10 compatible.
But it causes unit tests to fail because it imports enum.StrEnum , which has only been introduced in Python 3.11 .
I wonder whether it is worthwhile raising a ticket for this.

@rusackas

Copy link
Copy Markdown
Member Author

I actually opened a revert PR for that dependency today, but I'm considering removing support for python 3.10 instead. That might be more forward leaning, but I want to get more consensus.

hainenber pushed a commit that referenced this pull request Jul 18, 2026
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

github_actions Pull requests that update GitHub Actions code size/XS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants