classification
Title: Check against misspellings of assert etc. in mock
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.10
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: gregory.p.smith Nosy List: cjw296, gregory.p.smith, lisroach, mariocj89, michael.foord, miss-islington, rbcollins, rhettinger, terry.reedy, vabr2, veky, xtreak
Priority: normal Keywords: patch

Created on 2020-09-28 18:19 by vabr2, last changed 2020-12-14 19:22 by vabr2. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 23165 merged vabr2, 2020-11-05 15:14
PR 23729 merged vabr2, 2020-12-10 09:21
PR 23737 merged vabr2, 2020-12-10 19:47
Messages (17)
msg377611 - (view) Author: Václav Brožek (vabr2) * Date: 2020-09-28 18:19
Recently we cleaned up the following typos in mocks in unittests of our codebase:

* Wrong prefixes of mock asserts: asert/aseert/assrt -> assert
* Wrong attribute names around asserts: autospect/auto_spec -> autospec, set_spec -> spec_set

Especially the asserts are dangerous, because a misspelled assert_called will fail silently. We found real bugs in production code which were masked by a misspelled assert_called.

There is prior work done to report similar cases of assert misspellings with an AttributeError: https://github.com/testing-cabal/mock/commit/7c530f0d9aa48d2538501761098df7a5a8979a7d, and adding new cases will be an easy change. I suppose that adding similar error signalling for the wrong argument names will not be much harder.

I'm prepared to implement it, if people of this project would be happy to have such checks.
msg377612 - (view) Author: Vedran Čačić (veky) * Date: 2020-09-28 18:41
How about we actually _solve_ the problem, instead of masking it with layer upon layer of obfuscation?

Python has standalone functions. assert_called (and company) should just be functions, they shouldn't be methods, and the problem is solved elegantly. The whole reason the problem exists in the first place is that Java, where Python copied the API from, has no standalone functions. In Pythonic design, the problem shouldn't exist at all.
msg377620 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2020-09-28 20:12
Václav, I support your suggestion and suggest that you move forward to prepare a PR.

Vedran, this API has been set in stone for a long time, so changing is disruptive.  If you really think the API needs to be redesigned, consider bringing it up on python-ideas.  The proposal would need broad support to move forward.
msg377625 - (view) Author: Vedran Čačić (veky) * Date: 2020-09-28 20:40
It would be a valid argument if the API _worked_. Obviously, it doesn't. Every few years, the same story repeats. "We've found even more missspellings of assert, we need to add them too. They cause real bugs in our tests." I have a strong feeling it will never end.
msg377841 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2020-10-02 23:43
Vedran, you explained why many use pytest instead of unittest. But we have the latter and a stability policy.

I am not familiar with the existing mock code, but one already invented solution for misspelling tolerance without enumeration is the soundex algorithm.  I have not read the details for over a decade, but I belive soundex(<assert variation>) = 'asrt' for all examples given here.  Perhaps it could be used to broaden the test.
msg378255 - (view) Author: Vedran Čačić (veky) * Date: 2020-10-08 16:19
Sorry, but 

1) Stability policy is great when we hold on to it. If we add new spellings of assert every few years, what kind of stability it is? "My" solution is of the same type: just add one more thing to the API. But that solution is better, because a) it doesn't clash, and b) it must be done only once.

2) Soundex has nothing to do with it. It was invented to mitigate errors when transferring _sounds_ (phonemes) over the telephone wire. It would surprise me very much if those were in any useful sense correlated with errors encountered typing graphemes on the mechanical keyboard.
msg378299 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python committer) Date: 2020-10-09 03:12
See also below issues for previous discussion on making the assert helpers as top level functions : 

https://bugs.python.org/issue24651
https://bugs.python.org/issue30949
msg378302 - (view) Author: Vedran Čačić (veky) * Date: 2020-10-09 04:11
Of course, that's why I wrote "my" in quotes above. It's not my solution, it's the idea that many people independently had. Because it is _the_ solution, of course. :-]

I'd just like to point out in the above thread (first link you provided), how _many_ people operate under the assumption that "misspelling problems are now solved". I'm sure many of their opinions would be different if they knew we'd be having this discussion now.

Let's not make the same mistake again. I assure you that in few more years we'll find some other creative ways to misspell arrest_* methods. ;-)
msg378569 - (view) Author: Václav Brožek (vabr2) * Date: 2020-10-13 17:10
Thank you all for the informative replies (and sorry for my long silence, I was sick).

I agree that the general solution (module-level assert) is worth doing, and I just added the current motivation to https://bugs.python.org/issue24651.

I still think that the cost associated with bad misspellings compared to the effort to extend the existing solution (adding patterns to [1]) is strongly in favour of extending the solution: the recent clean-up we had cost us many hours of work and involved several people (especially cases with potential or real bugs being discovered after the fixed typos). Adding a pattern to [1] seems much cheaper than the cost it saves.

The general solution is unlikely to be implemented soon, and even once it is, migrating existing code to use it seems unrealistic from the cost perspective. That's why I think that adding the newly found patterns to [1] makes sense.


[1] https://github.com/python/cpython/blob/master/Lib/unittest/mock.py#L634
msg380412 - (view) Author: Václav Brožek (vabr2) * Date: 2020-11-05 15:19
A pull request implementing the first part of this issue (Wrong prefixes of mock asserts: asert/aseert/assrt -> assert) is at https://github.com/python/cpython/pull/23165.

I acknowledge that this is a controversial topic and I put forward my reasons to carry on with the above pull request in msg378569. I welcome a further discussion on the PR.
msg380419 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2020-11-05 17:04
New changeset 4662fa9bfe4a849fe87bfb321d8ef0956c89a772 by vabr-g in branch 'master':
bpo-41877 Check for asert, aseert, assrt in mocks (GH-23165)
https://github.com/python/cpython/commit/4662fa9bfe4a849fe87bfb321d8ef0956c89a772
msg380422 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2020-11-05 17:49
Thanks for the patch!  I'm leaving this open as dealing with the other aspect:

* Wrong attribute names around asserts: autospect/auto_spec -> autospec, set_spec -> spec_set

is still a possibility.  (given you also found a number of those in our codebase leading to hidden testing bugs)


Vedran: We're not claiming these are fixes for the fundamental problem.  But they are useful practical bandaids on the existing APIs that a hundred of millions of lines of existing unittest code around the world make use of to prevent common unintended consequences of our existing API's now well know flaws.  issue24651 looks like the better place to take up future design ideas.
msg382820 - (view) Author: Václav Brožek (vabr2) * Date: 2020-12-10 09:45
https://github.com/python/cpython/pull/23729 is now a follow-up to improve docs and error message about the assert misspellings.

I'm now looking at the misspelled arguments: autospec and spec_set. These are accepted by patch, patch.object and patch.multiple, and spec_set is also accepted by create_autospec.

The three frequent misspellings I saw in our codebase are auto_spec, autospect and set_spec. I could add a check to look for them in kwargs, and also add an "unsafe" argument (default False), which, when True, disables the check. This would mimic what is already done for the misspelled asserts.
msg382838 - (view) Author: miss-islington (miss-islington) Date: 2020-12-10 18:35
New changeset 9fc571359af9320fddbe4aa2710a767f168c1707 by vabr-g in branch 'master':
bpo-41877: Improve docs for assert misspellings check in mock (GH-23729)
https://github.com/python/cpython/commit/9fc571359af9320fddbe4aa2710a767f168c1707
msg382839 - (view) Author: Václav Brožek (vabr2) * Date: 2020-12-10 19:50
https://github.com/python/cpython/pull/23737 has the initial draft of the check against misspelled arguments.
msg383000 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2020-12-14 18:30
New changeset fdb9efce6ac211f973088eef508740c3fa2bd182 by vabr-g in branch 'master':
bpo-41877: Check for misspelled speccing arguments (GH-23737)
https://github.com/python/cpython/commit/fdb9efce6ac211f973088eef508740c3fa2bd182
msg383001 - (view) Author: Václav Brožek (vabr2) * Date: 2020-12-14 19:22
The three PRs which landed for this bug implement what I planned in the original comment, so I'm closing this bug.
History
Date User Action Args
2020-12-14 19:22:54vabr2setstatus: open -> closed
resolution: fixed
messages: + msg383001

stage: patch review -> resolved
2020-12-14 18:30:16gregory.p.smithsetmessages: + msg383000
2020-12-10 19:50:39vabr2setmessages: + msg382839
2020-12-10 19:47:44vabr2setpull_requests: + pull_request22596
2020-12-10 18:35:40miss-islingtonsetnosy: + miss-islington
messages: + msg382838
2020-12-10 09:45:01vabr2setmessages: + msg382820
2020-12-10 09:21:43vabr2setpull_requests: + pull_request22589
2020-11-05 17:49:31gregory.p.smithsetmessages: + msg380422
2020-11-05 17:04:46gregory.p.smithsetmessages: + msg380419
2020-11-05 16:57:37gregory.p.smithsetassignee: gregory.p.smith
2020-11-05 16:52:02gregory.p.smithsetnosy: + gregory.p.smith
2020-11-05 15:19:18vabr2setmessages: + msg380412
2020-11-05 15:14:34vabr2setkeywords: + patch
stage: patch review
pull_requests: + pull_request22077
2020-10-13 17:10:31vabr2setmessages: + msg378569
2020-10-09 04:11:48vekysetmessages: + msg378302
2020-10-09 03:12:56xtreaksetnosy: + lisroach, cjw296, xtreak, rbcollins, mariocj89
messages: + msg378299
2020-10-08 16:19:24vekysetmessages: + msg378255
2020-10-02 23:43:15terry.reedysetnosy: + terry.reedy
messages: + msg377841
2020-09-28 20:40:03vekysetmessages: + msg377625
2020-09-28 20:12:58rhettingersetnosy: + rhettinger, michael.foord
messages: + msg377620
2020-09-28 18:41:04vekysetnosy: + veky
messages: + msg377612
2020-09-28 18:19:17vabr2create