Fix racey TestAttachAfterDetach #10200
Conversation
|
LGTM |
|
fixes race locally for me |
| if err != nil { | ||
| t.Fatalf("prompt read failed: %v", err) | ||
| select { | ||
| case err := <-readErr: |
There was a problem hiding this comment.
I'm not sure why we wait for this in this select? Isn't it better to leave only <-cmdErr branch?
There was a problem hiding this comment.
I guess technically if there was a read error, the string.Contains would fail.
Is that what you are getting at?
There was a problem hiding this comment.
No, I mean that your test is not deterministic, because you can get signal through readErr or cmdErr and in branch about cmdErr you read readErr anyway, so why we need to wait on readErr concurrently with cmdErr if process is still expected to finish.
There was a problem hiding this comment.
Even better, no more select.
There was a problem hiding this comment.
meh, maybe I can't do that without select (and make sure the test doesn't hang)
cpuguy83
force-pushed
the
fix_racey_TestAttachAfterDetach
branch
3 times, most recently
from
6441cf0
to
26bf5fe
Compare
|
@LK4D4 Updated. |
| } | ||
|
|
||
| if !strings.Contains(string(bytes[:n]), "/ #") { | ||
| t.Fatalf("failed to get a new prompt. got %s", string(bytes[:n])) | ||
| if err := <-cmdErr; err != nil { |
There was a problem hiding this comment.
lol, I'm pretty sure that this is equivalent of cmd.Wait :)
There was a problem hiding this comment.
I need to stop doing this stuff at night!
|
@cpuguy83 Thank you for fixing the race! The slight problem with this fix is that it doesn't fail cleanly if tested against the bug it was first created for(commits between #9511 - #10079). The second attach process is just closed then and this blocks the |
|
@tonistiigi That's what I was doing, but I assumed (I guess falsely) that read would not actually block here. I'll update. |
|
@cpuguy83 I think the real issue for the blocking is that one is supposed to close the tty after calling I get a fail with |
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
|
@tonistiigi Ah yep. Ok, this is fixed. |
cpuguy83
force-pushed
the
fix_racey_TestAttachAfterDetach
branch
from
26bf5fe
to
6ef8057
Compare
|
LGTM |
No description provided.