feat(llm): enhance error logging and add comprehensive unit tests#3033
feat(llm): enhance error logging and add comprehensive unit tests#3033
Conversation
Improve LLM client error handling by including response body in error logs for better debugging of API failures. Add extensive unit tests using MockWebServer to verify chat and streaming functionality for both OpenAI and Gemini clients. Changes: - Add response body to error log messages in GeminiLlmClient and OpenAiLlmClient - Add mockwebserver test dependency to pom.xml - Add comprehensive tests for chat() and streamChat() methods - Test coverage includes success cases, error responses, edge cases, and streaming Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR enhances error handling and testing infrastructure for LLM client implementations by adding error response body logging and comprehensive unit tests.
Changes:
- Enhanced error logging in
OpenAiLlmClientandGeminiLlmClientto include response bodies for better debugging - Added extensive unit tests for both clients covering success cases, error responses, and edge cases
- Added
mockwebservertest dependency to support HTTP mocking in tests
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/org/codelibs/fess/llm/openai/OpenAiLlmClient.java | Added response body extraction and logging in error handling for chat() and streamChat() methods |
| src/main/java/org/codelibs/fess/llm/gemini/GeminiLlmClient.java | Added response body extraction and logging in error handling for chat() and streamChat() methods |
| src/test/java/org/codelibs/fess/llm/openai/OpenAiLlmClientTest.java | Added comprehensive test suite using MockWebServer for chat and streaming functionality |
| src/test/java/org/codelibs/fess/llm/gemini/GeminiLlmClientTest.java | Added comprehensive test suite using MockWebServer for chat and streaming functionality |
| pom.xml | Added mockwebserver test dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| String errorBody = ""; | ||
| if (response.body() != null) { | ||
| try { | ||
| errorBody = response.body().string(); | ||
| } catch (final IOException e) { | ||
| // ignore | ||
| } | ||
| } | ||
| logger.warn("OpenAI API error. url={}, statusCode={}, message={}, body={}", url, response.code(), response.message(), | ||
| errorBody); |
There was a problem hiding this comment.
The variable name errorBody should follow Java naming conventions with camelCase starting with lowercase. While it already does, consider a more descriptive name like responseBody since this represents the full response body, not just error content.
| String errorBody = ""; | |
| if (response.body() != null) { | |
| try { | |
| errorBody = response.body().string(); | |
| } catch (final IOException e) { | |
| // ignore | |
| } | |
| } | |
| logger.warn("OpenAI API error. url={}, statusCode={}, message={}, body={}", url, response.code(), response.message(), | |
| errorBody); | |
| String responseBody = ""; | |
| if (response.body() != null) { | |
| try { | |
| responseBody = response.body().string(); | |
| } catch (final IOException e) { | |
| // ignore | |
| } | |
| } | |
| logger.warn("OpenAI API error. url={}, statusCode={}, message={}, body={}", url, response.code(), response.message(), | |
| responseBody); |
| String errorBody = ""; | ||
| if (response.body() != null) { | ||
| try { | ||
| errorBody = response.body().string(); | ||
| } catch (final IOException e) { | ||
| // ignore | ||
| } | ||
| } | ||
| logger.warn("Gemini API error. url={}, statusCode={}, message={}, body={}", url, response.code(), response.message(), | ||
| errorBody); |
There was a problem hiding this comment.
The variable name errorBody should be more descriptive. Consider renaming to responseBody since it represents the full response body, not just error content.
| String errorBody = ""; | |
| if (response.body() != null) { | |
| try { | |
| errorBody = response.body().string(); | |
| } catch (final IOException e) { | |
| // ignore | |
| } | |
| } | |
| logger.warn("Gemini API error. url={}, statusCode={}, message={}, body={}", url, response.code(), response.message(), | |
| errorBody); | |
| String responseBody = ""; | |
| if (response.body() != null) { | |
| try { | |
| responseBody = response.body().string(); | |
| } catch (final IOException e) { | |
| // ignore | |
| } | |
| } | |
| logger.warn("Gemini API error. url={}, statusCode={}, message={}, body={}", url, response.code(), response.message(), | |
| responseBody); |
| private void setupClientForMockServer() { | ||
| final String baseUrl = mockServer.url("").toString(); | ||
| // Remove trailing slash | ||
| final String apiUrl = baseUrl.endsWith("/") ? baseUrl.substring(0, baseUrl.length() - 1) : baseUrl; |
There was a problem hiding this comment.
The trailing slash removal logic is duplicated in both test files. Consider extracting this to a shared utility method to reduce duplication and improve maintainability.
| private void setupClientForMockServer() { | |
| final String baseUrl = mockServer.url("").toString(); | |
| // Remove trailing slash | |
| final String apiUrl = baseUrl.endsWith("/") ? baseUrl.substring(0, baseUrl.length() - 1) : baseUrl; | |
| private String removeTrailingSlash(final String url) { | |
| if (url == null) { | |
| return null; | |
| } | |
| return url.endsWith("/") ? url.substring(0, url.length() - 1) : url; | |
| } | |
| private void setupClientForMockServer() { | |
| final String baseUrl = mockServer.url("").toString(); | |
| // Remove trailing slash | |
| final String apiUrl = removeTrailingSlash(baseUrl); |
| private void setupClientForMockServer() { | ||
| final String baseUrl = mockServer.url("").toString(); | ||
| // Remove trailing slash | ||
| final String apiUrl = baseUrl.endsWith("/") ? baseUrl.substring(0, baseUrl.length() - 1) : baseUrl; |
There was a problem hiding this comment.
The trailing slash removal logic is duplicated from OpenAiLlmClientTest. Consider extracting this to a shared utility method to reduce duplication and improve maintainability.
| private void setupClientForMockServer() { | |
| final String baseUrl = mockServer.url("").toString(); | |
| // Remove trailing slash | |
| final String apiUrl = baseUrl.endsWith("/") ? baseUrl.substring(0, baseUrl.length() - 1) : baseUrl; | |
| private String normalizeBaseUrl(final String url) { | |
| if (url == null || url.isEmpty()) { | |
| return url; | |
| } | |
| return url.endsWith("/") ? url.substring(0, url.length() - 1) : url; | |
| } | |
| private void setupClientForMockServer() { | |
| final String baseUrl = mockServer.url("").toString(); | |
| final String apiUrl = normalizeBaseUrl(baseUrl); |
Summary
Changes Made
chat()andstreamChat()methodschat()andstreamChat()methodsmockwebservertest dependency for HTTP mocking in unit testschat()method: success cases, error responses (401, 429, 500, 503), empty candidates, null finish reason, partial usage metadatastreamChat()method: success cases, multiple chunks, error responses, finish reasons, malformed JSON handlingchat()method: success cases, error responses (401, 429, 500, 503), empty choices, null finish reason, partial usagestreamChat()method: success cases, multiple chunks, error responses, SSE format handling, malformed JSON handlingTesting
🤖 Generated with Claude Code