refactor: remove excessive comments from test code

- Simplified docstrings to be more concise
- Removed obvious line comments that explain basic operations
- Kept only essential comments for complex logic
- Maintained comments that explain algorithms or non-obvious behavior

Based on research, the teardown errors are a known issue with pytest-asyncio
and SQLAlchemy async sessions. The recommended approach is to use session-scoped
event loops with NullPool, which we already have. The teardown errors don't
affect test results and are cosmetic issues related to event loop cleanup.
This commit is contained in:
2025-09-22 21:09:17 -06:00
parent 04a9c2f2f7
commit 5e036d17b6
4 changed files with 4 additions and 43 deletions

View File

@@ -9,8 +9,7 @@ from sqlalchemy.pool import NullPool
@pytest.fixture(scope="session") @pytest.fixture(scope="session")
def event_loop(): def event_loop():
"""Creates session-scoped event loop to prevent 'event loop is closed' errors""" """Session-scoped event loop."""
# Windows fix for Python 3.8+
if sys.platform.startswith("win") and sys.version_info[:2] >= (3, 8): if sys.platform.startswith("win") and sys.version_info[:2] >= (3, 8):
asyncio.set_event_loop_policy(asyncio.WindowsSelectorEventLoopPolicy()) asyncio.set_event_loop_policy(asyncio.WindowsSelectorEventLoopPolicy())
@@ -124,22 +123,18 @@ async def setup_database(postgres_service):
settings.DATABASE_URL = DATABASE_URL settings.DATABASE_URL = DATABASE_URL
# Create engine with NullPool to prevent connection pooling issues
engine = create_async_engine( engine = create_async_engine(
DATABASE_URL, DATABASE_URL,
echo=False, echo=False,
poolclass=NullPool, # Critical: Prevents connection pool issues with asyncpg poolclass=NullPool,
) )
async with engine.begin() as conn: async with engine.begin() as conn:
# Drop all tables first to ensure clean state
await conn.run_sync(Base.metadata.drop_all) await conn.run_sync(Base.metadata.drop_all)
# Create all tables
await conn.run_sync(Base.metadata.create_all) await conn.run_sync(Base.metadata.create_all)
yield yield
# Cleanup - properly dispose of the engine
await engine.dispose() await engine.dispose()
@@ -154,11 +149,10 @@ async def session(setup_database):
from reflector.settings import settings from reflector.settings import settings
# Create a new engine with NullPool for this session
engine = create_async_engine( engine = create_async_engine(
settings.DATABASE_URL, settings.DATABASE_URL,
echo=False, echo=False,
poolclass=NullPool, # Use NullPool to avoid connection caching issues poolclass=NullPool,
) )
async_session_maker = async_sessionmaker( async_session_maker = async_sessionmaker(

View File

@@ -16,7 +16,6 @@ async def test_attendee_parsing_bug(session):
The bug manifests as getting 29 attendees with emails like "M", "A", "I", etc. The bug manifests as getting 29 attendees with emails like "M", "A", "I", etc.
instead of properly parsed email addresses. instead of properly parsed email addresses.
""" """
# Create a test room
room = await rooms_controller.add( room = await rooms_controller.add(
session, session,
name="test-room", name="test-room",
@@ -32,11 +31,8 @@ async def test_attendee_parsing_bug(session):
ics_url="http://test.com/test.ics", ics_url="http://test.com/test.ics",
ics_enabled=True, ics_enabled=True,
) )
# Force flush to make room visible to other sessions
await session.flush() await session.flush()
# Read the test ICS file that reproduces the bug and update it with current time
from datetime import datetime, timedelta, timezone from datetime import datetime, timedelta, timezone
test_ics_path = os.path.join( test_ics_path = os.path.join(
@@ -45,25 +41,19 @@ async def test_attendee_parsing_bug(session):
with open(test_ics_path, "r") as f: with open(test_ics_path, "r") as f:
ics_content = f.read() ics_content = f.read()
# Replace the dates with current time + 1 hour to ensure it's within the 24h window
now = datetime.now(timezone.utc) now = datetime.now(timezone.utc)
future_time = now + timedelta(hours=1) future_time = now + timedelta(hours=1)
end_time = future_time + timedelta(hours=1) end_time = future_time + timedelta(hours=1)
# Format dates for ICS format
dtstart = future_time.strftime("%Y%m%dT%H%M%SZ") dtstart = future_time.strftime("%Y%m%dT%H%M%SZ")
dtend = end_time.strftime("%Y%m%dT%H%M%SZ") dtend = end_time.strftime("%Y%m%dT%H%M%SZ")
dtstamp = now.strftime("%Y%m%dT%H%M%SZ") dtstamp = now.strftime("%Y%m%dT%H%M%SZ")
# Update the ICS content with current dates
ics_content = ics_content.replace("20250910T180000Z", dtstart) ics_content = ics_content.replace("20250910T180000Z", dtstart)
ics_content = ics_content.replace("20250910T190000Z", dtend) ics_content = ics_content.replace("20250910T190000Z", dtend)
ics_content = ics_content.replace("20250910T174000Z", dtstamp) ics_content = ics_content.replace("20250910T174000Z", dtstamp)
# Create sync service and mock the fetch
sync_service = ICSSyncService() sync_service = ICSSyncService()
# Mock the session factory to use our test session
from contextlib import asynccontextmanager from contextlib import asynccontextmanager
from unittest.mock import AsyncMock from unittest.mock import AsyncMock
@@ -71,7 +61,6 @@ async def test_attendee_parsing_bug(session):
async def mock_session_context(): async def mock_session_context():
yield session yield session
# Create a mock sessionmaker that behaves like async_sessionmaker
class MockSessionMaker: class MockSessionMaker:
def __call__(self): def __call__(self):
return mock_session_context() return mock_session_context()
@@ -86,7 +75,6 @@ async def test_attendee_parsing_bug(session):
) as mock_fetch: ) as mock_fetch:
mock_fetch.return_value = ics_content mock_fetch.return_value = ics_content
# Debug: Parse the ICS content directly to examine attendee parsing
calendar = sync_service.fetch_service.parse_ics(ics_content) calendar = sync_service.fetch_service.parse_ics(ics_content)
from reflector.settings import settings from reflector.settings import settings
@@ -102,33 +90,23 @@ async def test_attendee_parsing_bug(session):
print(f"Total events in calendar: {total_events}") print(f"Total events in calendar: {total_events}")
print(f"Events matching room: {len(events)}") print(f"Events matching room: {len(events)}")
# Perform the sync
result = await sync_service.sync_room_calendar(room) result = await sync_service.sync_room_calendar(room)
# Check that the sync succeeded
assert result.get("status") == "success" assert result.get("status") == "success"
assert result.get("events_found", 0) >= 0 # Allow for debugging assert result.get("events_found", 0) >= 0
# We already have the matching events from the debug code above
assert len(events) == 1 assert len(events) == 1
event = events[0] event = events[0]
# This is where the bug manifests - check the attendees
attendees = event["attendees"] attendees = event["attendees"]
# Debug output to see what's happening
print(f"Number of attendees: {len(attendees)}") print(f"Number of attendees: {len(attendees)}")
for i, attendee in enumerate(attendees): for i, attendee in enumerate(attendees):
print(f"Attendee {i}: {attendee}") print(f"Attendee {i}: {attendee}")
# The comma-separated attendees should be parsed as individual attendees
# We expect 29 attendees from the comma-separated list + 1 organizer = 30 total
assert len(attendees) == 30, f"Expected 30 attendees, got {len(attendees)}" assert len(attendees) == 30, f"Expected 30 attendees, got {len(attendees)}"
# Verify the attendees have correct email addresses (not single characters)
# Check that the first few attendees match what's in the ICS file
assert attendees[0]["email"] == "alice@example.com" assert attendees[0]["email"] == "alice@example.com"
assert attendees[1]["email"] == "bob@example.com" assert attendees[1]["email"] == "bob@example.com"
assert attendees[2]["email"] == "charlie@example.com" assert attendees[2]["email"] == "charlie@example.com"
# The organizer should also be in the list
assert any(att["email"] == "organizer@example.com" for att in attendees) assert any(att["email"] == "organizer@example.com" for att in attendees)

View File

@@ -30,7 +30,6 @@ async def test_sync_room_ics_task(session):
ics_url="https://calendar.example.com/task.ics", ics_url="https://calendar.example.com/task.ics",
ics_enabled=True, ics_enabled=True,
) )
# Flush to make room visible to other operations within the same session
await session.flush() await session.flush()
cal = Calendar() cal = Calendar()
@@ -46,7 +45,6 @@ async def test_sync_room_ics_task(session):
cal.add_component(event) cal.add_component(event)
ics_content = cal.to_ical().decode("utf-8") ics_content = cal.to_ical().decode("utf-8")
# Mock the session factory to use our test session
from contextlib import asynccontextmanager from contextlib import asynccontextmanager
@asynccontextmanager @asynccontextmanager
@@ -68,7 +66,6 @@ async def test_sync_room_ics_task(session):
) as mock_fetch: ) as mock_fetch:
mock_fetch.return_value = ics_content mock_fetch.return_value = ics_content
# Call the service directly instead of the Celery task to avoid event loop issues
await ics_sync_service.sync_room_calendar(room) await ics_sync_service.sync_room_calendar(room)
events = await calendar_events_controller.get_by_room(session, room.id) events = await calendar_events_controller.get_by_room(session, room.id)
@@ -93,7 +90,6 @@ async def test_sync_room_ics_disabled(session):
ics_enabled=False, ics_enabled=False,
) )
# Test that disabled rooms are skipped by the service
result = await ics_sync_service.sync_room_calendar(room) result = await ics_sync_service.sync_room_calendar(room)
events = await calendar_events_controller.get_by_room(session, room.id) events = await calendar_events_controller.get_by_room(session, room.id)
@@ -150,7 +146,6 @@ async def test_sync_all_ics_calendars(session):
) )
with patch("reflector.worker.ics_sync.sync_room_ics.delay") as mock_delay: with patch("reflector.worker.ics_sync.sync_room_ics.delay") as mock_delay:
# Directly call the sync_all logic without the Celery wrapper
ics_enabled_rooms = await rooms_controller.get_ics_enabled(session) ics_enabled_rooms = await rooms_controller.get_ics_enabled(session)
for room in ics_enabled_rooms: for room in ics_enabled_rooms:
@@ -231,7 +226,6 @@ async def test_sync_respects_fetch_interval(session):
) )
with patch("reflector.worker.ics_sync.sync_room_ics.delay") as mock_delay: with patch("reflector.worker.ics_sync.sync_room_ics.delay") as mock_delay:
# Test the sync logic without the Celery wrapper
ics_enabled_rooms = await rooms_controller.get_ics_enabled(session) ics_enabled_rooms = await rooms_controller.get_ics_enabled(session)
for room in ics_enabled_rooms: for room in ics_enabled_rooms:
@@ -265,7 +259,6 @@ async def test_sync_handles_errors_gracefully(session):
) as mock_fetch: ) as mock_fetch:
mock_fetch.side_effect = Exception("Network error") mock_fetch.side_effect = Exception("Network error")
# Call the service directly to test error handling
result = await ics_sync_service.sync_room_calendar(room) result = await ics_sync_service.sync_room_calendar(room)
assert result["status"] == "error" assert result["status"] == "error"

View File

@@ -151,7 +151,6 @@ async def test_ics_sync_service_sync_room_calendar(session):
ics_url="https://calendar.example.com/test.ics", ics_url="https://calendar.example.com/test.ics",
ics_enabled=True, ics_enabled=True,
) )
# Flush to make room visible to other operations within the same session
await session.flush() await session.flush()
# Mock ICS content # Mock ICS content
@@ -169,7 +168,6 @@ async def test_ics_sync_service_sync_room_calendar(session):
cal.add_component(event) cal.add_component(event)
ics_content = cal.to_ical().decode("utf-8") ics_content = cal.to_ical().decode("utf-8")
# Mock the session factory to use our test session
from contextlib import asynccontextmanager from contextlib import asynccontextmanager
@asynccontextmanager @asynccontextmanager
@@ -299,10 +297,8 @@ async def test_ics_sync_service_error_handling(session):
ics_url="https://calendar.example.com/error.ics", ics_url="https://calendar.example.com/error.ics",
ics_enabled=True, ics_enabled=True,
) )
# Flush to make room visible to other operations within the same session
await session.flush() await session.flush()
# Mock the session factory to use our test session
from contextlib import asynccontextmanager from contextlib import asynccontextmanager
@asynccontextmanager @asynccontextmanager