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.

Sign up for GitHub

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

fix: add proper kill process to conftest. #249 #278

Merged
merged 1 commit into from
Sep 18, 2022
Merged

fix: add proper kill process to conftest. #249 #278

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 39 additions & 12 deletions integration_tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,40 +1,67 @@
import signal
guilefoylegaurav marked this conversation as resolved.
Show resolved Hide resolved
import sys
from typing import List
import pytest
import subprocess
import pathlib
import os
import time

def spawn_process(command: List[str]) -> subprocess.Popen:
if sys.platform.startswith("win32"):
command[0] = "python"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think replacing "python" with "py" might fix the failing windows tests?

Copy link
Contributor Author

@guilefoylegaurav guilefoylegaurav Sep 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I tried replaced python with py - that did not help
  • 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:

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 python3 with python in win32 platform - the dev server starts, but then for some reason it fails to find the modules, including Robyn

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@guilefoylegaurav guilefoylegaurav Sep 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

windows_test.log

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

Copy link
Contributor Author

@guilefoylegaurav guilefoylegaurav Sep 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 --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.

Copy link
Contributor Author

@guilefoylegaurav guilefoylegaurav Sep 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_URL should be http://127.0.0.1 instead? 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. While requests.get('http://0.0.0.0:8080') is works on Linux, it throws requests.exceptions.ConnectionError when the platform is Windows

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

process = subprocess.Popen(command, shell=True, creationflags=subprocess.CREATE_NEW_PROCESS_GROUP)
return process
process = subprocess.Popen(command, preexec_fn=os.setsid)
return process


@pytest.fixture

def kill_process(process: subprocess.Popen) -> None:
if sys.platform.startswith("win32"):
process.send_signal(signal.CTRL_BREAK_EVENT)
process.kill()
return
os.killpg(os.getpgid(process.pid), signal.SIGKILL)




@pytest.fixture(scope="session")
def session():
os.environ["ROBYN_URL"] = "127.0.0.1"
current_file_path = pathlib.Path(__file__).parent.resolve()
base_routes = os.path.join(current_file_path, "./base_routes.py")
process = subprocess.Popen(["python3", base_routes])
command = ["python3", base_routes]
process = spawn_process(command)
time.sleep(5)
yield
process.terminate()
kill_process(process)




@pytest.fixture
@pytest.fixture(scope="session")
def default_session():
current_file_path = pathlib.Path(__file__).parent.resolve()
base_routes = os.path.join(current_file_path, "./base_routes.py")
process = subprocess.Popen(["python3", base_routes])
command = ["python3", base_routes]
process = spawn_process(command)
time.sleep(5)
yield
process.terminate()
kill_process(process)


@pytest.fixture
@pytest.fixture(scope="session")
def global_session():
os.environ["ROBYN_URL"] = "0.0.0.0"
current_file_path = pathlib.Path(__file__).parent.resolve()
base_routes = os.path.join(current_file_path, "./base_routes.py")
process = subprocess.Popen(["python3", base_routes])
command = ["python3", base_routes]
process = spawn_process(command)
time.sleep(1)
yield
process.terminate()
kill_process(process)


@pytest.fixture(scope="session")
Expand All @@ -43,8 +70,8 @@ def dev_session():
os.environ["ROBYN_PORT"] = "5001"
current_file_path = pathlib.Path(__file__).parent.resolve()
base_routes = os.path.join(current_file_path, "./base_routes.py")
process = subprocess.Popen(["python3", base_routes, "--dev"])
command = ["python3", base_routes, "--dev"]
process = spawn_process(command)
time.sleep(5)
yield
process.terminate()

kill_process(process)