test: Add integration test for kubectl attach#96
Conversation
There was a problem hiding this comment.
Pull Request Overview
Adds integration test coverage for the kubectl attach command to verify logs and asciicast functionality, complementing existing kubectl exec tests.
Key Changes
- Added timeout support to kubectl wrapper with new
CommandWithTimeoutmethod - Updated test pod configuration to output periodic messages for attach testing
- Renamed assertion function to support both exec and attach operations
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/integration/testutil/kubectl.go | Adds context and timeout support for kubectl commands |
| test/integration/testutil/kind.go | Modifies test pod to output periodic messages and adds linter exception |
| test/integration/testutil/assertions.go | Renames function to support both exec and attach operations |
| test/integration/single_user_test.go | Adds kubectl attach integration test |
| test/integration/concurrent_users_test.go | Updates function calls to use renamed assertion function |
Comments suppressed due to low confidence (1)
test/integration/single_user_test.go:158
- The test expects kubectl attach to timeout and return an ExitError, but doesn't verify that the timeout actually occurred. Consider checking that the error is specifically a timeout error rather than any ExitError, or add a comment explaining why any ExitError is acceptable.
output, err = user.Kubectl.CommandWithTimeout(2*time.Second, "attach", "test-pod")
| // Wait for the logs to be flushed | ||
| time.Sleep(100 * time.Millisecond) |
There was a problem hiding this comment.
Just FYI, when Go 1.25 lands later this year, we should try to use synctest to avoid these sleeps.
| } | ||
|
|
||
| func (k *Kubectl) executeKubectl(stdIn io.Reader, cmdOptions ...string) ([]byte, error) { | ||
| func (k *Kubectl) CommandWithTimeout(timeout time.Duration, cmdOptions ...string) ([]byte, error) { |
There was a problem hiding this comment.
While this might be good enough for now, I would expose a method with a context e.g.
func (k *Kubectl) CommandContext(ctx context.Context, cmdOptions ...string) ([]byte, error) { }so it's more reusable (like fixing the contextcheck lint issue). I think it's common to have method with context as its first argument in Go.
There was a problem hiding this comment.
Yes, I think it's a good idea. I updated it in 70c296a
Changes
kubectl attach