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

chore(ci): disable external network calls #13142

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

miketheman
Copy link
Member

As some tests spin up a local server, allow clients to communicate with
local addresses only.

Refs: https://pypi.org/project/pytest-socket/

As some tests spin up a local server, allow clients to communicate with
local addresses only.

Refs: https://pypi.org/project/pytest-socket/

Signed-off-by: Mike Fiedler <[email protected]>
Not using the `network` marker as a filter - there are currently 133,
and not all of them are actually making network calls.

Signed-off-by: Mike Fiedler <[email protected]>
@ichard26
Copy link
Member

ichard26 commented Jan 3, 2025

Does this work in subprocesses? pip's functional test suite makes very heavy use of subprocesses (sometimes nested 2-3 times over). Also we use pytest-xdist, so it should work with that as well.

In general, I'm 👍 with the idea. Once the test suite is in better shape, we should enforce test hygiene by requiring tests to opt-in into external network access.

@ichard26 ichard26 added the skip news Does not need a NEWS file entry (eg: trivial changes) label Jan 3, 2025
@ichard26
Copy link
Member

ichard26 commented Jan 3, 2025

Does this work in subprocesses? pip's functional test suite makes very heavy use of subprocesses (sometimes nested 2-3 times over). Also we use pytest-xdist, so it should work with that as well.

Given that the diff only really touches the unit tests, no, it does not work in subprocesses. There are certainly many more being performed in the functional tests. This patch is probably still a good idea for our unit tests though.

@miketheman
Copy link
Member Author

Does this work in subprocesses?

Probably not! That's not something I've encountered needing before. Since the plugin hooks into Python's socket library, any subprocess is likely to not have this applied, as they'll get their own Python context. That might make this a less-valid approach for this test suite.

Also we use pytest-xdist, so it should work with that as well.

pytest-socket isn't currently tested with xdist, nor do the tests it runs test the xdist plugin, but experientially running it for warehouse over 4k+ tests works just fine.

In general, I'm 👍 with the idea. Once the test suite is in better shape, we should enforce test hygiene by requiring tests to opt-in into external network access.

Glad to hear it! This change might not be the correct implementation, given the subprocesses question. I'll mull on how this might apply to subprocesses - but if you've got some ideas, please share!

@ichard26
Copy link
Member

ichard26 commented Mar 6, 2025

Just so people are aware: I do plan on picking up this PR at some point. However, I'll likely do that during my larger effort to refactor the test suite. Enforcing that tests do not access the network unless allowed to would be one piece of the project.

In addition, I'd like to upstream a change so pytest-socket can intercept network requests created by Python subprocesses.

@ichard26
Copy link
Member

So I have a draft PR to add subprocess support to pytest-socket. miketheman/pytest-socket#409

However, this isn't quite enough for pytest-socket to intercept all our network calls as the functional tests actually do their work in their own virtual environment. This isn't something pytest-socket can easily support. To work around this, we need to inject pytest-socket into those environments (and probably delete its .dist-info directory to avoid unwanted interference). This would still need a .pth file so miketheman/pytest-socket#409 is still relevant. Another problem is that I wouldn't want to import all of pytest-socket (and pytest) in every single subprocess as that's slow, so I need to break out the .pth part to be able to function independently of the pytest-related parts of pytest-socket.

In other words, there is still a fair bit of work left here :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news Does not need a NEWS file entry (eg: trivial changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants