test: enable SQLAlchemy deprecation warnings#40274
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #6a1b1e
Actionable Suggestions - 2
-
pytest.ini - 2
- misleading comment contradicts behavior · Line 26-26
- filterwarnings order prevents always action · Line 29-30
Additional Suggestions - 1
-
pytest.ini - 1
-
commented error filters unreachable dead code · Line 31-47The commented-out `error:` filter lines (31-47) are unreachable dead code in the current configuration - they can never trigger because the `ignore` filter on line 29 catches all RemovedIn20Warning warnings first.
-
Review Details
-
Files reviewed - 1 · Commit Range:
26b7357..4d63925- pytest.ini
-
Files skipped - 0
-
Tools
- 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 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
| asyncio_mode = auto | ||
|
|
||
| # `ignore` virtually reproduces to `-p no:warnings`. | ||
| # Always print RemovedIn20Warning when SQLALCHEMY_WARN_20=1. |
There was a problem hiding this comment.
The comment on line 26 states 'Always print RemovedIn20Warning when SQLALCHEMY_WARN_20=1' but the current filter configuration will silently ignore all such warnings instead.
Code Review Run #6a1b1e
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
This is not true.
Later lines overrule earlier lines.
The documentation gives an example for this. https://docs.pytest.org/en/stable/how-to/capture-warnings.html
There was a problem hiding this comment.
The suggestion in the PR comment is valid and should be applied. The ignore filter on line 29 will override the always filter on line 30, making the latter ineffective. The suggested fix rearranges the filters to ensure always is applied before ignore, aligning with the intended behavior described in the comment on line 26.
pytest.ini
28: filterwarnings =
29: ignore
30: always::sqlalchemy.exc.RemovedIn20Warning
pytest.ini
28: filterwarnings =
29: always::sqlalchemy.exc.RemovedIn20Warning
30: ignore
| ignore | ||
| always::sqlalchemy.exc.RemovedIn20Warning |
There was a problem hiding this comment.
The ignore filter on line 29 will catch all RemovedIn20Warning warnings before the always filter on line 30 can act, making the latter completely ineffective. This contradicts the comment on line 26 which promises to 'Always print RemovedIn20Warning'. In pytest's filterwarnings, the first matching filter wins.
Code suggestion
Check the AI-generated fix before applying
--- pytest.ini (lines 28-30) ---
28: filterwarnings =
29: - ignore
30: - always::sqlalchemy.exc.RemovedIn20Warning
30: + always::sqlalchemy.exc.RemovedIn20Warning
31: + ignore
Code Review Run #6a1b1e
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
This is not true.
Later lines overrule earlier lines.
The documentation gives an example for this. https://docs.pytest.org/en/stable/how-to/capture-warnings.html
There was a problem hiding this comment.
The suggestion in the PR comment is valid and should be applied. The ignore filter on line 29 will override the always filter on line 30, making the latter ineffective. The suggested fix rearranges the filters to ensure always is applied before ignore, aligning with the intended behavior described in the comment on line 26.
pytest.ini
28: filterwarnings =
29: - ignore
30: - always::sqlalchemy.exc.RemovedIn20Warning
30: + always::sqlalchemy.exc.RemovedIn20Warning
31: + ignore
There was a problem hiding this comment.
The example in the official documentation clearly shows that a more specific rule on a later line prevails against a more general rule on an earlier line. https://docs.pytest.org/en/stable/how-to/capture-warnings.html
[pytest]
filterwarnings =
error
ignore::UserWarning
ignore:function ham\(\) is deprecated:DeprecationWarning
There was a problem hiding this comment.
The suggestion in the PR comment is valid and should be applied. The ignore filter on line 29 will override the always filter on line 30, making the latter ineffective. The suggested fix rearranges the filters to ensure always is applied before ignore, aligning with the intended behavior described in the comment on line 26.
pytest.ini
28: filterwarnings =
29: - ignore
30: - always::sqlalchemy.exc.RemovedIn20Warning
30: + always::sqlalchemy.exc.RemovedIn20Warning
31: + ignore
sadpandajoe
requested review from
betodealmeida,
Copilot,
sadpandajoe and
villebro
| # `-p no:warnings` temporarily disabled in favor of more finely tuned `filterwarnings`. | ||
| #addopts = -p no:warnings |
There was a problem hiding this comment.
| # `-p no:warnings` temporarily disabled in favor of more finely tuned `filterwarnings`. | |
| #addopts = -p no:warnings |
Dead code so we should just remove it.
There was a problem hiding this comment.
Once the SQLAlchemy migration is over, happy to return to the original set-up.
Just asking for another confirmation from you, and happy to just remove it then.
|
|
||
| # `ignore` virtually reproduces to `-p no:warnings`. | ||
| # Always print RemovedIn20Warning when SQLALCHEMY_WARN_20=1. | ||
| # Additionally, raise errors for refactored RemovedIn20Warning cases to prevent regression. |
There was a problem hiding this comment.
this is a bit misleading since the comment says it raises errors, but we have commented out every error. We should either have one error: or remove the comment until we actually start raising errors.
There was a problem hiding this comment.
Have you had a chance to look at #40273 ?
This PR is just to set the stage to crush one deprecation warning after another or concurrently.
For instance, #40274 , #40275 and #40276 illustrate how to build on top of this.
Each PR uncomments a warning type, and makes this comment correct.
Is this acceptable?
Otherwise, #40275 is quite low stakes, and I would be happy to get that merged stacked with this one together.
| # error:Passing a string to Connection.execute\(\) is deprecated:sqlalchemy.exc.RemovedIn20Warning | ||
| # error:"Query" object is being merged into a Session:sqlalchemy.exc.RemovedIn20Warning | ||
| # error:"SavedQuery" object is being merged into a Session:sqlalchemy.exc.RemovedIn20Warning | ||
| # error:"SqlaTable" object is being merged into a Session:sqlalchemy.exc.RemovedIn20Warning | ||
| # error:"SqlMetric" object is being merged into a Session:sqlalchemy.exc.RemovedIn20Warning | ||
| # error:"TableColumn" object is being merged into a Session:sqlalchemy.exc.RemovedIn20Warning | ||
| # error:"TaggedObject" object is being merged into a Session:sqlalchemy.exc.RemovedIn20Warning | ||
| # error:The ``as_declarative\(\)`` function is now available:sqlalchemy.exc.RemovedIn20Warning | ||
| # error:The autoload parameter is deprecated:sqlalchemy.exc.RemovedIn20Warning | ||
| # error:The connection.execute\(\) method:sqlalchemy.exc.RemovedIn20Warning | ||
| # error:The current statement is being autocommitted using implicit autocommit:sqlalchemy.exc.RemovedIn20Warning | ||
| # error:The `database` package is deprecated:sqlalchemy.exc.RemovedIn20Warning | ||
| # error:The ``declarative_base\(\)`` function is now available:sqlalchemy.exc.RemovedIn20Warning | ||
| # error:The Engine.execute\(\) method is considered legacy:sqlalchemy.exc.RemovedIn20Warning | ||
| # error:The legacy calling style of select\(\) is deprecated:sqlalchemy.exc.RemovedIn20Warning | ||
| # error:The "whens" argument to case:sqlalchemy.exc.RemovedIn20Warning | ||
| # error:"User" object is being merged into a Session:sqlalchemy.exc.RemovedIn20Warning |
There was a problem hiding this comment.
Lots of dead code, either remove it or add them in future prs that actually use them.
There was a problem hiding this comment.
I found this useful because this is a comprehensive list of DeprecationWarning.
Unlike people appending one warning category after another, by uncommenting people are actually able to work on different warning categories in parallel and not run into merge conflicts with pytest.ini later.
I put a checklist into #40273 to maintain some form of checklist of ongoing efforts.
Does this justify the dead code to support a transitional migration effort?
Co-authored-by: Joe Li <joe@preset.io>
Co-authored-by: Joe Li <joe@preset.io>
Code Review Agent Run #63f75dActionable 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 |
SUMMARY
See #40273 .
This PR refactors the unit test set-up to allow developers to locally enable deprecation warnings for the upcoming migration to SQLAlchemy 2.0.
The difference will only be for local developers actively working on this topic by deliberately setting the environment variable
SQLALCHEMY_WARN_20.The CI or any other testing set-up is not affected by this.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
Only if you set the environment variable
SQLALCHEMY_WARN_20, SQLAlchemy will emit warnings during unit tests:ADDITIONAL INFORMATION