Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 7 minutes and 26 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis pull request adds three new test files to the project. The Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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.
🧹 Nitpick comments (2)
pkg/selector/selector_test.go (1)
29-43: Serializeos.Stdinswapping to avoid future test flakiness.This test mutates process-global stdin. It’s safe today, but can race if package tests later use parallel execution. Consider guarding the swap with a package-level mutex.
🔧 Suggested hardening
import ( "os" "path/filepath" + "sync" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) + +var stdinSwapMu sync.Mutex func TestSelectOne_RequiresTerminal(t *testing.T) { + stdinSwapMu.Lock() + t.Cleanup(stdinSwapMu.Unlock) + tmpDir := t.TempDir() inputPath := filepath.Join(tmpDir, "stdin.txt") require.NoError(t, os.WriteFile(inputPath, []byte("not-a-tty"), 0o644))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/selector/selector_test.go` around lines 29 - 43, Test mutates global os.Stdin in TestSelectOne_RequiresTerminal causing potential races; add a package-level mutex (e.g., var stdinMu sync.Mutex) and lock it before swapping os.Stdin in TestSelectOne_RequiresTerminal, then in the t.Cleanup restore the original stdin and unlock the mutex so the swap/un-swap is protected; ensure the mutex is declared at package scope and used around any other tests that touch os.Stdin.pkg/cmd/config/configutil/configutil_test.go (1)
45-49: Avoid positional assertions for section names unless order is a contract.These checks are brittle against harmless
Sections()reordering. Prefer validating expected names are present without index coupling.🔧 Suggested assertion style
sections := result.Sections() require.Len(t, sections, 12) - assert.Equal(t, "upstreams", sections[0].Name) - assert.Equal(t, "stream_routes", sections[len(sections)-1].Name) + names := make([]string, 0, len(sections)) + for _, s := range sections { + names = append(names, s.Name) + } + assert.Contains(t, names, "upstreams") + assert.Contains(t, names, "stream_routes")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/config/configutil/configutil_test.go` around lines 45 - 49, The test is brittle because it asserts section names by position (using sections[0].Name and sections[len(sections)-1].Name) instead of checking presence; change the assertions to validate that the expected section names appear in the slice returned by result.Sections() (e.g., extract all section.Name values from the Sections() result and use membership/collection assertions like assert.Contains or require.ElementsMatch) and remove the index-dependent checks so the test does not depend on ordering; keep references to Sections() and the Name field when locating the code to update.
🤖 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/cmd/config/configutil/configutil_test.go`:
- Around line 45-49: The test is brittle because it asserts section names by
position (using sections[0].Name and sections[len(sections)-1].Name) instead of
checking presence; change the assertions to validate that the expected section
names appear in the slice returned by result.Sections() (e.g., extract all
section.Name values from the Sections() result and use membership/collection
assertions like assert.Contains or require.ElementsMatch) and remove the
index-dependent checks so the test does not depend on ordering; keep references
to Sections() and the Name field when locating the code to update.
In `@pkg/selector/selector_test.go`:
- Around line 29-43: Test mutates global os.Stdin in
TestSelectOne_RequiresTerminal causing potential races; add a package-level
mutex (e.g., var stdinMu sync.Mutex) and lock it before swapping os.Stdin in
TestSelectOne_RequiresTerminal, then in the t.Cleanup restore the original stdin
and unlock the mutex so the swap/un-swap is protected; ensure the mutex is
declared at package scope and used around any other tests that touch os.Stdin.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 00784beb-6500-4770-a17e-e25fbeb888a4
📒 Files selected for processing (3)
pkg/cmd/config/configutil/configutil_test.gopkg/selector/selector_test.gopkg/tableprinter/table_test.go
There was a problem hiding this comment.
Pull request overview
This PR expands unit test coverage for shared utilities to address edge cases and error paths called out in Issue #14.
Changes:
- Added selector tests for missing IDs and non-terminal stdin behavior.
- Added tableprinter tests for empty renders, tabular output, and flush/write failures.
- Added broad configutil tests for file parsing, diff helpers, pagination fetching, plugin metadata fetching, and secret ID extraction.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/tableprinter/table_test.go | Adds TablePrinter rendering/flush error-path coverage. |
| pkg/selector/selector_test.go | Adds SelectOne edge-case coverage (invalid items, non-terminal stdin). |
| pkg/cmd/config/configutil/configutil_test.go | Adds comprehensive coverage for config parsing, diff/summary, pagination helpers, plugin metadata, and secret ID parsing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func TestSelectOne_AllItemsMissingIDs(t *testing.T) { | ||
| items := []Item{ | ||
| {ID: "", Label: "first"}, | ||
| {ID: "", Label: "second"}, | ||
| } | ||
|
|
||
| _, err := SelectOne("Select a route", items) | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "no items available") | ||
| } |
There was a problem hiding this comment.
TestSelectOne_AllItemsMissingIDs is likely to fail in CI/non-interactive runs: SelectOne checks isTerminalStdin() before filtering out items with empty IDs, so when stdin is not a TTY it returns interactive selection requires a terminal instead of no items available. To make this test deterministic, either (a) arrange for os.Stdin to be a TTY/pty for this test (skipping if unavailable), or (b) adjust SelectOne to build/filter options first and only require a terminal when there are selectable options.
|
Addressed the selector determinism feedback in a follow-up commit. I did not change the DiffResult.Sections() order assertions: that order is intentional in production code (base resources first, then dependents), so the test is verifying a documented contract rather than incidental slice ordering. |
Summary
Validation
Refs #14
Summary by CodeRabbit