fix: add proper kill process to conftest. #249 #278
Conversation
✅ Deploy Preview for robyn canceled.
|
| import pytest | ||
| import subprocess | ||
| import pathlib | ||
| import os | ||
| import time | ||
|
|
||
| def spawn_process(command: List[str]) -> subprocess.Popen[bytes]: | ||
| if sys.platform.startswith("win32"): | ||
| command[0] = "python" |
There was a problem hiding this comment.
Do you think replacing "python" with "py" might fix the failing windows tests?
There was a problem hiding this comment.
- I tried replaced
pythonwithpy- that did not help - Apparently the command
python base_routes.py --devdoes not work in Windows 10 as well because inside dev_event_handler.py we have this:
self.processes.append(subprocess.Popen(["python3", self.file_name], start_new_session=False))
-
Hence the test corresponding to starting up a dev server also fails on Windows
-
I tried adding a conditional inside dev_event_handler.py that replaces
python3withpythonin win32 platform - the dev server starts, but then for some reason it fails to find the modules, including Robyn
There was a problem hiding this comment.
Apparently the command python base_routes.py --dev does not work in Windows 10 as well because inside dev_event_handler.py we have this:
This should be the case as we dropped dev server support for windows. But it shouldn't be failing. Can you share the error log please?
There was a problem hiding this comment.
This is the log file obtained
Traceback relevant to this particular failure:
Traceback (most recent call last):
File "C:\Users\Gaurav Bhattacharjee\OneDrive\Desktop\Projects\rob\robyn\integration_tests\base_routes.py", line 209, in <module>
app.start(port=ROBYN_PORT, url=ROBYN_URL)
File "\\?\C:\Users\Gaurav Bhattacharjee\OneDrive\Desktop\Projects\rob\robyn\robyn\__init__.py", line 163, in start
event_handler.start_server_first_time()
File "\\?\C:\Users\Gaurav Bhattacharjee\OneDrive\Desktop\Projects\rob\robyn\robyn\dev_event_handler.py", line 14, in start_server_first_time
self.processes.append(subprocess.Popen(["python3", self.file_name], start_new_session=False))
File "C:\Users\Gaurav Bhattacharjee\AppData\Local\Programs\Python\Python310\lib\subprocess.py", line 969, in __init__
self._execute_child(args, executable, preexec_fn, close_fds,
File "C:\Users\Gaurav Bhattacharjee\AppData\Local\Programs\Python\Python310\lib\subprocess.py", line 1438, in _execute_child
hp, ht, pid, tid = _winapi.CreateProcess(executable, args,
FileNotFoundError: [WinError 2] The system cannot find the file specified
There was a problem hiding this comment.
This should be the case as we dropped dev server support for windows. But it shouldn't be failing. Can you share the error log please?
I suspect that test_dev_index_request() would still fail anyways, on Windows?
- Trying to run
python base_routes.py --devon Windows 10 does not work (in my machine at least), and throws up errors instead (traceback included in the previous message) test_dev_index_request()tries to run the aforementioned command, expects a server to get created, and then tries makes a GET request to the server. However, the command seems to fail.
There was a problem hiding this comment.
Also, test_global_index_request() is failing in Windows.
def test_global_index_request(global_session):
BASE_URL = "http://0.0.0.0:5000"
res = requests.get(f"{BASE_URL}")
assert(os.getenv("ROBYN_URL") == "0.0.0.0")
assert(res.status_code == 200)
- I feel
BASE_URLshould behttp://127.0.0.1instead? Reference
0.0.0.0 means "all IPv4 addresses on the local machine". It is a meta-address which is non-routable.
If you want to access the server locally, i.e. client on the same machine as the server, either use the IP address 127.0.0.1 (loopback Internet protocol) or the equivalent named domain name (localhost)
r = requests.get('http://127.0.0.1:80')
r = requests.get('http://localhost:80')
- If this change is made,
test_global_index_request()would pass in both Windows and Linux. Whilerequests.get('http://0.0.0.0:8080')is works on Linux, it throwsrequests.exceptions.ConnectionErrorwhen the platform is Windows
There was a problem hiding this comment.
I suspect that test_dev_index_request() would still fail anyways, on Windows?
Trying to run python base_routes.py --dev on Windows 10 does not work (in my machine at least), and throws up errors instead (traceback included in the previous message)
test_dev_index_request() tries to run the aforementioned command, expects a server to get created, and then tries makes a GET request to the server. However, the command seems to fail.
@guilefoylegaurav , you're correct about this. This needs to be fixed. I will create an issue for this. Thank you!
There was a problem hiding this comment.
I feel BASE_URL should be http://127.0.0.1 instead? Reference
This should be "0.0.0.0" only as this test is indeed for a session that is available throughout the IP. We have the local ip adress test already availble.
|
@guilefoylegaurav , I hope all's good. Just following up on this PR. Do let me know if you require any more insights from me. |
|
My apologies @sansyrox for semi-ghosting you. Was very inappropriate of me. Have resumed work on the PR and shall update you shortly. |
|
@guilefoylegaurav , not a problem :D I hope you had an amazing trip ✨ |
|
Thanks for approving this PR @sansyrox
|
|
Hey @guilefoylegaurav ,
Since these tests were not failing before(read a month back), I don't think that we should be disabling them. Instead, we should be focusing on fixing them. Let be in the PRs.
Those are clippy errors. But I don't know how to fix them at the moment. We integrated clippy in our system a few weeks back. So, still getting used to it 😅 . If you have any input, I all ears 😄 |
* fix: fix problem of process persistence post test session * replace process.terminate() with kill() equivalent * scope pytest fixtures to session level
|
My bad, my commits were seriously messed up. I really apologize for this |
Description
This PR fixes #249
Changes
process.terminate()withkill()equivalentDocumentation
No documentation update is required
How I've tested my work
global_index_request()dev_index_request()failed)lsof -i tcp:5000outputs nothing post test session