Skip to content

fix: wait for prompt on rich param CLI test #15406

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 7, 2024
Merged

fix: wait for prompt on rich param CLI test #15406

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 57 additions & 46 deletions cli/update_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,9 @@ func TestUpdateValidateRichParameters(t *testing.T) {
err := inv.Run()
require.NoError(t, err)

ctx := testutil.Context(t, testutil.WaitLong)
inv, root = clitest.New(t, "update", "my-workspace", "--always-prompt")
inv = inv.WithContext(ctx)
clitest.SetupConfig(t, member, root)
doneChan := make(chan struct{})
pty := ptytest.New(t).Attach(inv)
Expand All @@ -333,18 +335,16 @@ func TestUpdateValidateRichParameters(t *testing.T) {
assert.NoError(t, err)
}()

matches := []string{
stringParameterName, "$$",
"does not match", "",
"Enter a value", "abc",
}
for i := 0; i < len(matches); i += 2 {
match := matches[i]
value := matches[i+1]
pty.ExpectMatch(match)
pty.WriteLine(value)
}
<-doneChan
pty.ExpectMatch(stringParameterName)
pty.ExpectMatch("> Enter a value (default: \"\"): ")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

coder/internal#203

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

pty.WriteLine("$$")
pty.ExpectMatch("does not match")
pty.ExpectMatch("> Enter a value (default: \"\"): ")
pty.WriteLine("")
pty.ExpectMatch("does not match")
pty.ExpectMatch("> Enter a value (default: \"\"): ")
pty.WriteLine("abc")
_ = testutil.RequireRecvCtx(ctx, t, doneChan)
})

t.Run("ValidateNumber", func(t *testing.T) {
Expand All @@ -369,7 +369,9 @@ func TestUpdateValidateRichParameters(t *testing.T) {
err := inv.Run()
require.NoError(t, err)

ctx := testutil.Context(t, testutil.WaitLong)
inv, root = clitest.New(t, "update", "my-workspace", "--always-prompt")
inv.WithContext(ctx)
clitest.SetupConfig(t, member, root)
doneChan := make(chan struct{})
pty := ptytest.New(t).Attach(inv)
Expand All @@ -379,21 +381,16 @@ func TestUpdateValidateRichParameters(t *testing.T) {
assert.NoError(t, err)
}()

matches := []string{
numberParameterName, "12",
"is more than the maximum", "",
"Enter a value", "8",
}
for i := 0; i < len(matches); i += 2 {
match := matches[i]
value := matches[i+1]
pty.ExpectMatch(match)

if value != "" {
pty.WriteLine(value)
}
}
<-doneChan
pty.ExpectMatch(numberParameterName)
pty.ExpectMatch("> Enter a value (default: \"\"): ")
pty.WriteLine("12")
pty.ExpectMatch("is more than the maximum")
pty.ExpectMatch("> Enter a value (default: \"\"): ")
pty.WriteLine("")
pty.ExpectMatch("is not a number")
pty.ExpectMatch("> Enter a value (default: \"\"): ")
pty.WriteLine("8")
_ = testutil.RequireRecvCtx(ctx, t, doneChan)
})

t.Run("ValidateBool", func(t *testing.T) {
Expand All @@ -418,7 +415,9 @@ func TestUpdateValidateRichParameters(t *testing.T) {
err := inv.Run()
require.NoError(t, err)

ctx := testutil.Context(t, testutil.WaitLong)
inv, root = clitest.New(t, "update", "my-workspace", "--always-prompt")
inv = inv.WithContext(ctx)
clitest.SetupConfig(t, member, root)
doneChan := make(chan struct{})
pty := ptytest.New(t).Attach(inv)
Expand All @@ -428,18 +427,16 @@ func TestUpdateValidateRichParameters(t *testing.T) {
assert.NoError(t, err)
}()

matches := []string{
boolParameterName, "cat",
"boolean value can be either", "",
"Enter a value", "false",
}
for i := 0; i < len(matches); i += 2 {
match := matches[i]
value := matches[i+1]
pty.ExpectMatch(match)
pty.WriteLine(value)
}
<-doneChan
pty.ExpectMatch(boolParameterName)
pty.ExpectMatch("> Enter a value (default: \"\"): ")
pty.WriteLine("cat")
pty.ExpectMatch("boolean value can be either \"true\" or \"false\"")
pty.ExpectMatch("> Enter a value (default: \"\"): ")
pty.WriteLine("")
pty.ExpectMatch("boolean value can be either \"true\" or \"false\"")
pty.ExpectMatch("> Enter a value (default: \"\"): ")
pty.WriteLine("false")
_ = testutil.RequireRecvCtx(ctx, t, doneChan)
})

t.Run("RequiredParameterAdded", func(t *testing.T) {
Expand Down Expand Up @@ -485,7 +482,9 @@ func TestUpdateValidateRichParameters(t *testing.T) {
require.NoError(t, err)

// Update the workspace
ctx := testutil.Context(t, testutil.WaitLong)
inv, root = clitest.New(t, "update", "my-workspace")
inv.WithContext(ctx)
clitest.SetupConfig(t, member, root)
doneChan := make(chan struct{})
pty := ptytest.New(t).Attach(inv)
Expand All @@ -508,7 +507,7 @@ func TestUpdateValidateRichParameters(t *testing.T) {
pty.WriteLine(value)
}
}
<-doneChan
_ = testutil.RequireRecvCtx(ctx, t, doneChan)
})

t.Run("OptionalParameterAdded", func(t *testing.T) {
Expand Down Expand Up @@ -555,7 +554,9 @@ func TestUpdateValidateRichParameters(t *testing.T) {
require.NoError(t, err)

// Update the workspace
ctx := testutil.Context(t, testutil.WaitLong)
inv, root = clitest.New(t, "update", "my-workspace")
inv.WithContext(ctx)
clitest.SetupConfig(t, member, root)
doneChan := make(chan struct{})
pty := ptytest.New(t).Attach(inv)
Expand All @@ -566,7 +567,7 @@ func TestUpdateValidateRichParameters(t *testing.T) {
}()

pty.ExpectMatch("Planning workspace...")
<-doneChan
_ = testutil.RequireRecvCtx(ctx, t, doneChan)
})

t.Run("ParameterOptionChanged", func(t *testing.T) {
Expand Down Expand Up @@ -612,7 +613,9 @@ func TestUpdateValidateRichParameters(t *testing.T) {
require.NoError(t, err)

// Update the workspace
ctx := testutil.Context(t, testutil.WaitLong)
inv, root = clitest.New(t, "update", "my-workspace")
inv.WithContext(ctx)
clitest.SetupConfig(t, member, root)
doneChan := make(chan struct{})
pty := ptytest.New(t).Attach(inv)
Expand All @@ -636,7 +639,7 @@ func TestUpdateValidateRichParameters(t *testing.T) {
}
}

<-doneChan
_ = testutil.RequireRecvCtx(ctx, t, doneChan)
})

t.Run("ParameterOptionDisappeared", func(t *testing.T) {
Expand Down Expand Up @@ -683,7 +686,9 @@ func TestUpdateValidateRichParameters(t *testing.T) {
require.NoError(t, err)

// Update the workspace
ctx := testutil.Context(t, testutil.WaitLong)
inv, root = clitest.New(t, "update", "my-workspace")
inv.WithContext(ctx)
clitest.SetupConfig(t, member, root)
doneChan := make(chan struct{})
pty := ptytest.New(t).Attach(inv)
Expand All @@ -707,7 +712,7 @@ func TestUpdateValidateRichParameters(t *testing.T) {
}
}

<-doneChan
_ = testutil.RequireRecvCtx(ctx, t, doneChan)
})

t.Run("ParameterOptionFailsMonotonicValidation", func(t *testing.T) {
Expand Down Expand Up @@ -739,7 +744,9 @@ func TestUpdateValidateRichParameters(t *testing.T) {
require.NoError(t, err)

// Update the workspace
ctx := testutil.Context(t, testutil.WaitLong)
inv, root = clitest.New(t, "update", "my-workspace", "--always-prompt=true")
inv.WithContext(ctx)
clitest.SetupConfig(t, member, root)

doneChan := make(chan struct{})
Expand All @@ -762,7 +769,7 @@ func TestUpdateValidateRichParameters(t *testing.T) {
pty.ExpectMatch(match)
}

<-doneChan
_ = testutil.RequireRecvCtx(ctx, t, doneChan)
})

t.Run("ImmutableRequiredParameterExists_MutableRequiredParameterAdded", func(t *testing.T) {
Expand Down Expand Up @@ -804,7 +811,9 @@ func TestUpdateValidateRichParameters(t *testing.T) {
require.NoError(t, err)

// Update the workspace
ctx := testutil.Context(t, testutil.WaitLong)
inv, root = clitest.New(t, "update", "my-workspace")
inv.WithContext(ctx)
clitest.SetupConfig(t, member, root)
doneChan := make(chan struct{})
pty := ptytest.New(t).Attach(inv)
Expand All @@ -828,7 +837,7 @@ func TestUpdateValidateRichParameters(t *testing.T) {
}
}

<-doneChan
_ = testutil.RequireRecvCtx(ctx, t, doneChan)
})

t.Run("MutableRequiredParameterExists_ImmutableRequiredParameterAdded", func(t *testing.T) {
Expand Down Expand Up @@ -874,7 +883,9 @@ func TestUpdateValidateRichParameters(t *testing.T) {
require.NoError(t, err)

// Update the workspace
ctx := testutil.Context(t, testutil.WaitLong)
inv, root = clitest.New(t, "update", "my-workspace")
inv.WithContext(ctx)
clitest.SetupConfig(t, member, root)
doneChan := make(chan struct{})
pty := ptytest.New(t).Attach(inv)
Expand All @@ -898,6 +909,6 @@ func TestUpdateValidateRichParameters(t *testing.T) {
}
}

<-doneChan
_ = testutil.RequireRecvCtx(ctx, t, doneChan)
})
}
2 changes: 1 addition & 1 deletion pty/ptytest/ptytest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be misleading, since we don't strip the the string of ANSI codes when matching.

return buffer.String()
}

Expand Down
Loading