-
Notifications
You must be signed in to change notification settings - Fork 192
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
tests: add --db-backend pytest option #6625
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6625 +/- ##
==========================================
+ Coverage 77.51% 77.90% +0.40%
==========================================
Files 560 567 +7
Lines 41444 42147 +703
==========================================
+ Hits 32120 32831 +711
+ Misses 9324 9316 -8 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
.github/workflows/benchmark.yml
Outdated
@@ -73,4 +73,4 @@ jobs: | |||
alert-threshold: 200% | |||
comment-on-alert: true | |||
fail-on-alert: false | |||
alert-comment-cc-users: '@chrisjsewell,@giovannipizzi' | |||
alert-comment-cc-users: '@giovannipizzi,@agoscinski,@GeigerJ2' |
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.
I have no idea if this benchmark alert works, somebody should test it :-)
tests/test_markers.py
Outdated
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.
These tests should perhaps live in test_conftest.py
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.
Just went through a first time, but my brain is mush at this point. Will take it for another spin tomorrow. Just for clarification, because I'm still rather new to this whole CI business: Now, with the PSQL installation removed from the GH workflows, how/when are tests ever ran with PSQL (and I assume by default then only the ones marked with requires_psql
).
a68d6ed
to
3ec5b76
Compare
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.
Thanks for this feature, @danielhollas, very nice. LGTM from my side!
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.
I also give a late review for the PR. Neat test feature!
Thanks for the reviews! (btw: I don't have commit rights to this repo so please merge when you want) |
f412abe
to
a22f08b
Compare
Thanks for the great work, @danielhollas! I just squash-merged this, using a slightly modified version of your PR description as the commit message. Hope that's OK for you! |
Of course! Thanks for taking this over the finish line, I appreciate it! |
Pair coded and discussed with @GeigerJ2 🫶
Instead of choosing the database backend (SQLite or Postgres) implicitly via markers, we add an explicit
--db-backend
option to pytest.Important
The default backend was changed from
psql
tosqlite
to make local testing easier.The tests running in CI are still using Postgres in most places.
The only change in CI should be switching the rabbitmq tests and release tests to use sqlite.
Here's the error if the user provides an invalid option
EDIT: To clarify, the existing presto marker works as before.