-
-
Notifications
You must be signed in to change notification settings - Fork 117
Add SQLAlchemy to third-party daily tests #561
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
Conversation
Thank you, sqlalchemy will be a great addition in my opinion as well. I've created a PR which they have taken over on high speed, best look here: I assume they finish very soon. Currently they are left with some issues on python 3.7 |
What just came to my mind, possibly we should also test against their |
sqlalchemy/sqlalchemy#12472 has been merged - sqlachemy tests are now all passing 🎉 (thanks @Daraan!) Thoughts from someone over at SQLAlchemy about which branches we should test against would be very much appreciated! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I'll wait a bit with merging, in case we want to change/extend the tested branches.
haven't really checked it, but it's a mistery to me. seemed like python behaviour is different for whatever reason in that version of ubuntu.
that may make sense, since that's what's released on pypi, at least until 2.1 is out later this year (hepefully). Not sure if you want to run them on main + last version on pypi? |
main and rel_2_0 right now. by the end of this year we'll hopefully have 2.1 releases going and it will eventually move to "main + rel_2_1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
other than the branch consideration the change looks fine to me.
thanks for adding this test to your ci.
What will happen in case of failure? will an issue be filed?
The workflow runs every day. If it fails, it opens an issue on this repo, and then somebody involved here will hopefully take a look and triage it; as a result we'll either change something on our side or file an issue on your side to get something changed.
Sounds good, let's do that. |
ok, great. Thanks for adding this |
Alright, I added rel_2_0 to the test matrix. The test setup needed a little tweaking since the pyproject.toml file in rel_2_0 doesn't have a |
yes, 2.0 still uses setup.cfg. It was migrated for 2.1 to pyproject, but 2.0 was kept as it was for consistency |
.github/workflows/third_party.yml
Outdated
- name: Install uv | ||
run: curl -LsSf https://astral.sh/uv/install.sh | sh | ||
- name: Checkout sqlalchemy | ||
run: for i in {1..2}; do git clone -b ${{ matrix.checkout-ref }} --depth=1 https://github.com/sqlalchemy/sqlalchemy.git && break; done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency's sake, I'd like to keep this command similar to the command in the other tests, i.e. using &&
.
.github/workflows/third_party.yml
Outdated
env: | ||
TOX_WORKERS: -n4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this seems to be the default anyway, I'd prefer to leave this out and let SQLAlchemy decide what's best here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good! I added it explicitly to match what run-tests.yaml
does, but you're right that the same default gets set in tox anyway. It makes to sense to defer to SQLAlchemy sets to, especially if it ever changes in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC it's there because historically it was 2 since the actions had 2 cpus available, but then migrated to 4.
I'll likely also remove it from the ci file definition in sqlalchemy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense, thanks for the context!
Brought up in #560:
SQLAlchemy relies heavily on runtime introspection, is actively maintained, and has a test suite that runs pretty fast (~5m). Seems like a good candidate for the daily tests.
These tests are based on SQLAlchemy's
run-test.yaml
&run-on-pr.yaml
workflows, with some setup borrowed from the pydantic tests.A few notes about the setup: