chore(tests): parallelize pkg/config tests (partial — t.Setenv users stay serial)#175
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, 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 the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR adds test concurrency annotations to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
Poem
🚥 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)
Review rate limit: 0/5 reviews remaining, refill in 3 minutes and 8 seconds. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/config/config_test.go (1)
58-60: Handle working-directory setup/teardown errors explicitly.Line 58 and Line 60 currently ignore errors; if either fails, the test can leak state or mask root cause.
Suggested hardening
- orig, _ := os.Getwd() + orig, err := os.Getwd() + require.NoError(t, err) require.NoError(t, os.Chdir(dir)) - t.Cleanup(func() { os.Chdir(orig) }) + t.Cleanup(func() { require.NoError(t, os.Chdir(orig)) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/config_test.go` around lines 58 - 60, The test currently ignores errors from os.Getwd() and the cleanup os.Chdir(orig); update the setup to check and fail the test on error (use require.NoError or t.Fatalf) for os.Getwd() and the initial os.Chdir(dir), and change the t.Cleanup closure to capture and handle the error returned by os.Chdir(orig) (call require.NoError or log/fatal inside the closure) so any failure to restore the working directory is surfaced; reference the os.Getwd, os.Chdir and t.Cleanup calls in pkg/config/config_test.go when making these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/config/config_test.go`:
- Around line 58-60: The test currently ignores errors from os.Getwd() and the
cleanup os.Chdir(orig); update the setup to check and fail the test on error
(use require.NoError or t.Fatalf) for os.Getwd() and the initial os.Chdir(dir),
and change the t.Cleanup closure to capture and handle the error returned by
os.Chdir(orig) (call require.NoError or log/fatal inside the closure) so any
failure to restore the working directory is surfaced; reference the os.Getwd,
os.Chdir and t.Cleanup calls in pkg/config/config_test.go when making these
changes.
Per CodeRabbit nitpick on PR #175: TestLoad_MissingDefaultFile_NoError ignored errors from os.Getwd() and the t.Cleanup os.Chdir(orig). If either fails the test could leak working-directory state into other tests or mask the root cause of a failure. Wrap both in require.NoError.
cristim
added
triaged
…stay serial) Per-test classification (10 parallel + 8 documented-serial): Parallel (10) — pure, no env/chdir/global mutation: - TestLoad_Defaults - TestLoad_ExplicitMissingFile_Error - TestLoad_YAMLFile - TestLoad_InvalidYAML_Error - TestLoad_DryRunAndPurchaseConflict - TestLoad_PurchaseFlagSetsDryRunFalse - TestLoad_UnknownCloud_Error - TestLoad_NegativeThreshold_Error - TestValidate_EmptyEnabledClouds_Error - TestLoad_YAMLRoundtrip Serial (8) — Go's t.Setenv / os.Chdir mutate process-wide state and panic if combined with t.Parallel(). Each gets an inline NOT-parallel comment so future contributors don't accidentally add t.Parallel(): - TestLoad_MissingDefaultFile_NoError (os.Chdir) - TestLoad_EnvOverridesYAML (t.Setenv) - TestLoad_EnvVars (t.Setenv x11) - TestLoad_InvalidEnvVar_Error (t.Setenv) - TestLoad_InvalidDurationEnvVar_Error (t.Setenv) - TestLoad_FlagOverridesEnvAndYAML (t.Setenv) - TestLoad_CUDLYCONFIGEnv (t.Setenv) - TestLoad_ArgPathTakesPrecedenceOverCUDLYCONFIG (t.Setenv) Verified: `go test -race -count=2 ./config/...` (from pkg/) passes cleanly twice. Refs #58
Per CodeRabbit nitpick on PR #175: TestLoad_MissingDefaultFile_NoError ignored errors from os.Getwd() and the t.Cleanup os.Chdir(orig). If either fails the test could leak working-directory state into other tests or mask the root cause of a failure. Wrap both in require.NoError.
cristim
force-pushed
the
chore/tparallel-pkg-config
branch
from
e33cbcf to
e963008
Compare
|
Rebased onto current |
|
✅ Actions performedReview triggered.
|
1 participant
Summary
Continues the
t.Parallel()adoption sweep from #58. This PR targetspkg/config/and uses the standard Go testing mixed-mode pattern: 10 pure tests are parallelised, while 8 tests that mutate process-wide state (t.Setenv/os.Chdir) intentionally remain serial — Go'st.Setenvpanics if combined witht.Parallel(), so making them parallel is not a "fix" but a regression.To prevent future contributors from accidentally adding
t.Parallel()to the serial tests, each one carries an inline// NOT parallel: ...comment explaining why.Per-test classification
Parallel (10) — pure, no env/chdir/global mutation:
TestLoad_DefaultsTestLoad_ExplicitMissingFile_ErrorTestLoad_YAMLFileTestLoad_InvalidYAML_ErrorTestLoad_DryRunAndPurchaseConflictTestLoad_PurchaseFlagSetsDryRunFalseTestLoad_UnknownCloud_ErrorTestLoad_NegativeThreshold_ErrorTestValidate_EmptyEnabledClouds_ErrorTestLoad_YAMLRoundtripSerial (8) — documented in-source:
TestLoad_MissingDefaultFile_NoError—os.ChdirTestLoad_EnvOverridesYAML—t.SetenvTestLoad_EnvVars—t.Setenv(×11)TestLoad_InvalidEnvVar_Error—t.SetenvTestLoad_InvalidDurationEnvVar_Error—t.SetenvTestLoad_FlagOverridesEnvAndYAML—t.SetenvTestLoad_CUDLYCONFIGEnv—t.SetenvTestLoad_ArgPathTakesPrecedenceOverCUDLYCONFIG—t.SetenvOut of scope
internal/config/(Postgres integration tests — explicitly deferred per chore(tests): finish t.Parallel() adoption sweep across remaining packages #58 body, needs a separate refactor).t.Setenvusers to a different env-injection pattern — would change the test design rather than just parallelise.Test plan
cd pkg && go test -race -count=2 ./config/...passes cleanly twice locallyRefs #58
Summary by CodeRabbit