test: add unit tests for chat_completions translation module#234
Conversation
|
Missing Signed-off-by: ddf4e41. All commits require sign-off (via |
|
PR too large: 1211 lines added (limit: 750, excludes Cargo files, tests, docs, examples, and benchmarks). Please split into smaller PRs. Add |
Cover ResponseContext construction/defaults, request translation edge cases (empty instructions, content part collapse, function call serialization, tool definitions, tool_choice variants, text format, reasoning), and response translation edge cases (finish reasons, completed_at, empty/null content, refusals, array content parts, logprobs attachment, usage token details, service tier fallbacks, tool call output items, full response resource shape). Resolves praxis-proxy#231 Signed-off-by: Sébastien Han <seb@redhat.com>
leseb
force-pushed
the
leseb/fix-issue-231
branch
from
ddf4e41 to
44acca4
Compare
| // ------------------------------------------------------------------------- | ||
|
|
||
| #[test] | ||
| fn empty_instructions_do_not_produce_system_message() { |
There was a problem hiding this comment.
duplicates simple_inputs_map_or_drop_cleanly
| "input": [{"foo": "bar"}] | ||
| })); | ||
|
|
||
| assert!(error.contains("unknown")); |
There was a problem hiding this comment.
both UnsupportedInputItemType and UnsupportedContentPartType could be caught by "unknown", intentional?
praxis-bot
left a comment
praxis-bot
left a comment
There was a problem hiding this comment.
PR Review: test: add unit tests for chat_completions translation module
One-line summary: Thorough, well-structured test suite with two convention adjustments needed.
Overall assessment
This PR adds 64 well-organized unit tests covering ResponseContext construction, request translation edge cases (messages, content parts, function calls, tools, tool_choice, text format, reasoning), and response translation paths (finish reasons, usage, service tier, tool calls, refusals, logprobs, resource shape). The tests correctly verify both happy-path and edge-case behavior, use no doc comments on test functions, avoid inline comments in test bodies, and consistently use to_owned() for &str conversions. Two convention issues should be fixed.
Findings
| # | Severity | Location | Finding |
|---|---|---|---|
| 1 | Medium | mod.rs:675 |
Separator comments use 73 dashes instead of 77 |
| 2 | Medium | mod.rs:1204 |
Test utility functions placed mid-module instead of at the end |
| ); | ||
| } | ||
|
|
||
| // ------------------------------------------------------------------------- |
There was a problem hiding this comment.
[Medium] Separator comments inside the test module use 73 dashes (total width 80 including indent), but the convention requires 77 dashes regardless of nesting depth. The implementation file (chat_completions.rs) uses 77-dash separators at the top level. All 19 separator blocks in this PR should be updated. Correct form:
// ---------------------------------------------------------------------------
// ResponseContext construction
// ---------------------------------------------------------------------------| // Response translation: finish reasons | ||
| // ------------------------------------------------------------------------- | ||
|
|
||
| fn make_response_context(request: &Value) -> super::chat_completions::ResponseContext { |
There was a problem hiding this comment.
[Medium] make_response_context and simple_chat_response (line 1209) are test utility functions placed between test sections. The project convention requires test utility functions to go last inside mod tests, after all #[test] functions. Move both to the end of the module, just before the closing }.
|
As this PR resolves #231, I can't see any test case for;
|
3 participants
Summary
apis/src/openai/translation/chat_completions.rs, covering all major code pathsResponseContextconstruction, request translation (messages, content parts, tools, tool_choice, text format, reasoning), and response translation (finish reasons, usage, service tier, tool calls, refusals, logprobs, full resource shape)Test areas covered
ResponseContextconstruction & defaultsResolves #231
Test plan
cargo test --lib -p praxis-ai-apis -- translation::tests)