-
Notifications
You must be signed in to change notification settings - Fork 964
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
conftest: put transaction manager in its own fixture #16796
Conversation
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
@woodruffw Looks like this breaks some tests (probably because it's session-scoped). |
Whoops, fixed! |
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'm 👍 generally, but I'm curious about the timing of this - since the tm.begin()/tm.doom()
calls now happen after get_db_session_for_app_config()
is called - does that change the behavior at all?
I might not be following, but I think the order is correct here -- if my understanding of
But it's possible I'm missing where the scope has changed 🙂 |
Maybe I didn't phrase the question well? I don't think it will matter, so let's ship this and find out. |
Ah, I see what you mesn! Yeah, I think that won't matter, but that is indeed a slight difference. |
This moves the
tm
local from thewebtest
fixture into its own fixture.This should have no impact on its own, but it will hopefully make it easier to tease apart whatever session/TM mismatch is causing CI failures like https://github.com/pypi/warehouse/actions/runs/11057423074/job/30721387457?pr=16778.
See: #15634 (comment)