Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Added missing decorators to tests that depended on the DB but weren't marked as such #802

Closed
wants to merge 1 commit into from
Closed

Added missing decorators to tests that depended on the DB but weren't marked as such #802

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions tests/test_fixtures.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -617,13 +617,15 @@ def test_block_with_block(self, django_db_blocker):
with pytest.raises(RuntimeError):
Item.objects.exists()

@pytest.mark.django_db
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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@blueyed Ah, my mistake. I didn't realize. I'll dig into it and see what I can find

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it fails, it's because it's not actually setting up the DB.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@blueyed blueyed Jan 18, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's mostly about speed also: transactional_db tests are especially slow (FYI).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revised solution here: #807

Expand Down