Tell warehouse.db to reuse db_session pytest fixture

[benjaoming]

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:

• Investigate if the explicit transaction manager is necessary - or maybe can be moved into the db_session fixture
• Comment back in sqlalchemy_session_persistence = 'commit' Nope, that's a required addition
• Remove SQL debug statements
• Fix some issues with stubs in tests/unit/test_db.py no need in new approach

[benjaoming]

Will have a look at fixing the stubbed out unit tests in tests/unit/test_db.py 👍

[miketheman]

@benjaoming Thanks for taking a stab at this! Looks like you're coming along well. Please run a make tests locally as well, since that ought to point out any missing coverage when you're done.

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 😉

[benjaoming]

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 😉

It's coming along well 👍 I've run make tests and it worked.

Happy to address all feedback!

[miketheman]

Some comments inline, but this is shaping up nicely!

[benjaoming]

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 👍

[benjaoming]

I'm pulling in the other approach since it's better than what's in this PR currently ⏳

[miketheman]

@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!

[miketheman]

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?

[benjaoming]

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.

[benjaoming]

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 ⏳

[benjaoming]

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:

1. 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.

2. 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, the database 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

1. It avoids pulling in db_session in webtest but instead creates the session, using its own variant of app_config (app_config_dbsession_from_env).

2. ...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)

[benjaoming]

Thanks for the review 🙏 Working out some updates now and will request another review when ready ⏳

[benjaoming]

In an attempt to reorganize fixtures, I noted that this line overwrites config from the global scope:

warehouse/tests/conftest.py

Line 253 in eeb119f

config = get_config(request)

config is defined here:

warehouse/tests/conftest.py

Line 42 in eeb119f

from warehouse import admin, config, email, static

Is there an issue with linting not picking this up?

[di]

This looks great to me, thank you! Will wait for @miketheman to approve before merging.

[miketheman]

@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! 🚀

[benjaoming]

@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.

[jzohrab]

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.