mindflayer / python-mocket

a socket mock framework - for all kinds of socket animals, web-clients included
BSD 3-Clause "New" or "Revised" License
279 stars 41 forks source link

Fixes for allowing making a mixture of unmocked and mocked HTTPS requests using aiohttp #213

Closed ento closed 6 months ago

ento commented 6 months ago

This PR attempts to fix a few issues:

In a project I'm working with, a mocketized test would fail if it ran after a test that makes a real HTTPS request. Both use aiohttp. Clearing aiohttp's internal cache that holds an SSLContext object fixed the issue.

I encountered more issues while trying to write a PR for the above:

sonarcloud[bot] commented 6 months ago

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

mindflayer commented 6 months ago

Hi @ento, I see the same pattern here, with the addition of wanting to solve many problems at the same time. Could you please open an issue for each problem you are trying to solve, with a snippet of code that shows what is failing? Many thanks!

ento commented 6 months ago

Sure, opened a few issues:

215: The primary issue I wanted to fix

217: "Too many open files" error

216: Can't use 'no verify' mode, which seemed to be needed to write a test for #215

209 corresponds to the middle two bullet points (Can't make mocked HTTPS requests using aiohttp and Python 3.11)

If it's this repo's general process to require an issue when opening a PR, it'll be useful to have a 'CONTRIBUTING' file or a section in the README that instructs contributors about the process - will save one round of back-and-forth for new contributors and the maintainer alike :)

mindflayer commented 6 months ago

will save one round of back-and-forth for new contributors and the maintainer alike :)

I agree, but please note Mocket is still, and probably it'll ever be, a very small project. When people contribute, it's mostly for opening issues. I don't mind if people reach out to me directly with a PR, the problem here was the amount of issues in a single one. I really wanted to keep track of what we are doing here.

mindflayer commented 6 months ago

Hi @ento, I merged all your commits, with a very small change to get rid of an unnecessary else. The only piece of code I am not 100% sure about is the test using psutil. I've just seen it failing a couple times, still not sure why.

ento commented 6 months ago

Re: issue vs PR - that's fair. Thanks for the new release!

Re: the psutil test - I wanted to test it in a single, short test case that checks something close to the root cause ("no dangling open file descriptor"), but switching it out for a test that checks the symptom ("no error raised after making many requests with a low ulimit value", e.g. the example in #217) might be better, although it'll take a little longer to run.

mindflayer commented 6 months ago

Hi @ento, I am sorry if I haven't made any progress with your feature proposal related to the strict mode. I was counting on working on it as soon as I finished preparing the release with all your bugfixes, but it's being a nightmare at work.

ento commented 5 months ago

@mindflayer No worries, I'm not exactly blocked by the feature proposal getting taken up (I can always use a temporary fork). Your well-being comes first!