From 761dd55ee8d3b7694cead0e4f2555826d914f298 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 12 Dec 2025 13:41:46 +0200 Subject: [PATCH] fix(coderd/database): sort template version variables and fix test flake (#21233) Previously the GetTemplateVersionVariables query did not sort output, relying on PostgreSQL on-disk ordering which is undeterministic. Variables are now sorted by name because there is no alternative for ordering. Tests were adjusted to accommodate the new ordering, previously they relied on data being written to disk in insert order. --- cli/templatepush_test.go | 235 ++++++++---------- coderd/database/queries.sql.go | 2 +- .../queries/templateversionvariables.sql | 2 +- 3 files changed, 111 insertions(+), 128 deletions(-) diff --git a/cli/templatepush_test.go b/cli/templatepush_test.go index 28c5adc20f..58c1b803db 100644 --- a/cli/templatepush_test.go +++ b/cli/templatepush_test.go @@ -52,10 +52,9 @@ func TestTemplatePush(t *testing.T) { clitest.SetupConfig(t, templateAdmin, root) pty := ptytest.New(t).Attach(inv) - execDone := make(chan error) - go func() { - execDone <- inv.Run() - }() + ctx := testutil.Context(t, testutil.WaitMedium) + inv = inv.WithContext(ctx) + w := clitest.StartWithWaiter(t, inv) matches := []struct { match string @@ -64,11 +63,11 @@ func TestTemplatePush(t *testing.T) { {match: "Upload", write: "yes"}, } for _, m := range matches { - pty.ExpectMatch(m.match) + pty.ExpectMatchContext(ctx, m.match) pty.WriteLine(m.write) } - require.NoError(t, <-execDone) + w.RequireSuccess() // Assert that the template version changed. templateVersions, err := client.TemplateVersionsByTemplate(context.Background(), codersdk.TemplateVersionsByTemplateRequest{ @@ -100,9 +99,7 @@ func TestTemplatePush(t *testing.T) { clitest.SetupConfig(t, templateAdmin, root) pty := ptytest.New(t).Attach(inv) - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitMedium) - defer cancel() - + ctx := testutil.Context(t, testutil.WaitMedium) inv = inv.WithContext(ctx) w := clitest.StartWithWaiter(t, inv) @@ -111,6 +108,7 @@ func TestTemplatePush(t *testing.T) { w.RequireSuccess() // Assert that the template version changed. + ctx = testutil.Context(t, testutil.WaitMedium) templateVersions, err := client.TemplateVersionsByTemplate(ctx, codersdk.TemplateVersionsByTemplateRequest{ TemplateID: template.ID, }) @@ -134,9 +132,6 @@ func TestTemplatePush(t *testing.T) { ProvisionApply: echo.ApplyComplete, }) - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) - defer cancel() - for i, tt := range []struct { wantMessage string wantMatch string @@ -153,6 +148,7 @@ func TestTemplatePush(t *testing.T) { clitest.SetupConfig(t, templateAdmin, root) pty := ptytest.New(t).Attach(inv) + ctx := testutil.Context(t, testutil.WaitMedium) inv = inv.WithContext(ctx) w := clitest.StartWithWaiter(t, inv) @@ -161,6 +157,7 @@ func TestTemplatePush(t *testing.T) { w.RequireSuccess() // Assert that the template version changed. + ctx = testutil.Context(t, testutil.WaitMedium) templateVersions, err := client.TemplateVersionsByTemplate(ctx, codersdk.TemplateVersionsByTemplateRequest{ TemplateID: template.ID, }) @@ -196,10 +193,9 @@ func TestTemplatePush(t *testing.T) { clitest.SetupConfig(t, templateAdmin, root) pty := ptytest.New(t).Attach(inv) - execDone := make(chan error) - go func() { - execDone <- inv.Run() - }() + ctx := testutil.Context(t, testutil.WaitMedium) + inv = inv.WithContext(ctx) + w := clitest.StartWithWaiter(t, inv) matches := []struct { match string @@ -209,14 +205,14 @@ func TestTemplatePush(t *testing.T) { {match: "Upload", write: "no"}, } for _, m := range matches { - pty.ExpectMatch(m.match) + pty.ExpectMatchContext(ctx, m.match) if m.write != "" { pty.WriteLine(m.write) } } // cmd should error once we say no. - require.Error(t, <-execDone) + w.RequireError() }) t.Run("NoLockfileIgnored", func(t *testing.T) { @@ -245,21 +241,19 @@ func TestTemplatePush(t *testing.T) { clitest.SetupConfig(t, templateAdmin, root) pty := ptytest.New(t).Attach(inv) - execDone := make(chan error) - go func() { - execDone <- inv.Run() - }() + ctx := testutil.Context(t, testutil.WaitMedium) + inv = inv.WithContext(ctx) + w := clitest.StartWithWaiter(t, inv) { - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitMedium) - defer cancel() + ctx := testutil.Context(t, testutil.WaitMedium) pty.ExpectNoMatchBefore(ctx, "No .terraform.lock.hcl file found", "Upload") pty.WriteLine("no") } // cmd should error once we say no. - require.Error(t, <-execDone) + w.RequireError() }) t.Run("PushInactiveTemplateVersion", func(t *testing.T) { @@ -285,6 +279,8 @@ func TestTemplatePush(t *testing.T) { ) clitest.SetupConfig(t, templateAdmin, root) pty := ptytest.New(t).Attach(inv) + ctx := testutil.Context(t, testutil.WaitMedium) + inv = inv.WithContext(ctx) w := clitest.StartWithWaiter(t, inv) matches := []struct { @@ -294,14 +290,15 @@ func TestTemplatePush(t *testing.T) { {match: "Upload", write: "yes"}, } for _, m := range matches { - pty.ExpectMatch(m.match) + pty.ExpectMatchContext(ctx, m.match) pty.WriteLine(m.write) } w.RequireSuccess() // Assert that the template version didn't change. - templateVersions, err := client.TemplateVersionsByTemplate(context.Background(), codersdk.TemplateVersionsByTemplateRequest{ + ctx = testutil.Context(t, testutil.WaitMedium) + templateVersions, err := client.TemplateVersionsByTemplate(ctx, codersdk.TemplateVersionsByTemplateRequest{ TemplateID: template.ID, }) require.NoError(t, err) @@ -344,7 +341,9 @@ func TestTemplatePush(t *testing.T) { clitest.SetupConfig(t, templateAdmin, root) pty := ptytest.New(t).Attach(inv) - waiter := clitest.StartWithWaiter(t, inv) + ctx := testutil.Context(t, testutil.WaitMedium) + inv = inv.WithContext(ctx) + w := clitest.StartWithWaiter(t, inv) matches := []struct { match string @@ -353,14 +352,15 @@ func TestTemplatePush(t *testing.T) { {match: "Upload", write: "yes"}, } for _, m := range matches { - pty.ExpectMatch(m.match) + pty.ExpectMatchContext(ctx, m.match) pty.WriteLine(m.write) } - waiter.RequireSuccess() + w.RequireSuccess() // Assert that the template version changed. - templateVersions, err := client.TemplateVersionsByTemplate(context.Background(), codersdk.TemplateVersionsByTemplateRequest{ + ctx = testutil.Context(t, testutil.WaitMedium) + templateVersions, err := client.TemplateVersionsByTemplate(ctx, codersdk.TemplateVersionsByTemplateRequest{ TemplateID: template.ID, }) require.NoError(t, err) @@ -541,16 +541,13 @@ func TestTemplatePush(t *testing.T) { clitest.SetupConfig(t, templateAdmin, root) pty := ptytest.New(t).Attach(inv) - ctx := testutil.Context(t, testutil.WaitShort) + setupCtx := testutil.Context(t, testutil.WaitMedium) now := dbtime.Now() - require.NoError(t, tt.setupDaemon(ctx, store, owner, wantTags, now)) + require.NoError(t, tt.setupDaemon(setupCtx, store, owner, wantTags, now)) - cancelCtx, cancel := context.WithCancel(ctx) - t.Cleanup(cancel) - done := make(chan error) - go func() { - done <- inv.WithContext(cancelCtx).Run() - }() + ctx := testutil.Context(t, testutil.WaitMedium) + inv = inv.WithContext(ctx) + clitest.Start(t, inv) // Only used for output, disregard exit status. require.Eventually(t, func() bool { jobs, err := store.GetProvisionerJobsCreatedAfter(ctx, time.Time{}) @@ -564,11 +561,8 @@ func TestTemplatePush(t *testing.T) { }, testutil.WaitShort, testutil.IntervalFast) if tt.expectOutput != "" { - pty.ExpectMatch(tt.expectOutput) + pty.ExpectMatchContext(ctx, tt.expectOutput) } - - cancel() - <-done }) } }) @@ -613,10 +607,9 @@ func TestTemplatePush(t *testing.T) { clitest.SetupConfig(t, templateAdmin, root) pty := ptytest.New(t).Attach(inv) - execDone := make(chan error) - go func() { - execDone <- inv.Run() - }() + ctx := testutil.Context(t, testutil.WaitMedium) + inv = inv.WithContext(ctx) + w := clitest.StartWithWaiter(t, inv) matches := []struct { match string @@ -625,11 +618,11 @@ func TestTemplatePush(t *testing.T) { {match: "Upload", write: "yes"}, } for _, m := range matches { - pty.ExpectMatch(m.match) + pty.ExpectMatchContext(ctx, m.match) pty.WriteLine(m.write) } - require.NoError(t, <-execDone) + w.RequireSuccess() // Verify template version tags template, err := client.Template(context.Background(), template.ID) @@ -643,8 +636,6 @@ func TestTemplatePush(t *testing.T) { t.Run("DeleteTags", func(t *testing.T) { t.Parallel() - ctx := testutil.Context(t, testutil.WaitLong) - // Start the first provisioner with no tags. client, provisionerDocker, api := coderdtest.NewWithAPI(t, &coderdtest.Options{ IncludeProvisionerDaemon: true, @@ -682,10 +673,9 @@ func TestTemplatePush(t *testing.T) { clitest.SetupConfig(t, templateAdmin, root) pty := ptytest.New(t).Attach(inv) - execDone := make(chan error) - go func() { - execDone <- inv.WithContext(ctx).Run() - }() + ctx := testutil.Context(t, testutil.WaitMedium) + inv = inv.WithContext(ctx) + w := clitest.StartWithWaiter(t, inv) matches := []struct { match string @@ -694,11 +684,11 @@ func TestTemplatePush(t *testing.T) { {match: "Upload", write: "yes"}, } for _, m := range matches { - pty.ExpectMatch(m.match) + pty.ExpectMatchContext(ctx, m.match) pty.WriteLine(m.write) } - require.NoError(t, <-execDone) + w.RequireSuccess() // Verify template version tags template, err := client.Template(ctx, template.ID) @@ -740,10 +730,9 @@ func TestTemplatePush(t *testing.T) { clitest.SetupConfig(t, templateAdmin, root) pty := ptytest.New(t).Attach(inv) - execDone := make(chan error) - go func() { - execDone <- inv.Run() - }() + ctx := testutil.Context(t, testutil.WaitMedium) + inv = inv.WithContext(ctx) + w := clitest.StartWithWaiter(t, inv) matches := []struct { match string @@ -752,11 +741,11 @@ func TestTemplatePush(t *testing.T) { {match: "Upload", write: "yes"}, } for _, m := range matches { - pty.ExpectMatch(m.match) + pty.ExpectMatchContext(ctx, m.match) pty.WriteLine(m.write) } - require.NoError(t, <-execDone) + w.RequireSuccess() // Verify template version tags template, err := client.Template(context.Background(), template.ID) @@ -818,10 +807,9 @@ func TestTemplatePush(t *testing.T) { inv.Stdin = pty.Input() inv.Stdout = pty.Output() - execDone := make(chan error) - go func() { - execDone <- inv.Run() - }() + ctx := testutil.Context(t, testutil.WaitMedium) + inv = inv.WithContext(ctx) + w := clitest.StartWithWaiter(t, inv) matches := []struct { match string @@ -830,11 +818,11 @@ func TestTemplatePush(t *testing.T) { {match: "Upload", write: "yes"}, } for _, m := range matches { - pty.ExpectMatch(m.match) + pty.ExpectMatchContext(ctx, m.match) pty.WriteLine(m.write) } - require.NoError(t, <-execDone) + w.RequireSuccess() // Assert that the template version changed. templateVersions, err := client.TemplateVersionsByTemplate(context.Background(), codersdk.TemplateVersionsByTemplateRequest{ @@ -884,10 +872,9 @@ func TestTemplatePush(t *testing.T) { inv.Stdin = pty.Input() inv.Stdout = pty.Output() - execDone := make(chan error) - go func() { - execDone <- inv.Run() - }() + ctx := testutil.Context(t, testutil.WaitMedium) + inv = inv.WithContext(ctx) + w := clitest.StartWithWaiter(t, inv) matches := []struct { match string @@ -896,11 +883,11 @@ func TestTemplatePush(t *testing.T) { {match: "Upload", write: "yes"}, } for _, m := range matches { - pty.ExpectMatch(m.match) + pty.ExpectMatchContext(ctx, m.match) pty.WriteLine(m.write) } - require.NoError(t, <-execDone) + w.RequireSuccess() // Assert that the template version changed. templateVersions, err := client.TemplateVersionsByTemplate(context.Background(), codersdk.TemplateVersionsByTemplateRequest{ @@ -952,10 +939,9 @@ func TestTemplatePush(t *testing.T) { inv.Stdin = pty.Input() inv.Stdout = pty.Output() - execDone := make(chan error) - go func() { - execDone <- inv.Run() - }() + ctx := testutil.Context(t, testutil.WaitMedium) + inv = inv.WithContext(ctx) + w := clitest.StartWithWaiter(t, inv) matches := []struct { match string @@ -964,11 +950,11 @@ func TestTemplatePush(t *testing.T) { {match: "Upload", write: "yes"}, } for _, m := range matches { - pty.ExpectMatch(m.match) + pty.ExpectMatchContext(ctx, m.match) pty.WriteLine(m.write) } - require.NoError(t, <-execDone) + w.RequireSuccess() // Assert that the template version changed. templateVersions, err := client.TemplateVersionsByTemplate(context.Background(), codersdk.TemplateVersionsByTemplateRequest{ @@ -1005,7 +991,9 @@ func TestTemplatePush(t *testing.T) { clitest.SetupConfig(t, templateAdmin, root) pty := ptytest.New(t).Attach(inv) - waiter := clitest.StartWithWaiter(t, inv) + ctx := testutil.Context(t, testutil.WaitMedium) + inv = inv.WithContext(ctx) + w := clitest.StartWithWaiter(t, inv) matches := []struct { match string @@ -1015,13 +1003,13 @@ func TestTemplatePush(t *testing.T) { {match: "template has been created"}, } for _, m := range matches { - pty.ExpectMatch(m.match) + pty.ExpectMatchContext(ctx, m.match) if m.write != "" { pty.WriteLine(m.write) } } - waiter.RequireSuccess() + w.RequireSuccess() template, err := client.TemplateByName(context.Background(), owner.OrganizationID, templateName) require.NoError(t, err) @@ -1054,9 +1042,7 @@ func TestTemplatePush(t *testing.T) { inv.Stdin = strings.NewReader("invalid tar content that would cause failure") - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitMedium) - defer cancel() - + ctx := testutil.Context(t, testutil.WaitMedium) err := inv.WithContext(ctx).Run() require.NoError(t, err, "Should succeed without reading from stdin") @@ -1107,31 +1093,31 @@ func TestTemplatePush(t *testing.T) { clitest.SetupConfig(t, templateAdmin, root) pty := ptytest.New(t).Attach(inv) - execDone := make(chan error) - go func() { - execDone <- inv.Run() - }() + ctx := testutil.Context(t, testutil.WaitMedium) + inv = inv.WithContext(ctx) + w := clitest.StartWithWaiter(t, inv) // Select "Yes" for the "Upload " prompt - pty.ExpectMatch("Upload") + pty.ExpectMatchContext(ctx, "Upload") pty.WriteLine("yes") - pty.ExpectMatch("var.string_var") - pty.ExpectMatch("Enter value:") - pty.WriteLine("test-string") + // Variables are prompted in alphabetical order. + // Boolean variable automatically selects the first option ("true") + pty.ExpectMatchContext(ctx, "var.bool_var") - pty.ExpectMatch("var.number_var") - pty.ExpectMatch("Enter value:") + pty.ExpectMatchContext(ctx, "var.number_var") + pty.ExpectMatchContext(ctx, "Enter value:") pty.WriteLine("42") - // Boolean variable automatically selects the first option ("true") - pty.ExpectMatch("var.bool_var") - - pty.ExpectMatch("var.sensitive_var") - pty.ExpectMatch("Enter value:") + pty.ExpectMatchContext(ctx, "var.sensitive_var") + pty.ExpectMatchContext(ctx, "Enter value:") pty.WriteLine("secret-value") - require.NoError(t, <-execDone) + pty.ExpectMatchContext(ctx, "var.string_var") + pty.ExpectMatchContext(ctx, "Enter value:") + pty.WriteLine("test-string") + + w.RequireSuccess() }) t.Run("ValidateNumberInput", func(t *testing.T) { @@ -1154,23 +1140,22 @@ func TestTemplatePush(t *testing.T) { clitest.SetupConfig(t, templateAdmin, root) pty := ptytest.New(t).Attach(inv) - execDone := make(chan error) - go func() { - execDone <- inv.Run() - }() + ctx := testutil.Context(t, testutil.WaitMedium) + inv = inv.WithContext(ctx) + w := clitest.StartWithWaiter(t, inv) // Select "Yes" for the "Upload " prompt - pty.ExpectMatch("Upload") + pty.ExpectMatchContext(ctx, "Upload") pty.WriteLine("yes") - pty.ExpectMatch("var.number_var") + pty.ExpectMatchContext(ctx, "var.number_var") pty.WriteLine("not-a-number") - pty.ExpectMatch("must be a valid number") + pty.ExpectMatchContext(ctx, "must be a valid number") pty.WriteLine("123.45") - require.NoError(t, <-execDone) + w.RequireSuccess() }) t.Run("DontPromptForDefaultValues", func(t *testing.T) { @@ -1198,19 +1183,18 @@ func TestTemplatePush(t *testing.T) { clitest.SetupConfig(t, templateAdmin, root) pty := ptytest.New(t).Attach(inv) - execDone := make(chan error) - go func() { - execDone <- inv.Run() - }() + ctx := testutil.Context(t, testutil.WaitMedium) + inv = inv.WithContext(ctx) + w := clitest.StartWithWaiter(t, inv) // Select "Yes" for the "Upload " prompt - pty.ExpectMatch("Upload") + pty.ExpectMatchContext(ctx, "Upload") pty.WriteLine("yes") - pty.ExpectMatch("var.without_default") + pty.ExpectMatchContext(ctx, "var.without_default") pty.WriteLine("test-value") - require.NoError(t, <-execDone) + w.RequireSuccess() }) t.Run("VariableSourcesPriority", func(t *testing.T) { @@ -1268,21 +1252,20 @@ cli_overrides_file_var: from-file`) clitest.SetupConfig(t, templateAdmin, root) pty := ptytest.New(t).Attach(inv) - execDone := make(chan error) - go func() { - execDone <- inv.Run() - }() + ctx := testutil.Context(t, testutil.WaitMedium) + inv = inv.WithContext(ctx) + w := clitest.StartWithWaiter(t, inv) // Select "Yes" for the "Upload " prompt - pty.ExpectMatch("Upload") + pty.ExpectMatchContext(ctx, "Upload") pty.WriteLine("yes") // Only check for prompt_var, other variables should not prompt - pty.ExpectMatch("var.prompt_var") - pty.ExpectMatch("Enter value:") + pty.ExpectMatchContext(ctx, "var.prompt_var") + pty.ExpectMatchContext(ctx, "Enter value:") pty.WriteLine("from-prompt") - require.NoError(t, <-execDone) + w.RequireSuccess() template, err := client.TemplateByName(context.Background(), owner.OrganizationID, "test-template") require.NoError(t, err) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 4ea20f1bed..4c81d43441 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -15301,7 +15301,7 @@ func (q *sqlQuerier) InsertTemplateVersionTerraformValuesByJobID(ctx context.Con } const getTemplateVersionVariables = `-- name: GetTemplateVersionVariables :many -SELECT template_version_id, name, description, type, value, default_value, required, sensitive FROM template_version_variables WHERE template_version_id = $1 +SELECT template_version_id, name, description, type, value, default_value, required, sensitive FROM template_version_variables WHERE template_version_id = $1 ORDER BY name ` func (q *sqlQuerier) GetTemplateVersionVariables(ctx context.Context, templateVersionID uuid.UUID) ([]TemplateVersionVariable, error) { diff --git a/coderd/database/queries/templateversionvariables.sql b/coderd/database/queries/templateversionvariables.sql index ff6c16a6df..3e37ed01d4 100644 --- a/coderd/database/queries/templateversionvariables.sql +++ b/coderd/database/queries/templateversionvariables.sql @@ -23,4 +23,4 @@ VALUES ) RETURNING *; -- name: GetTemplateVersionVariables :many -SELECT * FROM template_version_variables WHERE template_version_id = $1; +SELECT * FROM template_version_variables WHERE template_version_id = $1 ORDER BY name;