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

Fix urllib3 mocking to conform to current API #602

Merged
merged 7 commits into from
May 29, 2024

Conversation

Perfect5th
Copy link
Contributor

@Perfect5th Perfect5th commented Jul 1, 2023

The expected responses from requests requests are now instances of urllib3.response.HTTPResponse, not http.client.HTTPResponse. This change both fixes that, and ensures the response can be read by fixing the incorrect HTTP message (it was missing a CRLF). I also added a Content-Type header because sometimes the request body does not get read if this header is absent.

tox should pass for py38 and py310. I didn't test py35 and py36 as both are now EOL.

fixes #601

@Perfect5th
Copy link
Contributor Author

New rev with fixes for 3.5 and 3.6 - turns out the old behaviour of the mock was what they wanted. Proof tox is passing for these versions: https://github.com/Perfect5th/talisker/actions/runs/5438859830/jobs/9890351211

Will work on other failing CI bits soon (docs, etc)

@Perfect5th
Copy link
Contributor Author

Bumping the Sphinx version gets the docs building again (see https://github.com/Perfect5th/talisker/actions/runs/5439103405), so this now also closes #599 and therefore closes #600

tests/test_requests.py Outdated Show resolved Hide resolved
tests/test_requests.py Outdated Show resolved Hide resolved
Copy link

@wgrant wgrant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM now.

@verterok
Copy link
Contributor

verterok commented May 7, 2024

Looks like this was left behind, sorry for missing it.
I just tried to run the CI jobs and looks like one dependency needs to be updated:

diff --git a/requirements.tests.txt b/requirements.tests.txt
index d36d68c..2765aa8 100644
--- a/requirements.tests.txt
+++ b/requirements.tests.txt
@@ -16,7 +16,7 @@ setuptools==44.0.0;python_version<"3.10"
 setuptools>64;python_version>="3.10"
 coverage==5.0.3;python_version<"3.10"
 coverage>=6;python_version>="3.10"
-flaky==3.6.1
+flaky==3.8.1

 # for integration tests
 # eventlet is pinned until https://github.com/benoitc/gunicorn/pull/2581

If you want to include the change in your PR, it should land (tested locally)

@Perfect5th
Copy link
Contributor Author

Perfect5th commented May 10, 2024

I've added the suggested flaky dependency version change to the PR, thanks @verterok !

@verterok
Copy link
Contributor

@Perfect5th Hi Again!
managed to get CI passing...this time actually passing \o/ (verterok#2)
My changes are below, which are basically tweaks to the requirements for older python versions

diff --git a/.github/workflows/tox.yml b/.github/workflows/tox.yml
index 280a7e5..30f9f34 100644
--- a/.github/workflows/tox.yml
+++ b/.github/workflows/tox.yml
@@ -32,6 +32,8 @@ jobs:
         uses: actions/setup-python@v2
         with:
           python-version: ${{ matrix.python }}
+        env:
+          PIP_TRUSTED_HOST: "pypi.python.org pypi.org files.pythonhosted.org"
       - name: Install dependencies
         run: |
           sudo apt update
diff --git a/requirements.devel.txt b/requirements.devel.txt
index 5ea0a4f..cea4f60 100644
--- a/requirements.devel.txt
+++ b/requirements.devel.txt
@@ -1 +1 @@
-tox==3.14.3
+tox==3.28.0
diff --git a/requirements.tests.txt b/requirements.tests.txt
index 2765aa8..809d891 100644
--- a/requirements.tests.txt
+++ b/requirements.tests.txt
@@ -16,7 +16,8 @@ setuptools==44.0.0;python_version<"3.10"
 setuptools>64;python_version>="3.10"
 coverage==5.0.3;python_version<"3.10"
 coverage>=6;python_version>="3.10"
-flaky==3.8.1
+flaky==3.7.0;python_version<"3.6"
+flaky==3.8.1;python_version>="3.6"

 # for integration tests
 # eventlet is pinned until https://github.com/benoitc/gunicorn/pull/2581
diff --git a/setup.cfg b/setup.cfg
index 32977d4..5e74955 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -51,7 +51,9 @@ install_requires =
        contextvars~=2.4;python_version>="3.5" and python_version<"3.7"

 [options.extras_require]
-gunicorn = gunicorn>=19.7.0
+gunicorn =
+       gunicorn>=19.7.0;python_version>"3.6"
+       gunicorn==19.7.0,<21.0;python_version>="3.5" and python_version<"3.8"
 raven = raven>=6.4.0
 celery =
        celery~=4.4;python_version~="3.5.0"
diff --git a/setup.py b/setup.py
index 26576db..5668f57 100644
--- a/setup.py
+++ b/setup.py
@@ -160,7 +160,8 @@ setup(
             'gevent>=20.9.0',
         ],
         gunicorn=[
-            'gunicorn>=19.7.0',
+            'gunicorn>=19.7.0;python_version>"3.6"',
+            'gunicorn>=19.7.0,<21.0;python_version>="3.5" and python_version<"3.8"',
         ],
         pg=[
             'sqlparse>=0.4.2',

I can piggyback on your PR and create a brand new PR including it if you prefer, but it looks simpler to just land this one

Cheers

- bump tox version
- differentiate flaky and gunicorn versions for different python
  versions
@Perfect5th
Copy link
Contributor Author

Thanks again, @verterok ! I've implemented your diff. 🤞

@verterok verterok merged commit 8d5d549 into canonical-ols:master May 29, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tests broken
3 participants