Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdded unit tests verifying Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 |
There was a problem hiding this comment.
Pull request overview
Adds test coverage around a6 debug logs container auto-detection and container selection edge cases, aligning with CI’s “auto-detect container name” behavior and the follow-up testing gaps noted in #14.
Changes:
- Added an E2E test that runs
a6 debug logswithout--containerto validate CI-style auto-detection behavior. - Added unit tests for
chooseContainercovering single-container and multiple-container selection outcomes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
test/e2e/debug_logs_test.go |
Adds an E2E test for debug logs container auto-detection behavior. |
pkg/cmd/debug/logs/logs_test.go |
Adds unit coverage for chooseContainer single/multiple container edge cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func TestDebugLogs_AutoDetectContainer(t *testing.T) { | ||
| env := setupRouteEnv(t) | ||
| stdout, stderr, err := runA6WithEnv(env, "debug", "logs", "--tail", "10") | ||
| if err != nil && strings.Contains(stderr, "no APISIX container found") { | ||
| t.Skipf("skipping auto-detect test because no CI-style apisix container is running: %s", stderr) | ||
| } | ||
| require.NoError(t, err, "debug logs auto-detect failed: stdout=%s stderr=%s", stdout, stderr) | ||
| assert.NotEmpty(t, stdout) |
There was a problem hiding this comment.
TestDebugLogs_AutoDetectContainer only skips when the CLI reports "no APISIX container found". However, the auto-detect path (docker ps --filter name=apisix) can also fail with "multiple APISIX containers found" in common local setups (e.g., running apisix plus apisix-dashboard/apisix-* containers), which would make this new E2E test fail even though it’s simply not a CI-style environment. Consider extending the skip condition to also cover the multiple-containers case (or more generally, skip when auto-detect fails due to non-CI container topology).
Summary
Validation
Refs #14
Summary by CodeRabbit