getsentry / responses

A utility for mocking out the Python Requests library.
Apache License 2.0
4.14k stars 354 forks source link

RecursionError with passthru and _real_send override #670

Closed Seluj78 closed 1 year ago

Seluj78 commented 1 year ago

Describe the bug

When using the responses._real_send = self.r_mock.unbound_on_send() solution described here, I am getting a recursion error.

Additional context

This issue replaces this one

Version of responses

0.23.3

Steps to Reproduce

You can run unittest on this file and get the error :)

import os
import re
import unittest
import requests

os.environ["ENVIRONMENT"] = "tests"

import responses

def my_function():
    # Send an email
    requests.post("https://example.org")

    # Do something else with the passthru
    requests.post("http://localhost:7700/indexes/test/documents")

    return "OK"

class _TestCase(unittest.TestCase):
    def setUp(self):
        self.r_mock = responses.RequestsMock(assert_all_requests_are_fired=True)
        self.r_mock.start()
        self.r_mock.add_passthru(re.compile(rf"http://localhost:7700.*"))
        responses._real_send = self.r_mock.unbound_on_send()

    def tearDown(self):
        self.r_mock.stop()
        self.r_mock.reset()

class MyTest(_TestCase):

    def test_indexing(self):
        self.r_mock.add(responses.POST, "https://example.org", status=200)
        self.assertEqual("OK", my_function())

Expected Result

The test should run fine

Actual Result

============================= test session starts ==============================
collecting ... collected 1 item

reproduce.py::MyTest::test_indexing 

============================== 1 failed in 0.19s ===============================
FAILED                               [100%]
reproduce.py:34 (MyTest.test_indexing)
self = <reproduce.MyTest testMethod=test_indexing>

    def test_indexing(self):
        self.r_mock.add(responses.POST, "https://example.org", status=200)
>       self.assertEqual("OK", my_function())

reproduce.py:37: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
reproduce.py:16: in my_function
    requests.post("http://localhost:7700/indexes/test/documents")
.venv/lib/python3.11/site-packages/requests/api.py:115: in post
    return request("post", url, data=data, json=json, **kwargs)
.venv/lib/python3.11/site-packages/requests/api.py:59: in request
    return session.request(method=method, url=url, **kwargs)
.venv/lib/python3.11/site-packages/requests/sessions.py:587: in request
    resp = self.send(prep, **send_kwargs)
.venv/lib/python3.11/site-packages/requests/sessions.py:701: in send
    r = adapter.send(request, **kwargs)
.venv/lib/python3.11/site-packages/responses/__init__.py:1127: in send
    return self._on_request(adapter, request, **kwargs)
.venv/lib/python3.11/site-packages/responses/__init__.py:1033: in _on_request
    return _real_send(adapter, request, **kwargs)
.venv/lib/python3.11/site-packages/responses/__init__.py:1127: in send
    return self._on_request(adapter, request, **kwargs)
E   RecursionError: maximum recursion depth exceeded while calling a Python object
!!! Recursion detected (same locals & position)
beliaev-maksim commented 1 year ago

is it possible to simplify an example to avoid any application deployment?

https://stackoverflow.com/help/minimal-reproducible-example

Seluj78 commented 1 year ago

Sure enough, I just updated the issue. I had it that way to make sure it was similar to my real setup but it woks that way as well.

beliaev-maksim commented 1 year ago

@Seluj78 and we certainly need flask here ? and 4 test cases ?

Seluj78 commented 1 year ago

I've put 4 test cases to showcase that 3 work, one doesn't, and it seems like it's the one with the passthru that doesn't work. I don't know about flask, let me see if I can reduce the example even further.

Seluj78 commented 1 year ago

Updated once more without flask :) Sorry about that

beliaev-maksim commented 1 year ago

@Seluj78 looking at example I think the issue is that you mock the request library with responses. Then within responses you add passthrough which leads for responses to call _real_send. While in this case this function is overwritten by again mocked responses object.

this is different from the solution I provided in moto since in moto there are 2 responses objects.

in your case you will need to intercept it with another responses, eg

import os
import re
import unittest
import requests

os.environ["ENVIRONMENT"] = "tests"

import responses

def my_function():
    # Send an email
    requests.post("https://example.org")

    # Do something else with the passthru
    requests.post("http://localhost:7700/indexes/test/documents")

    return "OK"

class _TestCase(unittest.TestCase):
    def setUp(self):
        self.r_mock = responses.RequestsMock(assert_all_requests_are_fired=True)
        self.r_mock2 = responses.RequestsMock(assert_all_requests_are_fired=True)
        self.r_mock2.start()
        self.r_mock.start()
        self.r_mock.add_passthru(re.compile(rf"http://localhost:7700.*"))
        responses._real_send = self.r_mock2.unbound_on_send()

    def tearDown(self):
        self.r_mock.stop()
        self.r_mock2.stop()
        self.r_mock.reset()
        self.r_mock2.reset()

class MyTest(_TestCase):

    def test_indexing(self):
        self.r_mock.add(responses.POST, "https://example.org", status=200)
        self.r_mock2.add(responses.POST, re.compile(rf"http://localhost:7700.*"), status=200)
        self.assertEqual("OK", my_function())

if you want to send the real response (eg you run e2e test with real application deployed), then you do not need to overwrite _real_send at all. Just leave it as is

Seluj78 commented 1 year ago

I see. The problem then is that, without overwriting _real_send, I still get a problem with my class setup of responses and how different registeries are used combined with moto, see https://github.com/getsentry/responses/issues/669

I didn't find a good example in your documentation about how to setup responses like I did here without having the problem I mentioned in the other issue

beliaev-maksim commented 1 year ago

@Seluj78 do you try to add pass through in your version of responses, not in moto ?

Seluj78 commented 1 year ago

Yes that's correct, I never touch moto's version of responses, the only thing I do (in the setUp method of my base testing class) is

        self.r_mock = responses.RequestsMock(assert_all_requests_are_fired=True)
        self.r_mock.start()
        self.r_mock.add_passthru(re.compile(rf"{MEILISEARCH_URL}.*"))

and then try to self.r_mock.add...

beliaev-maksim commented 1 year ago

ok, then we need to find a way to tell moto that it must real send into mocked version, while your call to responses should result in passing through into real requests.

that means we need to apply responses._real_send = self.r_mock2.unbound_on_send() not on the lib level.

@bblommers any though on this ? I can think of changing responses to add this as argument, however, would be better if we can articulate from outside without touching codebase

Seluj78 commented 1 year ago

Thank you for your time 🙏 Do let me know if you need any further information.

bblommers commented 1 year ago

Hi @beliaev-maksim!

Do you mean something like responses._real_send = responses_mock_created_by_moto.unbound_on_send(), for that to be part of Moto? Or am I misunderstanding you?

beliaev-maksim commented 1 year ago

@Seluj78 @bblommers please look at #671

I updated responses in the way that it will allow to explicitly set real send from custom adapter. (Please see test case in PR)

that means if in moto there will be a way for users to pass as argument the adapter send function, then users will be able to instantiate responses, then pass unbound on send method to moto and enjoy both libraries.

@bblommers feasible change on moto side ? I would like to test the change on feature branches on both repos before we merge it anywhere, then if positive. We first release new responses, then moto can catch up

Seluj78 commented 1 year ago

Wonderful! What would you need from me ? A comprehensive test case that includes moto and responses for you to test with ?

bblommers commented 1 year ago

Wonderful! What would you need from me ? A comprehensive test case that includes moto and responses for you to test with ?

That would be very useful @Seluj78 - once I have a test case, I will have a look how to best integrate this in Moto.

Seluj78 commented 1 year ago

Alright. Give me a few, I'll try and modify the test case I have provided here to include moto and reproduce the error(s) I was having with the double responses registry !

Seluj78 commented 1 year ago

Here is a test case I made that showcases the two errors. I hope you'll find it useful @bblommers @beliaev-maksim !

import re
import unittest
import requests
import responses
from moto import mock_ec2
from moto.core import patch_resource
import boto3

ec2_resource = boto3.resource("ec2", region_name="us-east-1")

def my_function():
    # Send an email
    requests.post("https://example.org")

    # Do something else with the passthru
    requests.post("http://localhost:7700/indexes/test/documents")

    return "OK"

def create_instance_and_send_email():
    ec2_resource.create_instances(
        ImageId="ami-12345678",
        MinCount=1,
        MaxCount=1,
        InstanceType="t2.micro",
        KeyName="my-key-pair",
    )
    my_function()
    return "OK"

class _TestCase(unittest.TestCase):
    def setUp(self):
        self.r_mock = responses.RequestsMock(assert_all_requests_are_fired=True)
        self.r_mock.start()
        self.r_mock.add_passthru(re.compile(rf"http://localhost:7700.*"))

        # If you leave the _real_send commented, you will get an error because
        # self.r_mock.add(responses.POST, "https://example.org", status=200)
        # will not have been executed. You can verify this by putting a breakpoint inside
        # `create_instance_and_send_email` and running it in debug mode (I'm using Pycharm to do this).
        # Once at the breakpoint, you can do `import responses; responses.registered()` and see that nothing is registered.
        # If you're able with your debugger, move back up the stacktrace back to the test `test_create_instance`
        # and run once more `import responses; responses.registered()` as well as `self.r_mock.registered()`
        # to see the different and what I think the problem is.

        # If you uncomment _real_send4, you will get the RecursionError previously mentioned.

        # responses._real_send = self.r_mock.unbound_on_send()

    def tearDown(self):
        self.r_mock.stop()
        self.r_mock.reset()

# I have not tried to use @mock_xxx on the base class, I don't know if it will work.
@mock_ec2
class MyTest(_TestCase):
    def setUp(self):
        super().setUp()
        patch_resource(ec2_resource)

    def test_indexing(self):
        self.r_mock.add(responses.POST, "https://example.org", status=200)
        self.assertEqual("OK", my_function())

    def test_create_instance(self):
        self.r_mock.add(responses.POST, "https://example.org", status=200)
        self.assertEqual("OK", create_instance_and_send_email())

Do let me know if you have questions about it. 😄

bblommers commented 1 year ago

Thanks @Seluj78, that makes sense.

I've managed to get this to work without any modifications to Moto, by installing the PR from @beliaev-maksim .

pip install https://github.com/beliaev-maksim/responses/archive/refs/heads/mbeliaev/real_send.zip

The only change I made was in _TestCase.setup:

from moto.core.models import responses_mock

def setUp(self):
    self.r_mock = responses.RequestsMock(assert_all_requests_are_fired=True)
    self.r_mock.start()
    self.r_mock.add_passthru(re.compile(rf"http://localhost:7700.*"))
    responses_mock._real_send = self.r_mock.unbound_on_send()

@beliaev-maksim The RequestsMock that Moto uses is initialized on import, so changing that to pass the real_adapter_send-argument is not trivial. (Do-able, I guess, just not easy.)

Is it an option to make the attribute part of the public API instead? I.e. RequestsMock.real_send = ..? Being able to set this on demand, instead of needing to know the value on instantiation, would be much easier (for me at least.. :slightly_smiling_face: )

beliaev-maksim commented 1 year ago

@bblommers not encouraged, but you can try RequestsMock._real_send

bblommers commented 1 year ago

@beliaev-maksim Or alternatively - can there be a RequestsMock.set_real_send(..)? Again, not a must, but it would make things much easier.

beliaev-maksim commented 1 year ago

@bblommers can you please try so far with private attribute to see if that works?

We can consider expanding it in the future

bblommers commented 1 year ago

@beliaev-maksim Yes, it does work! @Seluj78 's test case works when overwriting _real_send - sorry if that wasn't clear.

Seluj78 commented 1 year ago

(I still haven't had the time to test your PR @beliaev-maksim, but I will let you know when I do)

Seluj78 commented 1 year ago

I'm having trouble making the test setup work. @bblommers could you send me what you had that worked ?

bblommers commented 1 year ago

@Seluj78 Slightly adapted from your own test:

import unittest
import requests
import responses
from moto import mock_dynamodb
from moto.core.models import responses_mock
import boto3

def my_function():
    # Mock this website
    requests.post("https://example.org")

    # Passthrough this website
    assert requests.get("http://ip.jsontest.com").status_code == 200

    return "OK"

def create_instance_and_send_email():
    ddb = boto3.client("dynamodb", "us-east-1")
    assert ddb.list_tables()["TableNames"] == []
    my_function()
    return "OK"

@mock_dynamodb
class MyTest(unittest.TestCase):
    def setUp(self):
        self.r_mock = responses.RequestsMock(assert_all_requests_are_fired=True)
        responses_mock._real_send = self.r_mock.unbound_on_send()
        self.r_mock.start()
        self.r_mock.add_passthru("http://ip.jsontest.com")

    def tearDown(self):
        self.r_mock.stop()
        self.r_mock.reset()

    def test_indexing(self):
        self.r_mock.add(responses.POST, "https://example.org", status=200)
        self.assertEqual("OK", my_function())

    def test_create_instance(self):
        self.r_mock.add(responses.POST, "https://example.org", status=200)
        self.assertEqual("OK", create_instance_and_send_email())

This fails with responses==0.23.3, but succeeds with the latest: pip install https://github.com/getsentry/responses/archive/refs/heads/master.zip

Seluj78 commented 1 year ago

@bblommers these fail for me with

FAILED                               [100%]
reproduce.py:37 (MyTest.test_indexing)
self = <reproduce.MyTest testMethod=test_indexing>

    def tearDown(self):
>       self.r_mock.stop()

reproduce.py:35: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <responses.RequestsMock object at 0x103f55450>, allow_assert = True

    def stop(self, allow_assert: bool = True) -> None:
        if self._patcher:
            # prevent stopping unstarted patchers
            self._patcher.stop()

            # once patcher is stopped, clean it. This is required to create a new
            # fresh patcher on self.start()
            self._patcher = None

        if not self.assert_all_requests_are_fired:
            return

        if not allow_assert:
            return

        not_called = [m for m in self.registered() if m.call_count == 0]
        if not_called:
>           raise AssertionError(
                "Not all requests have been executed {0!r}".format(
                    [(match.method, match.url) for match in not_called]
                )
            )
E           AssertionError: Not all requests have been executed [('POST', 'https://example.org/')]

.venv/lib/python3.11/site-packages/responses/__init__.py:1158: AssertionError

Using responses installed from master

bblommers commented 1 year ago

@Seluj78 I had to explicitly uninstall responses first.

I've created an example repo that shows the test passes.

Seluj78 commented 12 months ago

Ok thanks !

I'll be OOO for the next week and a half, I'll try it when I come back !

Seluj78 commented 11 months ago

@beliaev-maksim Is there a way that the fix could make its way in a release on pypi ?

beliaev-maksim commented 11 months ago

@Seluj78 can you confirm that it is working on master ?

Seluj78 commented 11 months ago

@beliaev-maksim I just tried using @bblommers's way of installing and it worked on my reproduce test. I am currently running it on my 700 tests but it seems that it's all working ! I need to fix some tests (I'm getting some AssertionError: Not all requests have been executed) but other than that it works !! :D

Seluj78 commented 11 months ago
Screenshot 2023-10-19 at 14 16 12

It's working :)

beliaev-maksim commented 11 months ago

@Seluj78 track https://github.com/getsentry/responses/issues/681