Closed bandophahita closed 10 months ago
Looks like we have a problem with Silently
not detecting the fact they are instances of the Describable
protocol
========================================================== FAILURES ==========================================================
___________________________________________ TestSilently.test_implements_protocol ____________________________________________
self = <test_actions.TestSilently object at 0x104e5f5f0>
def test_implements_protocol(self) -> None:
q1 = SilentlyAnswerable(FakeQuestion())
q2 = SilentlyPerformable(FakeAction())
q3 = SilentlyResolvable(FakeResolution())
# Debug().perform_as(None)
assert isinstance(q1, Answerable)
assert isinstance(q2, Performable)
assert isinstance(q3, Resolvable)
> assert isinstance(q1, Describable)
E assert False
E + where False = isinstance(<screenpy.actions.silently.SilentlyAnswerable object at 0x1055980e0>, Describable)
tests/test_actions.py:851: AssertionError
================================================== short test summary info ===================================================
FAILED tests/test_actions.py::TestSilently::test_implements_protocol - assert False
https://docs.python.org/3/whatsnew/3.12.html#typing
isinstance() checks against runtime-checkable protocols now use inspect.getattr_static() rather than hasattr() to lookup whether attributes exist. This means that descriptors and getattr() methods are no longer unexpectedly evaluated during isinstance() checks against runtime-checkable protocols. However, it may also mean that some objects which used to be considered instances of a runtime-checkable protocol may no longer be considered instances of that protocol on Python 3.12+, and vice versa. Most users are unlikely to be affected by this change. (Contributed by Alex Waygood in gh-102433.)
This looks like it cannot be solved simply by updating the fake objects. The problem is isinstance
will no longer work to identify the SilentlyMixin
subclasses as belonging to protocols via the __getattr__
method.
Put another way; adding the method to FakeQuestion
works but SilentlyAnswerable(FakeQuestion())
does not
def get_mock_question_class() -> Any:
class FakeQuestion(Question):
def __new__(cls, *_, **__):
rt = mock.create_autospec(FakeQuestion, instance=True)
rt.describe.return_value = "FakeQuestion"
rt.answered_by.return_value = True
return rt
def describe(self) -> str:
return ""
return FakeQuestion
This is because SilentlyMixin
does a poor job of "masquerading" as the original object. The new isinstance
doesn't "see" the methods of the duck object underneath.
One idea would be to include the describe
method on SilentlyPerformable
, SilentlyAnswerable
, and SilentlyResolvable
but that only fixes the problem for isinstance(q1, Describable)
.
The whole point of the SilentlyMixin
was to help any class wrapped in Silently
to still act and behave like the original class; this includes instance checks.
A coworker helped me realize we can just wrap the (protocol) methods at runtime. This eliminates any need to create a whole new class.
python 3.12 released this month. We need to add 3.12 to the mix of tests, etc.