gh-61215: New mock to wait for multi-threaded events to happen #16094
Conversation
Spoke offline with @voidspace and suggested to keep this PR for the multithreading use case only as it is not trivial to get this working for multiprocess. It was also suggested to rename the mock to make it clear it is about multithreading and keep the @tirkarthi @voidspace @lisroach if this looks good I'll get started with the docs. |
|
Gentle ping @tirkarthi @voidspace @lisroach! Let me know if the general idea/naming looks good and I'll get to work on the documentation. |
|
Sorry, got caught up with new job. I will try to get to the PR after two weeks. |
|
I am fine with the PR. The primary concern in my implementation was to have mock_timeout fit in the API along with either to have a default or wait forever and hope we have reached some consensus on it as per the PR. It would be helpful if one of the core dev accepts this idea for merge. Also does it needs some more discussion from other channels like discourse, mailing list etc since I don't have experience in proposing new API. Thanks Mario for picking up the PR. |
mariocj89
force-pushed
the
pu/eventmock
branch
2 times, most recently
from
317eb79
to
54c4a32
Compare
What does it mean to have timeout namespaced? Have a parameter called "mock_timeout"? |
|
Yeah, just prepending it with “mock_”, mothibg fancy. having just ‘timeout’ can easily collide with existing functions that take a timeout parameter. |
|
Gentle ping @lisroach @voidspace, just wished yesterday that I had this mock in a codebase :). |
|
@mariocj89 - might be worth emailing or otherwise contacting @lisroach and @voidspace offline, I know GitHub notifications can be noisy for some people and a ping on here may not get to them. |
|
I don't think adding an additional class is justified enough. It does not really make Mock anymore "threaded" than it already is, just adds a convenience method on top of the threading facility. @cjw296 was already confused with the name above. Recall that unittest is somewhat hard to navigate already. I'm also not convinced that implementation is convenient enough: e.g. the implementation does not allow to wait for arbitrary conditions. One way or another let's get it through. |
|
Perhaps with the release of 3.9 it's time to consider this change for 3.10? |
mariocj89
force-pushed
the
pu/eventmock
branch
from
b0383ae
to
681abde
Compare
|
I love the functionality of this, obviously it is solving some real-world problems. I think @Kentzo makes a good point, does it add enough functionality to justify creating a new class? Adding the capability to Mock on the backend is a nicer experience for end-users, who don't have to learn about the new class to get the same functionality. @Kentzo have the backwards compatibility issues with #20759 been resolved? I agree with Pablo that backwards compatibility is a must, even if we have to sacrifice some features. ThreadingMock fits with the Mock code-style and behavior, and the addition of a new class would not cause any breaking changes. |
|
@lisroach Yes, the issue that was pointed out (I changed the |
|
Just one plea here: many people are stuck on older versions of Python, and so only get to use new stuff via the backport, if at all possible, please have a thought to writing code in such a way that it'll run on Python 3.6. We already had to skip the "position-only args" commit in the backport, and that was pretty painful. |
I'm absolutely fine with that. A year ago there was a discussion where it seems that you and @voidspace where leaning more towards having specific mocks for different functionality, where it was discussed to have this implemented as such. But if things have changed I'm 100% fine with that :). Should we then close this PR go with @Kentzo proposal? |
|
|
|
|
OK! Let's see the results: 1. wasm32-emscripten node:
2. wasm32-emscripten node (pthreads) 3.xFails with: 3. wasm32-wasi 3.x
|
|
I'm worried 2 might come from flaky tests, will review them all to make sure we can make them more resilient (as you know, with timeouts we are exposed to the host). Need also to investigate what is going on with the error to start a thread. |
|
@mariocj89 Since the failures are in wasm related builds where threads might not be supported maybe you need to wrap the test file to skip when threads are not working. I see cpython/Lib/test/support/threading_helper.py Lines 216 to 247 in 5890621 |
|
Indeed, just spoke with @pablogsal who told me the same :P. Will send a PR before EOD. |
|
It seems below tests are totally skipped and maybe a related one needs to be added to the mock related tests as well. Since the test will be backported to mock backport project maybe the helper function needs to be inlined in case mock backport runs on wasm builds to not rely on |
|
PR to add the marker: #106366 Will followup trying to harden the rests of the tests. |
* main: (167 commits) pythongh-91053: make func watcher tests resilient to other func watchers (python#106286) pythongh-104050: Add more type hints to Argument Clinic DSLParser() (python#106354) pythongh-106359: Fix corner case bugs in Argument Clinic converter parser (python#106361) pythongh-104146: Remove unused attr 'parameter_indent' from clinic.DLParser (python#106358) pythongh-106320: Remove private _PyErr C API functions (python#106356) pythongh-104050: Annotate Argument Clinic DSLParser attributes (python#106357) pythongh-106320: Create pycore_modsupport.h header file (python#106355) pythongh-106320: Move _PyUnicodeWriter to the internal C API (python#106342) pythongh-61215: New mock to wait for multi-threaded events to happen (python#16094) Document PYTHONSAFEPATH along side -P (python#106122) Replace the esoteric term 'datum' when describing dict comprehensions (python#106119) pythongh-104050: Add more type hints to Argument Clinic DSLParser() (python#106343) pythongh-106320: _testcapi avoids private _PyUnicode_EqualToASCIIString() (python#106341) pythongh-106320: Add pycore_complexobject.h header file (python#106339) pythongh-106078: Move DecimalException to _decimal state (python#106301) pythongh-106320: Use _PyInterpreterState_GET() (python#106336) pythongh-106320: Remove private _PyInterpreterState functions (python#106335) pythongh-104922: Doc: add note about PY_SSIZE_T_CLEAN (python#106314) pythongh-106217: Truncate the issue body size of `new-bugs-announce-notifier` (python#106329) pythongh-104922: remove PY_SSIZE_T_CLEAN (python#106315) ...
|
@cjw296 even though the API looks neat to me, I showed it to a colleague who does not have a lot of mocking experiencet and suggested to use Whilst I like how What do you think? 1 or 2? I'm fine with one, but if we want to change it, it is possibly now or never. |
|
Maybe it should be |
|
Sounds good. PR ready: #106414 |
|
There seems to be another problem with this -- I just got a test flake, apparently due to the use of short fixed timeouts in the test added here. See https://github.com/python/cpython/actions/runs/5559403783/jobs/10155484726. |
|
Not sure what a "test flake" means in this context? Looking at the test again, I'm unclear it's sensible: cpython/Lib/test/test_unittest/testmock/testthreadingmock.py Lines 166 to 180 in 0db85ee Why the delays of 0.2, 0.5 and 0.6? Lines 178-180 suppose that because FWIW, this is the first time I'm aware of this test failing, and it's run nightly on a bunch of different python versions in the backport CI: https://app.circleci.com/pipelines/github/testing-cabal/mock?branch=master @mariocj89 - what are your thoughts on this? What can we do to make this test reliable. A bonus would be to make them faster to run, these threading tests are noticeably much slower than the other mock tests... |
|
It is a hard balance to keep the tests fast but reliable. I will try changing this to have short retrying timeouts to make that happen. Will send a PR on Monday if there is not rush as I don't have access to a PC this weekend |
A flaky test is a test that usually passes but sometimes fails. Tests that rely on timeouts are often flaky (but there are other causes). Any large complex system has flaky tests. In this case, the test failed in CI on macOS only; Windows and Ubuntu were fine. Re-running the test on my machine locally it passes. That makes it flaky in my book. Note that, as defense against flaky tests, our CI machinery re-runs a failed test once and if it passes the second time, it is accepted. In this case we were apparently unlucky with the timeouts twice in a row. Thanks for having a second look at these tests! |
|
#106822 should prevent any future failure. |
|
Is this change going to be included in 3.12? |
|
I haven’t checked the source but I see no evidence of a backport to the 3.12 branch, so presumably not. |
|
@Kentzo - it doesn't look like
|
|
Is it possible to use ThreadingMock with auto spec feature? i.e. let patch or create_autospec create a ThreadingMock instance instead of a MagicMock instance. |
|
Per the mocks docs: You can specify an alternative class of That should work in theory, give it a try! |
This is not possible with autospec. The #1 reason for using patch :-(.
|
This PR implements the change proposed in bpo-17013 and takes over #12818 (agreed offline with @tirkarthi).
https://bugs.python.org/issue17013
CC: @tirkarthi @voidspace @lisroach