chore: close test gaps and add release docs for v0.6.0 - #54
Conversation
Test coverage improvements (73.8% → 77.0%): - GitLab: resolve mode (ResolvePreviousReviews, ListDiscussions, ResolveDiscussion), multi-line (line_range, suggestion offset), description update, MR versions, put() helper, pagination - GitHub: multi-line (start_line), description update (PATCH), single-line suggestion contract - Model: parseSummaryJSON (valid, malformed, fenced, prose-wrapped, missing fields), validateSummary edge cases - Config: loadGitHubCIEnv (platform, project ID, MR IID from ref) Release documentation: - CHANGELOG.md (Keep a Changelog format, v0.1 through v0.6.0) - ROADMAP.md updated to v0.6.0 with shipped features checked off - .code-reviewer.example.yaml: added missing config fields
|
Warning Review limit reached
Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR adds configuration and release documentation updates, GitHub CI and review-client tests, GitLab review-client coverage, and summary parsing and validation tests. ChangesReview coverage and release documentation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
internal/model/summarize_test.go (1)
9-13: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the decoded values, not just validation side effects.
A successful
parseSummaryJSONresult already guarantees these fields are non-empty viavalidateSummary. A broken field mapping or fixed placeholder result would still pass. Add expected values per successful fixture and compare them.Proposed test strengthening
tests := []struct { - name string - input string - wantErr string + name string + input string + wantErr string + wantTitle string + wantClassification string + wantRiskLevel string }{ { - name: "valid_json", - input: `...`, + name: "valid_json", + input: `...`, + wantTitle: "Fix auth", + wantClassification: "fix", + wantRiskLevel: "low", }, } ... - if res.Title == "" || res.Classification == "" || res.RiskLevel == "" { - t.Errorf("missing required fields in successful parse: %+v", res) + if res.Title != tt.wantTitle || + res.Classification != tt.wantClassification || + res.RiskLevel != tt.wantRiskLevel { + t.Errorf("unexpected result: %+v", res) }Also applies to: 70-74
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/model/summarize_test.go` around lines 9 - 13, Strengthen the successful parseSummaryJSON test cases by adding expected decoded field values to each fixture and comparing the returned summary against them. Update the test table and assertions in the parseSummaryJSON test so validation alone cannot pass with incorrect field mappings or placeholder values; retain the existing wantErr checks for failure cases.internal/github/client_test.go (1)
850-854: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert request methods and paths in the HTTP handlers.
These handlers return successful responses without validating the endpoint. The tests could pass even if the client targets the wrong URL; add method and path assertions before decoding or responding.
Also applies to: 870-876, 897-903, 938-944
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/github/client_test.go` around lines 850 - 854, Update the HTTP handlers in TestGetDescription_Success and the additionally affected test handlers to assert the expected request method and URL path before writing or decoding the response. Use each test’s intended endpoint, preserving the existing response behavior after validation.internal/gitlab/client_test.go (1)
1459-1552: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider collapsing these two near-duplicate draft-note tests into one table-driven test.
TestSubmitReview_MultiLineDraftNoteandTestSubmitReview_SingleLineSuggestionshare identical mock-server wiring (notes/draft_notes/bulk_publish handlers) and request shape; only theReviewCommentfields and the expected suggestion substring differ. As per path instructions,**/*_test.gofiles should "Prefer table-driven tests," and this pair is a natural candidate: a sharedhttptesthandler factory plus a[]struct{ comment vcs.ReviewComment; wantSubstrings []string }table would remove the duplicated switch-case boilerplate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/gitlab/client_test.go` around lines 1459 - 1552, Collapse TestSubmitReview_MultiLineDraftNote and TestSubmitReview_SingleLineSuggestion into one table-driven test with shared httptest server setup and request execution. Define cases containing the ReviewComment and expected suggestion substring, reuse the common notes, draft_notes, and bulk_publish handler behavior, and retain the existing line_range assertion for the multi-line case.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/config/config_test.go`:
- Around line 974-980: Update the test setup around Load() to clear the
competing CI_PROJECT_ID environment variable with t.Setenv("CI_PROJECT_ID", "")
before loading configuration, ensuring GitHub detection is selected regardless
of the runner environment. Keep the existing GitHub variables and assertions
unchanged, and preserve t.Setenv-based cleanup and race-safe test isolation.
In `@internal/github/client_test.go`:
- Line 858: Update the test function containing the GetDescription call and the
other client calls at the referenced locations to use a cancellable test context
instead of context.Background(). Create and manage the context with the test’s
cleanup lifecycle, then pass it to each client method so cancellation and
deadlines propagate through the HTTP requests.
- Around line 965-971: Add an assertion in the single-line comment test
alongside the existing Line and StartLine checks to verify that comment.Side
equals "RIGHT", preserving coverage of the expected single-line serialization
mapping.
In `@internal/model/summarize_test.go`:
- Line 8: Add Go doc comments immediately before TestParseSummaryJSON and
TestValidateSummary in internal/model/summarize_test.go, with each comment
beginning with its corresponding function name and briefly describing the test.
- Line 54: Update parseSummaryJSON to use the shared JSON extraction and parsing
strategy already used by parseReviewJSON instead of calling json.Unmarshal
directly, while preserving SummaryResult validation and the existing
clean/parse/validation sequence.
---
Nitpick comments:
In `@internal/github/client_test.go`:
- Around line 850-854: Update the HTTP handlers in TestGetDescription_Success
and the additionally affected test handlers to assert the expected request
method and URL path before writing or decoding the response. Use each test’s
intended endpoint, preserving the existing response behavior after validation.
In `@internal/gitlab/client_test.go`:
- Around line 1459-1552: Collapse TestSubmitReview_MultiLineDraftNote and
TestSubmitReview_SingleLineSuggestion into one table-driven test with shared
httptest server setup and request execution. Define cases containing the
ReviewComment and expected suggestion substring, reuse the common notes,
draft_notes, and bulk_publish handler behavior, and retain the existing
line_range assertion for the multi-line case.
In `@internal/model/summarize_test.go`:
- Around line 9-13: Strengthen the successful parseSummaryJSON test cases by
adding expected decoded field values to each fixture and comparing the returned
summary against them. Update the test table and assertions in the
parseSummaryJSON test so validation alone cannot pass with incorrect field
mappings or placeholder values; retain the existing wantErr checks for failure
cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b7af2027-9782-43fd-870f-ff78076bc86f
📒 Files selected for processing (7)
.code-reviewer.example.yamlCHANGELOG.mdROADMAP.mdinternal/config/config_test.gointernal/github/client_test.gointernal/gitlab/client_test.gointernal/model/summarize_test.go
| defer srv.Close() | ||
|
|
||
| client := NewClient(srv.URL, "token") | ||
| desc, err := client.GetDescription(context.Background(), "owner/repo", "1") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use cancellable test contexts instead of context.Background().
All four client calls in this test function use a non-cancellable root context. Use a cancellable test context here so request cancellation and deadlines can propagate through the HTTP requests.
Also applies to: 883, 916, 957.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/github/client_test.go` at line 858, Update the test function
containing the GetDescription call and the other client calls at the referenced
locations to use a cancellable test context instead of context.Background().
Create and manage the context with the test’s cleanup lifecycle, then pass it to
each client method so cancellation and deadlines propagate through the HTTP
requests.
| "testing" | ||
| ) | ||
|
|
||
| func TestParseSummaryJSON(t *testing.T) { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add doc comments to exported test functions.
internal/model/summarize_test.go#L8-L8: add aTestParseSummaryJSON ...doc comment.internal/model/summarize_test.go#L79-L79: add aTestValidateSummary ...doc comment.
As per coding guidelines, “All exported Go functions and types MUST have doc comments.”
📍 Affects 1 file
internal/model/summarize_test.go#L8-L8(this comment)internal/model/summarize_test.go#L79-L79
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/model/summarize_test.go` at line 8, Add Go doc comments immediately
before TestParseSummaryJSON and TestValidateSummary in
internal/model/summarize_test.go, with each comment beginning with its
corresponding function name and briefly describing the test.
Source: Coding guidelines
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| res, err := parseSummaryJSON(tt.input) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the supported parser's signature and existing callers before migration.
ast-grep outline internal/model --items all --type function --match 'parseReviewJSON|parseSummaryJSON'
rg -n -C 4 '\bfunc\s+parseReviewJSON\b|\bparseReviewJSON\s*\(' internal/modelRepository: OpticDiff/code-reviewer
Length of output: 10984
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files 'internal/model/*.go'
echo "== summarize.go relevant section =="
if [ -f internal/model/summarize.go ]; then
nl -ba internal/model/summarize.go | sed -n '1,140p'
fi
echo "== provider.go parseReviewJSON implementation =="
nl -ba internal/model/provider.go | sed -n '145,215p'
echo "== direct json.Unmarshal calls in internal/model =="
rg -n -C 3 'json\.Unmarshal' internal/model || true
echo "== summary tests around parseSummaryJSON =="
nl -ba internal/model/summarize_test.go | sed -n '1,90p'Repository: OpticDiff/code-reviewer
Length of output: 761
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== provider.go parseReviewJSON implementation =="
sed -n '145,215p' internal/model/provider.go
echo "== summarize.go summary parser implementation =="
sed -n '1,140p' internal/model/summarize.go
echo "== direct json.Unmarshal calls in internal/model =="
grep -Rn -C 3 'json\.Unmarshal' internal/model || true
echo "== summarize_test.go around parseSummaryJSON calls =="
sed -n '1,110p' internal/model/summarize_test.goRepository: OpticDiff/code-reviewer
Length of output: 9155
Route summary model-output parsing through the shared JSON parser.
parseSummaryJSON() in internal/model/summarize.go:48-62 still uses raw json.Unmarshal() on model output. Move the JSON extraction/unmarshalling into a shared helper or reuse parseReviewJSON()’s strategy so SummaryResult parsing stays covered by the clean/parse/validation sequence.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/model/summarize_test.go` at line 54, Update parseSummaryJSON to use
the shared JSON extraction and parsing strategy already used by parseReviewJSON
instead of calling json.Unmarshal directly, while preserving SummaryResult
validation and the existing clean/parse/validation sequence.
Source: Coding guidelines
- Clear CI_PROJECT_ID in GitHub CI env test to prevent env leak - Assert Side=RIGHT on single-line review comment test - Strengthen parseSummaryJSON: assert decoded field values per fixture
1 participant
Pre-release hardening for v0.6.0. Addresses every E9-flagged gap.
Test Coverage: 73.8% → 77.0%
GitLab client (+14.6%)
TestResolvePreviousReviewsTestResolveDiscussion_Success{"resolved":true}TestListDiscussions_PaginationTestCreateDiscussion_MultiLineline_rangewith start/end refsTestSubmitReview_MultiLineDraftNotesuggestion:-N+0offsetTestSubmitReview_SingleLineSuggestionsuggestion:-0+0TestGetDescription_SuccessTestSetDescription_SuccessTestGetMRVersions_SuccessGitHub client (+4.9%)
TestGetDescription_SuccessTestSetDescription_SuccessTestSubmitReview_MultiLinestart_line+ line=EndLine contractTestSubmitReview_SingleLinestart_lineomittedModel (+3.7%)
TestParseSummaryJSONTestValidateSummaryConfig (+2.9%)
TestLoadGitHubCIEnvRelease Docs
CHANGELOG.md— Keep a Changelog format, v0.1 through v0.6.0ROADMAP.md— Updated to v0.6.0, shipped features checked.code-reviewer.example.yaml— Added missing config fieldsSummary by CodeRabbit
Documentation
Quality Improvements