Restores the expected behavior of certain fixtures, while fixing a bug related to fixtures now tearing down properly

[SalmonMode]

With pytest==5.3.3, fixtures now tear down when they should. This highlighted an issue in the django_db_blocker fixture, in that it did not actually trigger the database to be set up. To fix this, the django_db_blocker fixture was renamed to _django_db_blocker, so that another fixture could be made to take the place of django_db_blocker by taking the name and requesting both the _django_db_blocker and django_db_setup fixtures to make sure the DB is set up when using the blocker.

Additionally, some fixtures were relying on an older method of handling cleanup/finalization, and this was causing the blocker fix to have issues. Those fixtures have been updated to use the yield fixture strategy.

[blueyed]

This highlighted an issue in the django_db_blocker fixture, in that it did not actually trigger the database to be set up.

I am not really sure if that should be changed. While it might not be a typical use case it would be good for them to be kept separate, if that is possible (i.e. that it would raise django.db.utils.OperationalError with #806).

Additionally, some fixtures were relying on an older method of handling cleanup/finalization, and this was causing the blocker fix to have issues. Those fixtures have been updated to use the yield fixture strategy.

That makes sense in general already.

[blueyed]

especially since it is possible for _django_db_blocker to
be needed when django_db_setup wouldn't normally have been run (e.g. if a test
isn't marked with pytest.mark.django_db).

This now implicitly "marks" the test then, doesn't it?

[SalmonMode]

I believe it's almost the same. The only difference being that the DB is still blocked if you don't use the mark.

[blueyed]

Additionally, some fixtures were relying on an older method of handling cleanup/finalization, and this was causing the blocker fix to have issues. Those fixtures have been updated to use the yield fixture strategy.

That makes sense in general already.

Doesn't this show an issue in pytest then also, if the following does not work in the same way anymore?

diff --git i/pytest_django/fixtures.py w/pytest_django/fixtures.py
index b2cc825..f1a3abd 100644
--- i/pytest_django/fixtures.py
+++ w/pytest_django/fixtures.py
@@ -108,7 +108,9 @@ def django_db_setup(
             **setup_databases_args
         )

-    def teardown_database():
+    yield
+
+    if not django_db_keepdb:
         with django_db_blocker.unblock():
             try:
                 teardown_databases(db_cfg, verbosity=request.config.option.verbose)
@@ -119,9 +121,6 @@ def teardown_database():
                     )
                 )

-    if not django_db_keepdb:
-        request.addfinalizer(teardown_database)
-

 def _django_db_fixture_helper(
     request, django_db_blocker, transactional=False, reset_sequences=False

[SalmonMode]

@blueyed Thanks for checking this out!

I am not really sure if that should be changed. While it might not be a typical use case it would be good for them to be kept separate, if that is possible (i.e. that it would raise django.db.utils.OperationalError with #806).

It was my understanding that django_db_blocker would never be needed unless the DB was setup up. Otherwise, what would it do? I didn't see any tests where django_db_blocker was used but the DB wasn't set up at least through django_db_setup, so I'm not sure if there's a use-case for it. If there is a use case, then a test for it should definitely be added with the PR.

That said, _django_db_blocker is still a fixture, and it's doing only what django_db_blocker used to do, which is returning the _DatabaseBlocker instance. So if there's a use case, _django_db_blocker can be used. Maybe it should be renamed to something like pre_django_db_blocker so that it doesn't give the impression of "this is private, don't use"?

Additionally, some fixtures were relying on an older method of handling cleanup/finalization, and this was causing the blocker fix to have issues. Those fixtures have been updated to use the yield fixture strategy.

That makes sense in general already.

Doesn't this show an issue in pytest then also, if the following does not work in the same way anymore?

So I went back and undid my changes to these fixtures to get the exact problems they were having, but it turns out they work fine the old way (I even rebuilt my venv to make sure), so it must have been something about my old venv or the state the code was in at the moment I hit that issue.

So I guess we can just chalk these changes to yield fixtures up to general cleanup.

I can still see going through request.addfinalizer and request.getfixturevalue (rather than requesting fixtures and using yield fixtures) causing issues. But in that sense it's sort of like using addCleanup in unittest.TestCase along with a teardown method. You just have to make sure you are doing things in the right order, and aren't accidentally repeating steps. But this was always the case.

I could actually fix it up so that the _live_server_helper fixture isn't autouse and doesn't need to check request.fixturenames for live_server, or even do request.getfixturevalue("transactional_db"), by giving it a similar treatment to django_db_blocker. But I think these are enough changes for one day haha.

[blueyed]

E   AssertionError: assert False
E    +  where False = <built-in method startswith of str object at 0x7f39df269348>('test_custom_db_name_py37-django22_gw')
E    +    where <built-in method startswith of str object at 0x7f39df269348> = 'test_custom_db_name_gw0_py37-django22'.startswith

It appears "just" to be in the wrong order here?! (tox suffix and xdist suffix) I.e. the fixtures might be used not in the order being used as arguments.

[SalmonMode]

Ah, great catch! I'll add in a fix for it to make them deterministic

[SalmonMode]

I think it's upset about the coverage because I added a yield statement to get it to not complain about the other yield statement haha. But I'm not really sure how to read these coverage reports. It looks like it's saying I increased coverage in several areas, but if that's the case, it shouldn't be failing.

[blueyed]

No idea about "coverage changes", but your patch is not covered:

+	if test_name.endswith("_{}".format(suffix)):
+	    continue

With regard to that I still think this is only a workaround for a bug/issue in pytest then: it should never create duplicate suffixes in the first place.

[SalmonMode]

Ah, of course! This ran with pytest 5.3.4, which doesn't have my change in pytest! I'll pin 5.3.3 for this branch temporarily, but while I'm confident I'll hit that line, I'm not 100% positive.

The issue we saw in pytest-dev/pytest#6492 may have been related to the number of tests being run or the fact that they had set the database name to be something specific. Or some combination. I'll have to mess around with it just to be sure.

What I think was happening in general, and why I believe this to be a pytest-django issue, is that every single test (in _set_suffix_to_test_databases) was getting its DB name from db_settings.get("TEST", {}).get("NAME") (which the first time through would be test_{}".format(db_settings["NAME"]) if sqlite3 wasn't being used, or whatever it was set to be through django settings), then adding the determined suffix to the end, and putting it back in db_settings["TEST"]["NAME"], overwriting what was just pulled from there. It seems to be doing this to make sure it doesn't step on the toes of other environments or workers that could be running.

The problem, is that because every worker maintains the same runtime, but still runs all fixtures for every individual test group passed to them (which depends on how load was distributed by pytest-xdist), db_settings["TEST"]["NAME"] persisted from one test group to the next for that worker. Because these settings were persistent, the logic had it adding the suffix over and over for each new test group every time django_db_modify_db_settings_parallel_suffix was executed. Previously, it assumed, based on the problematic teardown issue shown in pytest-dev/pytest#6436 that this fixture would actually only be run once for that worker's whole session. But, with the change from pytest-dev/pytest#6438, that fixture's cache was considered invalid, so it would be torn down and executed again. Now it checks to make sure this step wasn't already performed.

The change you mentioned not being covered wasn't being hit because the change from pytest-dev/pytest#6438 wasn't applied in this run, so the fixture was never executed for that worker more than once, which means there's no way for the suffix to have already been added to the test database name for that runtime.

[SalmonMode]

Definitely shouldn't be merged as I"m investigating an issue with postgres that's potentially related

Edit: all is good now

[SalmonMode]

I'm narrowing down the problem, although it's a slow process. Luckily, I've been able to finally reproduce it on my end so I can test a lot faster (I had another error showing that turned out to be a red herring and fixed itself once I re-cloned/built everything).

The problem seems to be a result of something connecting to the database right before it tries to recreate the DB. So if you previously ran it with --reuse-db, and then tried to run it with both --reuse-db and --create-db, when it tries to drop the table so it can recreate it, it gets an error saying something else is connected to that DB so it can't drop it.

It's worth mentioning that I only see this when postgres and xdist are used.

[SalmonMode]

@blueyed Looks like pytest-dev/pytest#6551 did the trick. After some minor tweaking of this branch, everything is passing, except for certain things, which looks like a change in terms of what's supported. Maybe those tox environments should be retired?