Ronny/claude experiment/opt in function definition#118
Open
RonnyPfannschmidt wants to merge 15 commits into
Open
Ronny/claude experiment/opt in function definition#118RonnyPfannschmidt wants to merge 15 commits into
RonnyPfannschmidt wants to merge 15 commits into
Conversation
…orts This commit refactors PyobjMixin to remove lazy loading and make Python objects available eagerly through constructors. Major changes include: - Added obj parameter to Node.__init__ to store Python objects at creation time - Removed _getobj() method and lazy property from PyobjMixin - Module imports now happen in Module.collect() instead of makemodule hook - Class and Function nodes receive their Python objects through constructors - Instance management moved to FixtureRequest for test methods - Updated pytest_pyfunc_call to use request.instance for method calls - Fixed unittest support with special handling for TestCaseFunction Known issues (to be addressed in follow-up commits): - Fixtures defined on test classes use different instances than test methods - Some parametrized test fixtures may not be found correctly - Static/class method tests with fixtures may fail - Some mypy errors remain to be fixed The refactoring successfully eliminates the confusing lazy loading behavior while maintaining most of pytest's functionality. Edge cases remain that need further work to fully resolve instance management for class-based tests. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…nagement - PyobjMixin now requires obj to be passed eagerly through constructor - Module imports its obj in collect() and collects marks from it - Class and Function receive obj at construction time - Instance management moved to FixtureRequest - Unittest instances managed consistently with regular test classes - Added proper type annotations for obj attributes (Module: types.ModuleType, Class: type, Function: Callable) - Fixed module-level marks collection (e.g., pytestmark) Still has ~130 test failures, mainly: - Some doctest edge cases - Some mark inheritance edge cases - Path/import related fixture issues Co-Authored-By: Claude <noreply@anthropic.com>
Major changes: - Objects are now passed eagerly through constructors for Module, Class, and Function - PyobjMixin.__init__ now extracts markers from objects when provided - Module imports happen in pytest_pycollect_makemodule to support hooks that access mod.obj - Fixed DoctestModule to import modules before accessing obj - Added support for session-scoped method fixtures with deprecation warnings - Fixed class-level parametrize and skip markers Fixes: - Module-level marks (pytestmark) now properly collected - Class-level marks (@pytest.mark.skip, @pytest.mark.parametrize) working - Doctest modules no longer fail with 'NoneType has no __name__' - Session-scoped method fixtures work with temporary instance creation and warnings Test status: - Down from ~130 failures to 47 failures - All marker tests passing - All pathlib tests passing - Doctest integration tests fixed Known issues: - ~47 remaining test failures to investigate - Session-scoped method fixtures require filterwarnings markers in tests
- Fixed PyobjMixin.__init__ to not pass obj to Node.__init__ since Node doesn't accept it - Removed unnecessary type: ignore comment - Added type annotation for module variable - Fixed test mock to not override obj type in FunctionDefinition
Using @DataClass on a subclass of a non-dataclass is problematic. Changed to a regular __init__ method that accepts obj and _nodeid parameters. Made obj a required parameter since it's always passed.
The test's MyCollector.__init__ needs to accept **kwargs to be cooperative since Class.from_parent now passes obj parameter.
The class has a module-scoped fixture defined as a method, which triggers a deprecation warning about creating temporary instances.
- Module.obj is now a property that lazy loads the module on first access - This preserves correct error handling (errors happen during collect, not makemodule) - Hooks can still access mod.obj which will trigger the import - Markers are extracted when the module is imported - Both import errors and hook customization work correctly - Added type: ignore with TODO for the property override type issue
…arent Function.from_parent now requires callobj parameter since the refactoring. Updated the test to pass the obj parameter received from the hook.
When callobj is not provided to Function.from_parent, it now tries to get it from the parent's obj attribute using the function name. This maintains backward compatibility with tests and hooks that don't pass callobj explicitly.
The xunit setup_method/teardown_method fixtures now pass the bound method instead of the unbound function, matching the expected behavior where setup_method receives the actual test method from the instance.
Successfully refactored PyobjMixin to use eager object passing: - Objects are now passed through constructors (Module, Class, Function) - Module.obj uses lazy loading via cached property for correct error handling - Markers are properly extracted from Python objects - Function.from_parent auto-resolves callobj when not provided - Fixed setup_method/teardown_method to receive bound methods - Session-scoped method fixtures work with deprecation warnings Test results: - Reduced failures from ~130 to 9 - Most core functionality working correctly - Remaining failures are edge cases that need investigation
…ce property - Properly detect and handle classmethods, staticmethods, and instance methods based on method type rather than parameter names - Update resolve_fixture_function to bind classmethods to the class - Add instance property to Function class that creates and caches instances for test methods in classes (needed for fixture handling) - Fix mypy type errors by properly initializing _instance attribute 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Work in progress on making Function.from_parent handle callobj resolution. Contains incomplete changes that need further cleanup. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Reviewer's GuideThis PR opt-in enables unified binding of fixture and test functions defined as methods by propagating the underlying Python object ( Flow diagram for unified test function invocation (call_with_appropriate_args)flowchart TD
A["call_with_appropriate_args(func, kwargs, pyfuncitem)"] --> B{Is pyfuncitem in a class?}
B -- No --> C["Call func(**kwargs)"]
B -- Yes --> D["Get parent class and method type"]
D --> E{Is staticmethod?}
E -- Yes --> F["Call func(**kwargs)"]
E -- No --> G{Is classmethod?}
G -- Yes --> H["Is func already bound?"]
H -- Yes --> I["Call func(**kwargs)"]
H -- No --> J["Call func(cls, **kwargs)"]
G -- No --> K["Get instance from pyfuncitem._request"]
K -- Instance exists --> L["Call func(instance, **kwargs)"]
K -- No instance --> M["Raise error: No instance available"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- This PR touches multiple subsystems (fixture binding, node.obj refactor, call engine changes); please consider splitting it into smaller focused PRs to ease review and reduce risk of regressions.
- There’s duplicated logic around binding instance/class methods in both call_fixture_func and call_with_appropriate_args—merging that into a single helper would simplify maintenance and avoid inconsistencies.
- The new lazy import behavior in Module.obj (and doctest collectors) alters when modules are loaded—please add a regression test to verify syntax/import errors are still reported at the intended phase.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- This PR touches multiple subsystems (fixture binding, node.obj refactor, call engine changes); please consider splitting it into smaller focused PRs to ease review and reduce risk of regressions.
- There’s duplicated logic around binding instance/class methods in both call_fixture_func and call_with_appropriate_args—merging that into a single helper would simplify maintenance and avoid inconsistencies.
- The new lazy import behavior in Module.obj (and doctest collectors) alters when modules are loaded—please add a regression test to verify syntax/import errors are still reported at the intended phase.
## Individual Comments
### Comment 1
<location> `src/_pytest/python.py:320-329` </location>
<code_context>
_ALLOW_MARKERS = True
+ obj: object # Will be more specific in subclasses
+
+ def __init__(self, *args, obj=None, **kwargs):
+ super().__init__(*args, **kwargs)
+ self.obj = obj
+ # Extract markers from the object if provided and markers are allowed
+ if obj is not None and self._ALLOW_MARKERS:
+ from _pytest.mark.structures import get_unpacked_marks
+
+ marks = get_unpacked_marks(obj)
+ self.own_markers.extend(marks)
+ # Update keywords with the new marks
+ self.keywords.update((mark.name, mark) for mark in marks)
@property
</code_context>
<issue_to_address>
**issue:** Eagerly setting obj in PyobjMixin may break lazy loading in subclasses.
This approach may interfere with subclasses that rely on lazy loading of obj, potentially impacting error handling and performance. Deferring marker extraction until obj is accessed would help preserve lazy behavior.
</issue_to_address>
### Comment 2
<location> `src/_pytest/python.py:150` </location>
<code_context>
fail(msg, pytrace=False)
+def call_with_appropriate_args(
+ func: Callable[..., Any], kwargs: dict[str, Any], pyfuncitem: Function
+) -> Any:
</code_context>
<issue_to_address>
**issue (complexity):** Consider removing the call_with_appropriate_args helper and directly calling pyfuncitem.obj, as pytest now ensures it is correctly bound.
```suggestion
# The explicit descriptor‐and‐binding logic in `call_with_appropriate_args`
# can be dropped in favor of using the already‐bound `pyfuncitem.obj`.
# Simply call it directly – pytest now constructs Function objects
# with a `callobj` that is the correct bound method or function.
--- a/_pytest/python.py
@@ def pytest_pyfunc_call(pyfuncitem: Function) -> object|None:
- assert callable(testfunction)
- # Call the function with appropriate arguments
- result = call_with_appropriate_args(testfunction, testargs, pyfuncitem)
+ # testfunction is already the correct bound method or free function
+ result = testfunction(**testargs)
# And drop the entire `call_with_appropriate_args(...)` helper:
- def call_with_appropriate_args(func: Callable[..., Any],
- kwargs: dict[str, Any],
- pyfuncitem: Function) -> Any:
- """Call a function with the appropriate positional and keyword arguments."""
- # (all of the branches for staticmethod / classmethod / instance)
- …
- raise RuntimeError(...)
```
This preserves the existing behavior (because `pyfuncitem.obj` is already bound by pytest's
`callobj` mechanism) and removes a large chunk of overlapping descriptor‐inspection code.
</issue_to_address>
### Comment 3
<location> `src/_pytest/fixtures.py:1359-1361` </location>
<code_context>
if instance is not None and hasattr(fixturefunc, "__self__"):
if not isinstance(instance, fixturefunc.__self__.__class__):
return fixturefunc
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge nested if conditions ([`merge-nested-ifs`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/merge-nested-ifs))
```suggestion
if instance is not None and hasattr(fixturefunc, "__self__") and not isinstance(instance, fixturefunc.__self__.__class__):
return fixturefunc
```
<br/><details><summary>Explanation</summary>Too much nesting can make code difficult to understand, and this is especially
true in Python, where there are no brackets to help out with the delineation of
different nesting levels.
Reading deeply nested code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two `if` conditions can be combined using
`and` is an easy win.
</details>
</issue_to_address>
### Comment 4
<location> `src/_pytest/python.py:470-477` </location>
<code_context>
if not collect_imported_tests and isinstance(self, Module):
# Do not collect functions and classes from other modules.
if inspect.isfunction(obj) or inspect.isclass(obj):
assert hasattr(
self.obj, "__name__"
) # Module objects have __name__
if obj.__module__ != self.obj.__name__:
continue
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge nested if conditions ([`merge-nested-ifs`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/merge-nested-ifs))
```suggestion
if not collect_imported_tests and isinstance(self, Module) and (inspect.isfunction(obj) or inspect.isclass(obj)):
assert hasattr(
self.obj, "__name__"
) # Module objects have __name__
if obj.__module__ != self.obj.__name__:
continue
```
<br/><details><summary>Explanation</summary>Too much nesting can make code difficult to understand, and this is especially
true in Python, where there are no brackets to help out with the delineation of
different nesting levels.
Reading deeply nested code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two `if` conditions can be combined using
`and` is an easy win.
</details>
</issue_to_address>
### Comment 5
<location> `src/_pytest/compat.py:160` </location>
<code_context>
def getfuncargnames(
function: Callable[..., object],
*,
name: str = "",
cls: type | None = None,
) -> tuple[str, ...]:
"""Return the names of a function's mandatory arguments.
Should return the names of all function arguments that:
* Aren't bound to an instance or type as in instance or class methods.
* Don't have default values.
* Aren't bound with functools.partial.
* Aren't replaced with mocks.
The cls arguments indicate that the function should be treated as a bound
method even though it's not unless the function is a static method.
The name parameter should be the original name in which the function was collected.
"""
# TODO(RonnyPfannschmidt): This function should be refactored when we
# revisit fixtures. The fixture mechanism should ask the node for
# the fixture names, and not try to obtain directly from the
# function object well after collection has occurred.
# The parameters attribute of a Signature object contains an
# ordered mapping of parameter names to Parameter instances. This
# creates a tuple of the names of the parameters that don't have
# defaults.
try:
parameters = signature(function).parameters.values()
except (ValueError, TypeError) as e:
from _pytest.outcomes import fail
fail(
f"Could not determine arguments of {function!r}: {e}",
pytrace=False,
)
arg_names = tuple(
p.name
for p in parameters
if (
p.kind is Parameter.POSITIONAL_OR_KEYWORD
or p.kind is Parameter.KEYWORD_ONLY
)
and p.default is Parameter.empty
)
if not name:
name = function.__name__
# If this function should be treated as a bound method even though
# it's passed as an unbound method or function, and its first parameter
# wasn't defined as positional only, remove the first parameter name.
if not any(p.kind is Parameter.POSITIONAL_ONLY for p in parameters) and (
# Not using `getattr` because we don't want to resolve the staticmethod.
# Not using `cls.__dict__` because we want to check the entire MRO.
(
cls
and not isinstance(
inspect.getattr_static(cls, name, default=None), staticmethod
)
)
or
# Handle bound methods (e.g., from plugin instances)
hasattr(function, "__self__")
):
arg_names = arg_names[1:]
# Remove any names that will be replaced with mocks.
if hasattr(function, "__wrapped__"):
arg_names = arg_names[num_mock_patch_args(function) :]
return arg_names
</code_context>
<issue_to_address>
**suggestion (code-quality):** Invert any/all to simplify comparisons ([`invert-any-all`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/invert-any-all/))
```suggestion
if all(p.kind is not Parameter.POSITIONAL_ONLY for p in parameters) and (
```
</issue_to_address>
### Comment 6
<location> `src/_pytest/fixtures.py:734-737` </location>
<code_context>
def _fillfixtures(self) -> None:
item = self._pyfuncitem
# Ensure instance exists if this is a test method
if self.instance is not None:
# Instance has been created by the property getter
pass
for argname in item.fixturenames:
if argname not in item.funcargs:
item.funcargs[argname] = self.getfixturevalue(argname)
</code_context>
<issue_to_address>
**suggestion (code-quality):** Remove redundant conditional ([`remove-redundant-if`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/remove-redundant-if/))
```suggestion
```
</issue_to_address>
### Comment 7
<location> `src/_pytest/fixtures.py:936` </location>
<code_context>
def call_fixture_func(
fixturefunc: _FixtureFunc[FixtureValue],
request: FixtureRequest,
kwargs,
fixturedef: FixtureDef[FixtureValue] | None = None,
) -> FixtureValue:
# Check if this is an unbound method that needs self
# This happens for high-scope (session/module) fixtures that are instance methods
if fixturedef is not None:
expects_self = getattr(fixturedef, "_expects_self", False)
is_classmethod = getattr(fixturedef, "_is_classmethod", False)
is_staticmethod = isinstance(fixturedef.func, staticmethod)
# Check if the function is already a bound method (from a plugin instance)
is_bound_method = inspect.ismethod(fixturefunc)
# If it's an instance method but we don't have an instance, we need to provide self
# BUT skip this if the method is already bound (from plugin instances)
if (
expects_self
and not is_classmethod
and not is_staticmethod
and request.instance is None
and not is_bound_method
):
# Get or create an instance for this fixture
# Look for the class from the fixture's qualname
qualname = getattr(fixturedef.func, "__qualname__", "")
if "." in qualname:
class_name = qualname.split(".")[-2]
module = getattr(fixturedef.func, "__module__", None)
if module:
import sys
if module in sys.modules:
mod = sys.modules[module]
if hasattr(mod, class_name):
cls = getattr(mod, class_name)
# Create instance and call method
instance = cls()
if inspect.isgeneratorfunction(fixturefunc):
fixturefunc = cast(
Callable[..., Generator[FixtureValue]], fixturefunc
)
generator = fixturefunc(instance, **kwargs)
try:
fixture_result = next(generator)
except StopIteration:
raise ValueError(
f"{request.fixturename} did not yield a value"
) from None
finalizer = functools.partial(
_teardown_yield_fixture, fixturefunc, generator
)
request.addfinalizer(finalizer)
return fixture_result
else:
fixturefunc = cast(
Callable[..., FixtureValue], fixturefunc
)
return fixturefunc(instance, **kwargs)
# Normal case - fixture is bound or doesn't need self
if inspect.isgeneratorfunction(fixturefunc):
fixturefunc = cast(Callable[..., Generator[FixtureValue]], fixturefunc)
generator = fixturefunc(**kwargs)
try:
fixture_result = next(generator)
except StopIteration:
raise ValueError(f"{request.fixturename} did not yield a value") from None
finalizer = functools.partial(_teardown_yield_fixture, fixturefunc, generator)
request.addfinalizer(finalizer)
else:
fixturefunc = cast(Callable[..., FixtureValue], fixturefunc)
fixture_result = fixturefunc(**kwargs)
return fixture_result
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Use named expression to simplify assignment and conditional ([`use-named-expression`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-named-expression/))
- Lift return into if ([`lift-return-into-if`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/lift-return-into-if/))
- Extract duplicate code into function ([`extract-duplicate-method`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/extract-duplicate-method/))
- Inline variable that is immediately returned ([`inline-immediately-returned-variable`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/inline-immediately-returned-variable/))
- Low code quality found in call\_fixture\_func - 19% ([`low-code-quality`](https://docs.sourcery.ai/Reference/Default-Rules/comments/low-code-quality/))
<br/><details><summary>Explanation</summary>
The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines.
- Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.</details>
</issue_to_address>
### Comment 8
<location> `src/_pytest/fixtures.py:1155` </location>
<code_context>
def __init__(
self,
config: Config,
baseid: str | None,
argname: str,
func: _FixtureFunc[FixtureValue],
scope: Scope | _ScopeName | Callable[[str, Config], _ScopeName] | None,
params: Sequence[object] | None,
ids: tuple[object | None, ...] | Callable[[Any], object | None] | None = None,
*,
_ispytest: bool = False,
# only used in a deprecationwarning msg, can be removed in pytest9
_autouse: bool = False,
) -> None:
check_ispytest(_ispytest)
# The "base" node ID for the fixture.
#
# This is a node ID prefix. A fixture is only available to a node (e.g.
# a `Function` item) if the fixture's baseid is a nodeid of a parent of
# node.
#
# For a fixture found in a Collector's object (e.g. a `Module`s module,
# a `Class`'s class), the baseid is the Collector's nodeid.
#
# For a fixture found in a conftest plugin, the baseid is the conftest's
# directory path relative to the rootdir.
#
# For other plugins, the baseid is the empty string (always matches).
self.baseid: Final = baseid or ""
# Whether the fixture was found from a node or a conftest in the
# collection tree. Will be false for fixtures defined in non-conftest
# plugins.
self.has_location: Final = baseid is not None
# The fixture factory function.
self.func: Final = func
# The name by which the fixture may be requested.
self.argname: Final = argname
if scope is None:
scope = Scope.Function
elif callable(scope):
scope = _eval_scope_callable(scope, argname, config)
if isinstance(scope, str):
scope = Scope.from_user(
scope, descr=f"Fixture '{func.__name__}'", where=baseid
)
self._scope: Final = scope
# If the fixture is directly parametrized, the parameter values.
self.params: Final = params
# If the fixture is directly parametrized, a tuple of explicit IDs to
# assign to the parameter values, or a callable to generate an ID given
# a parameter value.
self.ids: Final = ids
# The names requested by the fixtures.
argnames = getfuncargnames(func, name=argname)
# Determine the type of method and handle first parameter accordingly
is_classmethod = isinstance(func, classmethod)
is_staticmethod = isinstance(func, staticmethod)
# Check if the function is already bound (has __self__)
# This happens for plugin fixtures where an instance method is registered
is_bound_method = hasattr(func, "__self__")
# Check if this is actually a method defined in the class
# (not just a fixture registered from a class context)
is_class_method = False
if (
not is_bound_method
and baseid
and "::" in baseid
and hasattr(func, "__name__")
):
# Extract the class from baseid and check if func is actually a method of that class
# Only filter first param for actual class methods, not for fixtures registered by the class
# Check if func is a method by seeing if it has __qualname__ indicating it's defined in a class
qualname = getattr(func, "__qualname__", "")
# If qualname contains a dot, it's a method (e.g., "TestClass.method_name")
# xunit fixtures have qualname like "Class._register_setup_method_fixture.<locals>.xunit_setup_method_fixture"
# which contains "<locals>" indicating it's a nested function, not a class method
is_class_method = "." in qualname and "<locals>" not in qualname
# Check if first parameter is "cls" - this indicates a classmethod even if
# it's been wrapped by the fixture decorator
if not is_classmethod and is_class_method and argnames and argnames[0] == "cls":
is_classmethod = True
# For classmethods and instance methods defined in classes,
# the first parameter is not a fixture (it's cls or self)
# We detect this based on the method type, not parameter name
final_argnames: tuple[str, ...]
final_expects_self: bool
final_is_classmethod: bool
if is_bound_method:
# Already bound method (e.g., plugin fixtures) - self is already bound
# Don't filter any parameters
final_argnames = argnames
final_expects_self = False
final_is_classmethod = False
elif is_classmethod:
# Classmethod: first param is cls (regardless of actual name)
final_argnames = argnames[1:] if argnames else ()
final_expects_self = True
final_is_classmethod = True
elif is_staticmethod:
# Staticmethod: no special first parameter
final_argnames = argnames
final_expects_self = False
final_is_classmethod = False
elif is_class_method and argnames:
# Instance method in a class - first parameter is self (regardless of actual name)
final_argnames = argnames[1:] if argnames else ()
final_expects_self = True
final_is_classmethod = False
else:
# Regular function, not a method
final_argnames = argnames
final_expects_self = False
final_is_classmethod = False
self.argnames: Final = final_argnames
self._expects_self: Final = final_expects_self
self._is_classmethod: Final = final_is_classmethod
# If the fixture was executed, the current value of the fixture.
# Can change if the fixture is executed with different parameters.
self.cached_result: _FixtureCachedResult[FixtureValue] | None = None
self._finalizers: Final[list[Callable[[], object]]] = []
# only used to emit a deprecationwarning, can be removed in pytest9
self._autouse = _autouse
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Merge duplicate blocks in conditional ([`merge-duplicate-blocks`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/merge-duplicate-blocks/))
- Remove redundant conditional [×2] ([`remove-redundant-if`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/remove-redundant-if/))
</issue_to_address>
### Comment 9
<location> `src/_pytest/fixtures.py:1318` </location>
<code_context>
def bind_fixture_function(
func: Any,
instance: object | None,
is_classmethod: bool = False,
is_staticmethod: bool = False,
expects_self: bool = False,
) -> Any:
"""Bind a fixture function to an instance or class as needed.
This provides a single abstraction for handling:
- Regular functions (no binding)
- Instance methods (bind to instance)
- Class methods (bind to class)
- Static methods (no binding)
Args:
func: The function to potentially bind
instance: The instance to bind to (if any)
is_classmethod: Whether this is a classmethod
is_staticmethod: Whether this is a staticmethod
expects_self: Whether the function expects a self/cls parameter
Returns:
The appropriately bound (or unbound) function
"""
# Static methods never need binding
if is_staticmethod:
if isinstance(func, staticmethod):
return func.__func__
return func
# Get the underlying function
if isinstance(func, classmethod):
base_func = func.__func__
else:
base_func = getimfunc(func)
# No binding needed if function doesn't expect self/cls
if not expects_self:
return base_func
# Class methods bind to the class
if is_classmethod:
if instance is not None:
return base_func.__get__(None, instance.__class__)
# No instance, can't bind classmethod properly
# This shouldn't normally happen
return base_func
# Instance methods bind to the instance
if instance is not None:
return base_func.__get__(instance)
# Instance method but no instance - can't bind
return base_func
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Swap positions of nested conditionals ([`swap-nested-ifs`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/swap-nested-ifs/))
- Lift code into else after jump in control flow ([`reintroduce-else`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/reintroduce-else/))
- Hoist nested repeated code outside conditional statements ([`hoist-similar-statement-from-if`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/hoist-similar-statement-from-if/))
- Replace if statement with if expression [×2] ([`assign-if-exp`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/assign-if-exp/))
</issue_to_address>
### Comment 10
<location> `src/_pytest/fixtures.py:1415` </location>
<code_context>
def _get_fixture_instance(
request: FixtureRequest,
fixturedef: FixtureDef[Any],
) -> Any | None:
"""Get or create an instance for method fixtures.
For function-scoped fixtures, returns the test instance if available.
For other scopes (session, module, class), creates a temporary instance
and issues a warning about the deprecated pattern.
Returns None if no instance can be obtained.
"""
# First try to get the existing instance from the request
if hasattr(request, "instance") and request.instance is not None:
return request.instance
# For non-function scoped fixtures that need self, we need to create a fake instance
if fixturedef.scope != "function":
# Try to find the class that owns this fixture
cls = None
# Check if it's already a bound method
if hasattr(fixturedef.func, "__self__"):
return fixturedef.func.__self__
# Try to get the class from parent nodes
current = (
request._pyfuncitem if hasattr(request, "_pyfuncitem") else request.node
)
while current is not None:
if hasattr(current, "cls") and current.cls is not None:
cls = current.cls
break
current = current.parent
if cls is not None:
# Create a temporary instance and warn with exact location
import warnings
instance = cls()
# Get the fixture's location for the warning
filename = fixturedef.func.__code__.co_filename
lineno = fixturedef.func.__code__.co_firstlineno
from _pytest.warning_types import PytestDeprecationWarning
warnings.warn_explicit(
f"Creating a temporary instance for {fixturedef.scope}-scoped "
f"fixture '{fixturedef.argname}' defined as a method in {cls.__name__}. "
f"This is deprecated and may lead to unexpected behavior. "
f"Consider defining the fixture as a standalone function instead of a method, "
f"or change its scope to 'function'.",
category=PytestDeprecationWarning,
filename=filename,
lineno=lineno,
module=cls.__module__ if hasattr(cls, "__module__") else None,
)
return instance
return None
</code_context>
<issue_to_address>
**issue (code-quality):** Extract code out into function ([`extract-method`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/extract-method/))
</issue_to_address>
### Comment 11
<location> `src/_pytest/python.py:857` </location>
<code_context>
def collect(self) -> Iterable[nodes.Item | nodes.Collector]:
if not safe_getattr(self.obj, "__test__", True):
return []
if hasinit(self.obj):
assert self.parent is not None
assert hasattr(self.obj, "__name__") # Class objects have __name__
self.warn(
PytestCollectionWarning(
f"cannot collect test class {self.obj.__name__!r} because it has a "
f"__init__ constructor (from: {self.parent.nodeid})"
)
)
return []
elif hasnew(self.obj):
assert self.parent is not None
assert hasattr(self.obj, "__name__") # Class objects have __name__
self.warn(
PytestCollectionWarning(
f"cannot collect test class {self.obj.__name__!r} because it has a "
f"__new__ constructor (from: {self.parent.nodeid})"
)
)
return []
self._register_setup_class_fixture()
self._register_setup_method_fixture()
# Pass the class object directly to parsefactories
# Fixtures will be discovered from the class, not an instance
self.session._fixturemanager.parsefactories(self)
return super().collect()
</code_context>
<issue_to_address>
**issue (code-quality):** Extract duplicate code into method ([`extract-duplicate-method`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/extract-duplicate-method/))
</issue_to_address>
### Comment 12
<location> `src/_pytest/python.py:1762-1763` </location>
<code_context>
@classmethod
def from_parent(cls, parent, *, callobj=None, **kw) -> Self:
"""The public constructor."""
if callobj is None:
# Try to get the callable from the parent using the name
name = kw.get("name")
if name and hasattr(parent, "obj"):
callobj = getattr(parent.obj, name, None)
if callobj is None:
raise ValueError(f"Could not find callobj for {name} in {parent}")
return super().from_parent(parent=parent, callobj=callobj, **kw)
</code_context>
<issue_to_address>
**suggestion (code-quality):** Hoist conditional out of nested conditional ([`hoist-if-from-if`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/hoist-if-from-if/))
```suggestion
if callobj is None:
raise ValueError(f"Could not find callobj for {name} in {parent}")
```
</issue_to_address>
### Comment 13
<location> `src/_pytest/python.py:1818` </location>
<code_context>
def _traceback_filter(self, excinfo: ExceptionInfo[BaseException]) -> Traceback:
if hasattr(self, "obj") and not self.config.getoption("fulltrace", False):
code = _pytest._code.Code.from_function(get_real_func(self.obj))
path, firstlineno = code.path, code.firstlineno
traceback = excinfo.traceback
ntraceback = traceback.cut(path=path, firstlineno=firstlineno)
if ntraceback == traceback:
ntraceback = ntraceback.cut(path=path)
if ntraceback == traceback:
ntraceback = ntraceback.filter(filter_traceback)
if not ntraceback:
ntraceback = traceback
ntraceback = ntraceback.filter(excinfo)
# issue364: mark all but first and last frames to
# only show a single-line message for each frame.
if self.config.getoption("tbstyle", "auto") == "auto":
if len(ntraceback) > 2:
ntraceback = Traceback(
(
ntraceback[0],
*(t.with_repr_style("short") for t in ntraceback[1:-1]),
ntraceback[-1],
)
)
return ntraceback
return excinfo.traceback
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Add guard clause ([`last-if-guard`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/last-if-guard/))
- Hoist conditional out of nested conditional ([`hoist-if-from-if`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/hoist-if-from-if/))
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+320
to
+329
| def __init__(self, *args, obj=None, **kwargs): | ||
| super().__init__(*args, **kwargs) | ||
| self.obj = obj | ||
| # Extract markers from the object if provided and markers are allowed | ||
| if obj is not None and self._ALLOW_MARKERS: | ||
| from _pytest.mark.structures import get_unpacked_marks | ||
|
|
||
| marks = get_unpacked_marks(obj) | ||
| self.own_markers.extend(marks) | ||
| # Update keywords with the new marks |
There was a problem hiding this comment.
issue: Eagerly setting obj in PyobjMixin may break lazy loading in subclasses.
This approach may interfere with subclasses that rely on lazy loading of obj, potentially impacting error handling and performance. Deferring marker extraction until obj is accessed would help preserve lazy behavior.
| fail(msg, pytrace=False) | ||
|
|
||
|
|
||
| def call_with_appropriate_args( |
There was a problem hiding this comment.
issue (complexity): Consider removing the call_with_appropriate_args helper and directly calling pyfuncitem.obj, as pytest now ensures it is correctly bound.
Suggested change
| def call_with_appropriate_args( | |
| # The explicit descriptor‐and‐binding logic in `call_with_appropriate_args` | |
| # can be dropped in favor of using the already‐bound `pyfuncitem.obj`. | |
| # Simply call it directly – pytest now constructs Function objects | |
| # with a `callobj` that is the correct bound method or function. | |
| --- a/_pytest/python.py | |
| @@ def pytest_pyfunc_call(pyfuncitem: Function) -> object|None: | |
| - assert callable(testfunction) | |
| - # Call the function with appropriate arguments | |
| - result = call_with_appropriate_args(testfunction, testargs, pyfuncitem) | |
| + # testfunction is already the correct bound method or free function | |
| + result = testfunction(**testargs) | |
| # And drop the entire `call_with_appropriate_args(...)` helper: | |
| - def call_with_appropriate_args(func: Callable[..., Any], | |
| - kwargs: dict[str, Any], | |
| - pyfuncitem: Function) -> Any: | |
| - """Call a function with the appropriate positional and keyword arguments.""" | |
| - # (all of the branches for staticmethod / classmethod / instance) | |
| - … | |
| - raise RuntimeError(...) |
This preserves the existing behavior (because pyfuncitem.obj is already bound by pytest's
callobj mechanism) and removes a large chunk of overlapping descriptor‐inspection code.
Comment on lines
+1359
to
1361
| if instance is not None and hasattr(fixturefunc, "__self__"): | ||
| if not isinstance(instance, fixturefunc.__self__.__class__): | ||
| return fixturefunc |
There was a problem hiding this comment.
suggestion (code-quality): Merge nested if conditions (merge-nested-ifs)
Suggested change
| if instance is not None and hasattr(fixturefunc, "__self__"): | |
| if not isinstance(instance, fixturefunc.__self__.__class__): | |
| return fixturefunc | |
| if instance is not None and hasattr(fixturefunc, "__self__") and not isinstance(instance, fixturefunc.__self__.__class__): | |
| return fixturefunc | |
Explanation
Too much nesting can make code difficult to understand, and this is especiallytrue in Python, where there are no brackets to help out with the delineation of
different nesting levels.
Reading deeply nested code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two if conditions can be combined using
and is an easy win.
Comment on lines
470
to
477
| if not collect_imported_tests and isinstance(self, Module): | ||
| # Do not collect functions and classes from other modules. | ||
| if inspect.isfunction(obj) or inspect.isclass(obj): | ||
| if obj.__module__ != self._getobj().__name__: | ||
| assert hasattr( | ||
| self.obj, "__name__" | ||
| ) # Module objects have __name__ | ||
| if obj.__module__ != self.obj.__name__: | ||
| continue |
There was a problem hiding this comment.
suggestion (code-quality): Merge nested if conditions (merge-nested-ifs)
Suggested change
| if not collect_imported_tests and isinstance(self, Module): | |
| # Do not collect functions and classes from other modules. | |
| if inspect.isfunction(obj) or inspect.isclass(obj): | |
| if obj.__module__ != self._getobj().__name__: | |
| assert hasattr( | |
| self.obj, "__name__" | |
| ) # Module objects have __name__ | |
| if obj.__module__ != self.obj.__name__: | |
| continue | |
| if not collect_imported_tests and isinstance(self, Module) and (inspect.isfunction(obj) or inspect.isclass(obj)): | |
| assert hasattr( | |
| self.obj, "__name__" | |
| ) # Module objects have __name__ | |
| if obj.__module__ != self.obj.__name__: | |
| continue | |
Explanation
Too much nesting can make code difficult to understand, and this is especiallytrue in Python, where there are no brackets to help out with the delineation of
different nesting levels.
Reading deeply nested code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two if conditions can be combined using
and is an easy win.
| @@ -160,10 +160,15 @@ def getfuncargnames( | |||
| if not any(p.kind is Parameter.POSITIONAL_ONLY for p in parameters) and ( | |||
There was a problem hiding this comment.
suggestion (code-quality): Invert any/all to simplify comparisons (invert-any-all)
Suggested change
| if not any(p.kind is Parameter.POSITIONAL_ONLY for p in parameters) and ( | |
| if all(p.kind is not Parameter.POSITIONAL_ONLY for p in parameters) and ( |
| The appropriately bound (or unbound) function | ||
| """ | ||
| # Static methods never need binding | ||
| if is_staticmethod: |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Swap positions of nested conditionals (
swap-nested-ifs) - Lift code into else after jump in control flow (
reintroduce-else) - Hoist nested repeated code outside conditional statements (
hoist-similar-statement-from-if) - Replace if statement with if expression [×2] (
assign-if-exp)
|
|
||
| if cls is not None: | ||
| # Create a temporary instance and warn with exact location | ||
| import warnings |
There was a problem hiding this comment.
issue (code-quality): Extract code out into function (extract-method)
| @@ -749,6 +855,7 @@ def collect(self) -> Iterable[nodes.Item | nodes.Collector]: | |||
| return [] | |||
| if hasinit(self.obj): | |||
| assert self.parent is not None | |||
There was a problem hiding this comment.
issue (code-quality): Extract duplicate code into method (extract-duplicate-method)
Comment on lines
+1762
to
+1763
| if callobj is None: | ||
| raise ValueError(f"Could not find callobj for {name} in {parent}") |
There was a problem hiding this comment.
suggestion (code-quality): Hoist conditional out of nested conditional (hoist-if-from-if)
Suggested change
| if callobj is None: | |
| raise ValueError(f"Could not find callobj for {name} in {parent}") | |
| if callobj is None: | |
| raise ValueError(f"Could not find callobj for {name} in {parent}") |
| @@ -1676,7 +1816,7 @@ def setup(self) -> None: | |||
| self._request._fillfixtures() | |||
|
|
|||
| def _traceback_filter(self, excinfo: ExceptionInfo[BaseException]) -> Traceback: | |||
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Add guard clause (
last-if-guard) - Hoist conditional out of nested conditional (
hoist-if-from-if)
Owner
Author
RonnyPfannschmidt
left a comment
RonnyPfannschmidt
left a comment
There was a problem hiding this comment.
this is a failed experiment i'll later document - its getting gradually worse -
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Implement support for method-based fixtures and test functions by automatically binding self or cls, centralize fixture and test function binding logic, lazily create class instances for fixtures with deprecation warnings for non-function-scoped method fixtures, and refactor node obj handling to eagerly extract underlying Python objects and markers
New Features:
Bug Fixes:
Enhancements:
Tests: