bpo-35226: Fix equality for nested unittest.mock.call objects. by cjw296 · Pull Request #10555 · python/cpython (original) (raw)

A bunch of comments. I'd personally prefer @vstinner or @voidspace to review this PR as I don't agree with it (now, good news, I am just a random reviewer).

When validating calls in mocks, AFAIK people are expected to use mock_calls and call_list, that works file, see the example of the warnings you put:

m = Mock() m.factory(important=True).deliver() m.mock_calls[-1] == call.factory(important=False).deliver() True

I think this is just a wrong usage of call, people should just always call call_list if that is what they want.
This works as expected if the API is used as documetned:

m = Mock() m.factory(important=True).deliver() m.mock_calls == call.factory(important=False).deliver().call_list() False

The way call is done, allows you to also have the option of validating the call without caring about the parent argumetns if you wanted.

With that said about the API, I see three problems about this PR though:

  1. It does not address the original concern about the usage (which I disagree with though), and with it the call __eq__ "feels" different. Right now __eq__ on a mock and a call should be done by using a method that returns you all the calls that happend (in case you want to validate all the arguments), this PR removes that option, and what is worse, people might be already using this API with that objective in their minds (which is what it seems to provde). I would not backport this to previous versions of Python.
  2. Adds warnings about the a usage of call that is not the intended per the documentation.
  3. Makes quite a refactor for a PR that plans to backport, I'd just "leave previous versions as they are" to minimize risks... :/ Maybe I am too paranoid.

TL;DR;

I would just change this whole PR for a warning in the call class saying:

When used with nested call, make sure to use call_list() if you want to compare recursively with the value of the argumetns.

Real apologies if you feel I am being "hard on the PR" but I just find things are fine. call(x=1).foo == call(x=2).foo because that is how the API is. If you want to check recursively you should do: call(x=1).foo.call_list() == call(x=2).foo.call_list(). Anyway, as said, just a random reviewer, there are 3 core devs in this PR, one of them the original designer of this API :)