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.

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

pytest --pdb breaks Django test cases #1977

Closed
adamchainz opened this issue Oct 3, 2016 · 9 comments
Closed

pytest --pdb breaks Django test cases #1977

adamchainz opened this issue Oct 3, 2016 · 9 comments

Comments

@adamchainz
Copy link
Member

Since #1890, when running with --pdb, Django test cases don't get per-test transactions. This is similar to #1932; Django has an equivalent _pre_setup function that needs calling per test to set up transactions (source - later in file for transaction support under TestCase). Since the --pdb change, the test case isn't called, so _pre_setup isn't called, so tests don't get transactional isolation.

A developer who always ran tests with --pdb noticed this first after our upgrade to 3.0.2. It took a long time to find that it was just the --pdb flag stopping database rollback, and not some weird behaviour in our app causing the inter-test failures 😢

@nicoddemus nicoddemus added type: bug problem that needs to be addressed type: regression indicates a problem that was introduced in a release which was working previously labels Oct 3, 2016
@nicoddemus
Copy link
Member

Thanks for the report! Sorry to hear this change brought you problems! 😞

I'm starting to think we probably should add a flag that is opted in to avoid this type of regression... apparently it is common for frameworks to override the TestCase.run method but not TestCase.debug.

@mbyt what do you think? Perhaps a --disable-unittest-teardown flag?

@adamchainz
Copy link
Member Author

The other option is that it gets fixed upstream in Django, and then a piece of magic is added to pytest-django to fix it for the older Django versions.

@mbyt
Copy link
Contributor

mbyt commented Oct 6, 2016

My conclusion is still the same as for #1932: If people overwrite unittest.TestCase __call__ or run and still want the post-mortem debugging feature, they need to overwrite debug in the same way (this is also true for standard unittest). So similar to adamchainz I suggests it would be anyway good if this gets fixed in Django upstream.

However for this case _post_teardown is empty, so there would not be any problem :.

What is unfortunate is that quite some people seem to overwriting run and thus now lose time when running into this issue. It would be really good, to at least somehow more pro-actively communicate this (for some cases backward incompatible) change.

@nicoddemus
Copy link
Member

It would be really good, to at least somehow more pro-actively communicate this (for some cases backward incompatible) change.

I agree. I regret we didn't release this in 3.1.0, but I don't think we could have foreseen that it could cause this much trouble. 😞

@The-Compiler
Copy link
Member

I need to say this: I knew why I mentioned (after the merge though...) that it should go to features 😉

@nicoddemus
Copy link
Member

@The-Compiler if only. 😢

@adamchainz
Copy link
Member Author

_post_teardown is not empty in all of Django's test case classes, only in SimpleTestCase. If you're using the database in anyway you have to use TransactionTestCase or TestCase which roll back the database to the pre-test setup - see https://github.com/django/django/blob/master/django/test/testcases.py#L909 . So that's how --pdb here breaks things - the first test doesn't reset the database and the second one gets an unclean state which is normally enough to break it...

I'll report this to Django and try find a fix to add to pytest-django.

@nicoddemus
Copy link
Member

@adamchainz thanks a lot!

Please keep this thread updated. 😁

I'm closing this for now, but we should re-open if we decide to take another route in the future.

@nicoddemus nicoddemus removed type: bug problem that needs to be addressed type: regression indicates a problem that was introduced in a release which was working previously labels Oct 7, 2016
@blueyed
Copy link
Contributor

blueyed commented Oct 26, 2016

@adamchainz
We have a fix for pytest-django in pytest-dev/pytest-django#406.
Have you reported this to Django already? Please followup on the pytest-django PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants