James VeitchandClaude Sonnet 5
3bd857f0ab
Found by running a genuine end-to-end test — real docker-compose ComfyUI harness, real Ollama, real llama-server (router mode), actual workflow graphs submitted via POST /prompt and polled via /history, not direct Python calls to provider classes. Every structured_output=True ChatCompletion call failed with: RuntimeError: asyncio.run() cannot be called from a running event loop Root cause: _run_async() tried asyncio.get_running_loop() first and only spun up an isolated worker thread if that succeeded; otherwise it called asyncio.run(coro) directly on the current thread. Under pytest or a standalone script, get_running_loop() never spuriously raises, so this worked and the worker-thread path was rarely exercised for real. Under ComfyUI's actual async execution engine (Python 3.13, node functions invoked synchronously from inside an already-running event loop), get_running_loop() sometimes raised anyway — routing straight into asyncio.run(coro) on the one thread guaranteed to already have a loop running, reproducing the crash every time. Confirmed via a live-instrumented debug run against the real container: the coroutine executed correctly whenever the worker-thread path was taken (get_running_loop() succeeding), and crashed only via the direct-call fallback. Fixed by removing the conditional entirely: always run the coroutine in a freshly spawned worker thread. A new thread never has an ambient loop, so asyncio.run() is safe there unconditionally, regardless of what the calling thread's loop state actually is. Also traced down a second symptom from the same end-to-end run: non-structured chat() calls sometimes returned an empty response with no error. Verified via direct curl calls to Ollama and llama-server (bypassing comfydv entirely) that this was a real, transient GPU/Metal memory-pressure issue on the test machine (`ggml_metal_synchronize: error: Insufficient Memory`, Ollama's own backend left in a wedged state returning HTTP 200 with an empty zero-value body) — not a comfydv bug. Restarting the backend processes resolved it; no code change was needed for that part. Added tests/test_ollama_provider.py coverage for the specific shape that broke: _run_async called synchronously from within code that is itself already executing inside asyncio.run() — the actual pattern ComfyUI's execution engine uses, which no prior test exercised (every existing call site invoked _run_async from a plain synchronous pytest function with no ambient loop). Re-verified end-to-end after the fix: 5/5 real workflows passing through actual ComfyUI execution — Ollama basic chat, Ollama full lifecycle (Load->Chat->Unload), Ollama structured output, llama.cpp basic chat, llama.cpp structured output — all with real generated content, not mocks. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0132ojafeazQ3ephcBejEWFj
Tests
This directory contains comprehensive pytest tests for the comfydv package.
Running Tests
# Run all tests
uv run pytest
# Run with verbose output
uv run pytest -v
# Run with coverage report
uv run pytest --cov=src/comfydv --cov-report=html
# Run specific test file
uv run pytest tests/test_format_string.py
# Run specific test class
uv run pytest tests/test_format_string.py::TestVariableExtraction
# Run specific test
uv run pytest tests/test_format_string.py::TestVariableExtraction::test_extract_simple_single_variable
# Run tests matching a pattern
uv run pytest -k "jinja2"
Coverage reports are available in htmlcov/index.html after running with --cov-report=html.
Test Coverage
Current: 75% (203 statements total, 51 missed)
format_string.py: 78% coverage__init__.py: 100% coveragecircuit_breaker.py: 68% coveragerandom_choice.py: 60% coverageutils.py: 75% coverage
Test Structure
conftest.py
Contains pytest configuration, fixtures, and mocks for ComfyUI dependencies:
- Mock ComfyUI modules (
comfy,server,folder_paths,aiohttp) - Uses
pytest_configurehook to install mocks before test collection - Provides
format_string_classfixture usingimportlibto directly load module - Fixtures for test data and class instances
- Pytest hooks for early mock installation
test_format_string.py
Comprehensive test suite for the FormatString node with 47 tests organized into classes:
- TestVariableExtraction (12 tests): Variable extraction from templates
- TestSimpleFormatting (4 tests): Python format string rendering
- TestJinja2Formatting (5 tests): Jinja2 template rendering
- TestDynamicOutputs (6 tests): Dynamic output configuration
- TestOutputConsistency (3 tests): Outputs match RETURN_TYPES/RETURN_NAMES
- TestInputTypes (4 tests): INPUT_TYPES method validation
- TestIsChanged (5 tests): Cache invalidation logic
- TestStatePersistence (2 tests): State saving/loading
- TestEdgeCases (4 tests): Error handling and edge cases
- TestTimeNowFunction (2 tests): time_now utility function
Mocking Strategy
The tests use importlib.util to directly load the format_string.py module, bypassing the package __init__.py which has ComfyUI dependencies. This allows testing without ComfyUI installation while maintaining the root __init__.py for ComfyUI extension discovery.
Writing Tests
Example test
def test_new_feature(self, format_string_class, sample_data):
"""Test that new feature works correctly."""
result = format_string_class.some_method(sample_data["name"])
assert result == expected_value
Available fixtures
format_string_class: Fresh FormatString class with reset statesample_templates: Dictionary of sample template stringssample_data: Dictionary of sample data for templates
Test naming convention
Use descriptive names: test_<what>_<condition>_<expected>
Troubleshooting
Debugging failed tests
# Run with verbose output
uv run pytest -vv --tb=long
# Run with pdb debugger
uv run pytest --pdb
Test isolation
Each test is independent. The format_string_class fixture provides a fresh instance with reset state.
Future Improvements
- Add integration tests with actual ComfyUI installation
- Add JavaScript tests for frontend functionality
- Increase coverage for circuit_breaker and random_choice nodes
- Add property-based testing with Hypothesis