outer mock broken by nested mocks

Bug #1687039 reported by David Chudzicki
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
requests-mock
New
Undecided
Unassigned

Bug Description

I was using 0.6 and nesting didn't work: the inner mock didn't take effect.

I was thrilled when I saw "Allow for nested mocking".

However, now the outer mock doesn't take effect. This is still broken from my perspective.

----

In [8]: with requests_mock.Mocker() as m:
   ...: m.get('http://test.com', text='resp')
   ...: with requests_mock.Mocker() as m2:
   ...: m2.get('http://david.com', text='hi')
   ...: r = requests.get('http://test.com')
   ...: print(r.content)
   ...:

<snip>

NoMockAddress: No mock address: GET http://test.com/
---

I actually prefer the old behavior (and am suggesting my team revert to the older version), because at least then it was clear that nested mocks don't work. Now it's easy to get tripped up by the mistake I just made: We have a base class that sets up a bunch of mocks. I was writing a test that needed a new mock. Initially my test didn't happen to use the mocks from the base class, so I didn't know that anything was wrong. But then things changed slightly and the base class's mocks were required, and it all broke.

I think the ideal behavior would be to try the inner mock first, and if there's no match go to the outer mock.

Revision history for this message
Jamie Lennox (jamielennox) wrote :

Hey David, Thanks for the report.

So i'm sorry to have flipped behaviour like that, interestingly the reason I did was because someone filed a bug report saying the old way was unintuitive. At the time I did consider chaining up from inner to outer mocks, but there are a few implementation details that make that difficult and that was the first time i'd even heard of somebody doing nested mocks.

So before i commit to anything i'd really like to understand the use case because I've never seen why people use requests-mock this way. You say "I was writing a test that needed a new mock", what's the rationale for using a new mock instead of adding a new route to the old mock?

The way i've always done my tests is (untested):

class TestStuff(testtools.TestCase):

    def setUp(self):
        super(TestStuff, self).setUp()
        self.requests_mock = self.useFixture(rm_fixture.Fixture())
        // some common routes, eg auth
        self.requests_mock.get('https://test.com/login', json={})

    def test_something(self):
        // test specific mocks
        self.requests_mock.get('https://test.com/path', json={})

        // do test

(I like the fixture because we use fixtures elsewhere, but all it does is creates a requests_mock.Mocker(), call rm.start() and addCleanup(rm.stop) so you don't need to use it)

This means that you have one mocker object per test and your test suite builds up relevant mocks in the setUp, and typically we have a test class inheritence that means you get all the common mocks based on what you are inheriting.

I'm not claiming my way is correct, but approaching tests that way I'm not sure why you're in a situation where you have a new mocker object for a test and that test also relies on a mocker that was set up earlier. Why not just reuse the first one?

Revision history for this message
David Chudzicki (dchudz) wrote :

I actually did something very similar to your version initially! In our case we're explicitly calling the __enter__ and __exit__ in the setUp and tearDown methods, not using a fixture, but the effect is similar. I had a pull request adding some tests which add mocks to the same mocker the way your example does, and a colleague objected to it in code review, b/c my equivalent of "self.requests_mock.get('https://test.com/path', json={})" isn't called inside a context manager.

Of course the mock will be cleaned up between tests. But my colleague wanted to see my test's mock in an explicit context manager. The explicit context manager inside the test is nice because:

(a) that's the syntax I'm used to (but it breaks things given that's we're already setting up mocks in the setup) and
(b) especially if the test needed to use a couple different mocks, using two different context manager calls to make sure they didn't interfere with each other would be good.

Revision history for this message
David Chudzicki (dchudz) wrote :

I should have also mentioned-- thanks for your work on this library! I really appreciate having it.

Revision history for this message
Jamie Lennox (jamielennox) wrote :

Thanks, it's always nice to here that people are using your stuff (even the problems).

So my concern is primarily between the interaction of the two mockers. I can understand the desire to isolate certain tests/behaviours/components, i'm still a little unsure from an intuitive perspective what a test author would expect in the scenario when they have many mockers.

So two possible solutions, one existing one new:

Existing: The way I have (and therefore requests-mock has) enforced that a behaviour has actually occurred is by examining history when the operation is complete. What this means is that in a test setup you can create hundreds of mocks that may never get called, and then after the test you look at request_history or any of the helpers to make sure that things happened in the order you expect.

I've always preferred this way because you can have giant fixture style lists of responses that you can just register and never use rather than have to tweak per test. The mock style vs mox.

When you create a mock like:

m = self.requests_mock.get('https://test.com/path')

that m is a useable object and has options like:

m.request_history
m.last_request
m.call_count

and all the other things that self.requests_mock has. So typically to make sure that the specific thing i'm testing occurred and not all the setup is to do assertions like:

self.assertTrue(m.called_once)

rather that have to inspect the full:

self.requests_mock.request_history

It doesn't give you isolation obviously because that mock is still there for later - but IMO you have a reasonable certainty from that that your test did the right thing, because otherwise it would fail.

New one:

I'm going to need to think about this a fair bit more, but what about a context manager within the mocker, like:

with self.requests_mock.new() as mocker:
    mocker.get('http://test.com', json={})

(new() is a terrible name). This is mentally what i feel like you're asking for and you can interrogate mocker independently of self.requests_mock. But I'm torn on what it provides.

So that route would only be available for the lifetime of mocker, but I think that any requests should probably still get added to self.requests_mock.request_history. You ensure that nothing after has access to that route - but you're pretty sure that's not happening anyway, you'd see it in call_count or something. Also mocks get evaluated in a last declared first fashion so if you're trying to override an existing address you can already do that.

I haven't looked yet as to how hard that would be to implement, I suspect not as easy as you'd think. But am I on the right track with the functionality?

Revision history for this message
Oren S (oren-shm) wrote :

Hi. I'm a relatively new to requests_mock and also have an interest in this discussion.
First of all, thanks a bunch for providing this library and maintaining it. As David said, it is much appreciated.

As for the subject of nested mocks, I got here after I wrote a test and assumed mocks are unrelated frames and thus can be nested. Needless to say, I failed miserably :)

In my specific use-case I try to develop my code as a modular system as much as I can. In this specific instance authentication is provided by another service (http) so it needs to be mocked as well. I am trying to provide a testing utility which will allow a test writer to write something like:

with requests_mock.Mocker() as m1, authentication_mocker(...) as m2:
    call_a_complex_functionality() # also does authentication
    assert m1.called_once # make sure the functionality you test actually resulted in a request
    assert m2.called_once # make sure request is properly authenticated

I want the authentication client developer to write their own testing support, and that the code for its mocks will be able to run regardless of the other parts of the test.

I hope this input may clear the need.

P.S / Disclaimer
Maybe fixtures may help me here (not sure, just diving into them now) or maybe my design is wrong. Will have to rethink it and any tips are welcome :)

Revision history for this message
Jamie Lennox (jamielennox) wrote :

So I'm still not entirely sure how to handle the nested mock situation, and I'm wary of changing behaviour that i've had people argue on both sides for.

Having said that I'll chime in here because I think what you're asking for is doable today - because it's a pattern i use a bit.

(Note: this assumes a class approach to tests)

My suggestion on how to handle all of this is (using the fixture or not) is to initiate requests_mock in the setUp() portion of your tests. With no urls registered it's job will simply be to cut off your testing from any requests access, which is somewhat useful in itself. You don't have to use the fixture, what it does is trivial and you can replicate it easily in whatever suite you're using: https://github.com/openstack/requests-mock/blob/master/requests_mock/contrib/fixture.py#L24

Then all your fixtures/components just access self.requests_mock to add their state. Now it's harder to check self.requests_mock.called_once because you don't know all the components that have touched it however when you do:

   m = self.requests_mock.get('https://www.google.com')
   resp = requests.get('https://www.google.com')

not only does it set self.requests_mock.called, it sets m.called, so your components can assert that the urls that they establish are called the correct amount of times, without checking the global call_count.

Still, have a look at fixtures (https://pypi.python.org/pypi/fixtures), i use them for things almost exactly like the authentication_mocker and pass the requests_mock object to each fixture.

Let me know any concerns with this approach.

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.