From 1012814f0dbccaea567658ca3cb5bd19df9ea080 Mon Sep 17 00:00:00 2001 From: Alessio Bogon Date: Fri, 3 Jun 2022 23:35:44 +0200 Subject: [PATCH 1/9] Fix _after_postgeneration being invoked twice --- pytest_factoryboy/fixture.py | 17 ++++++++++++----- tests/test_postgen_dependencies.py | 19 +++++++++++++++++++ 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/pytest_factoryboy/fixture.py b/pytest_factoryboy/fixture.py index fb7d2e0..e3ff226 100644 --- a/pytest_factoryboy/fixture.py +++ b/pytest_factoryboy/fixture.py @@ -305,11 +305,17 @@ def model_fixture(request: SubRequest, factory_name: str) -> Any: factory_class: FactoryType = request.getfixturevalue(factory_name) # Create model fixture instance - Factory: FactoryType = cast(FactoryType, type("Factory", (factory_class,), {})) - # equivalent to: - # class Factory(factory_class): - # pass - # it just makes mypy understand it. + fake_after_postgeneration_call = True + + class Factory(factory_class): # type: ignore[valid-type, misc] + @classmethod + def _after_postgeneration(cls, instance: object, create: bool, results: dict[str, Any] | None = None) -> Any: + # TODO: Explain this + if fake_after_postgeneration_call: + assert not results + return None + else: + return super()._after_postgeneration(instance, create=create, results=results) Factory._meta.base_declarations = { k: v @@ -369,6 +375,7 @@ def model_fixture(request: SubRequest, factory_name: str) -> Any: ) factoryboy_request.defer(deferred) + fake_after_postgeneration_call = False # Try to evaluate as much post-generation dependencies as possible factoryboy_request.evaluate(request) return instance diff --git a/tests/test_postgen_dependencies.py b/tests/test_postgen_dependencies.py index bca1327..57722bf 100644 --- a/tests/test_postgen_dependencies.py +++ b/tests/test_postgen_dependencies.py @@ -3,6 +3,7 @@ from dataclasses import dataclass, field from typing import TYPE_CHECKING +from unittest import mock import factory import pytest @@ -179,6 +180,24 @@ def test_postgenerationmethodcall_fixture(foo: Foo): assert foo.number == 456 +def test_postgeneration_called_once(request): + """Test that ``_after_postgeneration`` is called only once.""" + with mock.patch.object( + FooFactory, "_after_postgeneration", autospec=True, return_value=None + ) as mock_after_postgeneration: + foo = request.getfixturevalue("foo") + + assert mock_after_postgeneration.call_count == 1 + [call] = mock_after_postgeneration.mock_calls + [obj] = call.args + create = call.kwargs["create"] + results = call.kwargs["results"] + + assert obj is foo + assert create is True + assert results["set1"] == "set to 1" + + @dataclass class Ordered: value: str | None = None From 3756f973ef919d1fc7d43abdc7d4a57d685cc470 Mon Sep 17 00:00:00 2001 From: Alessio Bogon Date: Sat, 4 Jun 2022 10:56:02 +0200 Subject: [PATCH 2/9] depth should be from the caller point of view --- pytest_factoryboy/fixture.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pytest_factoryboy/fixture.py b/pytest_factoryboy/fixture.py index e3ff226..9751e0d 100644 --- a/pytest_factoryboy/fixture.py +++ b/pytest_factoryboy/fixture.py @@ -452,9 +452,9 @@ def subfactory_fixture(request: SubRequest, factory_class: FactoryType) -> Any: return request.getfixturevalue(fixture) -def get_caller_locals(depth: int = 2) -> dict[str, Any]: +def get_caller_locals(depth: int = 0) -> dict[str, Any]: """Get the local namespace of the caller frame.""" - return sys._getframe(depth).f_locals + return sys._getframe(depth + 2).f_locals class LazyFixture(Generic[T]): From bbf40427410cac9b262e96ac6860275951489767 Mon Sep 17 00:00:00 2001 From: Alessio Bogon Date: Sat, 4 Jun 2022 11:02:30 +0200 Subject: [PATCH 3/9] Be consistent --- pytest_factoryboy/fixture.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/pytest_factoryboy/fixture.py b/pytest_factoryboy/fixture.py index 9751e0d..cc467d1 100644 --- a/pytest_factoryboy/fixture.py +++ b/pytest_factoryboy/fixture.py @@ -24,7 +24,6 @@ import factory.declarations import factory.enums import inflection -from factory.declarations import NotProvided from typing_extensions import ParamSpec, TypeAlias from .compat import PostGenerationContext @@ -32,8 +31,6 @@ if TYPE_CHECKING: from _pytest.fixtures import SubRequest - from factory.builder import BuildStep - from factory.declarations import PostGeneration, PostGenerationContext from .plugin import Request as FactoryboyRequest @@ -366,7 +363,7 @@ def _after_postgeneration(cls, instance: object, create: bool, results: dict[str # that `value_provided` should be falsy postgen_value = evaluate(request, request.getfixturevalue(argname)) postgen_context = PostGenerationContext( - value_provided=(postgen_value is not NotProvided), + value_provided=(postgen_value is not factory.declarations.NotProvided), value=postgen_value, extra=extra, ) @@ -404,12 +401,12 @@ def deferred_impl(request: SubRequest) -> Any: def make_deferred_postgen( - step: BuildStep, + step: factory.builder.BuildStep, factory_class: FactoryType, fixture: str, instance: Any, attr: str, - declaration: PostGeneration, + declaration: factory.declarations.PostGenerationDeclaration, context: PostGenerationContext, ) -> DeferredFunction: """Make deferred function for the post-generation declaration. @@ -419,6 +416,7 @@ def make_deferred_postgen( :param fixture: Object fixture name e.g. "author". :param instance: Parent object instance. :param attr: Declaration attribute name e.g. "register_user". + :param declaration: Post-generation declaration. :param context: Post-generation declaration context. :note: Deferred function name results in "author__register_user". From 0045045dcc314176507b0fe1ec461ba5f140b4b6 Mon Sep 17 00:00:00 2001 From: Alessio Bogon Date: Sat, 4 Jun 2022 11:36:58 +0200 Subject: [PATCH 4/9] Nicer way to disable `_after_postgeneration` when it shouldn't run --- pytest_factoryboy/fixture.py | 47 +++++++++++++++++++++--------- tests/test_postgen_dependencies.py | 35 +++++++++++++--------- 2 files changed, 54 insertions(+), 28 deletions(-) diff --git a/pytest_factoryboy/fixture.py b/pytest_factoryboy/fixture.py index cc467d1..17c75e5 100644 --- a/pytest_factoryboy/fixture.py +++ b/pytest_factoryboy/fixture.py @@ -1,10 +1,12 @@ """Factory boy fixture integration.""" from __future__ import annotations +import contextlib import functools import sys from dataclasses import dataclass from inspect import signature +from types import MethodType from typing import ( TYPE_CHECKING, Any, @@ -12,6 +14,7 @@ Collection, Generic, Iterable, + Iterator, Mapping, Type, TypeVar, @@ -265,6 +268,7 @@ def get_deps( :return: List of the fixture argument names for dependency injection. """ model_name = get_model_name(factory_class) if model_name is None else model_name + # TODO: This does not take into account the custom _name. Fix it. parent_model_name = get_model_name(parent_factory_class) if parent_factory_class is not None else None def is_dep(value: Any) -> bool: @@ -288,6 +292,24 @@ def evaluate(request: SubRequest, value: LazyFixture[T] | T) -> T: return value.evaluate(request) if isinstance(value, LazyFixture) else value +def noop(*args: Any, **kwargs: Any) -> None: + """No-op function.""" + pass + + +@contextlib.contextmanager +def disable_method(method: MethodType) -> Iterator[None]: + """Disable a method.""" + klass = method.__self__ + method_name = method.__name__ + old_method = getattr(klass, method_name) + setattr(klass, method_name, noop) + try: + yield + finally: + setattr(klass, method.__name__, old_method) + + def model_fixture(request: SubRequest, factory_name: str) -> Any: """Model fixture implementation.""" factoryboy_request: FactoryboyRequest = request.getfixturevalue("factoryboy_request") @@ -302,17 +324,11 @@ def model_fixture(request: SubRequest, factory_name: str) -> Any: factory_class: FactoryType = request.getfixturevalue(factory_name) # Create model fixture instance - fake_after_postgeneration_call = True - - class Factory(factory_class): # type: ignore[valid-type, misc] - @classmethod - def _after_postgeneration(cls, instance: object, create: bool, results: dict[str, Any] | None = None) -> Any: - # TODO: Explain this - if fake_after_postgeneration_call: - assert not results - return None - else: - return super()._after_postgeneration(instance, create=create, results=results) + Factory: FactoryType = cast(FactoryType, type("Factory", (factory_class,), {})) + # equivalent to: + # class Factory(factory_class): + # pass + # it just makes mypy understand it. Factory._meta.base_declarations = { k: v @@ -331,7 +347,10 @@ def _after_postgeneration(cls, instance: object, create: bool, results: dict[str builder = factory.builder.StepBuilder(Factory._meta, kwargs, strategy) step = factory.builder.BuildStep(builder=builder, sequence=Factory._meta.next_sequence()) - instance = Factory(**kwargs) + # FactoryBoy invokes the `_after_postgeneration` method, but we will instead call it manually later, + # once we are able to evaluate all the related fixtures. + with disable_method(Factory._after_postgeneration): + instance = Factory(**kwargs) # Cache the instance value on pytest level so that the fixture can be resolved before the return request._fixturedef.cached_result = (instance, 0, None) @@ -372,8 +391,8 @@ def _after_postgeneration(cls, instance: object, create: bool, results: dict[str ) factoryboy_request.defer(deferred) - fake_after_postgeneration_call = False - # Try to evaluate as much post-generation dependencies as possible + # Try to evaluate as much post-generation dependencies as possible. + # This will finally invoke Factory._after_postgeneration, which was previously disabled factoryboy_request.evaluate(request) return instance diff --git a/tests/test_postgen_dependencies.py b/tests/test_postgen_dependencies.py index 57722bf..629b8ab 100644 --- a/tests/test_postgen_dependencies.py +++ b/tests/test_postgen_dependencies.py @@ -3,7 +3,6 @@ from dataclasses import dataclass, field from typing import TYPE_CHECKING -from unittest import mock import factory import pytest @@ -180,22 +179,30 @@ def test_postgenerationmethodcall_fixture(foo: Foo): assert foo.number == 456 -def test_postgeneration_called_once(request): - """Test that ``_after_postgeneration`` is called only once.""" - with mock.patch.object( - FooFactory, "_after_postgeneration", autospec=True, return_value=None - ) as mock_after_postgeneration: +class TestPostgenerationCalledOnce: + calls: list[tuple[Foo, bool, dict[str, Any] | None]] = [] + + @register(_name="foo") + class CollectingFooFactory(FooFactory): + class Meta: + model = Foo + + @classmethod + def _after_postgeneration(cls, obj: Foo, create: bool, results: dict[str, Any] | None = None) -> None: + TestPostgenerationCalledOnce.calls.append((obj, create, results)) + + def test_postgeneration_called_once(self, request): + """Test that ``_after_postgeneration`` is called only once.""" foo = request.getfixturevalue("foo") - assert mock_after_postgeneration.call_count == 1 - [call] = mock_after_postgeneration.mock_calls - [obj] = call.args - create = call.kwargs["create"] - results = call.kwargs["results"] + assert len(self.calls) == 1 + [call] = self.calls + [obj, create, results] = call - assert obj is foo - assert create is True - assert results["set1"] == "set to 1" + assert obj is foo + assert create is True + assert isinstance(results, dict) + assert results["set1"] == "set to 1" @dataclass From 1c41a824cb60dae411e476887fdd9e0a42636b63 Mon Sep 17 00:00:00 2001 From: Alessio Bogon Date: Sat, 4 Jun 2022 11:50:02 +0200 Subject: [PATCH 5/9] Simplify test, don't use class variables (makes the test runnable only once) --- tests/test_postgen_dependencies.py | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/tests/test_postgen_dependencies.py b/tests/test_postgen_dependencies.py index 629b8ab..6bb0453 100644 --- a/tests/test_postgen_dependencies.py +++ b/tests/test_postgen_dependencies.py @@ -180,29 +180,30 @@ def test_postgenerationmethodcall_fixture(foo: Foo): class TestPostgenerationCalledOnce: - calls: list[tuple[Foo, bool, dict[str, Any] | None]] = [] - - @register(_name="foo") - class CollectingFooFactory(FooFactory): + @register(_name="collector") + class CollectorFactory(factory.Factory): class Meta: - model = Foo + model = dict + + foo = factory.PostGeneration(lambda *args, **kwargs: 42) @classmethod - def _after_postgeneration(cls, obj: Foo, create: bool, results: dict[str, Any] | None = None) -> None: - TestPostgenerationCalledOnce.calls.append((obj, create, results)) + def _after_postgeneration( + cls, obj: dict[str, Any], create: bool, results: dict[str, Any] | None = None + ) -> None: + obj.setdefault("_after_postgeneration_calls", []).append((obj, create, results)) def test_postgeneration_called_once(self, request): """Test that ``_after_postgeneration`` is called only once.""" - foo = request.getfixturevalue("foo") - - assert len(self.calls) == 1 - [call] = self.calls - [obj, create, results] = call + foo = request.getfixturevalue("collector") + calls = foo["_after_postgeneration_calls"] + assert len(calls) == 1 + [[obj, create, results]] = calls assert obj is foo assert create is True assert isinstance(results, dict) - assert results["set1"] == "set to 1" + assert results["foo"] == 42 @dataclass From 04a18043baae392f7c82a8b98aacfb635816fa5d Mon Sep 17 00:00:00 2001 From: Alessio Bogon Date: Sat, 4 Jun 2022 11:52:54 +0200 Subject: [PATCH 6/9] Add changelog entry --- CHANGES.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGES.rst b/CHANGES.rst index 7866f57..950032d 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -3,7 +3,8 @@ Changelog Unreleased ---------- -- The fixture name for registered factories is now determined by the factory name (rather than the model name). This makes factories for builtin types (like ``dict``) easier to use. +- The fixture name for registered factories is now determined by the factory name (rather than the model name). This makes factories for builtin types (like ``dict``) easier to use. `#163 `_ +- Fix ``Factory._after_postgeneration`` being invoked twice. `#164 `_ `#156 `_ 2.4.0 ---------- From 3a5227716cc35ab9ba1711d8c9e4b8159857e575 Mon Sep 17 00:00:00 2001 From: Alessio Bogon Date: Sat, 4 Jun 2022 12:39:21 +0200 Subject: [PATCH 7/9] Remove TODO, I opened an issue for that --- pytest_factoryboy/fixture.py | 1 - 1 file changed, 1 deletion(-) diff --git a/pytest_factoryboy/fixture.py b/pytest_factoryboy/fixture.py index 17c75e5..c4bbb14 100644 --- a/pytest_factoryboy/fixture.py +++ b/pytest_factoryboy/fixture.py @@ -268,7 +268,6 @@ def get_deps( :return: List of the fixture argument names for dependency injection. """ model_name = get_model_name(factory_class) if model_name is None else model_name - # TODO: This does not take into account the custom _name. Fix it. parent_model_name = get_model_name(parent_factory_class) if parent_factory_class is not None else None def is_dep(value: Any) -> bool: From 48394b629d004c4e55c0488931a9a8cce5fa79b0 Mon Sep 17 00:00:00 2001 From: Alessio Bogon Date: Sat, 4 Jun 2022 12:46:00 +0200 Subject: [PATCH 8/9] Make nicer changelog --- CHANGES.rst | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/CHANGES.rst b/CHANGES.rst index 950032d..7a2b5e2 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -4,6 +4,22 @@ Changelog Unreleased ---------- - The fixture name for registered factories is now determined by the factory name (rather than the model name). This makes factories for builtin types (like ``dict``) easier to use. `#163 `_ + +.. code-block:: python + + # example + @register + class HTTPHeadersFactory(factory.Factory): + class Meta: + model = dict # no need to use a special dict subclass anymore + + Authorization = "Basic Zm9vOmJhcg==" + + + def test_headers(headers): + assert headers["Authorization"] == "Basic Zm9vOmJhcg==" + + - Fix ``Factory._after_postgeneration`` being invoked twice. `#164 `_ `#156 `_ 2.4.0 From 2b3fee3c3c9e518f8cfe4a905ec6d989e48f2cee Mon Sep 17 00:00:00 2001 From: Alessio Bogon Date: Sat, 4 Jun 2022 12:48:05 +0200 Subject: [PATCH 9/9] Explain how to migrate. --- CHANGES.rst | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 7a2b5e2..204967c 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -3,7 +3,7 @@ Changelog Unreleased ---------- -- The fixture name for registered factories is now determined by the factory name (rather than the model name). This makes factories for builtin types (like ``dict``) easier to use. `#163 `_ +- The generated fixture name is now determined by the factory (rather than the model). This makes factories for builtin types (like ``dict``) easier to use. You may need to change your factories to use the ``Factory`` naming convention, or use the ``register(_name=...)`` override. `#163 `_ .. code-block:: python @@ -19,7 +19,6 @@ Unreleased def test_headers(headers): assert headers["Authorization"] == "Basic Zm9vOmJhcg==" - - Fix ``Factory._after_postgeneration`` being invoked twice. `#164 `_ `#156 `_ 2.4.0