-
Notifications
You must be signed in to change notification settings - Fork 965
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
Tell warehouse.db to reuse db_session pytest fixture #16031
Tell warehouse.db to reuse db_session pytest fixture #16031
Conversation
…5634_test_user_profile-investigation
f1ed80c
to
36d93fe
Compare
36d93fe
to
9957047
Compare
b46f4b9
to
179424d
Compare
Will have a look at fixing the stubbed out unit tests in |
@benjaoming Thanks for taking a stab at this! Looks like you're coming along well. Please run a Since this is your first contribution, one of the repo admins will have to approve a test run each time, so let us know when you're seeing the finish line 😉 |
…5634_test_user_profile-investigation
It's coming along well 👍 I've run Happy to address all feedback! |
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.
Some comments inline, but this is shaping up nicely!
…ve app_config fixture
Co-authored-by: Mike Fiedler <[email protected]>
Co-authored-by: Mike Fiedler <[email protected]>
Thanks for looking @miketheman! I've addressed the feedback in this PR branch. But I added a new approach in another branch: https://github.com/benjaoming/warehouse/compare/issue_15634_test_user_profile-investigation...benjaoming:warehouse:issue_15634_test_user_profile-investigation-new-app_config?expand=1 LMK if you'd prefer to merge that in here for the time being. I'm converting the PR to a draft again, since it needs some more discussion 👍 |
04dd5e2
to
9369e47
Compare
…fig' into issue_15634_test_user_profile-investigation
8f0ee35
to
7a83ec5
Compare
I'm pulling in the other approach since it's better than what's in this PR currently ⏳ |
56e2863
to
fa19e40
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.
@benjaoming thanks for continuing to work on this!
I've looked at the most recent changes, and they are shaping up nicely.
Specifically there's now a single call at the top of the pyramid application startup that would change behaviors, which is better than on every worker startup.
I appreciate that this is an early foray into Pyramid - it can take a bit to grok the behaviors, but I think this is getting closer to something very useful.
I think it's fine to skip exploring autobegin
behaviors if this approach works.
Let me know if the comments aren't clear!
|
||
|
||
@pytest.fixture(scope="session") | ||
def app_config_dbsession_from_env(database): |
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.
Seeing this split out is helpful - since it allows me to ask the following question:
Historically, in the context of test scenarios we've been passing a session-scoped app_config
fixture that has a database
fixture passed in.
Is there any reason for us to maintain these two fixtures, or could we collapse these down to app_config
only?
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.
The database
is currently a session-wide fixture like app_config
. I suppose that since app_config
depends on the database
fixture, then database
is created firstly.
However, trying to include the code from database
in the app_config
fixture gave birth to this error 🤕
_________________________________________________ ERROR at teardown of test_user_profile _________________________________________________
@request.addfinalizer
def drop_database():
> janitor.drop()
tests/conftest.py:336:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
../lib/python3.11/site-packages/pytest_postgresql/janitor.py:90: in drop
self._dont_datallowconn(cur, db_to_drop)
../lib/python3.11/site-packages/pytest_postgresql/janitor.py:98: in _dont_datallowconn
cur.execute(f'ALTER DATABASE "{dbname}" with allow_connections false;')
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = <psycopg.Cursor [closed] [BAD] at 0x7ce3261f5c70>, query = 'ALTER DATABASE "tests" with allow_connections false;', params = None
def execute(
self,
query: Query,
params: Optional[Params] = None,
*,
prepare: Optional[bool] = None,
binary: Optional[bool] = None,
) -> Self:
"""
Execute a query or command to the database.
"""
try:
with self._conn.lock:
self._conn.wait(
self._execute_gen(query, params, prepare=prepare, binary=binary)
)
except e._NO_TRACEBACK as ex:
> raise ex.with_traceback(None)
E psycopg.errors.InvalidCatalogName: database "tests" does not exist
../lib/python3.11/site-packages/psycopg/cursor.py:732: InvalidCatalogName
I'm not sure, but I think what's happening is that since request.addfinalizer
adds something to a finalization stack, then maybe some garbage collection has destroyed something (i.e. the database) already at the point where janitor.drop()
is supposed to be called.
I tried out some different sequences, but in the end I think it's maybe easier for now to keep the old fixtures structure.
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.
So actually this error was very helpful to understand that a bit more refactoring is needed to arrive at the right place 👍
Will try to figure out a minimal change ⏳
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.
Is there any reason for us to maintain these two fixtures, or could we collapse these down to app_config only?
So, getting back to this question, I can think of 2 reasons to keep them separate:
-
To keep McCabe complexity low 😇 Not sure if you have mccabe rules on or off in flake8, but very long routines get hard to read and maintain. There's a clear responsibility defined for each of these two fixtures, so seems easy to maintain separate fixtures.
-
Actually pytest can help here: It knows how to reuse the same instance of a fixture, even if the path is different (see diagram below). So while the
app_config
fixture may have a variant, thedatabase
fixture remains the same.
I'm exploring this alternative approach: https://github.com/benjaoming/warehouse/compare/issue_15634_test_user_profile-investigation...benjaoming:warehouse:issue_15634_test_user_profile-investigation-refactor-dbsession?expand=1
-
It avoids pulling in
db_session
inwebtest
but instead creates the session, using its own variant ofapp_config
(app_config_dbsession_from_env
). -
...which effectively avoids having 2 app configs instantiated when only 1 is needed in the session.
A quick drawing (trying out SmartDraw, hence a bit messy)
Seeing this does make me want to add some documentation w ascii/mermaid diagrams to visualize how fixtures depend on each other.
There's also something about the names of fixtures and helper functions that could be more intuitive? webtest
is actually Warehouse's own "test client" - that is, if we look at Django or FastAPI, the name of the fixture would be better as client
or simply app
(Pyramid terminology).
Similar conftest.py for FastAPI
import pytest
from fastapi.testclient import TestClient
from main import create_app
@pytest.fixture
def app():
return create_app()
@pytest.fixture
def client(app):
return TestClient(app)
Co-authored-by: Mike Fiedler <[email protected]>
Thanks for the review 🙏 Working out some updates now and will request another review when ready ⏳ |
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.
This looks great to me, thank you! Will wait for @miketheman to approve before merging.
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.
@benjaoming Thanks so much for providing all the details and context, it's been super helpful to see how it gets peeled apart as it were.
I'm 👍 here as is, and if you wanted to take another pass once merged at addressing some of the refactors/todo items to make the fixtures a little "cleaner" and less circular, I'd love to review and merge that as well.
In the meantime, we can start to benefit from a working webtest
fixture! 🚀
@miketheman indeed, it's much better to get started with some test cases before spending additional time (prematurely) designing fixtures... those can always be improved when someone gets the inspiration to do so 👍 was fun to work on! thanks for commenting and guiding 🙏 and once again, I have to say that it was a really smart move by @jzohrab to leave a PR to help someone getting started with this.. that concept should have a name, if it doesn't already. |
We could call it a PFTMR - "Please Fix This Monstrosity Request" :-) Thanks @benjaoming for taking this over the finish line, I didn't have the mental bandwidth to take it on, and it looks like it was a lot of work. Critical work too, so great stuff. |
Hi all 👋
Tested locally and seems like the test case provided by @jzohrab (#15690) now works. I couldn't have grasped this without the introductury PR, so great idea to post that.
Fixes #15634 (or maybe doesn't)
p.s. I'm very new to Warehouse and may not understand common practices, please be gentle ❤️
Possible actions:
db_session
fixtureComment back inNope, that's a required additionsqlalchemy_session_persistence = 'commit'
Fix some issues with stubs inno need in new approachtests/unit/test_db.py