Mocker.copy fails to copy the adapter

[jamielennox]

I am trying to use django-rest-framework's RequestsClient with requests_mock. This has taken me down quite the rat hole. After reading a ton of the requests_mock code, I've concluded that the best way for me to do this is to essentially exclude the requests being made to http://testserver.

To do this, I create my own mocker instance before decorating my classes:

mocker = requests_mocker.Mocker() mocker.register_uri(requests_mock.ANY, re.compile(r"^https?://testserver/"), real_http=True)

@mocker class MyAPITestCase(APITestCase):

    client_class = RequestsClient

    ...

However, this does not work. After digging around, I discovered this is due to Mocker.decorate_class making a copy of itself, but failing to include the adaptor on the copy.

A quick fix for my code is to override copy:

class Mocker(requests_mock.Mocker):

    def copy(self):         m = Mocker(             kw=self._kw,             real_http=self._real_http         )         m._adapter = self._adapter         return m

Everything works as expected. I'd love to submit a patch, but I don't have a ton of time to work through all the contribution guidelines for the OpenStack project.

Launchpad Details: #LP1639352 brosner - 2016-11-04 21:13:52 +0000

[jamielennox]

Yea, whilst it made sense at the time i'm regretting putting it under the openstack banner. For what it's worth it uses the gerrit and testing systems, but doesn't fall under any of the contributor agreements or anything.

So I'm happy to fix the case, the concern i have though is that the logic for things like call_count exist on the adapter so I think you would end up not resetting this information per test. I think it would also mean that responses registered in individual tests would be assigned to the global object and available across tests.

This might be something in general that needs to be fixed in using the mocker for a class. Can you check with your solution if call count etc is maintained across tests? If not I will but I won't have much time over the next week or so. What we might be able to do there is do m._adapter = self._adapter.copy() and implement copy on the adapter and all the subsequent matchers.

The pattern I have found myself liking most is http://requests-mock.readthedocs.io/en/latest/fixture.html which creates a new mock for every test, but is defined once per class. This requires some form of testtools base class though, and i don't know what django-rest-framework does there. In general though there is probably something similar you can do to create the mock per-test in one place. I can have a look there at providing some better integration with django-rest-framework, but if you have any ideas/existing test code i'd love to see it.

Launchpad Details: #LPC Jamie Lennox - 2016-11-05 02:11:23 +0000

[rfportilla]

Hi, I just posted a new issue before I saw this. This is a problem for me. I would be more than happy to make this change. I also addressed your question below.

The question you asked about count is not an issue. When used as a decorator, the class gets modified before it is instantiated. That means every function is modified with its own instance of Mocker (which has its own instance of the Adapter) and nothing is shared between tests. The code below sub-classes the Mocker and replaces the copy function with deepcopy. The tests assert that count_once is True and that call_count is 1. Lastly, it prints a message when copy is called (when it applies decorator) and when test is called. It demonstrates that all the decorators are created, with their own instances, before any tests are called.

The only fix that is needed is to replace Mocker.copy code with "return deepcopy(self)"

from unittest import TestCase
from copy import deepcopy
import requests_mock
import requests

url1 = 'http://127.0.0.1/res1/'
url2 = 'http://127.0.0.1/res2/'

class MyMocker(requests_mock.Mocker):
  def __init__(self, *args, **kwargs):
    super().__init__(*args, **kwargs)

    self.register_uri('GET', url1, text='res1 return', status_code=200)
    self.register_uri('GET', url2, text='res1 return', status_code=200)

  def copy(self):
    self_copy = deepcopy(self)
    print('generate func decorator: {}'.format(self_copy))
    return self_copy

@MyMocker()
class LotsOfApiTests(TestCase):
  def test_url1_A(self, test_mock):
    print('url1_A: {}'.format(test_mock))
    r = requests.get(url1)
    self.assertTrue(test_mock.called_once)

  def test_url1_B(self, test_mock):
    print('url1_B: {}'.format(test_mock))
    r = requests.get(url1)
    self.assertTrue(test_mock.called_once)

  def test_url2_A(self, test_mock):
    print('url2_A: {}'.format(test_mock))
    r = requests.get(url2)
    self.assertEqual(test_mock.call_count, 1)

  def test_url2_B(self, test_mock):
    print('url2_B: {}'.format(test_mock))
    r = requests.get(url2)
    self.assertEqual(test_mock.call_count, 1)

if __name__ == '__main__':
  import unittest
  unittest.main()

[clslgrnc]

@jamielennox might do a release soon(ish), you should do a PR.

[jamielennox]

It's very late and I should leave this until tomorrow. First impression, i'm happy to fix the copy implementation if it's broken for a case, but can we be a bit more explicit about the problem than deepcopy? deepcopy on an instantiated object seems a really big hammer.

Secondly, just scanning your test case, and admitting this might be a simplified example, you should almost certainly use a def setUp(self) on the class rather than subclass the mocker.

I'm hoping to do a release when the docs for the nesting are changed. Though I just notice the bugs have been particularly active in the last few days (probably for this reason) so i'll go through them first.

[rfportilla]

Thanks for looking this over. I am not sure why you consider it a really big hammer. I ran a test to compare time with and without deepcopy and the performance was the same (even at 10^6 iterations). When I register uri's (this creates matcher objects), the time to deep copy went up significantly, but grows linearly. This is to be expected. I will attach the test code and output below.

Regarding using setUp, this was my first approach, but it doesn't scale well. I plan on releasing my sub class within my company b/c it represents an enterprise-wide API that is not consistently daccessed. Having this mocker published will guarantee a consistent testing interface. I considered subclassing the testcase, ex. my_test_base(TestCase). But then anyone who subclasses that will likely have their own setUp and calling the parent setUp becomes boiler plate. I don't think that makes more sense than subclassing the Mocker. The idea of subclassing a mock for a more complex and preset behavior (ex. creating a fake) is not outrageous. I have created many fakes with magicmock before. We should embrace this.

Lastly, in terms of expectations, this simply does not behave consistently. Without modification, I was able to use my subclass within a with block and with start and stop methods. This only fails when used as a decorator and it fails because of the copy. If you want me to consider other copy routines that only do partial copies, I can. But, it is, again, only partial copy and I don't think it will be any faster.

Test script

import time
from requests_mock import Mocker
from copy import deepcopy

n = 10 ** 4

def test_run(mock_obj, n):
  start_time = time.time()
  for i in range(n):
    c = mock_obj.copy()
  return time.time() - start_time

print('running {} copies with original code'.format(n))
ori_seconds = test_run(Mocker(), n)
print('run time for original: {}'.format(ori_seconds))

print('running {} copies with deepcopy'.format(n))
class test_mock(Mocker):
  def copy(self):
    return deepcopy(self)
ori_seconds = test_run(Mocker(), n)
print('run time for deepcopy: {}'.format(ori_seconds))

print('running {} copies with deepcopy and loaded with matchers'.format(n))
class test_mock_loaded(Mocker):
  def copy(self):
    return deepcopy(self)
mock_obj = test_mock_loaded()
for i in range(5):
  mock_obj.get('http://fake{}.com')
ori_seconds = test_run(mock_obj, n)
print('run time for deepcopy and loaded with matchers: {}'.format(ori_seconds))

Output:

running 10000 copies with original code
run time for original: 0.04102754592895508
running 10000 copies with deepcopy
run time for deepcopy: 0.031002283096313477
running 10000 copies with deepcopy and loaded with matchers
run time for deepcopy and loaded with matchers: 2.20149827003479

[jamielennox]

Sorry, let me be clearer on the big hammer. I get the python everything is a dict, but i've always been uneasy about the idea that you can deepcopy an instance and just continue with both of them in like a fork() way. There is randomness and things that happen within init that i don't like to treat as just data. This may not apply in this case, just gut reaction.

From a speed sense this is a testing library and will (/should) be nowhere near critical path so with the exception of a giant slowdown i'm not too performance orientated.

Ok, so going back and looking why we do a copy() in decorate at all, it was commited: https://review.opendev.org/#/c/475474/ - in which the whole point seems to be that the mocker creates a new adapter so if for whatever reason the test was to be executed twice you don't have the old adapter still in place. If we add the adapter here we obviously break that.

Crying kid - I'll come back for more, but don't want to lose that info.

But from reading this if we step back we can phrase the question - "how do i prepackage a bunch of mocks for reuse and distribution in libraries?". I'd agree that's a use case we should support and there might be a few options to do this that don't care about copy()

[rfportilla]

@jamielennox To me, it seems the need to use copy is tied to that is customary to use an instance of class (in this case Mocker) to decorate a class/method. In other uses, such as "with" block or explicit start/stop, the Mocker is instantiated when it is used. Therefore, there is no need for a copy b/c all of the data is passed in at time of use, when context is fully defined. In contrast, with the decorator approach, the Mocker is instantiated before it is applied to the methods, i.e. before context is fully defined. Therefore, to carry over any attributes (esp. those set in init), you have to copy those properties. In short, init does not get run again (unless we do it explicitly in copy).

I sense that you want to future proof this somehow. I think the best effort here is deepcopy and some good test cases. I think the greater risk is in just adding more attributes to copy method. This is more maintenance and more prone to complaints. JMHO

I will take your guidance. Please let me know how to proceed.