fix: wait for prompt on rich param CLI test #15406
Conversation
|
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @spikecurtis and the rest of your teammates on |
1b7b6a6 to
0049603
Compare
0049603 to
2412604
Compare
| @@ -198,7 +198,7 @@ func (e *outExpecter) expectMatcherFunc(ctx context.Context, str string, fn func | |||
| e.fatalf("read error", "%v (wanted %q; got %q)", err, str, buffer.String()) | |||
| return "" | |||
| } | |||
| e.logf("matched %q = %q", str, stripansi.Strip(buffer.String())) | |||
| e.logf("matched %q = %q", str, buffer.String()) | |||
There was a problem hiding this comment.
this can be misleading, since we don't strip the the string of ANSI codes when matching.
| } | ||
| <-doneChan | ||
| pty.ExpectMatch(stringParameterName) | ||
| pty.ExpectMatch("> Enter a value (default: \"\"): ") |
There was a problem hiding this comment.
I understand the rationale behind proposing this matcher. However, I'm a bit surprised to observe that stdin isn't buffered when input arrives prematurely. Is this the intended behavior, or are you exploring different matcher configurations to address the issue?
There was a problem hiding this comment.
It's a good point. I looked into this more and there is a race in cliui.Prompt that causes input to be lost in cases where input is being sent too early and we fail validation.
I'll separately work on a fix for this ^^
I still think this PR is an improvement: in many cases the explicit matches are fewer lines than the looping construct, and are closer to the timing a human would have.
It also prevents these tests from deadlocking on failure via the bare <-doneChan read.
There was a problem hiding this comment.
I agree, mitigating the risk of deadlocks is certainly valuable here.
As for the explicit matchers, I also find them preferable from a readability standpoint; they make the intent clear and reduce ambiguity, allowing developers to quickly grasp the expected input without needing to infer it.
| } | ||
| <-doneChan | ||
| pty.ExpectMatch(stringParameterName) | ||
| pty.ExpectMatch("> Enter a value (default: \"\"): ") |
There was a problem hiding this comment.
I agree, mitigating the risk of deadlocks is certainly valuable here.
As for the explicit matchers, I also find them preferable from a readability standpoint; they make the intent clear and reduce ambiguity, allowing developers to quickly grasp the expected input without needing to infer it.
Fixes a race in TestUpdateValidateRichParameters where the parameter is sent prior to the prompt.
Causes errors like: https://github.com/coder/coder/actions/runs/11681622439/job/32527173007