Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Responses 0.19.0 seems to break some s3 mocks #511

Closed
potiuk opened this issue Mar 7, 2022 · 16 comments · Fixed by #516
Closed

Responses 0.19.0 seems to break some s3 mocks #511

potiuk opened this issue Mar 7, 2022 · 16 comments · Fixed by #516
Assignees
Labels

Comments

@potiuk
Copy link

potiuk commented Mar 7, 2022

The 0.19.0 Responses seems to break some s3 mocks.

We noticed a lot of failing test cases in our CI after today's 0.19.0 upgrade. All of them are S3-mock related:
https://github.com/apache/airflow/runs/5447285682?check_suite_focus=true#step:8:18784

Environment

Apache Airflow development environment.

Which SDK and version?

  • responses 0.19.0

Steps to Reproduce

  1. This is the fastest way to bring the development environment of Airflow to demonstrate the error:
docker run --env "AIRFLOW__CORE__EXECUTOR=SequentialExecutor" \
   --env "AIRFLOW__CORE__SQL_ALCHEMY_CONN=sqlite:////root/airflow/airflow.db" \
   -it \
   ghcr.io/apache/airflow/main/ci/python3.7:8bb092fb6bbafbdca5a08d5f30329044fdd9794c
  1. Once the above command succeds, you should see this:
Airflow home: /root/airflow                                                                                                                                                                                                                                                                                      
Airflow sources: /opt/airflow                                                                                                                                                                                                                                                                                    
Airflow core SQL connection: sqlite:////root/airflow/airflow.db                                                                                         
                                                                                                                                                                                                                                                                                                                 
                                                                                                                                                        
Using already installed airflow version                                                                                                                                                                                                                                                                          
                                                                                                                                                        
                                                                                                                                                        
No need for www assets recompilation.                                                                                                                   
                                                                                                                                                                                                                                                                                                                 
===============================================================================================                                                         
             Checking integrations and backends                                                                                                         
===============================================================================================                                                         
-----------------------------------------------------------------------------------------------                                                                                                                                                                                                                  
                                                                                                                                                        
Disabled integrations: kerberos mongo redis cassandra openldap trino pinot rabbitmq                                                                     
                                                                                                                                                        
Enable them via --integration <INTEGRATION_NAME> flags (you can use 'all' for all)                                                                                                                                                                                                                               
                                                                                                                                                        
Your dags for webserver and scheduler are read from /root/airflow/dags directory                                                                        
                                                                                                                                                                                                                                                                                                                 
You can add /files/airflow-breeze-config directory and place variables.env                                                                                                                                                                                                                                       
In it to make breeze source the variables automatically for you                                                                                                                                                                                                                                                  
                                                                                                                                                        
                                                                                                                                                                                                                                                                                                                 
You can add /files/airflow-breeze-config directory and place .tmux.conf                                                                                                                                                                                                                                          
in it to make breeze use your local .tmux.conf for tmux                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                           
                                                                                                                                                        
You can add /files/airflow-breeze-config directory and place init.sh                                                                                                                                                                                                                                             
In it to make breeze source an initialization script automatically for you                                                                                                                                                                                                                                       
                                                                                                                                                                                                                                                                                                                 
root@6a1ac9c2648f:/opt/airflow# 
  1. Run example test (in the container):
pytest tests/providers/amazon/aws/hooks/test_s3.py::TestAwsS3Hook::test_delete_bucket_if_not_bucket_exist
  1. It fails with error inside moto library (but apparently related to responses library:
/usr/local/lib/python3.7/site-packages/moto/core/models.py:118: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
/usr/local/lib/python3.7/site-packages/moto/core/models.py:96: in start
    self.enable_patching(reset)
/usr/local/lib/python3.7/site-packages/moto/core/models.py:362: in enable_patching
    callback=convert_flask_to_responses_response(value),
/usr/local/lib/python3.7/site-packages/responses/__init__.py:603: in add
    self._registry.add(method)
/usr/local/lib/python3.7/site-packages/responses/registries.py:53: in add
....
  1. Downgrade responses to 0.18.0
root@6a1ac9c2648f:/opt/airflow# pip install responses==0.18.0                                                                                                                                                                                                                                                    
install responses==0.18.0                                                                                                                                                                                                                                                    
Collecting responses==0.18.0                                                                                                                                                                                                                                                                                     
  Downloading responses-0.18.0-py3-none-any.whl (38 kB)                                                                                                 
Requirement already satisfied: urllib3>=1.25.10 in /usr/local/lib/python3.7/site-packages (from responses==0.18.0) (1.26.8)                             
Requirement already satisfied: requests<3.0,>=2.0 in /usr/local/lib/python3.7/site-packages (from responses==0.18.0) (2.27.1)                                                                                                                                                                                    
Requirement already satisfied: charset-normalizer~=2.0.0 in /usr/local/lib/python3.7/site-packages (from requests<3.0,>=2.0->responses==0.18.0) (2.0.12)                                                                                                                                                         
Requirement already satisfied: certifi>=2017.4.17 in /usr/local/lib/python3.7/site-packages (from requests<3.0,>=2.0->responses==0.18.0) (2020.12.5)    
Requirement already satisfied: idna<4,>=2.5 in /usr/local/lib/python3.7/site-packages (from requests<3.0,>=2.0->responses==0.18.0) (3.3)                                                                                                                                                                         
Installing collected packages: responses                                                                                                                                                                                                                                                                         
  Attempting uninstall: responses                                                                                                                                                                                                                                                                                
    Found existing installation: responses 0.19.0                                                                                                                                                                                                                                                                
    Uninstalling responses-0.19.0:                                                                                                                      
      Successfully uninstalled responses-0.19.0                                                                                                                                                                                                                                                                  
Successfully installed responses-0.18.0                                                                                                                       
  1. Rerun the example test. This time it will succeed:
root@6a1ac9c2648f:/opt/airflow# pytest tests/providers/amazon/aws/hooks/test_s3.py::TestAwsS3Hook::test_delete_bucket_if_not_bucket_exist                                                                                                                                                                        
============================================================================================================================================== test session starts ==============================================================================================================================================
platform linux -- Python 3.7.12, pytest-6.2.5, py-1.11.0, pluggy-1.0.0 -- /usr/local/bin/python                                                         
cachedir: .pytest_cache                                                                                                                                                                                                                                                                                          
rootdir: /opt/airflow, configfile: pytest.ini                                                                                                                                                                                                                                                                    
plugins: anyio-3.5.0, instafail-0.4.2, timeouts-1.2.1, requests-mock-1.9.3, httpx-0.20.0, forked-1.4.0, xdist-2.5.0, asyncio-0.18.2, flaky-3.7.0, rerunfailures-9.1.1, cov-3.0.0                                                                                                                                 
asyncio: mode=strict                                                                                                                                                                                                                                                                                             
setup timeout: 0.0s, execution timeout: 0.0s, teardown timeout: 0.0s                                                                                                                                                                                                                                             
collected 1 item                                                                                                                                                                                                                                                                                                 
                                                                                                                                                                                                                                                                                                                 
tests/providers/amazon/aws/hooks/test_s3.py::TestAwsS3Hook::test_delete_bucket_if_not_bucket_exist PASSED                                                                                                                                                                                                 [100%] 
                                                                                                                                                                                                                                                                                                                 
=============================================================================================================================================== warnings summary ================================================================================================================================================
airflow/configuration.py:376                                                                                                                            
  /opt/airflow/airflow/configuration.py:376: FutureWarning: The 'log_filename_template' setting in [logging] has the old default value of '{{ ti.dag_id }}/{{ ti.task_id }}/{{ ts }}/{{ try_number }}.log'. This value has been changed to 'dag_id={{ ti.dag_id }}/run_id={{ ti.run_id }}/task_id={{ ti.task_id }
}/{%% if ti.map_index >= 0 %%}map_index={{ ti.map_index }}/{%% endif %%}attempt={{ try_number }}.log' in the running config, but please update your config before Apache Airflow 3.0.                                                                                                                            
    FutureWarning,                                                                                                                                                                                                                                                                                               
                                                                                                                                                                                                                                                                                                                 
-- Docs: https://docs.pytest.org/en/stable/warnings.html                                                                                                                                                                                                                                                         
========================================================================================================================================= 1 passed, 1 warning in 0.60s ==========================================================================================================================================
root@6a1ac9c2648f:/opt/airflow# 

Expected Result

I expect that the 0.19.0 release will not break moto's s3 mocking :).

Actual Result

It breaks it

@potiuk
Copy link
Author

potiuk commented Mar 7, 2022

Also - airlfow is quite a beast when it comes to installing it, hence all the Airlfow env and dependencies are ready-to-run in the docker image (and you can find all the python sources in /opt/airflow/airflow.

But you can also browse the sources in https://github.com/apache/airflow/blob/main/airflow/providers/amazon/aws/hooks/s3.py (we are talking about main - development branch of Airlfow.

potiuk added a commit to potiuk/airflow that referenced this issue Mar 7, 2022
Some of the tests failed previously with typing extensions above 4.
This PR attempts to relax the limit and check if the problems
still appear.

Also new tests (S3) started to fail when a new `responses` library
version has been released today.

So this change also add limits to the responses library in order
to make sure the tests pass.

Issue getsentry/responses#511 has been
opened to raise it to `responses` library maintainers.
potiuk added a commit to apache/airflow that referenced this issue Mar 7, 2022
Some of the tests failed previously with typing extensions above 4.
This PR attempts to relax the limit and check if the problems
still appear.

Also new tests (S3) started to fail when a new `responses` library
version has been released today.

So this change also add limits to the responses library in order
to make sure the tests pass.

Issue getsentry/responses#511 has been
opened to raise it to `responses` library maintainers.
@shugybugy-assaf
Copy link

respones 0.19.0 also break moto (versions 2.3.1 to 3.0.6)

@potiuk
Copy link
Author

potiuk commented Mar 7, 2022

Yep. I think this is the root cause.

@bblommers
Copy link
Contributor

Moto maintainer here - we've just released a fix, and moto >= 3.0.7.dev2 is the first dev-release compatible with 0.19.0.

(For reference, this was the issue that was raised with us.)

@potiuk
Copy link
Author

potiuk commented Mar 7, 2022

Cool I will test it in Aiflow in a moment

@potiuk
Copy link
Author

potiuk commented Mar 7, 2022

@bblommers: I confirm that "3.0.7.dev3" (which appeared in the meantime) fixes the issue :)

@potiuk
Copy link
Author

potiuk commented Mar 7, 2022

@bblommers : "3.0.7.dev4" released 3 minutes ago as well. (what a speed !)

@bblommers
Copy link
Contributor

@bblommers: I confirm that "3.0.7.dev3" (which appeared in the meantime) fixes the issue :)

Happy to hear that - thanks for letting me know!

@beliaev-maksim
Copy link
Collaborator

Hi @bblommers,
Do you think it is related to #480?

I see you override add method to fix it

@bblommers
Copy link
Contributor

Yes, it is related, @beliaev-maksim. There's two underlying issues:

  • We're using a custom CallbackResponse, and one of the values within that class doesn't like being copied
  • We also do not check whether mocks have already started - so we sometimes try to add the same URL's twice.

Guarding our own add-method was both the easiest and the most foolproof fix.

@andersk
Copy link

andersk commented Mar 16, 2022

Not sure if this is the same issue, but here’s a self-contained minimal test case that still fails with the current versions moto==3.1.0, responses==0.19.0 but succeeds after downgrading to responses==0.18.0:

import moto, requests, responses

with responses.RequestsMock() as rsps:
    with moto.mock_s3():
        pass
    rsps.add(responses.GET, "https://example.invalid", status=200)
    print(requests.request("GET", "https://example.invalid"))

git bisect implicates commit a35896e “Fix nested decorators causing mocks leaking” (#500).

@bblommers
Copy link
Contributor

@andersk @beliaev-maksim This is the equivalent test that does not involve Moto:

import requests, responses

with responses.RequestsMock() as rsps:
    print("inside responses mock")
    with responses.RequestsMock() as rsps2:
        rsps2.reset()
    rsps.add(responses.GET, "https://example.invalid", status=200)
    print(requests.request("GET", "https://example.invalid"))

(Moto does essentially the same thing - it starts a new instance of RequestsMock, and resets it occasionally.)

Both tests seem to pass after changing the reset-method, and undoing the self_patcher = None statement:

def reset(self):
    self._registry = FirstMatchRegistry()
    self._calls.reset()
    self.passthru_prefixes = ()
    #    self._patcher = None

I don't quite understand why the patcher should be None, or why that statement makes this test fail, tbh..

@beliaev-maksim
Copy link
Collaborator

@bblommers
I will check later

but we need to reset _patcher to avoid double decorator issue

@andersk
Copy link

andersk commented Mar 16, 2022

The self._patcher = None in RequestsMock.reset prevents RequestsMock.stop from ever calling self._patcher.stop(). Surely that can’t be right.

For example, this also fails with 0.19.0 and succeeded with 0.18.0:

import requests, responses

with responses.RequestsMock() as rsps:
    rsps.reset()

print(requests.get("https://example.com"))

@beliaev-maksim
Copy link
Collaborator

@bblommers @andersk
can you check if #516 fixes the issue for you?

@andersk
Copy link

andersk commented Mar 17, 2022

#516 works for me.

markstory pushed a commit that referenced this issue Mar 17, 2022
…516)

* Fixed the issue when `reset()` method was called in not stopped mock. See #511
@beliaev-maksim beliaev-maksim self-assigned this Jun 22, 2022
kosteev pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this issue Jul 10, 2022
Some of the tests failed previously with typing extensions above 4.
This PR attempts to relax the limit and check if the problems
still appear.

Also new tests (S3) started to fail when a new `responses` library
version has been released today.

So this change also add limits to the responses library in order
to make sure the tests pass.

Issue getsentry/responses#511 has been
opened to raise it to `responses` library maintainers.

GitOrigin-RevId: 082aafd2ab20656abed5e82be60b541a56142cad
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this issue Aug 30, 2022
Some of the tests failed previously with typing extensions above 4.
This PR attempts to relax the limit and check if the problems
still appear.

Also new tests (S3) started to fail when a new `responses` library
version has been released today.

So this change also add limits to the responses library in order
to make sure the tests pass.

Issue getsentry/responses#511 has been
opened to raise it to `responses` library maintainers.

GitOrigin-RevId: 082aafd2ab20656abed5e82be60b541a56142cad
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this issue Oct 4, 2022
Some of the tests failed previously with typing extensions above 4.
This PR attempts to relax the limit and check if the problems
still appear.

Also new tests (S3) started to fail when a new `responses` library
version has been released today.

So this change also add limits to the responses library in order
to make sure the tests pass.

Issue getsentry/responses#511 has been
opened to raise it to `responses` library maintainers.

GitOrigin-RevId: 082aafd2ab20656abed5e82be60b541a56142cad
aglipska pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this issue Oct 7, 2022
Some of the tests failed previously with typing extensions above 4.
This PR attempts to relax the limit and check if the problems
still appear.

Also new tests (S3) started to fail when a new `responses` library
version has been released today.

So this change also add limits to the responses library in order
to make sure the tests pass.

Issue getsentry/responses#511 has been
opened to raise it to `responses` library maintainers.

GitOrigin-RevId: 082aafd2ab20656abed5e82be60b541a56142cad
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this issue Dec 7, 2022
Some of the tests failed previously with typing extensions above 4.
This PR attempts to relax the limit and check if the problems
still appear.

Also new tests (S3) started to fail when a new `responses` library
version has been released today.

So this change also add limits to the responses library in order
to make sure the tests pass.

Issue getsentry/responses#511 has been
opened to raise it to `responses` library maintainers.

GitOrigin-RevId: 082aafd2ab20656abed5e82be60b541a56142cad
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this issue Jan 27, 2023
Some of the tests failed previously with typing extensions above 4.
This PR attempts to relax the limit and check if the problems
still appear.

Also new tests (S3) started to fail when a new `responses` library
version has been released today.

So this change also add limits to the responses library in order
to make sure the tests pass.

Issue getsentry/responses#511 has been
opened to raise it to `responses` library maintainers.

GitOrigin-RevId: 082aafd2ab20656abed5e82be60b541a56142cad
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants