Cannot easily mock out sleep function in order to simulate time in tests

[asqui]

Given the level of flexibility of this library, and a slightly elaborate context which I am using it in, it is desirable to be able to write an integration test where the concepts of 'time' (i.e. time.monotonic) and 'sleep' (i.e. tenacity.nap.sleep) are mocked out so that we can simulate in full fidelity how a retry scenario plays out, without having to actually sleep.

Using the testfixtures library and mock.patch I'm mocking out 'time' with an object that always returns the same 'virtual' time, and 'sleep' with a function that advances this 'virtual' time.

This approach works pretty well, save for one complication...

Since the signature of BaseRetrying.__init__() defaults the sleep argument to tenacity.nap.sleep this default is bound at import time, making it hard to mock out the sleep function after the fact.

Options I can think of:

1. Reach in to the method signature and mock out the default value - seems a bit messy.
2. Pass in a sleep function in the code under test, then mock this out in the test - I don't like this because it involves modifying production code simply for the sake of the test, and also makes this mocking strategy less portable if I want to use it in other tests (because now I need to mock out sleep at my callsite, rather than inside tenacity).
3. Make a change to the signature of BaseRetrying.__init__() to default sleep to None, and then default this to nap.sleep inside the method: self.sleep = sleep or nap.sleep.

Option 3 seems the neatest, because now I can just mock out tenacity.nap.sleep which is the most intuitive approach, but it does involve a change to the library which is why I wanted to solicit input first before preparing a PR.

Thoughts?

[dtantsur]

This issue blocks transition of ironic from retrying to tenacity.

I'd vote for option 3. Since None is not a valid sleep value, it's fine to change the default to it IMO.

[asqui]

Option 3 is unfortunately not sufficient for functions decorated with @retry.

When using the @retry decorator, this will construct a Retrying instance immediately, which binds Retrying.sleep for that instance to nap.sleep at import-time, and precludes any further mocking after that point.

I think we can solve both problems with a change at a lower level. Replacing the existing nap.sleep = time.sleep variable with a function definition def sleep(seconds): time.sleep(seconds) will allow patching of nap.time.sleep in tests.

This is essentially the same as the workaround employed here (as discussed in #25):
https://github.com/openstack/cinder/blob/307fccea4cc9c6496334b0fe137206cb48499bd5/cinder/utils.py#L56

[jd]

Why can't you mock myfunction.retry.sleep then?

[asqui]

It's true, when using the decorator approach you can then mock myfunction.retry.sleep.

I figured if we're going to make code changes to enable mocking in one of the two cases it would be nice to do it in a way that accommodates both with the same approach.

With the change in PR #236 you can use the same mocking approach (replacing tenacity.nap.time.sleep) regardless of exactly how the code-under-test is using Tenacity.

I think this makes writing tests:

• simpler (you only one single approach that will always work with Tenacity)
• more intuitive (you don't need to understand the fact that the decorator exposes a retrying member on the decorated function; you can just find nap.sleep quite quickly -- at least this is where I first ended up when trying to work out how to write a test for code using Tenacity.)
• more stable (tests don't need to change mocking approach if code-under-test is refactored to change from/to using the decorator)

[dtantsur]

Why can't you mock myfunction.retry.sleep then?

Sometimes myfunction is hidden inside of something (e.g. another function, which is not uncommon for ironic).

[asqui]

Just picking up this thread again -- are we happy with this approach?
If so, I can fix up PR #236 (satisfy pylint, and switch from mock.patch to pytest monkeypatch) and rebase it ready to merge.

[jd]

TBH I'm not really excited about that change. You could also fix that yourself easily by subclassing the Retrying class with your own default sleep in __init__ and use that through your code.

def mysleep(s):
    return time.sleep(s)

class MyRetrying(tenacity.Retrying):
    def __init__(self, *args, sleep=mysleep, **kwargs):
         super().MyRetrying(*args, sleep=sleep, **kwargs)

how_to_retry = MyRetrying(…)


# Later
@how_to_retry.wraps
def myfunc_retrying():
    …

(untested code but you get the idea)

[dtantsur]

@jd you mean that literally everyone who is serious about their project (and thus writes unit tests) has to subclass Retrying? Forking tenacity would be less work overall..

[jd]

I don't think insulting the few thousands of tenacity users by implying they are not serious about their project as they can't write unit tests is a good approach.

Like many, I use tenacity and I write unit tests. I don't test tenacity itself and it's retrying behavior since that is tested by tenacity itself (I hope so at least). If this is what you want to do, I provided a pretty simple solution with 5 lines of code.

If you think there are serious issues about testing code with tenacity, I'm happy to hear them.

But if from now your plan is to bully your way in, this is not going to work.

[asqui]

@jd You said you're "not really excited about the change". It's definitely true that you can work around this already in various ways, but I just think it would be better if you didn't need to.

In my particular scenario I'm trying to standardise a myriad of retry approaches in a large codebase to all use tenacity, and convince other developers that this is worthwhile. The need to apply workarounds like this in order to be able to test is a barrier to adoption of the library by other developers.

Forking the library and applying the patch I'm suggesting is also an approach, but again carries more overhead than having this in the library as standard, and available to all users of tenacity without each needing to visit this hurdle and come up with a mitigation approach. (e.g. https://github.com/openstack/cinder/blob/307fccea4cc9c6496334b0fe137206cb48499bd5/cinder/utils.py#L56 as discussed earlier in the comment trail)

So to summarise I think there are definite advantages to applying this change to the library, and I can see no downsides.

What are the disadvantages of this change which are making you "not really excited" about making it?

[jd]

What are the disadvantages of this change which are making you "not really excited" about making it?

That's an excellent question to ask. I don't think there is much. I'm just trying to be really suspicious about changes that are not useful and might be a pain to maintain in the long run. That's part of my job here. 😄

[asqui]

Hmmm, so just to be clear, the change being talked about here is to change the definition of nap.sleep from sleep = time.sleep to def sleep(seconds): time.sleep(seconds) in order to facilitate easier mocking, in a single place, that works for a variety of usage scenarios.

It's a pretty minor change, with a major benefit for end-to-end testability of code using the library.

Is there a specific aspect of this change that you think might be a pain to maintain in the long run? If not, then I'm failing to see the reasons behind not wanting to merge such a change.

[jd]

No, I think it should be fine.

[asqui]

Ok great! I'll tidy up the PR #236 and get it ready to merge.

[phillipuniverse]

It was a little bit tricky for me to navigate this thread to find exactly what I needed to mock. here's the easy answer:

class SomeTest(TestCase):

    @mock.patch("tenacity.nap.time.sleep", MagicMock())
    def test_something_with_retry(self):
        # invoke function that uses tenacity

[carlcsaposs-canonical]

Patching tenacity.nap.time.sleep didn't stop tenacity from retrying for me.

Here's what worked for me (pytest):

@pytest.fixture(autouse=True)
def foo(monkeypatch):
    monkeypatch.setattr(
        "tenacity.BaseRetrying.retry", tenacity.retry_if_result(lambda *args, **kwargs: False)
    )

Update: here's what worked for me on tenacity 8.2.2 (pytest):

import pytest


@pytest.fixture(autouse=True)
def disable_tenacity_retry(monkeypatch):
    for retry_class in (
        "retry_if_exception",
        "retry_if_exception_type",
        "retry_if_not_exception_type",
        "retry_unless_exception_type",
        "retry_if_exception_cause_type",
        "retry_if_result",
        "retry_if_not_result",
        "retry_if_exception_message",
        "retry_if_not_exception_message",
        "retry_any",
        "retry_all",
        "retry_always",
        "retry_never",
    ):
        monkeypatch.setattr(f"tenacity.{retry_class}.__call__", lambda *args, **kwargs: False)

[asqui]

Do you have a reproducer for the patch that didn't work for you?

[carlcsaposs-canonical]

Oops, I was testing on tenacity==6.3.0 since I was trying to see if it only worked on an older version.

Using tenacity==8.2.2, the suggested patch tenacity.nap.time.sleep still doesn't work for me, but the workaround I found also doesn't work.

Here are steps to reproduce (Ubuntu 22.04.2 LTS):

$ python3 -m venv venv
$ source venv/bin/activate
$ pip install tenacity
Collecting tenacity
  Using cached tenacity-8.2.2-py3-none-any.whl (24 kB)
Installing collected packages: tenacity
Successfully installed tenacity-8.2.2
$ python
Python 3.10.6 (main, May 29 2023, 11:10:38) [GCC 11.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.

>>> import tenacity
>>> @tenacity.retry(stop=tenacity.stop_after_delay(30), wait=tenacity.wait_fixed(5))
... def foo():
...     raise Exception
... 
>>> import unittest
>>> import unittest.mock
>>> class TestFoo(unittest.TestCase):
...     @unittest.mock.patch("tenacity.nap.time.sleep", unittest.mock.MagicMock())
...     def test_foo(self):
...         foo()
... 
>>> unittest.main()
E
======================================================================
ERROR: test_foo (__main__.TestFoo)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/user/venv/lib/python3.10/site-packages/tenacity/__init__.py", line 382, in __call__
    result = fn(*args, **kwargs)
  File "<stdin>", line 3, in foo
Exception

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/usr/lib/python3.10/unittest/mock.py", line 1369, in patched
    return func(*newargs, **newkeywargs)
  File "<stdin>", line 4, in test_foo
  File "/home/user/venv/lib/python3.10/site-packages/tenacity/__init__.py", line 289, in wrapped_f
    return self(f, *args, **kw)
  File "/home/user/venv/lib/python3.10/site-packages/tenacity/__init__.py", line 379, in __call__
    do = self.iter(retry_state=retry_state)
  File "/home/user/venv/lib/python3.10/site-packages/tenacity/__init__.py", line 326, in iter
    raise retry_exc from fut.exception()
tenacity.RetryError: RetryError[<Future at 0x7f3298410040 state=finished raised Exception>]

----------------------------------------------------------------------
Ran 1 test in 30.001s

FAILED (errors=1)

[jmehnle]

If one is writing sync code, or async code using asyncio, then patching time.sleep and asyncio.sleep works well enough, but for Trio-based code unfortunately patching trio.sleep fatally interferes with pytest-trio's (and probably trio's) internals. You'll get obscure LookupError: <ContextVar name='pytest-trio canary' at 0x107a1b650> exceptions. So that's not an option for Trio-based code.