From 68bea0cceb99ff041e0df08ac18e7b5d4eb34830 Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Tue, 18 Aug 2026 15:49:37 +0200 Subject: [PATCH] fix(tests): correct two independent AttributeErrors in test_task_executor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. `executor.max_concurrent` has never existed. Concurrency is capped by the module-level MAX_CONCURRENT_TASKS constant via asyncio.Semaphore( MAX_CONCURRENT_TASKS) in TaskExecutor.__init__ — confirmed with git log -p across this file's whole history (three commits), the name has always been the module constant, never an instance attribute. test_executor_initialization now asserts MAX_CONCURRENT_TASKS == 5 and the semaphore's initial count, instead of a name the class never had. test_concurrent_task_limit asserted `mock_execute.call_count <= executor.max_concurrent`, which — separately from the AttributeError — was asserting the wrong observable: process_minute() awaits the full batch via asyncio.gather before returning, so by the time the assertion runs all 10 scheduled tasks have executed; the semaphore bounds how many run concurrently mid-flight, not the eventual call_count. Reworded to assert all scheduled tasks still run (call_count == len(tasks)); a concurrency- in-flight assertion would need a task that can be observed mid-execution, which the AsyncMock stand-in does not provide. 2. _run_executor (src/tasks/executor.py) loads the executor module with the __import__ builtin directly (`__import__(module_path, fromlist= ['execute'])`), not importlib.import_module — this repo's own CLAUDE.md documents it as "the thing that will mislead you" about this module. importlib is never imported there, so patch('src.tasks.executor.importlib. import_module') failed at patch setup, before the three test_execute_task_* bodies ran at all. Switched to patch('builtins. __import__', side_effect=...) with a routing function that falls through to the real import for anything other than the target module — verified the call-recording shape empirically first (call('name', fromlist=[...])). Source is unchanged in both cases; both are test-only defects present since this file's initial commit. --- tests/test_task_executor.py | 98 ++++++++++++++++++++++++++----------- 1 file changed, 69 insertions(+), 29 deletions(-) diff --git a/tests/test_task_executor.py b/tests/test_task_executor.py index 50084af..f17ee32 100644 --- a/tests/test_task_executor.py +++ b/tests/test_task_executor.py @@ -4,7 +4,7 @@ Tests for the task executor module. import pytest from unittest.mock import AsyncMock, MagicMock, patch from datetime import datetime -from src.tasks.executor import TaskExecutor +from src.tasks.executor import TaskExecutor, MAX_CONCURRENT_TASKS from src.config import Settings @@ -17,7 +17,13 @@ class TestTaskExecutor: executor = TaskExecutor(test_settings) assert executor.settings == test_settings - assert executor.max_concurrent == 5 + # There is no `max_concurrent` instance attribute — concurrency is + # capped by MAX_CONCURRENT_TASKS (module constant) via + # asyncio.Semaphore(MAX_CONCURRENT_TASKS) in __init__. Verify the + # semaphore was built with that bound instead of asserting an + # attribute name the class has never had. + assert MAX_CONCURRENT_TASKS == 5 + assert executor.semaphore._value == 5 @patch('psycopg2.connect') def test_get_db_connection(self, mock_connect, test_settings: Settings): @@ -87,13 +93,25 @@ class TestTaskExecutor: task = {**sample_task_data, 'id': 1} - with patch.object(executor, 'get_db_connection') as mock_get_conn, \ - patch('src.tasks.executor.importlib.import_module') as mock_import: + # _run_executor (src/tasks/executor.py) loads the executor module with + # the __import__ builtin directly — `module = __import__(module_path, + # fromlist=['execute'])` — not importlib.import_module. This is the + # documented dynamic-loading trap in this repo's own CLAUDE.md + # ("executors are chosen by data, not code"). `importlib` is never + # imported in that module, so patching 'src.tasks.executor.importlib' + # fails at patch setup, before the test body runs at all. + real_import = __import__ - # Mock executor module - mock_executor_module = MagicMock() - mock_executor_module.execute = AsyncMock(return_value="Task completed successfully") - mock_import.return_value = mock_executor_module + def fake_import(name, globals=None, locals=None, fromlist=(), level=0): + if name == 'src.executors.example_executor': + return mock_executor_module + return real_import(name, globals, locals, fromlist, level) + + mock_executor_module = MagicMock() + mock_executor_module.execute = AsyncMock(return_value="Task completed successfully") + + with patch.object(executor, 'get_db_connection') as mock_get_conn, \ + patch('builtins.__import__', side_effect=fake_import) as mock_import: # Mock database mock_conn = MagicMock() @@ -106,7 +124,7 @@ class TestTaskExecutor: await executor.execute_task(task) # Should have imported executor module - mock_import.assert_called_with('src.executors.example_executor') + mock_import.assert_any_call('src.executors.example_executor', fromlist=['execute']) # Should have updated task status assert mock_cursor.execute.call_count >= 2 # Insert execution record + update task @@ -118,13 +136,20 @@ class TestTaskExecutor: task = {**sample_task_data, 'id': 1} - with patch.object(executor, 'get_db_connection') as mock_get_conn, \ - patch('src.tasks.executor.importlib.import_module') as mock_import: + # See test_execute_task_success: _run_executor uses the __import__ + # builtin directly, not importlib.import_module. + real_import = __import__ - # Mock executor that raises error - mock_executor_module = MagicMock() - mock_executor_module.execute = AsyncMock(side_effect=Exception("Task failed")) - mock_import.return_value = mock_executor_module + def fake_import(name, globals=None, locals=None, fromlist=(), level=0): + if name == 'src.executors.example_executor': + return mock_executor_module + return real_import(name, globals, locals, fromlist, level) + + mock_executor_module = MagicMock() + mock_executor_module.execute = AsyncMock(side_effect=Exception("Task failed")) + + with patch.object(executor, 'get_db_connection') as mock_get_conn, \ + patch('builtins.__import__', side_effect=fake_import): # Mock database mock_conn = MagicMock() @@ -149,19 +174,26 @@ class TestTaskExecutor: task = {**sample_task_data, 'id': 1, 'timeout_seconds': 1} + # See test_execute_task_success: _run_executor uses the __import__ + # builtin directly, not importlib.import_module. + import asyncio + real_import = __import__ + + def fake_import(name, globals=None, locals=None, fromlist=(), level=0): + if name == 'src.executors.example_executor': + return mock_executor_module + return real_import(name, globals, locals, fromlist, level) + + mock_executor_module = MagicMock() + + async def slow_execute(*args, **kwargs): + await asyncio.sleep(10) # Longer than timeout + return "Done" + + mock_executor_module.execute = slow_execute + with patch.object(executor, 'get_db_connection') as mock_get_conn, \ - patch('src.tasks.executor.importlib.import_module') as mock_import: - - # Mock executor that takes too long - import asyncio - mock_executor_module = MagicMock() - - async def slow_execute(*args, **kwargs): - await asyncio.sleep(10) # Longer than timeout - return "Done" - - mock_executor_module.execute = slow_execute - mock_import.return_value = mock_executor_module + patch('builtins.__import__', side_effect=fake_import): # Mock database mock_conn = MagicMock() @@ -303,8 +335,16 @@ class TestTaskExecutor: await executor.process_minute() - # Should only execute max_concurrent (5) tasks - assert mock_execute.call_count <= executor.max_concurrent + # process_minute awaits every task in this batch (asyncio.gather) + # before returning, so by the time we're back here all 10 have + # run to completion — the semaphore bounds how many can be + # in flight *concurrently*, not the eventual call_count, which + # this assertion conflated. Kept as a correctness check on the + # total (all scheduled tasks still get executed) since a + # concurrency-in-flight assertion needs a task that can be + # observed mid-execution, which mock_execute (an AsyncMock with + # no delay) does not provide. + assert mock_execute.call_count == len(tasks) @pytest.mark.unit