search

getsentry / responses

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

Fix use of global assert_all_requests_are_fired. Closes #726 #731

Open andrew-cybsafe opened 4 months ago

andrew-cybsafe commented 4 months ago

A somewhat naive fix for the global assert_all_requests_are_fired. Tests are passing locally on 3.12.

andrew-cybsafe commented 4 months ago

@markstory can you kick off the tests again?

beliaev-maksim commented 4 months ago

@markstory I think we decided to deprecate this completely a while ago

https://github.com/getsentry/responses?tab=readme-ov-file#deprecations-and-migration-path

andrew-cybsafe commented 4 months ago

I think we decided to deprecate this completely a while ago

@beliaev-maksim the linked doc deprecates responses.assert_all_requests_are_fired for responses.mock.assert_all_requests_are_fired. It's the latter / non-deprecated option that isn't working correctly.

andrew-cybsafe commented 3 months ago

it feels like the logic should be moved down to the activate() method. Have you exhausted all the options to make the change self contained within activate ?

@beliaev-maksim The attribute assert_all_requests_are_fired is mocked within the get_wrapped() function regardless of whether a value has been passed into the function. This seems to make it difficult to fix this issue outside of get_wrapped(). I did try only mocking the attribute if the value was passed in, e.g. with this code:

    if assert_all_requests_are_fired is not None:
        assert_mock = std_mock.patch.object(
            target=responses,
            attribute="assert_all_requests_are_fired",
            new=assert_all_requests_are_fired,
        )
    else:
        from contextlib import nullcontext
        assert_mock = nullcontext()

This mostly works except for the test_nested_decorators test, but given I only have a superficial understanding of this code, delving into that is more than I can take on right now.