Added missing decorators to tests that depended on the DB but weren't marked as such #802
Conversation
| def test_unblock_manually(self, django_db_blocker): | ||
| try: | ||
| django_db_blocker.unblock() | ||
| Item.objects.exists() | ||
| finally: | ||
| django_db_blocker.restore() | ||
|
|
||
| @pytest.mark.django_db | ||
| def test_unblock_with_block(self, django_db_blocker): | ||
| with django_db_blocker.unblock(): | ||
| Item.objects.exists() |
There was a problem hiding this comment.
Thanks, but I think the point of the tests is to actually ensure that the unblocking can be used manually, without requesting the db fixture / using the mark, which basically only triggers the unblocking for the test outside already.
It might be a good case for the pytest issue maybe, if you/we can figure out what is happening here maybe?
There was a problem hiding this comment.
@blueyed Ah, my mistake. I didn't realize. I'll dig into it and see what I can find
There was a problem hiding this comment.
@blueyed I think I've figured it out. Try running Test_django_db_blocker.test_unblock_manually on its own on pytest 5.3.2 so my changes aren't included. One would expect the test to pass.
There was a problem hiding this comment.
If it fails, it's because it's not actually setting up the DB.
There was a problem hiding this comment.
@SalmonMode
That fails with "django.db.utils.OperationalError: unable to open database file" for me, since it tries to access the "'/should_never_be_accessed'" DB (initial setting, adjusted when setting up the DB).
So this is also kind of expected in general - although should not fail in general.
pytest-django itself is used as a plugin etc and tests are rather fragile (and therefore this ("running that test alone") is not surprising).
It is the same with both 5.3.2 and 5.3.3 in that regard, isn't it?
There was a problem hiding this comment.
I just looked into why django orders their tests. They say it's because they need to ensure a clean database for every TestCase. The reason they go about it the way they do, is because of limitations with the unittest.TestCase fixture system. I'm not sure why they don't put the onus on the test writer to clean up after themselves, but they understandably want good test hygiene.
Pytest fixtures have no such limitation, and no such excuse. WIth pytest fixtures, it's easy to provide a fixture that tears itself down accordingly giving each test case a clean starting point.
There was a problem hiding this comment.
As for taking out the reordering logic, I think it would be best to have a more graceful departure from it. I would put out a warning that there are potentially hidden problems by relying on test order, and the plugin should discourage the reliance on it. And eventually the logic should be removed.
There was a problem hiding this comment.
It's mostly about speed also: transactional_db tests are especially slow (FYI).
There was a problem hiding this comment.
That I can understand. I do, however, have a plan for that as well in pytest-xdist.
https://github.com/SalmonMode/pytest-xdist/tree/set-test-group
I intend to add another hook to allow reordering of "work groups" so that the test writer can dictate which ones to hand off to workers first. So if they have their slow tests in one work group, then can move that to the top of the ordered dict.
SalmonMode
force-pushed
the
cnejame-fix-tests
branch
from
63a1d57 to
debecc8
Compare
|
Closing this, following up in #807 etc. |
Just a simple fix, but some tests were missing the marker that tells the
_django_db_markerfixture that they need the db.