-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
pythonTestAdapter fails if sockets are disabled for tests #22383
Comments
Hello! What is the reason you want to disable sockets? The sockets are how we send information on the run status back to the Python extension and gather test results. Therefore it would require some design changes to fix this. |
@eleanorjboyd It looks like https://pypi.org/project/pytest-socket/ plugin specifically used for disabling sockets for testing. |
@eleanorjboyd As @karthiknadig mentioned, sockets are disabled by the pytest-socket plugin for pytest, which is a good practice to ensure the code under test is not opening unexpected network connections. For instance, when testing code that might exercise a third-party API, you want to make sure your test suite isn't REALLY sending requests to that API—especially if your test suite runs in CI. As I mention in my reproduction repo, I suspect this might have a straightforward cause and possibly a straightforward fix. The I think the test adapter script shouldn't run in the same process as the test suite. Consider spinning off a subprocess, perhaps with asyncio.subprocess. |
Just for reference, here are some of the Python testing libraries that patch
It's also relatively common to just patch |
Hello! Thank you for the information as it provides important data about this problem. Would named pipes be a sufficient work-around here or do you anticipate that not working for the libraries you listed above? |
@eleanorjboyd As in os.mkfifo? I'm not very experienced with it. With some quick testing, it does appear to work. N.B.: The docs effective say it requires a POSIX system. Specifically:
No idea if that affects non-WSL Windows or vscode.dev. |
@johnchristopherjones On the python side we won't be using On the python side, it is enough to use |
@johnchristopherjones, we will look to work on this change soon but cannot promise a timeline. @karthiknadig or I will keep you in the loop as we merge changes related to switching to named pipes. Thanks! |
Hi @johnchristopherjones 👋 Current workaroundDowngrade your version of |
@magnasilvar 👋 Thanks for the tip! However, I think that's a separate bug. It's related insofar as that bug prevents a partial workaround using That's not a workaround I want to use because I also specifically don't want connections to services running on localhost, like databases. Allowing localhost would allow the test runner to work, much like turning off your firewall or setting a DMZ allows friends to connect to your Minecraft server—you're just circumventing the firewall. The underlying problem is that the test runner runtime environment is being controlled by the test suite environment. Consider the following use-cases:
Relatedly, we closed #21645, regarding |
Any updates here. I'm wondering what significant changes have been made to pytest vscode extension in the last month to cause issues like this, and who knows what else. |
hi @shanewazabbas, are you asking about changes in the last month since you are just not seeing this issue? We have made limited changes this month (I have been out of office) but we did increase the number of people in the new testing rewrite so that could see the problem. If you opt out of the rewrite does that fix it? just add this to your user settings.json Thanks |
hey @eleanorjboyd:
Ahhh that explains it. When I saw below logs, I thought python extension updated but didn't see any release updates so I was confused:
Btw something to think about regardless of the update, is performance of test discovery for large monorepos (like really large). Cause it reruns that collection logic on save, which I dont think is practical in all cases. Unless theres work being done to make collection discovery reuse previous results? Unclear on the roadmap for the new changes. |
Hi, thanks for the follow up. A few notes:
this error message is accidental, the extension is working fine I put this error message in an incorrect spot. The fix is in insiders and will be out with our next stable release in about a week. secondly, you can update the settings so it no longer re-runs discovery on save- would this help in your case? The idea of reusing discovery results is a complicated one so we haven't approached that topic yet. One thing we did consider was only running discovery on the file you save instead of discovery on the whole workspace. Would something like this be another solution to your issue? Thanks |
Good to know thanks.
Ah ok, I'll look into that.
Yeah that would definitely solve it for me 👍 , if that is possible with pytest. Cause it would greatly help with my dev time. |
Hello! Yes it should be added in the user settings.json file not the one for the given workspace. It is still working for me if I add it there. Let me know if that is still not working, thanks |
Ok I finally found it. For those like me you should Go to the menu Preferences / Settings. Search the "extensions" section, find "Python", search Opt Out Form (with blank else you won't find it) and edit in settings.json. Then add the "PythonTestAdapter" at the right place. And the tests are now ok. Very not easy to find... |
As others have noted, disabling all network activity in a unit test is critical for repeatability (google for "hermetic unit test"). And that's what pytest-socket does for you. Doing
did indeed work around the problem for me, but it really shouldn't be necessary. I get that you want to talk to the mother ship to manage A/B experiments, but it seems like that could (and should) be done outside the test setup context. |
@roysmith This issue has nothing to do with communication with experimentation server. The new test adapter uses socket connection to communicate VS Code UI to show live test status, and it fails to connect with VS Code to provide that info. The |
OK, I accept that I incorrectly analyzed the root cause, but the gist is still the same; it shouldn't be necessary to do this to these kinds of tests working. |
Hello! We have now switched our extension to using named pipes! This should resolve the socket disabled issue. If someone could try it out and let me know if it works that would be extremely helpful. To do so you just need to be on the latest version of the python insiders extension and a vscode version >= 1.89.0-20240415- the easiest way to do this is download vscode insiders and get the pre-release of the python extension. Thanks so much and happy coding! |
@eleanorjboyd Thanks for working on this. I just installed Code - Insiders and can confirm that my tests fail with Python extension v2024.4.1 but pass with v2024.5.11101014 (pre-release), so all looks good. |
Great! Thanks so much for confirming! |
I'm now receiving errors about the named pipe connection:
|
Hi @strawgate, what machine type are you working on and are you mocking out / removing any environment variables? What I can see from your logs is the extension knows the named pipe because it lists it here:
@karthiknadig thoughts? |
I suspect that the pytest_socket pligin is getting loaded before vscode_pytest. @strawgate One thing you could try is to disable Unix Domain socket from being blocked by the pytest_socket plugin. It may have a optional setting to do this. @eleanorjboyd We can try and match to copied (monkey patched) sockets, but I think the real solution would be this: #23279 It has more details on what is going on and why we need to use this communication channel, and the limitations of other modes of communication. |
I'm just observing this as a bystander, but I do see that pytest-socket does allow for that:
I also see you can configure it to only allow connections to (for example) localhost, which might be another possible workaround:
|
I'm using a Windows laptop using vscode remote ssh to an Ubuntu 22.04 host and a devcontainer on that Ubuntu host. I'm not mocking any environment variables and this is the repository I'm running tests from https://github.com/legrego/homeassistant-elasticsearch/tree/main/custom_components |
I did try this with no luck |
This comment was marked as off-topic.
This comment was marked as off-topic.
@phil-bell The issue is with @strawgate We have a tracking issue to change the communication channel from UDS to fifo that should address the problem. For your case the |
Type: Bug
Behaviour
Expected vs. Actual
Actual
When using
pytest
withpytest_socket
to disable network connections in test, the test suite fails. Test Results displays the following error:Meanwhile, the test suite passes with on the terminal with:
The test suite can be made to pass by opting out of the
pythonTestAdapter
experiment in Settings:Expected
The test runner succeeds like the CLI.
Steps to reproduce:
I've created a minimal reproduction repo:
https://github.com/johnchristopherjones/vscode-python-test-adapter-socket
The really short version is:
pytest_socket
@pytest.mark.disable_socket
:Or, alternatively:
Install
pytest_socket
Add this to your
pyproject.toml
:Try to run any tests. (⌘; A)
Diagnostic data
python.languageServer
setting: PylanceOutput for
Python
in thePython Test Log
panelUser Settings
Extension version: 2023.19.13031019
VS Code version: Code 1.83.1 (f1b07bd25dfad64b0167beb15359ae573aecd2cc, 2023-10-10T23:57:32.750Z)
OS version: Darwin arm64 22.6.0
Modes:
The text was updated successfully, but these errors were encountered: