Prevent prefix "called_" for methods on mock objects in safe mode #100690

cklein · 2023-01-02T15:51:01Z

Prevent prefix "called_" for methods on mock objects in safe mode

Pitch

In safe mode, there's already support for catching typos for accessing the assertion methods:

By default, accessing any attribute whose name starts with assert, assret, asert, aseert or assrt will raise an AttributeError.

Given you have a valid assertion to check whether a mocked function has been called:

assert mock_foo.called

If you now want to check the arguments, and do not pay full attention, you can end up with a tautology like

assert mock_foo.called_once_with(param="test")

The issue: mock_foo.called_once_with is not a valid (assertion) method and therefore an instance of mock.Mock is returned. Because instances of mock.Mock evaluate to true, the assertion is equivalent to assert True.
Like with the preventing the call of methods that start with assert and assret (issue 21238) and also disallowing the typos asert, aseert, and assrt (#23165), this error will not cause a test failure.

Analyzing public repositories on github.com, the Python standard library (thanks @terryjreedy for fixing it in #100647), and our internal code base revealed what seems to be a common source of errors. In our own code base, we have had more than 500 of these issues. More than 50% of those failed after fixing the assertion call, which could potentially have covered existing bugs by relying on bad tests.

Previous discussion

https://discuss.python.org/t/include-prefix-called-in-list-of-forbidden-method-prefixes-for-mock-objects-in-unsafe-mode/22249/4

Linked PRs

The text was updated successfully, but these errors were encountered:

cklein · 2023-01-02T16:02:24Z

Pinging @terryjreedy and @gpshead as requested in the discussion.

tirkarthi · 2023-01-03T10:51:27Z

To add a similar example Apache Airflow also fixed places where has_calls was used instead of assert_has_calls apache/airflow#20453

cklein · 2023-01-03T10:54:43Z

@tirkarthi Thanks, I'm going to add this name.

merwok · 2023-01-03T15:20:55Z

My opinion: this can only happen in test suites that use assert thing, which is not pyunit style but for example pytest style.
Our doc (and commit messages) should continue to not use assert thing style. But for the people that do use the other style, it’s worth detecting this little bug magnet, for the same reason assret_* or assert_thing are detected.

AlexWaygood · 2023-01-03T15:23:54Z

I also think this would be a useful check to add. It's very common to use unittest.mock in the context of a pytest test suite, and it's something I've used in the past. I would have appreciated this kind of check when writing those tests.

merwok · 2023-01-06T18:19:44Z

I categorized this request as a new feature, but the PR is marked bugfix with backports. Which should it be?
This is a good change, catching legitimate bugs in user code, and in a safe module, but it seems clear to me that it’s a new feature.

(adding @cjw296 here who reviewed the PR)

AlexWaygood · 2023-01-06T18:21:16Z

To me it also seemed like a new feature, which should probably go into main, but not be backported. (Though backports via the third-party mock package on PyPI seems fine.)

cjw296 · 2023-01-06T18:35:18Z

I don't agree: there was code there to catch this kind of problem, but it was insufficient / buggy. I'm fine with backporting this to 3.10 and 3.11, but since the two of you feel otherwise, I'm fine to remove those labels. They can always be added back later.

…en when using Mock (#100691) 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.

)

test_clean_storage_unmount incorrectly used 'called_with' instead of 'assert_called_with' which just made a mock function call and didn't check or assert that the function was actually called. As a result this function didn't actually test anything, the expected parameters were not correct so they have been updated. An error to catch this accidental mistake was added in the recently released mock 5.0.1 (python/cpython#100690) AttributeError: 'called_with' is not a valid assertion. Use a spec for the mock if 'called_with' is meant to be an attribute. The mock library ships in python as unittest.mock but also publishes the latest code as the 'mock' python package so that older python versions can get the latest code. The stable branches currently install mock but the master branch does not, so this error does not actually appear on master but only the <=yoga stable branches currently. The test is however wrong either way. Change-Id: I3076778e8fc62c086651d29abb2c5a3d9921be97

* Update charm-cinder from branch 'master' to ceafd9f67f3a5f404091d1f2fff7d2dcc9c71f47 - Fix missing assert_ prefix on called_with test_clean_storage_unmount incorrectly used 'called_with' instead of 'assert_called_with' which just made a mock function call and didn't check or assert that the function was actually called. As a result this function didn't actually test anything, the expected parameters were not correct so they have been updated. An error to catch this accidental mistake was added in the recently released mock 5.0.1 (python/cpython#100690) AttributeError: 'called_with' is not a valid assertion. Use a spec for the mock if 'called_with' is meant to be an attribute. The mock library ships in python as unittest.mock but also publishes the latest code as the 'mock' python package so that older python versions can get the latest code. The stable branches currently install mock but the master branch does not, so this error does not actually appear on master but only the <=yoga stable branches currently. The test is however wrong either way. Change-Id: I3076778e8fc62c086651d29abb2c5a3d9921be97

test_clean_storage_unmount incorrectly used 'called_with' instead of 'assert_called_with' which just made a mock function call and didn't check or assert that the function was actually called. As a result this function didn't actually test anything, the expected parameters were not correct so they have been updated. An error to catch this accidental mistake was added in the recently released mock 5.0.1 (python/cpython#100690) AttributeError: 'called_with' is not a valid assertion. Use a spec for the mock if 'called_with' is meant to be an attribute. The mock library ships in python as unittest.mock but also publishes the latest code as the 'mock' python package so that older python versions can get the latest code. The stable branches currently install mock but the master branch does not, so this error does not actually appear on master but only the <=yoga stable branches currently. The test is however wrong either way. Change-Id: I3076778e8fc62c086651d29abb2c5a3d9921be97

cklein added the type-feature A feature request or enhancement label Jan 2, 2023

cklein mentioned this issue Jan 2, 2023

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

Merged

This was referenced Jan 3, 2023

Replace called_with with assert_called_with getsentry/sentry#42712

Open

Replace called_with with assert_called_with capitalone/datacompy#182

Closed

cklein mentioned this issue Jan 3, 2023

Fix mock usage Yelp/paasta#3519

Merged

cjw296 closed this as completed Jan 6, 2023

sobolevn added a commit to sobolevn/cpython that referenced this issue Jan 7, 2023

pythongh-100690: [mock] hide ATTRIB_DENY_LIST and make it immutable

98c3166

bedevere-bot mentioned this issue Jan 7, 2023

gh-100690: [mock] hide ATTRIB_DENY_LIST and make it immutable #100819

Merged

cjw296 pushed a commit that referenced this issue Jan 7, 2023

gh-100690: [mock] hide ATTRIB_DENY_LIST and make it immutable (#100819

a109454

)

Prevent prefix "called_" for methods on mock objects in safe mode #100690

Prevent prefix "called_" for methods on mock objects in safe mode #100690

cklein commented Jan 2, 2023 •

edited by bedevere-bot

cklein commented Jan 2, 2023

tirkarthi commented Jan 3, 2023

cklein commented Jan 3, 2023

merwok commented Jan 3, 2023

AlexWaygood commented Jan 3, 2023

merwok commented Jan 6, 2023

AlexWaygood commented Jan 6, 2023

cjw296 commented Jan 6, 2023

Prevent prefix "called_" for methods on mock objects in safe mode #100690

Prevent prefix "called_" for methods on mock objects in safe mode #100690

Comments

cklein commented Jan 2, 2023 • edited by bedevere-bot

Prevent prefix "called_" for methods on mock objects in safe mode

Pitch

Previous discussion

Linked PRs

cklein commented Jan 2, 2023

tirkarthi commented Jan 3, 2023

cklein commented Jan 3, 2023

merwok commented Jan 3, 2023

AlexWaygood commented Jan 3, 2023

merwok commented Jan 6, 2023

AlexWaygood commented Jan 6, 2023

cjw296 commented Jan 6, 2023

cklein commented Jan 2, 2023 •

edited by bedevere-bot