critest: Add test for exec #269
Conversation
Add test for exec where tty and stdin set to true Signed-off-by: umohnani8 <[email protected]>
k8s-ci-robot
added
cncf-cla: yes
|
@mrunalp PTAL |
|
The exec test with stdin=true and tty=true is passing on cri-o, but fails on docker. Could be a bug in docker? |
|
For stdin=true and tty=true, docker exec will stop as soon as stdin is closed (and so with containerd @Random-Liu). Since the input is an empty buffer, it is closed immediately, so the output streams are closed immediately, too. @umohnani8 Could you keep the stdin open until getting the output in |
|
@feiskyer how do I do that in the test? I thought the StreamOptions was accounting for that. |
|
@umohnani8 Let me add a patch to your PR |
pkg/validate/streaming.go
Outdated
| framework.Logf("Get exec url: " + resp.Url) | ||
| return resp.Url | ||
| } | ||
|
|
||
| func checkExec(c internalapi.RuntimeService, execServerURL string) { | ||
| func checkExec(c internalapi.RuntimeService, execServerURL, stdout string, isTty bool) { | ||
| localOut := &bytes.Buffer{} |
There was a problem hiding this comment.
Use safe buffer. https://github.com/umohnani8/cri-tools/blob/094e42ff5cde024a4e6bae544c1993e0f88d83a6/pkg/validate/streaming.go#L236
bytes.Buffer is not thread safe.
Actually use similar code with checkAttach. https://github.com/umohnani8/cri-tools/blob/094e42ff5cde024a4e6bae544c1993e0f88d83a6/pkg/validate/streaming.go#L257
|
@feiskyer I see now, thanks for the patch :) |
|
Actually we should test different behaviors when /lgtm |
Add test for exec where tty and stdin set to true
Signed-off-by: umohnani8 [email protected]