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

[cklein]

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

• gh-100690: Prevent prefix "called_" for attributes of safe mock objects #100691
• gh-100690: [mock] hide ATTRIB_DENY_LIST and make it immutable #100819

[cklein]

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

[tirkarthi]

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

[cklein]

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

[merwok]

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]

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]

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]

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]

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.