Fix usage of proxy.py in test_proxy_functional #7773

bdraco · 2023-10-31T17:01:05Z

This test was running even if tls in tls was supported without the need to monkey patch because the failures were actually problems with usage of proxy.py.

The test would fail because of problems with usage of proxy.py, and not because tls in tls didn't work. This one turned out to have a lot of side trips to discover why things failed because proxy.py doesn't expect to run inside pytest, and is not easy to get error reporting out of when run this way.

On all systems: https://github.com/abhinavsingh/proxy.py/blob/30574fd0414005dfa8792a6e797023e862bdcf43/proxy/common/utils.py#L226 would raise on warning causing the proxy to die off because of DeprecationWarning: ssl.OP_NO_SSL*/ssl.OP_NO_TLS* options are deprecated
On windows --threadless does not work with proxy.py [Threadless] OSError: [WinError 10038] an operation was attempted on something that is not a socket abhinavsingh/proxy.py#492

codecov · 2023-10-31T17:17:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (25ef450) 96.98% compared to head (c44c7d8) 97.40%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7773      +/-   ##
==========================================
+ Coverage   96.98%   97.40%   +0.41%     
==========================================
  Files         107      107              
  Lines       32213    32207       -6     
  Branches     3744     3743       -1     
==========================================
+ Hits        31243    31371     +128     
+ Misses        756      632     -124     
+ Partials      214      204      -10

Flag	Coverage Δ
CI-GHA	`97.32% <100.00%> (+0.39%)`	⬆️
OS-Linux	`96.99% <100.00%> (+0.06%)`	⬆️
OS-Windows	`95.48% <100.00%> (?)`
OS-macOS	`96.61% <100.00%> (?)`
Py-3.10.11	`95.40% <100.00%> (?)`
Py-3.10.13	`96.79% <100.00%> (+0.02%)`	⬆️
Py-3.11.6	`96.50% <100.00%> (-0.01%)`	⬇️
Py-3.12.0	`96.57% <100.00%> (-0.01%)`	⬇️
Py-3.8.10	`95.37% <100.00%> (?)`
Py-3.8.18	`96.61% <100.00%> (-0.16%)`	⬇️
Py-3.9.13	`95.37% <100.00%> (?)`
Py-3.9.18	`96.75% <100.00%> (-0.03%)`	⬇️
Py-pypy7.3.13	`96.22% <100.00%> (?)`
VM-macos	`96.61% <100.00%> (?)`
VM-ubuntu	`96.99% <100.00%> (+0.06%)`	⬆️
VM-windows	`95.48% <100.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Dreamsorcerer · 2023-10-31T17:48:20Z

Huh, I thought it was meant to be supported on all platforms. @webknjaz Happy to just have this test skipped?

bdraco · 2023-10-31T17:53:08Z

I think its only supported on all platforms after python/cpython#31275

Dreamsorcerer · 2023-11-01T17:16:18Z

I think its only supported on all platforms after python/cpython#31275

That's a PR that appears to have been merged into 3.11+?

webknjaz · 2023-11-03T04:15:33Z

Can it be that the warning mentioned @ #6044 (comment) is a result of a flawed logic there? I think I saw a problem in related code, but never got to fixing it.. Or is this about some other problem?

bdraco · 2023-11-03T04:28:44Z

I think thats a different issue. AFAICT the proxy isn't ssl so its not tls in tls #6044 (comment)

The check might need to be if runtime_has_start_tls and proxy_req.is_ssl(): instead?

aiohttp/aiohttp/connector.py

Line 1248 in 30b7a4e

if runtime_has_start_tls:

bdraco · 2023-11-03T04:29:58Z

I think its only supported on all platforms after python/cpython#31275

That's a PR that appears to have been merged into 3.11+?

The way I read this is the test shouldn't run if that flag is set because tls in tls is supported on the version of python the test is running on, but before 3.11 monkey patching was needed to enable it

…s_macos

Dreamsorcerer · 2023-11-03T11:24:26Z

I think its only supported on all platforms after python/cpython#31275

That's a PR that appears to have been merged into 3.11+?

The way I read this is the test shouldn't run if that flag is set because tls in tls is supported on the version of python the test is running on, but before 3.11 monkey patching was needed to enable it

Right, I hadn't really looked at the test. Doesn't that mean the test should just be skipped on Python 3.11+? And if so, why does it pass on Ubuntu with 3.11+? I'm a bit confused why the behaviour is different on different platforms.

bdraco · 2023-11-03T12:08:50Z

I think its only supported on all platforms after python/cpython#31275

That's a PR that appears to have been merged into 3.11+?

The way I read this is the test shouldn't run if that flag is set because tls in tls is supported on the version of python the test is running on, but before 3.11 monkey patching was needed to enable it

Right, I hadn't really looked at the test. Doesn't that mean the test should just be skipped on Python 3.11+?

I think that's the case.

And if so, why does it pass on Ubuntu with 3.11+? I'm a bit confused why the behaviour is different on different platforms.

I would have expected it to be failing on Ubuntu 3.11. Maybe it's OpenSSL version dependent as well.

I think I'm going to have to build a Ubuntu setup to be sure

bdraco · 2023-11-03T12:09:32Z

Marking as draft until I can put together a Ubuntu env for local testing

…s_macos

bdraco · 2023-11-15T17:15:36Z

Unfortunately I'm getting different behavior on the ubuntu docker container. It might be openssl version dependent or the fact I'm running in a docker host.

Instead I adjusted the other failing test to run if we think tls in tls is supported so we have:

test_secure_https_proxy_absolute_path running if we think tls in tls is supported
test_https_proxy_unsupported_tls_in_tls running if we think tls in tls is unsupported

bdraco · 2023-11-15T17:17:08Z

If test_secure_https_proxy_absolute_path is failing on ubuntu w/3.11+ in the CI, I'll probably have to do some trial and error debugging here since it means there is something specific about the github runner that causes it to fail that I haven't identified yet

Dreamsorcerer · 2023-11-15T17:21:26Z

If test_secure_https_proxy_absolute_path is failing on ubuntu w/3.11+ in the CI

No problem. @webknjaz had a good tool for that, a github action that allows you to login to the server or something. I don't remember what it was called, but a quick search suggests it might be one of these 2: https://github.com/marketplace/actions/a-debugger-for-actions https://github.com/marketplace/actions/debug-via-ssh

bdraco · 2023-11-23T00:56:27Z

saved fix_test_https_proxy_unsupported_tls_in_tls_macos_subproc and trying to go back to the in process but with --threaded to see if it behaves better

…s_macos

…_tls_in_tls_macos' into fix_test_https_proxy_unsupported_tls_in_tls_macos

bdraco · 2023-11-23T12:17:57Z

Looks like this is good to go

The only one still failing reliably (and not related to this PR) is tests/test_web_server.py::test_unsupported_upgrade[pyloop] - TimeoutError on windows and mac only

Dreamsorcerer · 2023-11-23T12:25:37Z

Great, one step closer.

patchback · 2023-11-23T12:25:53Z

Backport to 3.9: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply 4d9fc63 on top of patchback/backports/3.9/4d9fc636dbad45678330f17b7d82b75cf91247bf/pr-7773

Backporting merged PR #7773 into master

Ensure you have a local repo clone of your fork. Unless you cloned it
from the upstream, this would be your origin remote.
Make sure you have an upstream repo added as a remote too. In these
instructions you'll refer to it by the name upstream. If you don't
have it, here's how you can add it:
```
$ git remote add upstream https://github.com/aio-libs/aiohttp.git
```

Ensure you have the latest copy of upstream and prepare a branch
that will hold the backported code:

$ git fetch upstream
$ git checkout -b patchback/backports/3.9/4d9fc636dbad45678330f17b7d82b75cf91247bf/pr-7773 upstream/3.9

Now, cherry-pick PR Fix usage of proxy.py in test_proxy_functional #7773 contents into that branch:
```
$ git cherry-pick -x 4d9fc636dbad45678330f17b7d82b75cf91247bf
```
If it'll yell at you with something like fatal: Commit 4d9fc636dbad45678330f17b7d82b75cf91247bf is a merge but no -m option was given., add -m 1 as follows instead:
```
$ git cherry-pick -m1 -x 4d9fc636dbad45678330f17b7d82b75cf91247bf
```
At this point, you'll probably encounter some merge conflicts. You must
resolve them in to preserve the patch from PR Fix usage of proxy.py in test_proxy_functional #7773 as close to the
original as possible.

Push this branch to your fork on GitHub:

$ git push origin patchback/backports/3.9/4d9fc636dbad45678330f17b7d82b75cf91247bf/pr-7773

Create a PR, ensure that the CI is green. If it's not — update it so that
the tests and any other checks pass. This is it!
Now relax and wait for the maintainers to process your pull request
when they have some cycles to do reviews. Don't worry — they'll tell you if
any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback · 2023-11-23T12:25:58Z

Backport to 3.10: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply 4d9fc63 on top of patchback/backports/3.10/4d9fc636dbad45678330f17b7d82b75cf91247bf/pr-7773

Backporting merged PR #7773 into master

Ensure you have a local repo clone of your fork. Unless you cloned it
from the upstream, this would be your origin remote.
Make sure you have an upstream repo added as a remote too. In these
instructions you'll refer to it by the name upstream. If you don't
have it, here's how you can add it:
```
$ git remote add upstream https://github.com/aio-libs/aiohttp.git
```

Ensure you have the latest copy of upstream and prepare a branch
that will hold the backported code:

$ git fetch upstream
$ git checkout -b patchback/backports/3.10/4d9fc636dbad45678330f17b7d82b75cf91247bf/pr-7773 upstream/3.10

Now, cherry-pick PR Fix usage of proxy.py in test_proxy_functional #7773 contents into that branch:
```
$ git cherry-pick -x 4d9fc636dbad45678330f17b7d82b75cf91247bf
```
If it'll yell at you with something like fatal: Commit 4d9fc636dbad45678330f17b7d82b75cf91247bf is a merge but no -m option was given., add -m 1 as follows instead:
```
$ git cherry-pick -m1 -x 4d9fc636dbad45678330f17b7d82b75cf91247bf
```
At this point, you'll probably encounter some merge conflicts. You must
resolve them in to preserve the patch from PR Fix usage of proxy.py in test_proxy_functional #7773 as close to the
original as possible.

Push this branch to your fork on GitHub:

$ git push origin patchback/backports/3.10/4d9fc636dbad45678330f17b7d82b75cf91247bf/pr-7773

Create a PR, ensure that the CI is green. If it's not — update it so that
the tests and any other checks pass. This is it!
Now relax and wait for the maintainers to process your pull request
when they have some cycles to do reviews. Don't worry — they'll tell you if
any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

bdraco · 2023-11-23T12:35:04Z

I'll get the patch backs done after breakfast and coffee. Up early due to jet lag

(cherry picked from commit 4d9fc63)

webknjaz · 2023-11-28T04:24:20Z

If test_secure_https_proxy_absolute_path is failing on ubuntu w/3.11+ in the CI

No problem. @webknjaz had a good tool for that, a github action that allows you to login to the server or something. I don't remember what it was called, but a quick search suggests it might be one of these 2: github.com/marketplace/actions/a-debugger-for-actions github.com/marketplace/actions/debug-via-ssh

@Dreamsorcerer it's this one https://github.com/marketplace/actions/debugging-with-tmate. I recommend using the options to restrict the user who can connect with it.

webknjaz · 2023-11-28T04:36:18Z

@bdraco thanks for figuring this out! I still remember how exhausting it was to integrate this in the first place…

bdraco · 2023-11-28T19:18:51Z

Cheers! This one was a bit of a rabbit hole. I can only imagine how it was to integrate in the first place 😉

webknjaz · 2023-11-30T16:31:05Z

@bdraco so I just realized that I accidentally downgraded the proxy.py version at the beginning of August via these PRs:

We should probably recover allowing pre-releases there to get the latest fixes early, as that project doesn't release often, despite my extensive contributions to their automation.

#7773 (comment)

bdraco · 2023-12-01T00:49:58Z

I haven't used pip-tools before so I'm not 100% sure if this is the right way to go about it, but #7927 gives it a shot

webknjaz · 2023-12-01T02:03:02Z

@bdraco that's good enough. I don't think we have the exact invocation documented + we don't normally run it manually — dependabot uses pip-tools to upgrade pairs of .in+.txt files. But I do recommend you get familiar with the tool — it's super useful. Eventually, I'll probably take it to the next level in this repo, lockfile-wise.

Fix test_https_proxy_unsupported_tls_in_tls on MacOS

e425cf6

bdraco mentioned this pull request Oct 31, 2023

Fix CONNECT always being treated as having an empty body #7772

Merged

5 tasks

bdraco added the bot:chronographer:skip This PR does not need to include a change note label Oct 31, 2023

bdraco marked this pull request as ready for review October 31, 2023 17:16

bdraco requested a review from asvetlov as a code owner October 31, 2023 17:16

Merge branch 'master' into fix_test_https_proxy_unsupported_tls_in_tl…

032e7eb

…s_macos

bdraco marked this pull request as draft November 3, 2023 12:09

Dreamsorcerer mentioned this pull request Nov 15, 2023

** 3.9 Beta testing ** #7675

Closed

8 tasks

bdraco added 3 commits November 15, 2023 10:57

Merge branch 'master' into fix_test_https_proxy_unsupported_tls_in_tl…

ea6be0b

…s_macos

adjust tests

f95bdf0

adjust tests

c62cd79

bdraco added 6 commits November 15, 2023 11:24

vierfy the proxy is actually running and we can connect

d68b650

legacy

0b1a604

cond

8bde66f

adjust workers

2e5972d

try disabling verify

e467cb9

debug logging

9f97902

bdraco force-pushed the fix_test_https_proxy_unsupported_tls_in_tls_macos branch from 679ac97 to d0d5790 Compare November 23, 2023 00:58

bdraco added 2 commits November 22, 2023 19:15

empty

07f97ba

cleanups

f6881fd

bdraco changed the title ~~Fix test_https_proxy_unsupported_tls_in_tls on systems with the new asyncio tls implementation~~ Fix usage of proxy.py in test_proxy_functional Nov 23, 2023

bdraco added 3 commits November 23, 2023 12:52

Merge branch 'master' into fix_test_https_proxy_unsupported_tls_in_tl…

4964b3e

…s_macos

preen

68a5577

Merge remote-tracking branch 'bdraco/fix_test_https_proxy_unsupported…

c44c7d8

…_tls_in_tls_macos' into fix_test_https_proxy_unsupported_tls_in_tls_macos

bdraco marked this pull request as ready for review November 23, 2023 12:18

Dreamsorcerer merged commit 4d9fc63 into aio-libs:master Nov 23, 2023
30 of 34 checks passed

bdraco added a commit to bdraco/aiohttp that referenced this pull request Nov 23, 2023

Fix usage of proxy.py in test_proxy_functional (aio-libs#7773)

0816f20

(cherry picked from commit 4d9fc63)

bdraco added a commit to bdraco/aiohttp that referenced this pull request Nov 23, 2023

Fix usage of proxy.py in test_proxy_functional (aio-libs#7773)

ee451da

(cherry picked from commit 4d9fc63)

Dreamsorcerer pushed a commit that referenced this pull request Nov 23, 2023

Fix usage of proxy.py in test_proxy_functional (#7773) (#7877)

a5e8aba

(cherry picked from commit 4d9fc63)

Dreamsorcerer pushed a commit that referenced this pull request Nov 23, 2023

Fix usage of proxy.py in test_proxy_functional (#7773) (#7876)

a049701

(cherry picked from commit 4d9fc63)

bdraco deleted the fix_test_https_proxy_unsupported_tls_in_tls_macos branch November 28, 2023 19:18

bdraco added a commit that referenced this pull request Dec 1, 2023

Restore proxy.py to 2.4.4rc4 for testing

62a10ee

#7773 (comment)

bdraco mentioned this pull request Dec 1, 2023

Pin proxy.py to 2.4.4rc4 for testing #7927

Merged

5 tasks

This was referenced Dec 3, 2023

[PR #7927/da2e349a backport][3.9] Pin proxy.py to 2.4.4rc4 for testing #7934

Merged

[PR #7927/da2e349a backport][3.10] Pin proxy.py to 2.4.4rc4 for testing #7935

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix usage of proxy.py in test_proxy_functional #7773

Fix usage of proxy.py in test_proxy_functional #7773

bdraco commented Oct 31, 2023 •

edited

Loading

codecov bot commented Oct 31, 2023 •

edited

Loading

Dreamsorcerer commented Oct 31, 2023

bdraco commented Oct 31, 2023

Dreamsorcerer commented Nov 1, 2023 •

edited

Loading

webknjaz commented Nov 3, 2023

bdraco commented Nov 3, 2023

bdraco commented Nov 3, 2023

Dreamsorcerer commented Nov 3, 2023

bdraco commented Nov 3, 2023 •

edited

Loading

bdraco commented Nov 3, 2023

bdraco commented Nov 15, 2023

bdraco commented Nov 15, 2023

Dreamsorcerer commented Nov 15, 2023 •

edited

Loading

bdraco commented Nov 23, 2023 •

edited

Loading

bdraco commented Nov 23, 2023

Dreamsorcerer commented Nov 23, 2023

patchback bot commented Nov 23, 2023 •

edited

Loading

patchback bot commented Nov 23, 2023 •

edited

Loading

bdraco commented Nov 23, 2023

webknjaz commented Nov 28, 2023

webknjaz commented Nov 28, 2023

bdraco commented Nov 28, 2023

webknjaz commented Nov 30, 2023

bdraco commented Dec 1, 2023

webknjaz commented Dec 1, 2023

Fix usage of proxy.py in test_proxy_functional #7773

Fix usage of proxy.py in test_proxy_functional #7773

Conversation

bdraco commented Oct 31, 2023 • edited Loading

codecov bot commented Oct 31, 2023 • edited Loading

Codecov Report

Dreamsorcerer commented Oct 31, 2023

bdraco commented Oct 31, 2023

Dreamsorcerer commented Nov 1, 2023 • edited Loading

webknjaz commented Nov 3, 2023

bdraco commented Nov 3, 2023

bdraco commented Nov 3, 2023

Dreamsorcerer commented Nov 3, 2023

bdraco commented Nov 3, 2023 • edited Loading

bdraco commented Nov 3, 2023

bdraco commented Nov 15, 2023

bdraco commented Nov 15, 2023

Dreamsorcerer commented Nov 15, 2023 • edited Loading

bdraco commented Nov 23, 2023 • edited Loading

bdraco commented Nov 23, 2023

Dreamsorcerer commented Nov 23, 2023

patchback bot commented Nov 23, 2023 • edited Loading

Backport to 3.9: 💔 cherry-picking failed — conflicts found

Backporting merged PR #7773 into master

patchback bot commented Nov 23, 2023 • edited Loading

Backport to 3.10: 💔 cherry-picking failed — conflicts found

Backporting merged PR #7773 into master

bdraco commented Nov 23, 2023

webknjaz commented Nov 28, 2023

webknjaz commented Nov 28, 2023

bdraco commented Nov 28, 2023

webknjaz commented Nov 30, 2023

bdraco commented Dec 1, 2023

webknjaz commented Dec 1, 2023

bdraco commented Oct 31, 2023 •

edited

Loading

codecov bot commented Oct 31, 2023 •

edited

Loading

Dreamsorcerer commented Nov 1, 2023 •

edited

Loading

bdraco commented Nov 3, 2023 •

edited

Loading

Dreamsorcerer commented Nov 15, 2023 •

edited

Loading

bdraco commented Nov 23, 2023 •

edited

Loading

patchback bot commented Nov 23, 2023 •

edited

Loading

patchback bot commented Nov 23, 2023 •

edited

Loading