From eb33f76c6dbf3d95003da1fd16beb76d3e9282e6 Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Fri, 15 Mar 2024 12:05:45 +0200 Subject: [PATCH 1/5] Initial implementation Signed-off-by: Danny Kopping --- cli/cliui/provisionerjob.go | 40 ++++++++++++++++++++++++++++---- cli/cliui/provisionerjob_test.go | 16 ++++++------- 2 files changed, 43 insertions(+), 13 deletions(-) diff --git a/cli/cliui/provisionerjob.go b/cli/cliui/provisionerjob.go index aeaea7a34cf45..990f8caa4ffab 100644 --- a/cli/cliui/provisionerjob.go +++ b/cli/cliui/provisionerjob.go @@ -54,6 +54,11 @@ func (err *ProvisionerJobError) Error() string { return err.Message } +const ( + ProvisioningStateQueued = "Queued" + ProvisioningStateRunning = "Running" +) + // ProvisionerJob renders a provisioner job with interactive cancellation. func ProvisionerJob(ctx context.Context, wr io.Writer, opts ProvisionerJobOptions) error { if opts.FetchInterval == 0 { @@ -63,8 +68,9 @@ func ProvisionerJob(ctx context.Context, wr io.Writer, opts ProvisionerJobOption defer cancelFunc() var ( - currentStage = "Queued" + currentStage = ProvisioningStateQueued currentStageStartedAt = time.Now().UTC() + currentQueuePos = -1 errChan = make(chan error, 1) job codersdk.ProvisionerJob @@ -74,7 +80,20 @@ func ProvisionerJob(ctx context.Context, wr io.Writer, opts ProvisionerJobOption sw := &stageWriter{w: wr, verbose: opts.Verbose, silentLogs: opts.Silent} printStage := func() { - sw.Start(currentStage) + out := currentStage + + if currentStage == ProvisioningStateQueued && currentQueuePos > 0 { + var queuePos string + if currentQueuePos == 1 { + queuePos = "next" + } else { + queuePos = fmt.Sprintf("position: %d", currentQueuePos) + } + + out = pretty.Sprintf(DefaultStyles.Warn, "%s (%s)", currentStage, queuePos) + } + + sw.Start(out) } updateStage := func(stage string, startedAt time.Time) { @@ -103,15 +122,26 @@ func ProvisionerJob(ctx context.Context, wr io.Writer, opts ProvisionerJobOption errChan <- xerrors.Errorf("fetch: %w", err) return } + if job.QueuePosition != currentQueuePos { + initialState := currentQueuePos == -1 + + currentQueuePos = job.QueuePosition + // Print an update when the queue position changes, but: + // - not initially, because the stage is printed at startup + // - not when we first in the queue, because it's redundant + if !initialState && currentQueuePos != 0 { + printStage() + } + } if job.StartedAt == nil { return } - if currentStage != "Queued" { + if currentStage != ProvisioningStateQueued { // If another stage is already running, there's no need // for us to notify the user we're running! return } - updateStage("Running", *job.StartedAt) + updateStage(ProvisioningStateRunning, *job.StartedAt) } if opts.Cancel != nil { @@ -143,8 +173,8 @@ func ProvisionerJob(ctx context.Context, wr io.Writer, opts ProvisionerJobOption } // The initial stage needs to print after the signal handler has been registered. - printStage() updateJob() + printStage() logs, closer, err := opts.Logs() if err != nil { diff --git a/cli/cliui/provisionerjob_test.go b/cli/cliui/provisionerjob_test.go index b180a1ec9b52d..19b9d0f107dc9 100644 --- a/cli/cliui/provisionerjob_test.go +++ b/cli/cliui/provisionerjob_test.go @@ -40,12 +40,12 @@ func TestProvisionerJob(t *testing.T) { close(test.Logs) test.JobMutex.Unlock() }() - test.PTY.ExpectMatch("Queued") + test.PTY.ExpectMatch(cliui.ProvisioningStateQueued) test.Next <- struct{}{} - test.PTY.ExpectMatch("Queued") - test.PTY.ExpectMatch("Running") + test.PTY.ExpectMatch(cliui.ProvisioningStateQueued) + test.PTY.ExpectMatch(cliui.ProvisioningStateRunning) test.Next <- struct{}{} - test.PTY.ExpectMatch("Running") + test.PTY.ExpectMatch(cliui.ProvisioningStateRunning) }) t.Run("Stages", func(t *testing.T) { @@ -71,9 +71,9 @@ func TestProvisionerJob(t *testing.T) { close(test.Logs) test.JobMutex.Unlock() }() - test.PTY.ExpectMatch("Queued") + test.PTY.ExpectMatch(cliui.ProvisioningStateQueued) test.Next <- struct{}{} - test.PTY.ExpectMatch("Queued") + test.PTY.ExpectMatch(cliui.ProvisioningStateQueued) test.PTY.ExpectMatch("Something") test.Next <- struct{}{} test.PTY.ExpectMatch("Something") @@ -104,11 +104,11 @@ func TestProvisionerJob(t *testing.T) { close(test.Logs) test.JobMutex.Unlock() }() - test.PTY.ExpectMatch("Queued") + test.PTY.ExpectMatch(cliui.ProvisioningStateQueued) test.Next <- struct{}{} test.PTY.ExpectMatch("Gracefully canceling") test.Next <- struct{}{} - test.PTY.ExpectMatch("Queued") + test.PTY.ExpectMatch(cliui.ProvisioningStateQueued) }) } From 686c2b73b516cae4f2d0b03389be5dede0ee013a Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Fri, 15 Mar 2024 14:14:19 +0200 Subject: [PATCH 2/5] Adding tests Signed-off-by: Danny Kopping --- cli/cliui/provisionerjob_test.go | 66 ++++++++++++++++++++++++++++++++ pty/ptytest/ptytest.go | 25 +++++++++++- 2 files changed, 89 insertions(+), 2 deletions(-) diff --git a/cli/cliui/provisionerjob_test.go b/cli/cliui/provisionerjob_test.go index 19b9d0f107dc9..b57339295aacc 100644 --- a/cli/cliui/provisionerjob_test.go +++ b/cli/cliui/provisionerjob_test.go @@ -2,8 +2,10 @@ package cliui_test import ( "context" + "fmt" "io" "os" + "regexp" "runtime" "sync" "testing" @@ -79,6 +81,70 @@ func TestProvisionerJob(t *testing.T) { test.PTY.ExpectMatch("Something") }) + t.Run("Queue Position", func(t *testing.T) { + t.Parallel() + + stage := cliui.ProvisioningStateQueued + + tests := []struct { + name string + queuePos int + expected string + }{ + { + name: "first", + queuePos: 0, + expected: fmt.Sprintf("%s$", stage), + }, + { + name: "next", + queuePos: 1, + expected: fmt.Sprintf(`%s %s$`, stage, regexp.QuoteMeta("(next)")), + }, + { + name: "other", + queuePos: 4, + expected: fmt.Sprintf(`%s %s$`, stage, regexp.QuoteMeta("(position: 4)")), + }, + } + + for _, tc := range tests { + tc := tc + + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + test := newProvisionerJob(t) + test.JobMutex.Lock() + test.Job.QueuePosition = tc.queuePos + test.Job.QueueSize = tc.queuePos + test.JobMutex.Unlock() + + go func() { + <-test.Next + test.JobMutex.Lock() + test.Job.Status = codersdk.ProvisionerJobRunning + now := dbtime.Now() + test.Job.StartedAt = &now + test.JobMutex.Unlock() + <-test.Next + test.JobMutex.Lock() + test.Job.Status = codersdk.ProvisionerJobSucceeded + now = dbtime.Now() + test.Job.CompletedAt = &now + close(test.Logs) + test.JobMutex.Unlock() + }() + test.PTY.ExpectRegexMatch(tc.expected) + test.Next <- struct{}{} + test.PTY.ExpectMatch(cliui.ProvisioningStateQueued) // step completed + test.PTY.ExpectMatch(cliui.ProvisioningStateRunning) + test.Next <- struct{}{} + test.PTY.ExpectMatch(cliui.ProvisioningStateRunning) + }) + } + }) + // This cannot be ran in parallel because it uses a signal. // nolint:paralleltest t.Run("Cancel", func(t *testing.T) { diff --git a/pty/ptytest/ptytest.go b/pty/ptytest/ptytest.go index 6ee737563d2d4..062ae32d103e2 100644 --- a/pty/ptytest/ptytest.go +++ b/pty/ptytest/ptytest.go @@ -6,6 +6,7 @@ import ( "context" "fmt" "io" + "regexp" "runtime" "strings" "sync" @@ -145,16 +146,36 @@ type outExpecter struct { } func (e *outExpecter) ExpectMatch(str string) string { + return e.expectMatchContext(str, e.ExpectMatchContext) +} + +func (e *outExpecter) ExpectRegexMatch(str string) string { + return e.expectMatchContext(str, e.ExpectRegexMatchContext) +} + +func (e *outExpecter) expectMatchContext(str string, impl func(ctx context.Context, str string) string) string { e.t.Helper() timeout, cancel := context.WithTimeout(context.Background(), testutil.WaitMedium) defer cancel() - return e.ExpectMatchContext(timeout, str) + return impl(timeout, str) } // TODO(mafredri): Rename this to ExpectMatch when refactoring. func (e *outExpecter) ExpectMatchContext(ctx context.Context, str string) string { + return e.expectMatcher(ctx, str, func(src, pattern string) bool { + return strings.Contains(src, pattern) + }) +} + +func (e *outExpecter) ExpectRegexMatchContext(ctx context.Context, str string) string { + return e.expectMatcher(ctx, str, func(src, pattern string) bool { + return regexp.MustCompile(pattern).MatchString(src) + }) +} + +func (e *outExpecter) expectMatcher(ctx context.Context, str string, matchFn func(src, pattern string) bool) string { e.t.Helper() var buffer bytes.Buffer @@ -168,7 +189,7 @@ func (e *outExpecter) ExpectMatchContext(ctx context.Context, str string) string if err != nil { return err } - if strings.Contains(buffer.String(), str) { + if matchFn(buffer.String(), str) { return nil } } From 993904caa51bc6e034e2f763bda596c502ab73bd Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Fri, 15 Mar 2024 14:59:31 +0200 Subject: [PATCH 3/5] Improve naming Signed-off-by: Danny Kopping --- pty/ptytest/ptytest.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pty/ptytest/ptytest.go b/pty/ptytest/ptytest.go index 062ae32d103e2..9664bff130cb4 100644 --- a/pty/ptytest/ptytest.go +++ b/pty/ptytest/ptytest.go @@ -146,14 +146,14 @@ type outExpecter struct { } func (e *outExpecter) ExpectMatch(str string) string { - return e.expectMatchContext(str, e.ExpectMatchContext) + return e.expectMatchContextFunc(str, e.ExpectMatchContext) } func (e *outExpecter) ExpectRegexMatch(str string) string { - return e.expectMatchContext(str, e.ExpectRegexMatchContext) + return e.expectMatchContextFunc(str, e.ExpectRegexMatchContext) } -func (e *outExpecter) expectMatchContext(str string, impl func(ctx context.Context, str string) string) string { +func (e *outExpecter) expectMatchContextFunc(str string, impl func(ctx context.Context, str string) string) string { e.t.Helper() timeout, cancel := context.WithTimeout(context.Background(), testutil.WaitMedium) @@ -164,18 +164,18 @@ func (e *outExpecter) expectMatchContext(str string, impl func(ctx context.Conte // TODO(mafredri): Rename this to ExpectMatch when refactoring. func (e *outExpecter) ExpectMatchContext(ctx context.Context, str string) string { - return e.expectMatcher(ctx, str, func(src, pattern string) bool { + return e.expectMatcherFunc(ctx, str, func(src, pattern string) bool { return strings.Contains(src, pattern) }) } func (e *outExpecter) ExpectRegexMatchContext(ctx context.Context, str string) string { - return e.expectMatcher(ctx, str, func(src, pattern string) bool { + return e.expectMatcherFunc(ctx, str, func(src, pattern string) bool { return regexp.MustCompile(pattern).MatchString(src) }) } -func (e *outExpecter) expectMatcher(ctx context.Context, str string, matchFn func(src, pattern string) bool) string { +func (e *outExpecter) expectMatcherFunc(ctx context.Context, str string, matchFn func(src, pattern string) bool) string { e.t.Helper() var buffer bytes.Buffer From 4df8a3b37c64ab2fe7d7cfd3fc89a293fabe8bb1 Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Fri, 15 Mar 2024 15:04:11 +0200 Subject: [PATCH 4/5] Typo, naming Signed-off-by: Danny Kopping --- cli/cliui/provisionerjob.go | 2 +- pty/ptytest/ptytest.go | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/cli/cliui/provisionerjob.go b/cli/cliui/provisionerjob.go index 990f8caa4ffab..f9ecbf3d8ab17 100644 --- a/cli/cliui/provisionerjob.go +++ b/cli/cliui/provisionerjob.go @@ -128,7 +128,7 @@ func ProvisionerJob(ctx context.Context, wr io.Writer, opts ProvisionerJobOption currentQueuePos = job.QueuePosition // Print an update when the queue position changes, but: // - not initially, because the stage is printed at startup - // - not when we first in the queue, because it's redundant + // - not when we're first in the queue, because it's redundant if !initialState && currentQueuePos != 0 { printStage() } diff --git a/pty/ptytest/ptytest.go b/pty/ptytest/ptytest.go index 9664bff130cb4..9f008b9238501 100644 --- a/pty/ptytest/ptytest.go +++ b/pty/ptytest/ptytest.go @@ -153,13 +153,13 @@ func (e *outExpecter) ExpectRegexMatch(str string) string { return e.expectMatchContextFunc(str, e.ExpectRegexMatchContext) } -func (e *outExpecter) expectMatchContextFunc(str string, impl func(ctx context.Context, str string) string) string { +func (e *outExpecter) expectMatchContextFunc(str string, fn func(ctx context.Context, str string) string) string { e.t.Helper() timeout, cancel := context.WithTimeout(context.Background(), testutil.WaitMedium) defer cancel() - return impl(timeout, str) + return fn(timeout, str) } // TODO(mafredri): Rename this to ExpectMatch when refactoring. @@ -175,7 +175,7 @@ func (e *outExpecter) ExpectRegexMatchContext(ctx context.Context, str string) s }) } -func (e *outExpecter) expectMatcherFunc(ctx context.Context, str string, matchFn func(src, pattern string) bool) string { +func (e *outExpecter) expectMatcherFunc(ctx context.Context, str string, fn func(src, pattern string) bool) string { e.t.Helper() var buffer bytes.Buffer @@ -189,7 +189,7 @@ func (e *outExpecter) expectMatcherFunc(ctx context.Context, str string, matchFn if err != nil { return err } - if matchFn(buffer.String(), str) { + if fn(buffer.String(), str) { return nil } } From 4367e657847c7bcfa57e62c50e16338b7a965add Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Mon, 18 Mar 2024 09:47:31 +0200 Subject: [PATCH 5/5] Ensuring tests don't run indefinitely Signed-off-by: Danny Kopping --- cli/cliui/provisionerjob_test.go | 89 +++++++++++++++++++++----------- 1 file changed, 58 insertions(+), 31 deletions(-) diff --git a/cli/cliui/provisionerjob_test.go b/cli/cliui/provisionerjob_test.go index 16301e7d0f326..f3661ca8d1597 100644 --- a/cli/cliui/provisionerjob_test.go +++ b/cli/cliui/provisionerjob_test.go @@ -11,6 +11,7 @@ import ( "testing" "time" + "github.com/coder/coder/v2/testutil" "github.com/stretchr/testify/assert" "github.com/coder/coder/v2/cli/cliui" @@ -27,7 +28,11 @@ func TestProvisionerJob(t *testing.T) { t.Parallel() test := newProvisionerJob(t) - go func() { + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() + + testutil.Go(t, func() { <-test.Next test.JobMutex.Lock() test.Job.Status = codersdk.ProvisionerJobRunning @@ -41,20 +46,26 @@ func TestProvisionerJob(t *testing.T) { test.Job.CompletedAt = &now close(test.Logs) test.JobMutex.Unlock() - }() - test.PTY.ExpectMatch(cliui.ProvisioningStateQueued) - test.Next <- struct{}{} - test.PTY.ExpectMatch(cliui.ProvisioningStateQueued) - test.PTY.ExpectMatch(cliui.ProvisioningStateRunning) - test.Next <- struct{}{} - test.PTY.ExpectMatch(cliui.ProvisioningStateRunning) + }) + testutil.Eventually(ctx, t, func(ctx context.Context) (done bool) { + test.PTY.ExpectMatch(cliui.ProvisioningStateQueued) + test.Next <- struct{}{} + test.PTY.ExpectMatch(cliui.ProvisioningStateQueued) + test.PTY.ExpectMatch(cliui.ProvisioningStateRunning) + test.Next <- struct{}{} + test.PTY.ExpectMatch(cliui.ProvisioningStateRunning) + return true + }, testutil.IntervalFast) }) t.Run("Stages", func(t *testing.T) { t.Parallel() test := newProvisionerJob(t) - go func() { + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() + + testutil.Go(t, func() { <-test.Next test.JobMutex.Lock() test.Job.Status = codersdk.ProvisionerJobRunning @@ -72,13 +83,16 @@ func TestProvisionerJob(t *testing.T) { test.Job.CompletedAt = &now close(test.Logs) test.JobMutex.Unlock() - }() - test.PTY.ExpectMatch(cliui.ProvisioningStateQueued) - test.Next <- struct{}{} - test.PTY.ExpectMatch(cliui.ProvisioningStateQueued) - test.PTY.ExpectMatch("Something") - test.Next <- struct{}{} - test.PTY.ExpectMatch("Something") + }) + testutil.Eventually(ctx, t, func(ctx context.Context) (done bool) { + test.PTY.ExpectMatch(cliui.ProvisioningStateQueued) + test.Next <- struct{}{} + test.PTY.ExpectMatch(cliui.ProvisioningStateQueued) + test.PTY.ExpectMatch("Something") + test.Next <- struct{}{} + test.PTY.ExpectMatch("Something") + return true + }, testutil.IntervalFast) }) t.Run("Queue Position", func(t *testing.T) { @@ -120,7 +134,10 @@ func TestProvisionerJob(t *testing.T) { test.Job.QueueSize = tc.queuePos test.JobMutex.Unlock() - go func() { + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() + + testutil.Go(t, func() { <-test.Next test.JobMutex.Lock() test.Job.Status = codersdk.ProvisionerJobRunning @@ -134,13 +151,16 @@ func TestProvisionerJob(t *testing.T) { test.Job.CompletedAt = &now close(test.Logs) test.JobMutex.Unlock() - }() - test.PTY.ExpectRegexMatch(tc.expected) - test.Next <- struct{}{} - test.PTY.ExpectMatch(cliui.ProvisioningStateQueued) // step completed - test.PTY.ExpectMatch(cliui.ProvisioningStateRunning) - test.Next <- struct{}{} - test.PTY.ExpectMatch(cliui.ProvisioningStateRunning) + }) + testutil.Eventually(ctx, t, func(ctx context.Context) (done bool) { + test.PTY.ExpectRegexMatch(tc.expected) + test.Next <- struct{}{} + test.PTY.ExpectMatch(cliui.ProvisioningStateQueued) // step completed + test.PTY.ExpectMatch(cliui.ProvisioningStateRunning) + test.Next <- struct{}{} + test.PTY.ExpectMatch(cliui.ProvisioningStateRunning) + return true + }, testutil.IntervalFast) }) } }) @@ -156,7 +176,11 @@ func TestProvisionerJob(t *testing.T) { } test := newProvisionerJob(t) - go func() { + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() + + testutil.Go(t, func() { <-test.Next currentProcess, err := os.FindProcess(os.Getpid()) assert.NoError(t, err) @@ -169,12 +193,15 @@ func TestProvisionerJob(t *testing.T) { test.Job.CompletedAt = &now close(test.Logs) test.JobMutex.Unlock() - }() - test.PTY.ExpectMatch(cliui.ProvisioningStateQueued) - test.Next <- struct{}{} - test.PTY.ExpectMatch("Gracefully canceling") - test.Next <- struct{}{} - test.PTY.ExpectMatch(cliui.ProvisioningStateQueued) + }) + testutil.Eventually(ctx, t, func(ctx context.Context) (done bool) { + test.PTY.ExpectMatch(cliui.ProvisioningStateQueued) + test.Next <- struct{}{} + test.PTY.ExpectMatch("Gracefully canceling") + test.Next <- struct{}{} + test.PTY.ExpectMatch(cliui.ProvisioningStateQueued) + return true + }, testutil.IntervalFast) }) }