pytest-dev / pytest-django

A Django plugin for pytest.
https://pytest-django.readthedocs.io/
Other
1.37k stars 344 forks source link

Make pytest-django work with hypothesis #912

Open kontsaki opened 3 years ago

kontsaki commented 3 years ago

Hello, Do you think that this is possible as described on https://github.com/HypothesisWorks/hypothesis/issues/733 and implement the trio solution?

I will be happy to try and tackle this if you think it's something pytest-django wants.

OtherBarry commented 2 years ago

@kontsaki any progress on this?

I'll give it a shot if you're not actively working on it.

kontsaki commented 2 years ago

Hello @OtherBarry, go ahead, I haven't worked on this at all.

OtherBarry commented 2 years ago

I've gotten this working locally for my current project, by adding the below to my conftest.py

from typing import Any, Callable

import django.db
import django.test
import pytest
from _pytest.nodes import Item
from django import VERSION
from pytest_django.django_compat import is_django_unittest
from pytest_django.fixtures import validate_django_db

def convert_to_django_test(
    item: Item, inner_test: Callable[..., Any]
) -> Callable[..., Any]:
    marker = item.get_closest_marker("django_db")
    if marker:
        (
            transactional,
            reset_sequences,
            databases,
            serialized_rollback,
        ) = validate_django_db(marker)
    else:
        (transactional, reset_sequences, databases, serialized_rollback,) = (
            False,
            False,
            None,
            False,
        )

    if transactional:
        test_case_class = django.test.TransactionTestCase
    else:
        test_case_class = django.test.TestCase

    _reset_sequences = reset_sequences
    _serialized_rollback = serialized_rollback
    _databases = databases

    class PytestDjangoTestCase(test_case_class):  # type: ignore[misc,valid-type]
        reset_sequences = _reset_sequences
        serialized_rollback = _serialized_rollback
        if _databases is not None:
            databases = _databases

        if not transactional:

            @classmethod
            def setUpClass(cls) -> None:
                super().setUpClass()
                if (3, 2) <= VERSION < (4, 1):
                    django.db.transaction.Atomic._ensure_durability = False  # type: ignore[attr-defined]

            @classmethod
            def tearDownClass(cls) -> None:
                if (3, 2) <= VERSION < (4, 1):
                    django.db.transaction.Atomic._ensure_durability = True  # type: ignore[attr-defined]
                super().tearDownClass()

    def _new_inner_test(*args: Any, **kwargs: Any) -> None:
        PytestDjangoTestCase.setUpClass()
        test_case = PytestDjangoTestCase(methodName="__init__")
        test_case._pre_setup()
        inner_test(*args, **kwargs)
        test_case._post_teardown()
        if VERSION >= (4, 0):
            PytestDjangoTestCase.doClassCleanups()
        PytestDjangoTestCase.tearDownClass()

    return _new_inner_test

@pytest.hookimpl(hookwrapper=True)  # type: ignore[misc]
def pytest_runtest_call(item: Item) -> Any:
    marker = item.get_closest_marker("django_db")
    if (
        marker is not None
        and not is_django_unittest(item)
        and hasattr(item, "obj")
        and hasattr(item.obj, "hypothesis")  # type: ignore[attr-defined]
    ):
        item.obj.hypothesis.inner_test = convert_to_django_test(  # type: ignore[attr-defined]
            item, item.obj.hypothesis.inner_test  # type: ignore[attr-defined]
        )
    yield

It's a bit hacky, and there's a lot of duplication from existing pytest-django code, but it seems to work.

When I have some more time I'll try and clean it up and make a PR. Alternatively if someone with some better knowledge of pytest/hypothesis/pytest-django wants to implement it properly, please do.

Zac-HD commented 2 years ago

I don't know Django at all, but would be very happy to help with review on the Hypothesis and Pytest parts, not least to fix https://github.com/HypothesisWorks/hypothesis/issues/2382 and https://github.com/HypothesisWorks/hypothesis/issues/3216.

Zac-HD commented 2 years ago

(oops, misclick 😅)

OtherBarry commented 2 years ago

Oh that first issue is a much smoother way of doing things, I'll see if I can get that to work (sans monkeypatch).

In the meantime, I'm getting some interesting side effects with my current implementation, that you might be able to help with.

  1. Any hypthosis tracebacks come through as Hypothesis _new_inner_test(...) produces unreliable results: .... Ideally this should display the original test name, rather than _new_inner_test. Is there an easy way to fix this?
  2. Tests are running a lot slower - running with max_examples=1000, my test suite has gone from taking ~10 minutes to taking ~25 minutes, with a few random hypothesis tests failing each time due to Unreliable test timings. I'm guessing the increased time is due to how I'm handling the django side, but could it stemming from the first hypothesis call including the convert_to_django_test time in it's total time?
OtherBarry commented 2 years ago

Made a simpler version, that doesn't handle transactions, but does run at the expected speeds:

import functools
import warnings
from typing import Any, Callable

import django.db
import django.test
import pytest
from _pytest.mark import Mark
from _pytest.nodes import Item
from pytest_django.fixtures import validate_django_db

def convert_to_django_test(
    django_db_marker: Mark, inner_test: Callable[..., Any]
) -> Callable[..., Any]:
    transactional, reset_sequences, databases, serialized_rollback = validate_django_db(
        django_db_marker
    )

    if transactional:
        warnings.warn(
            "The django_db decorator does not currently support using pytest with hypothesis in transaction mode. The database will not be reset between tests. Please use hypothesis.extra.django.TransactionTestCase.",
            RuntimeWarning,
        )
        return inner_test

    @functools.wraps(inner_test)
    def _new_inner_test(*args: Any, **kwargs: Any) -> None:
        atomics = django.test.TestCase._enter_atomics()  # type: ignore[attr-defined]
        inner_test(*args, **kwargs)
        django.test.TestCase._rollback_atomics(atomics)  # type: ignore[attr-defined]

    return _new_inner_test

@pytest.hookimpl(hookwrapper=True)  # type: ignore[misc]
def pytest_runtest_call(item: Item) -> Any:
    django_db_mark = item.get_closest_marker("django_db")
    hypothesis_mark = item.get_closest_marker("hypothesis")
    if django_db_mark is not None and hypothesis_mark is not None:
        item.obj.hypothesis.inner_test = convert_to_django_test(django_db_mark, item.obj.hypothesis.inner_test)  # type: ignore[attr-defined]
    yield
Zac-HD commented 2 years ago
  1. Any hypthosis tracebacks come through as Hypothesis _new_inner_test(...) produces unreliable results: .... Ideally this should display the original test name, rather than _new_inner_test. Is there an easy way to fix this?

Try @functools.wraps(inner_test) 🙂

  1. Tests are running a lot slower - running with max_examples=1000, my test suite has gone from taking ~10 minutes to taking ~25 minutes, with a few random hypothesis tests failing each time due to Unreliable test timings. I'm guessing the increased time is due to how I'm handling the django side, but could it stemming from the first hypothesis call including the convert_to_django_test time in its total time?

I'm pretty sure that this is purely the fact that transactions take longer than not using transactions! The correct response here is probably just to increase (or disable!) the deadline setting, since your tests aren't expected to always be well under 200ms when including rollback times.

Otherwise this looks good to me, since it should be fairly obvious how to add transaction support too!

Zac-HD commented 2 years ago

@OtherBarry - anything I can help with to turn this into a PR?

OtherBarry commented 2 years ago

@Zac-HD Other than adding the functools wrapper (now edited into my solution above), I haven't gotten very far.

My attempts to use the PytestDjangoTestCase from pytest-django result in the big slowdowns I mentioned above, even without using any transaction test cases, just the same atomic ones from my existing solution.

My guess is that there's some more time consuming logic that can be run once to setup/teardown the outer test, and then the rest can be for each hypothesis inner test. Unfortunately I don't have an in-depth enough understanding of how django test cases really work to know where to draw that line, and trial and error hasn't gotten me very far.

Additionally it's my first time contributing anything more than a small bug fix to something open source, so I don't really know how to best make a PR of my current solution (if that's even appropriate), as it requires adding hypothesis as a requirement for testing.

Zac-HD commented 2 years ago

Thanks for the update, and fair enough! (and I'm honoured that you've chosen our project for your first big OSS contribution 🥰)

@hramezani might be able to help out better than I, since he seems to have more than my (zero) experience with Django and pytest-djano maintainence. If it's general pytest or Hypothesis knowledge I can be more helpful 😅