Repository navigation
Check against misspellings of assert etc. in mock #86043
Description
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = 'https://github.com/gpshead'
closed_at = <Date 2020-12-14.19:22:54.220>
created_at = <Date 2020-09-28.18:19:17.863>
labels = ['type-feature', 'library', '3.10']
title = 'Check against misspellings of assert etc. in mock'
updated_at = <Date 2020-12-14.19:22:54.219>
user = 'https://github.com/vabr-g'bugs.python.org fields:
activity = <Date 2020-12-14.19:22:54.219>
actor = 'vabr2'
assignee = 'gregory.p.smith'
closed = True
closed_date = <Date 2020-12-14.19:22:54.220>
closer = 'vabr2'
components = ['Library (Lib)']
creation = <Date 2020-09-28.18:19:17.863>
creator = 'vabr2'
dependencies = []
files = []
hgrepos = []
issue_num = 41877
keywords = ['patch']
message_count = 17.0
messages = ['377611', '377612', '377620', '377625', '377841', '378255', '378299', '378302', '378569', '380412', '380419', '380422', '382820', '382838', '382839', '383000', '383001']
nosy_count = 12.0
nosy_names = ['rhettinger', 'terry.reedy', 'gregory.p.smith', 'rbcollins', 'cjw296', 'michael.foord', 'veky', 'lisroach', 'mariocj89', 'miss-islington', 'xtreak', 'vabr2']
pr_nums = ['23165', '23729', '23737']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue41877'
versions = ['Python 3.10']
Activity
vabr-g commentedon Sep 28, 2020
Recently we cleaned up the following typos in mocks in unittests of our codebase:
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: testing-cabal/mock@7c530f0, 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.
vedgar commentedon Sep 28, 2020
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.
rhettinger commentedon Sep 28, 2020
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.
vedgar commentedon Sep 28, 2020
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.
terryjreedy commentedon Oct 2, 2020
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.
vedgar commentedon Oct 8, 2020
Sorry, but
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.
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.
tirkarthi commentedon Oct 9, 2020
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
vedgar commentedon Oct 9, 2020
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. ;-)
vabr-g commentedon Oct 13, 2020
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
vabr-g commentedon Nov 5, 2020
A pull request implementing the first part of this issue (Wrong prefixes of mock asserts: asert/aseert/assrt -> assert) is at #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.
gpshead commentedon Nov 5, 2020
New changeset 4662fa9 by vabr-g in branch 'master':
bpo-41877 Check for asert, aseert, assrt in mocks (GH-23165)
4662fa9
gpshead commentedon Nov 5, 2020
Thanks for the patch! I'm leaving this open as dealing with the other aspect:
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. bpo-24651 looks like the better place to take up future design ideas.
vabr-g commentedon Dec 10, 2020
#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.
miss-islington commentedon Dec 10, 2020
New changeset 9fc5713 by vabr-g in branch 'master':
bpo-41877: Improve docs for assert misspellings check in mock (GH-23729)
9fc5713
vabr-g commentedon Dec 10, 2020
#23737 has the initial draft of the check against misspelled arguments.
gpshead commentedon Dec 14, 2020
New changeset fdb9efc by vabr-g in branch 'master':
bpo-41877: Check for misspelled speccing arguments (GH-23737)
fdb9efc
vabr-g commentedon Dec 14, 2020
The three PRs which landed for this bug implement what I planned in the original comment, so I'm closing this bug.