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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:59:36 | admin | set | github: 86043 |
2020-12-14 19:22:54 | vabr2 | set | status: open -> closed resolution: fixed messages:
+ msg383001
stage: patch review -> resolved |
2020-12-14 18:30:16 | gregory.p.smith | set | messages:
+ msg383000 |
2020-12-10 19:50:39 | vabr2 | set | messages:
+ msg382839 |
2020-12-10 19:47:44 | vabr2 | set | pull_requests:
+ pull_request22596 |
2020-12-10 18:35:40 | miss-islington | set | nosy:
+ miss-islington messages:
+ msg382838
|
2020-12-10 09:45:01 | vabr2 | set | messages:
+ msg382820 |
2020-12-10 09:21:43 | vabr2 | set | pull_requests:
+ pull_request22589 |
2020-11-05 17:49:31 | gregory.p.smith | set | messages:
+ msg380422 |
2020-11-05 17:04:46 | gregory.p.smith | set | messages:
+ msg380419 |
2020-11-05 16:57:37 | gregory.p.smith | set | assignee: gregory.p.smith |
2020-11-05 16:52:02 | gregory.p.smith | set | nosy:
+ gregory.p.smith
|
2020-11-05 15:19:18 | vabr2 | set | messages:
+ msg380412 |
2020-11-05 15:14:34 | vabr2 | set | keywords:
+ patch stage: patch review pull_requests:
+ pull_request22077 |
2020-10-13 17:10:31 | vabr2 | set | messages:
+ msg378569 |
2020-10-09 04:11:48 | veky | set | messages:
+ msg378302 |
2020-10-09 03:12:56 | xtreak | set | nosy:
+ lisroach, cjw296, xtreak, rbcollins, mariocj89 messages:
+ msg378299
|
2020-10-08 16:19:24 | veky | set | messages:
+ msg378255 |
2020-10-02 23:43:15 | terry.reedy | set | nosy:
+ terry.reedy messages:
+ msg377841
|
2020-09-28 20:40:03 | veky | set | messages:
+ msg377625 |
2020-09-28 20:12:58 | rhettinger | set | nosy:
+ rhettinger, michael.foord messages:
+ msg377620
|
2020-09-28 18:41:04 | veky | set | nosy:
+ veky messages:
+ msg377612
|
2020-09-28 18:19:17 | vabr2 | create | |