Skip to content

Add CI support for SQLAlchemy 2.0 #52233

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Dev-iL
Copy link
Contributor

@Dev-iL Dev-iL commented Jun 25, 2025

closes: #48953
related: #28723

  • Add the check_upgrade_sqlalchemy container entrypoint for testing SQLA v2 on CI
  • Fix some existing shellcheck violations reported by shellcheck-py.
  • Robustify multiple unit tests and amend some business logic.

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

@Copilot Copilot AI review requested due to automatic review settings June 25, 2025 09:29
@boring-cyborg boring-cyborg bot added area:dev-tools backport-to-v3-0-test Mark PR with this label to backport to v3-0-test branch labels Jun 25, 2025
Copilot

This comment was marked as outdated.

@Dev-iL Dev-iL force-pushed the Dev-iL/2506/sqla-v2-tests branch from 96d2e8d to 482fff4 Compare June 25, 2025 09:51
@Dev-iL Dev-iL requested a review from Copilot June 25, 2025 09:55
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces a new entrypoint function, check_upgrade_sqlalchemy, to handle upgrading SQLAlchemy for testing SQLA v2 while also addressing several shellcheck violations.

  • Adds check_upgrade_sqlalchemy implementations to both scripts/docker/entrypoint_ci.sh and Dockerfile.ci.
  • Updates Breeze parameters, commands, and CI workflow to include an upgrade_sqlalchemy flag.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
scripts/docker/entrypoint_ci.sh Adds check_upgrade_sqlalchemy function with minor quoting adjustments.
scripts/ci/docker-compose/devcontainer.env Introduces UPGRADE_SQLALCHEMY environment variable.
dev/breeze/src/airflow_breeze/params/shell_params.py Adds new boolean parameter upgrade_sqlalchemy.
dev/breeze/src/airflow_breeze/commands/developer_commands.py Adds upgrade_sqlalchemy option to developer commands.
dev/breeze/src/airflow_breeze/commands/common_options.py Adds a new click option for upgrade_sqlalchemy with help text.
Dockerfile.ci Adds check_upgrade_sqlalchemy function similar to the entrypoint script.
.github/workflows/run-unit-tests.yml Adds upgrade-sqlalchemy input to the CI workflow.
Comments suppressed due to low confidence (1)

dev/breeze/src/airflow_breeze/commands/common_options.py:157

  • [nitpick] The help text for the upgrade_sqlalchemy option mentions an 'experimental supported version'; consider revising it to clearly indicate that it upgrades SQLAlchemy to the latest version required for running tests, ensuring consistency with the function output.
option_upgrade_sqlalchemy = click.option(

@Dev-iL Dev-iL force-pushed the Dev-iL/2506/sqla-v2-tests branch 4 times, most recently from d3dd43d to d55a749 Compare June 25, 2025 13:09
@Dev-iL Dev-iL requested a review from vincbeck as a code owner June 25, 2025 13:09
@Dev-iL
Copy link
Contributor Author

Dev-iL commented Jun 25, 2025

The tests are failing as expected on SQLA v2 incompatibilities. Now what? Should I suggest in #28723 that developers rebase to my branch?

@potiuk
Copy link
Member

potiuk commented Jun 25, 2025

Yes. And make it a part of your PR and we can continuously rebase the PRs on top of yours. This is going to be a little involved and require rebasing here and there.

@Dev-iL Dev-iL requested a review from XD-DENG as a code owner June 25, 2025 15:01
@Dev-iL Dev-iL requested a review from ephraimbuddy as a code owner June 25, 2025 15:47
@Dev-iL
Copy link
Contributor Author

Dev-iL commented Jun 25, 2025

Should we perhaps decide on a single "main" branch (e.g. your one) and target the various SQLA2 PRs into it? Just to save the effort of rebasing across multiple forks...

@Dev-iL Dev-iL force-pushed the Dev-iL/2506/sqla-v2-tests branch 2 times, most recently from 7bde1ac to 34e7502 Compare June 26, 2025 15:43
@Dev-iL Dev-iL marked this pull request as draft June 28, 2025 06:37
@Dev-iL Dev-iL force-pushed the Dev-iL/2506/sqla-v2-tests branch 5 times, most recently from 5efe8cb to 9cb0ba2 Compare June 29, 2025 13:44
@Dev-iL Dev-iL force-pushed the Dev-iL/2506/sqla-v2-tests branch 3 times, most recently from bcd6137 to 5e048cc Compare June 29, 2025 21:36
@Dev-iL Dev-iL changed the title Add the check_upgrade_sqlalchemy container entrypoint for testing SQLA v2 Add support for SQLA2 Jun 29, 2025
@Dev-iL Dev-iL changed the title Add support for SQLA2 Add CI support for SQLAlchemy 2.0 Jun 29, 2025
@Dev-iL Dev-iL force-pushed the Dev-iL/2506/sqla-v2-tests branch 3 times, most recently from 8c96da0 to 918beb2 Compare June 30, 2025 07:16
@Dev-iL
Copy link
Contributor Author

Dev-iL commented Jun 30, 2025

@potiuk Good news - one of the recent builds was all-green (with the exception of some breeze outputs). Now I'm thinking about splitting this PR into two for better focus:

  1. The updates to the tests and business logic that allow dual support of SQLA 1.4 and 2.0.
  2. Enabling the SQLA2 tests in the CI.

Then, the 1st part can be merged (thus enabling "experimental" support for SQLA2), and the 2nd can wait for FAB5. Fixing the deprecations is an orthogonal effort anyway, so doesn't depend on any of this.

My only concern is that the 1st sub-PR will have no obvious raison d'etre when considered in isolation.

WDYT?

potiuk and others added 2 commits June 30, 2025 13:07
Switch to using flask-sqlalchemy db session management, and include Auth
Manager Provider Test Isolation and Reliability

- Refactor test fixtures to always use Flask app contexts for DB and app
  operations.
- Add a global pytest fixture to clear SQLAlchemy metadata before each
  test, reducing test flakiness.
- Standardize session access and cleanup patterns across all tests.
- Refactor user/role creation and deletion to ensure proper isolation.
- Update test logic to use new app and auth manager creation utilities.
- Remove or update redundant or fragile test code.
- Normalize test constants and improve code readability.

Note: Clearing SQLAlchemy metadata and explicit app context management
are workarounds for test isolation issues. The root cause of metadata
and pool persistence between tests should be investigated further for a
more robust solution.

General Theme:

The main strategies are:

* Ensuring proper use of Flask app contexts in tests
* Cleaning up database state and metadata between tests
* Using the correct SQLAlchemy session and interface patterns
* Refactoring test fixtures for better isolation and reliability
* Removing or updating code that is no longer needed or that could cause
  test flakiness

Key File-by-File Changes

1. providers/fab/tests/unit/fab/conftest.py (New file)

What: Adds a global pytest fixture that clears SQLAlchemy metadata
before each test.

Why: This is to prevent metadata leakage between tests, which can cause
flaky or non-deterministic test results.

Uncertainty: Clearing metadata is a workaround; the root cause of
metadata persistence between tests may need deeper investigation.

2. Test files for API endpoints, CLI commands, models, schemas, and views

What:

* Many test fixtures now use with app.app_context(): to ensure all DB
  and app operations are performed within a Flask application context.
* User and role creation/deletion is now always wrapped in an app
  context.
* Some teardown logic is moved into the fixture's context manager to
  ensure cleanup happens after the test.
* Session access is standardized to use appbuilder.session instead of
  get_session.
* Some test constants (e.g., default times) are normalized (e.g.,
  removing timezone info).
* Some test logic is refactored for clarity and reliability (e.g., using
  addfinalizer for logout in user endpoint tests).

Why:

Ensures that tests do not leak state or context, which can cause
failures when running tests in parallel or in different environments.
Using the correct session and context patterns is more robust and
future-proof.

Uncertainty:

While these changes improve test isolation, the need to clear metadata
and manage app contexts so explicitly suggests there may be deeper
issues with how the test environment is set up or torn down. Further
investigation into the test infrastructure may be warranted.

3. test_fab_auth_manager.py and related files

What:

* Switches to using the new create_app and get_auth_manager utilities
  for creating Flask apps and auth managers.
* Updates test logic to use the new app and session patterns.
* Fixes some test assertions to compare user IDs instead of user objects
  directly.
* Moves some permission synchronization logic to after DAG creation and
  session commit.

Why:

These changes align the tests with the latest best practices and APIs in
Airflow and Flask AppBuilder.  They also fix subtle bugs where tests
could pass or fail depending on object identity rather than value.

Uncertainty:
The need to manually commit and close sessions, and to synchronize
permissions, may indicate that the test setup/teardown is not fully
robust.

4. test_security.py

What:

* Removes some unused or redundant code (e.g., a test for DAG permission
  views).
* Updates session and app context usage.

Why:

Cleans up the test suite and ensures all tests are using the correct patterns.

Uncertainty:
The removal of some tests may need to be reviewed to ensure no loss of coverage.

5. Miscellaneous

What:
* Minor formatting and import cleanups.
* Some test parameters and constants are updated for consistency.

Why:

Improves code readability and maintainability.

Summary of Uncertainties and Next Steps

* Clearing SQLAlchemy metadata and disposing pools:

These are workarounds for test isolation issues. The root cause (why
metadata and pools persist between tests) should be investigated
further. Ideally, the test infrastructure should handle this
automatically.

App context management:

The need for explicit app contexts in so many places may indicate that
the test setup could be improved to provide a more consistent
environment.

Session and teardown logic:

Manual session management and teardown in tests can be error-prone.
Consider centralizing this logic or using more robust fixtures.
+ Fix some existing shellcheck violations
+ Synchronize breeze images
@Dev-iL Dev-iL force-pushed the Dev-iL/2506/sqla-v2-tests branch from 4485bb2 to 22a9b40 Compare June 30, 2025 10:08
@Dev-iL
Copy link
Contributor Author

Dev-iL commented Jun 30, 2025

Is it possible (and a good idea) to split the breeze changes from the github ones? The result will be having support for running SQLA2 tests on breeze but without affecting the CI. It should make it easier to work on fixing deprecations using

breeze testing core-tests --force-sa-warnings --upgrade-sqlalchemy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dev-tools backport-to-v3-0-test Mark PR with this label to backport to v3-0-test branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add tests for SQLAlchemy 2
2 participants