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

http: simplify test and fix minor issues #210

Merged
merged 2 commits into from
Jan 10, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 9 additions & 8 deletions contrib/drivers/http/src/jumpstarter_driver_http/driver.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import asyncio
import logging
import os
from dataclasses import dataclass, field
Expand Down Expand Up @@ -170,7 +169,9 @@ async def start(self):
return

self.runner = web.AppRunner(self.app)
await self.runner.setup()
if self.runner:
await self.runner.setup()

site = web.TCPSite(self.runner, self.host, self.port)
await site.start()
logger.info(f"HTTP server started at http://{self.host}:{self.port}")
Expand Down Expand Up @@ -224,17 +225,17 @@ def get_port(self) -> int:
def close(self):
if self.runner:
try:
loop = asyncio.get_running_loop()
if loop.is_running():
asyncio.create_task(self._async_cleanup(loop))
except RuntimeError:
anyio.run(self.runner.cleanup)
if anyio.get_current_task():
anyio.from_thread.run(self._async_cleanup)
except Exception as e:
logger.warning(f"HTTP server cleanup failed synchronously: {e}")
self.runner = None
super().close()

async def _async_cleanup(self, loop):
async def _async_cleanup(self):
try:
if self.runner:
await self.runner.shutdown()
await self.runner.cleanup()
logger.info("HTTP server cleanup completed asynchronously.")
except Exception as e:
Expand Down
84 changes: 64 additions & 20 deletions contrib/drivers/http/src/jumpstarter_driver_http/driver_test.py
Original file line number Diff line number Diff line change
@@ -1,34 +1,78 @@
from pathlib import Path
import os
import uuid
from tempfile import TemporaryDirectory

import aiohttp
import anyio
import pytest
from anyio import create_memory_object_stream

from jumpstarter.common.utils import serve
from jumpstarter.common.resources import ClientStreamResource

from .driver import HttpServer


@pytest.mark.asyncio
async def test_http_server():
with TemporaryDirectory() as source_dir, TemporaryDirectory() as server_dir:
server = HttpServer(root_dir=server_dir)
await server.start()
@pytest.fixture
def anyio_backend():
return "asyncio"

with serve(server) as client:
test_content = b"test content"
source_file_path = Path(source_dir) / "test.txt"
source_file_path.write_bytes(test_content)
@pytest.fixture
def temp_dir():
with TemporaryDirectory() as tmpdir:
yield tmpdir

uploaded_filename_url = client.put_local_file(str(source_file_path))
assert uploaded_filename_url == f"{client.get_url()}/test.txt"
@pytest.fixture
async def server(temp_dir):
server = HttpServer(root_dir=temp_dir)
await server.start()
try:
yield server
finally:
await server.stop()

files = client.list_files()
assert "test.txt" in files
@pytest.mark.anyio
async def test_http_server(server):
filename = "test.txt"
test_content = b"test content"

deleted_filename = client.delete_file("test.txt")
assert deleted_filename == "test.txt"
send_stream, receive_stream = create_memory_object_stream(max_buffer_size=1024)

files_after_deletion = client.list_files()
assert "test.txt" not in files_after_deletion
resource_uuid = uuid.uuid4()
server.resources[resource_uuid] = receive_stream

Copy link
Member

Choose a reason for hiding this comment

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

Hi, nice work,

Although I'd want us to consider if we still want to keep some test through the serve() / write_local_file method too.

The reason to propose it is that we otherwise stop testing the client slide at all, and all the inner interactions of client->exporter

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I figured I'd make it more similar to the tftp driver test and we can perhaps have a seperate test for the client, because in my initial attempt to do the HTTP GET in the context of serve didn't go well because of the mixed async/sync.
However, I will try again, perhaps it's simpler than it looks

Copy link
Member

Choose a reason for hiding this comment

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

Let's do it the way you think makes more sense, I will leave the accepted to this PR, and we can follow up with some client tests, or include it here.

Thank @bennyz !

await server.stop()
resource_handle = ClientStreamResource(uuid=resource_uuid).model_dump(mode="json")

async def send_data():
await send_stream.send(test_content)
await send_stream.aclose()

async with anyio.create_task_group() as tg:
tg.start_soon(send_data)

uploaded_url = await server.put_file(filename, resource_handle)
assert uploaded_url == f"{server.get_url()}/{filename}"

files = server.list_files()
assert filename in files

async with aiohttp.ClientSession() as session:
async with session.get(uploaded_url) as response:
assert response.status == 200
retrieved_content = await response.read()
assert retrieved_content == test_content

deleted_filename = await server.delete_file(filename)
assert deleted_filename == filename

files_after_deletion = server.list_files()
assert filename not in files_after_deletion

def test_http_server_host_config(temp_dir):
custom_host = "192.168.1.1"
server = HttpServer(root_dir=temp_dir, host=custom_host)
assert server.get_host() == custom_host

def test_http_server_root_directory_creation(temp_dir):
new_dir = os.path.join(temp_dir, "new_http_root")
_ = HttpServer(root_dir=new_dir)
assert os.path.exists(new_dir)
Loading