Skip to content

gh-100690: Prevent prefix "called_" for attributes of safe mock objects #100691

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Jan 6, 2023
Merged

gh-100690: Prevent prefix "called_" for attributes of safe mock objects #100691

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
154012f
Prevent calling assert functions without the prefix "assert_" in safe…
cklein Jan 3, 2023
26dc793
Merge remote-tracking branch 'python/main' into disallow_mock_method_…
cklein Jan 5, 2023
2bd97a1
Added tests to show that names from denylist still can be used with a…
cklein Jan 5, 2023
01946cb
Changed documentation
cklein Jan 6, 2023
e9b8f73
Reformatted NEWS
cklein Jan 6, 2023
674f5bc
Merge branch 'main' into disallow_mock_method_prefix
cjw296 Jan 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions Lib/test/test_unittest/testmock/testmock.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1645,12 +1645,36 @@ def test_mock_unsafe(self):
m.aseert_foo_call()
with self.assertRaisesRegex(AttributeError, msg):
m.assrt_foo_call()
with self.assertRaisesRegex(AttributeError, msg):
m.called_once_with()
with self.assertRaisesRegex(AttributeError, msg):
m.called_once()
with self.assertRaisesRegex(AttributeError, msg):
m.has_calls()

class Foo(object):
def called_once(self):
pass

def has_calls(self):
pass

m = Mock(spec=Foo)
m.called_once()
m.has_calls()

m.called_once.assert_called_once()
m.has_calls.assert_called_once()

m = Mock(unsafe=True)
m.assert_foo_call()
m.assret_foo_call()
m.asert_foo_call()
m.aseert_foo_call()
m.assrt_foo_call()
m.called_once()
m.called_once_with()
m.has_calls()

# gh-100739
def test_mock_safe_with_spec(self):
Expand Down
14 changes: 10 additions & 4 deletions Lib/unittest/mock.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -653,7 +653,7 @@ def __getattr__(self, name):
elif _is_magic(name):
raise AttributeError(name)
if not self._mock_unsafe and (not self._mock_methods or name not in self._mock_methods):
if name.startswith(('assert', 'assret', 'asert', 'aseert', 'assrt')):
if name.startswith(('assert', 'assret', 'asert', 'aseert', 'assrt')) or name in ATTRIB_DENY_LIST:
raise AttributeError(
f"{name!r} is not a valid assertion. Use a spec "
f"for the mock if {name!r} is meant to be an attribute.")
Expand Down Expand Up @@ -1062,6 +1062,10 @@ def _calls_repr(self, prefix="Calls"):
return f"\n{prefix}: {safe_repr(self.mock_calls)}."


# Denylist for forbidden attribute names in safe mode
ATTRIB_DENY_LIST = {name.removeprefix("assert_") for name in dir(NonCallableMock) if name.startswith("assert_")}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much prefer this approach! 🎉

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for being late to the party, but anyways :)

I think that ATTRIB_DENY_LIST is an implementation detail, so it should be _ATTRIB_DENY_LIST. And also, it is not suited for mutation, isn't it? So, it should be a frozenset instead :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this PR is already merged, I've opened #100819 with this proposal. Feel free to close if it should be public and mutable!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is already non-public by not being included in __all__, and the mutability is not really an issue.



class _AnyComparer(list):
"""A list which checks if it contains a call which may have an
argument of ANY, flipping the components of item and self from
Expand Down Expand Up @@ -1231,9 +1235,11 @@ class or instance) that acts as the specification for the mock object. If
`return_value` attribute.

* `unsafe`: By default, accessing any attribute whose name starts with
*assert*, *assret*, *asert*, *aseert* or *assrt* will raise an
AttributeError. Passing `unsafe=True` will allow access to
these attributes.
*assert*, *assret*, *asert*, *aseert*, or *assrt* raises an AttributeError.
Additionally, an AttributeError is raised when accessing
attributes that match the name of an assertion method without the prefix
`assert_`, e.g. accessing `called_once` instead of `assert_called_once`.
Passing `unsafe=True` will allow access to these attributes.

* `wraps`: Item for the mock object to wrap. If `wraps` is not None then
calling the Mock will pass the call through to the wrapped object
Expand Down
7 changes: 7 additions & 0 deletions Misc/NEWS.d/next/Library/2023-01-02-16-59-49.gh-issue-100690.2EgWPS.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
``Mock`` objects which are not unsafe will now raise an
``AttributeError`` when accessing an attribute that matches the name
of an assertion but without the prefix ``assert_``, e.g. accessing
``called_once`` instead of ``assert_called_once``.
This is in addition to this already happening for accessing attributes
with prefixes ``assert``, ``assret``, ``asert``, ``aseert``,
and ``assrt``.