Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check against misspellings of assert etc. in mock #86043

Closed
vabr-g mannequin opened this issue Sep 28, 2020 · 17 comments
Closed

Check against misspellings of assert etc. in mock #86043

vabr-g mannequin opened this issue Sep 28, 2020 · 17 comments
Assignees
Labels
3.10 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@vabr-g
Copy link
Mannequin

vabr-g mannequin commented Sep 28, 2020

BPO 41877
Nosy @rhettinger, @terryjreedy, @gpshead, @rbtcollins, @cjw296, @voidspace, @vedgar, @lisroach, @mariocj89, @miss-islington, @tirkarthi, @vabr-g
PRs
  • bpo-41877 Check for asert, aseert, assrt in mocks #23165
  • bpo-41877: Improve docs for assert misspellings check in mock #23729
  • bpo-41877: Check for misspelled speccing arguments #23737
  • 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']

    @vabr-g
    Copy link
    Mannequin Author

    vabr-g mannequin commented Sep 28, 2020

    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: 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.

    @vabr-g vabr-g mannequin added 3.10 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Sep 28, 2020
    @vedgar
    Copy link
    Mannequin

    vedgar mannequin commented 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
    Copy link
    Contributor

    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
    Copy link
    Mannequin

    vedgar mannequin commented 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
    Copy link
    Member

    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
    Copy link
    Mannequin

    vedgar mannequin commented Oct 8, 2020

    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.

    @tirkarthi
    Copy link
    Member

    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
    Copy link
    Mannequin

    vedgar mannequin commented 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
    Copy link
    Mannequin Author

    vabr-g mannequin commented 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
    Copy link
    Mannequin Author

    vabr-g mannequin commented 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 gpshead self-assigned this Nov 5, 2020
    @gpshead
    Copy link
    Member

    gpshead commented 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
    Copy link
    Member

    gpshead commented Nov 5, 2020

    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. bpo-24651 looks like the better place to take up future design ideas.

    @vabr-g
    Copy link
    Mannequin Author

    vabr-g mannequin commented 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
    Copy link
    Contributor

    New changeset 9fc5713 by vabr-g in branch 'master':
    bpo-41877: Improve docs for assert misspellings check in mock (GH-23729)
    9fc5713

    @vabr-g
    Copy link
    Mannequin Author

    vabr-g mannequin commented Dec 10, 2020

    #23737 has the initial draft of the check against misspelled arguments.

    @gpshead
    Copy link
    Member

    gpshead commented Dec 14, 2020

    New changeset fdb9efc by vabr-g in branch 'master':
    bpo-41877: Check for misspelled speccing arguments (GH-23737)
    fdb9efc

    @vabr-g
    Copy link
    Mannequin Author

    vabr-g mannequin commented 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.

    @vabr-g vabr-g mannequin closed this as completed Dec 14, 2020
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.10 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants