test: add unit tests for helpers (#815)#847
test: add unit tests for helpers (#815)#847planetf1 merged 2 commits intogenerative-computing:mainfrom
Conversation
Add 63 tests across openai_compatible_helpers, async_helpers, and server_type covering critical gaps in pure functions used by every OpenAI-compatible backend. - extract_model_tool_requests: 9 tests (single/multi/unknown/null args/malformed JSON/mixed known+unknown) - chat_completion_delta_merge: 7 tests (text/reasoning/tool assembly with index merging/force-separate/stop_reason) - message_to_openai_message: 5 tests (text/images/empty images list/roles) - messages_to_docs: 4 tests (empty/single/optional fields/across messages) - send_to_queue: 5 tests (coroutine/async-iter/direct-iter/exception propagation) - wait_for_all_mots: 2 tests (resolve all/empty list) - get_current_event_loop: 2 tests (running/no-loop) - ClientCache: 6 tests (put/get/evict-LRU/refresh/overwrite/size) - _server_type: 7 parametrized tests (localhost variants/openai/ unknown/malformed URL) Fixes generative-computing#815
|
The PR description has been updated. Please fill out the template for your PR to be reviewed. |
Remove tests that either test language builtins (OrderedDict.get, len()), duplicate existing coverage (assistant_role vs text_only), or can't meaningfully exercise the function under test without a real pending thunk (wait_for_all_mots).
ajbozarth
left a comment
ajbozarth
left a comment
There was a problem hiding this comment.
LGTM, just a small nit from Claude
ran uv run pytest test/helpers/:
62 passed, 1 warning in 7.47s| result = message_to_openai_message(msg) | ||
| assert len(result["content"]) == 3 # 1 text + 2 images | ||
|
|
||
| def test_with_empty_images_list(self): |
There was a problem hiding this comment.
Small nit from Claude:
No real caller passes images=[] — all production code uses None for "no images". This test covers a reachable-but-unreachable-in-practice code path caused by message_to_openai_message using is not None instead of a truthiness check. Fine to keep for coverage, but worth noting: if images=[] should ever be treated the same as images=None, the fix is a one-liner in the source (if msg.images: instead of if msg.images is not None:).
There was a problem hiding this comment.
thanks for pointer. I've not made a code change though currently.
f1d5cf4
3 participants
Misc PR
Type of PR
Description
41 new unit tests across
async_helpers,openai_compatible_helpers, andserver_type. Uses real objects throughout (no mocks). Follows project conventions (autounitmarker,asyncio_mode="auto").Coverage
openai_compatible_helpersasync_helpersserver_typeopenai_compatible_helpersextract_model_tool_requests(9),chat_completion_delta_merge(7),message_to_openai_message(4),messages_to_docs(4)async_helperssend_to_queue(5),get_current_event_loop(2),ClientCache(3)server_type_server_type(7 parametrised)Remaining
async_helpersgap (10%):wait_for_all_motsand_run_async_in_threadare one-liners that can't be meaningfully unit-tested without over-engineering. Both are implicitly exercised by e2e tests (parallel inference, sync-to-async bridging).Testing