CodeRabbit Generated Unit Tests: Add generated unit tests#351
CodeRabbit Generated Unit Tests: Add generated unit tests#351coderabbitai[bot] wants to merge 1 commit intomainfrom
Conversation
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
github-actions
Bot
added
the
size/S
Greptile SummaryThis PR adds generated unit tests across four files:
Confidence Score: 2/5Not safe to merge: Three blocking issues in internal/client/response_test.go requires significant fixes before merge.
|
| Filename | Overview |
|---|---|
| internal/client/response_test.go | New test file that fails to compile: SaveResponse is called with 2 args everywhere but the function requires 3 (fio fileio.FileIO was added); additionally HandleResponse tests with nil FileIO would panic, and one assertion about error wrapping contradicts production behavior. |
| cmd/config/config_test.go | New test file covering config init/show/remove paths; tests look structurally correct and use the real command plumbing through cmdutil.TestFactory. |
| internal/validate/atomicwrite_test.go | New test file covering AtomicWrite and AtomicWriteFromReader; correctness, permission, overwrite, and concurrency cases are well-structured with minor nit on the concurrent-write data generation. |
| internal/validate/path_test.go | New test file covering SafeOutputPath, SafeLocalFlagPath, SafeInputPath, and SafeEnvDirPath with thorough table-driven tests for path traversal, control characters, and symlink edge cases. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Test calls SaveResponse] -->|2 args: resp, path| B{Compile check}
B -->|FAIL: needs 3 args| C[Compilation error\nfio FileIO missing]
D[Test calls HandleResponse\nno FileIO set] --> E{Binary/file path?}
E -->|Yes| F[opts.FileIO is nil]
F --> G[fio.Save called on nil]
G --> H[Runtime PANIC]
I[TestParseJSONResponse_EmptyBody] --> J[dec.Decode on empty body]
J --> K[returns io.EOF]
K --> L[fmt.Errorf with percent-w wraps io.EOF]
L --> M[errors.Is == true]
M --> N[assertion inverted → t.Fatal fires]
Comments Outside Diff (2)
-
internal/client/response_test.go, line 196-202 (link)Nil
FileIOcauses runtime panic inHandleResponsetestsResponseOptions.FileIOis left asnilin everyHandleResponsetest that triggers a file save (binary auto-save,OutputPathset, etc.). InsideHandleResponse, the call chain reachesSaveResponse(opts.FileIO, ...)→fio.Save(...), which panics becausefiois a nil interface. All tests on lines 194–267 and 305–338 that exercise the binary or output-path paths would panic even after the compilation error is fixed. Each such test must supply a concreteFileIOimplementation inResponseOptions.FileIO. -
internal/validate/atomicwrite_test.go, line 129-134 (link)Concurrency test data generation: rune arithmetic produces single-digit ASCII only
string(rune('0'+n%10))produces the single-character string "0"–"9" as a rune-to-string conversion, which is valid Go. However, forn >= 10, the rune arithmetic silently wraps via%10, so goroutines 10–19 write the same digit strings as 0–9. The test also does not check for a valid JSON structure in the written file — it only checkslen(got) > 0. These are minor quality points for a concurrency smoke-test, but worth noting for reviewers.
Reviews (1): Last reviewed commit: "CodeRabbit Generated Unit Tests: Add gen..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| resp := newApiResp(body, map[string]string{"Content-Type": "application/octet-stream"}) | ||
|
|
||
| meta, err := SaveResponse(&localfileio.LocalFileIO{}, resp, "test_output.bin") | ||
| meta, err := SaveResponse(resp, "test_output.bin") |
There was a problem hiding this comment.
SaveResponse
SaveResponse was refactored to accept a fileio.FileIO as its first parameter (func SaveResponse(fio fileio.FileIO, resp *larkcore.ApiResp, outputPath string)), but every direct call in this test file passes only two arguments. This causes a compilation error that prevents the entire client package test binary from building.
The same issue occurs on lines 183, 403, 419, 439, 462, and all other direct SaveResponse calls in this file. A fileio.FileIO implementation (e.g. a real localfileio.New() or a test double) must be supplied as the first argument.
| meta, err := SaveResponse(resp, "test_output.bin") | |
| meta, err := SaveResponse(localfileio.New(), resp, "test_output.bin") |
| func TestParseJSONResponse_EmptyBody_ErrorIsNotWrappedIOEOF(t *testing.T) { | ||
| // GIVEN: an empty response body | ||
| resp := newApiResp([]byte{}, map[string]string{"Content-Type": "application/json"}) | ||
|
|
||
| // WHEN: ParseJSONResponse tries to parse the empty body | ||
| _, err := ParseJSONResponse(resp) | ||
|
|
||
| // THEN: error is returned | ||
| if err == nil { | ||
| t.Fatal("expected error for empty body") | ||
| } | ||
|
|
||
| // THEN: error is NOT a wrapped io.EOF (uses fmt.Errorf with verb v not w, | ||
| // so errors.Is chain is broken). This verifies the PR change. | ||
| if errors.Is(err, io.EOF) { | ||
| t.Fatal("error should not wrap io.EOF: format was changed from percent-w to percent-v") | ||
| } | ||
|
|
||
| // THEN: error message still contains useful context | ||
| if !strings.Contains(err.Error(), "response parse error") { | ||
| t.Errorf("expected 'response parse error' in error message, got: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
io.EOF, so the test always fails
The test asserts errors.Is(err, io.EOF) must be false, accompanied by a comment claiming the format verb is %v. But ParseJSONResponse currently uses %w in fmt.Errorf("response parse error: %w (body: %s)", err, ...). For an empty body, json.Decoder.Decode returns io.EOF, which is wrapped by %w, so errors.Is(err, io.EOF) is true — the t.Fatal fires every time. Either the assertion should be inverted to confirm wrapping, or the comment/test needs to be aligned with the actual format verb used in production.
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@abfe9a48aaffd81b40d0c2668538b273a0f57a4d🧩 Skill updatenpx skills add larksuite/cli#coderabbitai/utg/d5d31f0 -y -g |
|
|
3 participants
Unit test generation was requested by @huangxincola.
The following files were modified:
cmd/config/config_test.gointernal/client/response_test.gointernal/validate/atomicwrite_test.gointernal/validate/path_test.go