[issue43478] Disallow Mock spec arguments from being Mocks
Change by Matthew Suozzo : -- pull_requests: +29275 pull_request: https://github.com/python/cpython/pull/31090 ___ Python tracker <https://bugs.python.org/issue43478> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue43478] Disallow Mock spec arguments from being Mocks
New submission from Matthew Suozzo : An unfortunately common pattern over large codebases of Python tests is for spec'd Mock instances to be provided with Mock objects as their specs. This gives the false sense that a spec constraint is being applied when, in fact, nothing will be disallowed. The two most frequently observed occurrences of this anti-pattern are as follows: * Re-patching an existing autospec. def setUp(self): mock.patch.object(mod, 'Klass', autospec=True).start() self.addCleanup(mock.patch.stopall) @mock.patch.object(mod, 'Klass', autospec=True) # :( def testFoo(self, mock_klass): # mod.Klass has no effective spec. * Deriving an autospec Mock from an already-mocked object def setUp(self): mock.patch.object(mod, 'Klass').start() ... mock_klass = mock.create_autospec(mod.Klass) # :( # mock_klass has no effective spec This is fairly easy to detect using _is_instance_mock at patch time however it can break existing tests. I have a patch ready and it seems like this error case is not frequent enough that it would be disruptive to address. Another option would be add it as a warning for a version e.g. 3.10 and then potentially make it a breaking change in 3.11. However considering this is a test-only change with a fairly clear path to fix it, that might be overly cautious. -- components: Tests messages: 388532 nosy: msuozzo priority: normal severity: normal status: open title: Disallow Mock spec arguments from being Mocks type: enhancement versions: Python 3.10 ___ Python tracker <https://bugs.python.org/issue43478> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue43478] Disallow Mock spec arguments from being Mocks
Matthew Suozzo added the comment: I've fixed a bunch of these in our internal repo so I'd be happy to add that to a patch implementing raising exceptions for these cases. -- ___ Python tracker <https://bugs.python.org/issue43478> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue43478] Disallow Mock spec arguments from being Mocks
Matthew Suozzo added the comment: A few more things: Assertions on Mock-autospec'ed Mocks will silently pass since e.g. assert_called_once_with will now be mocked out. This may justify a more stringent stance on the pattern since it risks hiding real test failures. One complicating factor with the implementation is that autospec'd objects may have children that are already Mocked out. Failing eagerly, however, would break cases where the child is never used (and consequently risk causing more friction than may be necessary) but failing only upon access of that child makes it trickier for the user to trace back to where the parent was mocked. -- ___ Python tracker <https://bugs.python.org/issue43478> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue43478] Disallow Mock spec arguments from being Mocks
Matthew Suozzo added the comment: And to give some context for the above autospec child bit, this is the relevant code that determines the spec to use for each child: https://github.com/python/cpython/blob/master/Lib/unittest/mock.py#L2671-L2696 -- ___ Python tracker <https://bugs.python.org/issue43478> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue43798] Add position metadata to alias AST type
New submission from Matthew Suozzo : Given the increasing use of long `from typing import foo, bar, ...` import sequences, it's becoming more desirable to address individual components of the import node. Unfortunately, the ast.alias node doesn't contain source location metadata (e.g. lineno, col_offset). This metadata would be comparatively easy to add, backwards compatible, and would enable more precise diagnostics e.g. lints. -- components: Interpreter Core messages: 390663 nosy: matthew.suozzo priority: normal severity: normal status: open title: Add position metadata to alias AST type type: enhancement versions: Python 3.10, Python 3.6, Python 3.7, Python 3.8, Python 3.9 ___ Python tracker <https://bugs.python.org/issue43798> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue43798] Add position metadata to alias AST type
Change by Matthew Suozzo : -- keywords: +patch pull_requests: +24059 stage: -> patch review pull_request: https://github.com/python/cpython/pull/25324 ___ Python tracker <https://bugs.python.org/issue43798> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue43798] Add position metadata to alias AST type
Matthew Suozzo added the comment: Ah and one other question: Is this normally the sort of thing that would get backported? It should be very straightforward to do so, at least for 3.9 given the support for the new parser. -- versions: -Python 3.6, Python 3.7, Python 3.8 ___ Python tracker <https://bugs.python.org/issue43798> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue43478] Disallow Mock spec arguments from being Mocks
Change by Matthew Suozzo : -- keywords: +patch nosy: +matthew.suozzo nosy_count: 7.0 -> 8.0 pull_requests: +24061 stage: -> patch review pull_request: https://github.com/python/cpython/pull/25326 ___ Python tracker <https://bugs.python.org/issue43478> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue37251] Mocking a MagicMock with a function spec results in an AsyncMock
Matthew Suozzo added the comment: I don't think this was actually fixed for the create_autospec case. create_autospec still uses the only is_async_func check to enable use of AsyncMock and that still does a __code__ check. There was a test submitted to check this case but the test itself was bugged and discovered in the process of implementing https://bugs.python.org/issue43478. -- nosy: +matthew.suozzo ___ Python tracker <https://bugs.python.org/issue37251> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue37251] Mocking a MagicMock with a function spec results in an AsyncMock
Change by Matthew Suozzo : -- pull_requests: +24081 stage: needs patch -> patch review pull_request: https://github.com/python/cpython/pull/25347 ___ Python tracker <https://bugs.python.org/issue37251> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue42551] Generator `yield`s counted as primitive calls by cProfile
New submission from Matthew Suozzo :
# Issue
When profiling a generator function, the initial call and all subsequent yields
are aggregated into the same "ncalls" metric by cProfile.
## Example
>>> cProfile.run("""
... def foo():
... yield 1
... yield 2
... assert tuple(foo()) == (1, 2)
... """)
ncalls tottime percall cumtime percall filename:lineno(function)
...
30.0000.0000.0000.000 :2(foo)
...
This was unexpected behavior since it *looks* like a single call from the code.
This also complicates basic analysis about the frequency of a codepath's
execution where a generator might yield a variable number of times depending on
the input.
The profile interface can and does differentiate between call types: Normal
calls and "primitive" calls, i.e. those that are not induced via recursion, are
displayed as "all_calls/primitive_calls" e.g. "3/1" for a single initial calls
with 2 recursed calls (comparable to the example above).
This seems like an appropriate abstraction to apply in the generator case: Each
yield is better modeled as an 'interior' call to the generator, not as a call
on its own.
# Possible fix
I have two ideas that seem like they might address the problem:
* Add a new PyTrace_YIELD constant (and handle it in [3])
* Track some state from the `frame->f_gen` in the ProfilerEntry (injected in
[3], used in [4]) to determine whether this is the first or a subsequent call.
I've not been poking around for long so I don't have an intuition about which
would be less invasive (nor really whether either would work in practice).
As background, this seems to be the call chain from trace invocation to
callcount increment:
[0]:
https://github.com/python/cpython/blob/4e7a69bdb63a104587759d7784124492dcdd496e/Python/ceval.c#L4106-L4107
[1]:
https://github.com/python/cpython/blob/4e7a69bdb63a104587759d7784124492dcdd496e/Python/ceval.c#L4937
[2]:
https://github.com/python/cpython/blob/4e7a69bdb63a104587759d7784124492dcdd496e/Python/ceval.c#L4961
[3]:
https://github.com/python/cpython/blob/4e7a69bdb63a104587759d7784124492dcdd496e/Modules/_lsprof.c#L419
[4]:
https://github.com/python/cpython/blob/4e7a69bdb63a104587759d7784124492dcdd496e/Modules/_lsprof.c#L389
[5]:
https://github.com/python/cpython/blob/4e7a69bdb63a104587759d7784124492dcdd496e/Modules/_lsprof.c#L311-L316
--
components: C API
messages: 382373
nosy: matthew.suozzo
priority: normal
severity: normal
status: open
title: Generator `yield`s counted as primitive calls by cProfile
type: behavior
versions: Python 3.10, Python 3.6, Python 3.7, Python 3.8, Python 3.9
___
Python tracker
<https://bugs.python.org/issue42551>
___
___
Python-bugs-list mailing list
Unsubscribe:
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue42551] Generator `yield`s counted as primitive calls by cProfile
Matthew Suozzo added the comment: One of the problems with my proposed solution that I glossed over was how and where to count the primitive call. If the primitive call is only registered on RETURN (i.e. after all yields), a generator that is incompletely exhausted would have 0 primitive calls. However if the primitive call is registered immediately when the call context is entered, the recorded call would be instantaneous (i.e. 0s duration) which could throw off the percall statistics. Even with the incomplete generator case, I think registering the primitive call on RETURN is the best of the two options. Although seeing 2/0 for the call count when RETURN wasn't reached might seem odd, it lines up with a conceptual model of a coroutine: An iterator represents a function returning a sequence; Exhausting that iterator corresponds to the function call completing; Incompletely exhausting corresponds to an abbreviated/incomplete function call. -- ___ Python tracker <https://bugs.python.org/issue42551> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue42353] Proposal: re.prefixmatch method (alias for re.match)
Matthew Suozzo added the comment: > It just won't work unless you add explicit ".*" or ".*?" at the start of the > pattern But think of when regexes are used for validating input. Getting it to "just work" may be over-permissive validation that only actually checks the beginning of the input. They're one missed test case away from a crash or, worse, a security issue. This proposed name change would help make the function behavior obvious at the callsite. In the validator example, calling "prefixmatch" would stand out as wrong to even the most oblivious, documentation-averse user. > My point is that re.match is a common bug when people really want re.search. While I think better distinguishing the interfaces is a nice-to-have for usability, I think it has more absolute benefit to correctness. Again, confusion around the semantics of "match" were the motivation for adding "fullmatch" in the first place but that change only went so far to address the problem: It's still too easy to misuse the existing "match" interface and it's not realistic to remove it from the language. A new name would eliminate this class of error at a very low cost. -- nosy: +matthew.suozzo ___ Python tracker <https://bugs.python.org/issue42353> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
