chore: increase test coverage and fix temp directory use - #639
Conversation
|
Warning Review limit reached
Your plan currently allows 2 reviews/hour. Refill in 9 minutes and 42 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (15)
📝 WalkthroughWalkthroughAll tests now use explicit per-test ChangesTest Infrastructure Refactoring: Centralized Temporary Directories
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@config/config_test.go`:
- Line 71: The test currently ignores the error returned by
utils.CreateTempFileWithContent when assigning s.filename; update both call
sites (the one setting s.filename and the one on the later line) to capture the
error (e.g., filename, err := utils.CreateTempFileWithContent(...)) and fail the
test immediately on error (use s.T().Fatalf or require.NoError/suite assertions)
so a failed temp-file creation reports the real error instead of causing a
misleading failure later.
In `@test/files_test.go`:
- Line 69: The test currently ignores the error returned by
utils.CreateTempFileWithContent by using the blank identifier; update the call
to capture the error (e.g., filename, err :=
utils.CreateTempFileWithContent(...)) and add a fail-fast assertion
s.Require().NoError(err) immediately after to stop the test if temp file
creation fails; apply the same change for the other CreateTempFileWithContent
invocation in these tests.
In `@utils/tests.go`:
- Line 14: Update the GoDoc for CreateTempFileWithContent to state that the dir
argument must be a valid, non-empty directory and that the function no longer
falls back to os.TempDir(); change any phrasing implying an automatic fallback
to explicitly document the explicit-dir contract, and apply the same wording
update to the other temp helper functions in this file so their comments
accurately reflect the new requirement.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 956c946c-9533-416f-ab62-ea0658bc0502
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
cmd/root_test.gocmd/run/run_test.goconfig/config_test.gointernal/quantitative/runner_test.gorunner/check_base_test.gorunner/check_error_test.gorunner/check_logs_test.gorunner/check_response_test.gorunner/check_status_test.gorunner/run_test.gotest/files_test.goutils/tests.goutils/tests_test.gowaflog/read_test.go
417b709 to
3916d4e
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
waflog/read_test.go (1)
159-167:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd missing
FTWLogLinescleanup in this test path.This test creates
llviaNewFTWLogLinesbut does not registerll.Cleanup()(unlike adjacent tests), which can leak handles and make tests flaky over time.Suggested fix
ll, err := NewFTWLogLines(runnerConfig) s.Require().NoError(err) +s.T().Cleanup(func() { _ = ll.Cleanup() }) ll.WithStartMarker(bytes.ToLower([]byte(startMarkerLine))) ll.WithEndMarker(bytes.ToLower([]byte(endMarkerLine)))🤖 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 `@waflog/read_test.go` around lines 159 - 167, The test instantiates FTWLogLines via NewFTWLogLines (assigned to ll) but never calls ll.Cleanup(), risking resource leaks; after creating ll and asserting NoError (the NewFTWLogLines call), add a deferred cleanup by calling ll.Cleanup() (e.g., defer ll.Cleanup()) so FTWLogLines resources are released for this test path.
🤖 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.
Outside diff comments:
In `@waflog/read_test.go`:
- Around line 159-167: The test instantiates FTWLogLines via NewFTWLogLines
(assigned to ll) but never calls ll.Cleanup(), risking resource leaks; after
creating ll and asserting NoError (the NewFTWLogLines call), add a deferred
cleanup by calling ll.Cleanup() (e.g., defer ll.Cleanup()) so FTWLogLines
resources are released for this test path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d3f21cd4-38f2-44fe-99a4-db2d3573504b
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
cmd/root_test.gocmd/run/run_test.goconfig/config_test.gointernal/quantitative/runner_test.gorunner/check_base_test.gorunner/check_error_test.gorunner/check_logs_test.gorunner/check_response_test.gorunner/check_status_test.gorunner/run_test.gotest/files_test.goutils/tests.goutils/tests_test.gowaflog/read_test.go
✅ Files skipped from review due to trivial changes (1)
- test/files_test.go
3916d4e to
9b8fbff
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
waflog/read_test.go (1)
159-167:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd cleanup for
FTWLogLinesin this test case.After Line 165, this test creates
llbut does not registerll.Cleanup(), unlike adjacent updated tests. That can leak file descriptors across the suite.Suggested fix
ll, err := NewFTWLogLines(runnerConfig) s.Require().NoError(err) + s.T().Cleanup(func() { _ = ll.Cleanup() }) ll.WithStartMarker(bytes.ToLower([]byte(startMarkerLine))) ll.WithEndMarker(bytes.ToLower([]byte(endMarkerLine)))🤖 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 `@waflog/read_test.go` around lines 159 - 167, The test creates an FTWLogLines instance (ll := NewFTWLogLines(...)) but never registers its cleanup, risking leaked descriptors; after successfully creating ll (and after s.Require().NoError(err)), add a defer ll.Cleanup() (or s.T().Cleanup(ll.Cleanup)) so the FTWLogLines.Cleanup() method is invoked when the test finishes.
🤖 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.
Outside diff comments:
In `@waflog/read_test.go`:
- Around line 159-167: The test creates an FTWLogLines instance (ll :=
NewFTWLogLines(...)) but never registers its cleanup, risking leaked
descriptors; after successfully creating ll (and after
s.Require().NoError(err)), add a defer ll.Cleanup() (or
s.T().Cleanup(ll.Cleanup)) so the FTWLogLines.Cleanup() method is invoked when
the test finishes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 89fb2a07-0457-4ab9-9207-b18a586aa353
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
cmd/root_test.gocmd/run/run_test.goconfig/config_test.gointernal/quantitative/runner_test.gorunner/check_base_test.gorunner/check_error_test.gorunner/check_logs_test.gorunner/check_response_test.gorunner/check_status_test.gorunner/run_test.gotest/files_test.goutils/tests.goutils/tests_test.gowaflog/read_test.go
✅ Files skipped from review due to trivial changes (2)
- runner/check_status_test.go
- internal/quantitative/runner_test.go
9b8fbff to
b4432b3
Compare
3 participants
Summary by CodeRabbit
Tests
Chores