From 1c09e58b3da5347f40554711ca3039a53caa0629 Mon Sep 17 00:00:00 2001 From: Sarah French <15078782+SarahFrench@users.noreply.github.com> Date: Thu, 28 Aug 2025 14:33:50 +0100 Subject: [PATCH] Fix diagnostic comparison issues in `s3` backend tests (#37509) * Fix S3 backend test affected by making the Workspaces method return errors via diagnostics * Address diagnostics comparison issues in test by ensuring expected diagnostics are defined in the context of the config they're triggered by * Fix failing test case `TestBackendConfig_EC2MetadataEndpoint/envvar_invalid_mode` by making `diagnosticBase` struct comparable * Add compile-time checks that diagnostic types fulfil interfaces * Stop diagnosticBase implementing ComparableDiagnostic, re-add S3-specific comparer code to s3 package * Update tests to use the S3-specific comparer again * Fix test case missed in refactoring --- .../backend/remote-state/s3/backend_test.go | 24 ++++++++-------- .../backend/remote-state/s3/testing_test.go | 28 +++++++++++++++++++ internal/tfdiags/contextual.go | 6 ++++ internal/tfdiags/diagnostic_base.go | 7 +++++ internal/tfdiags/hcl.go | 1 + internal/tfdiags/rpc_friendly.go | 3 ++ 6 files changed, 58 insertions(+), 11 deletions(-) create mode 100644 internal/backend/remote-state/s3/testing_test.go diff --git a/internal/backend/remote-state/s3/backend_test.go b/internal/backend/remote-state/s3/backend_test.go index 865f4dda6d..3e0766054e 100644 --- a/internal/backend/remote-state/s3/backend_test.go +++ b/internal/backend/remote-state/s3/backend_test.go @@ -362,7 +362,7 @@ func TestBackendConfig_InvalidRegion(t *testing.T) { confDiags := b.Configure(configSchema) diags = diags.Append(confDiags) - if diff := cmp.Diff(diags, tc.expectedDiags, tfdiags.DiagnosticComparer); diff != "" { + if diff := cmp.Diff(diags, tc.expectedDiags, diagnosticComparer); diff != "" { t.Errorf("unexpected diagnostics difference: %s", diff) } }) @@ -528,7 +528,7 @@ func TestBackendConfig_DynamoDBEndpoint(t *testing.T) { raw, diags := testBackendConfigDiags(t, New(), backend.TestWrapConfig(config)) b := raw.(*Backend) - if diff := cmp.Diff(diags, tc.expectedDiags, tfdiags.DiagnosticComparer); diff != "" { + if diff := cmp.Diff(diags, tc.expectedDiags, diagnosticComparer); diff != "" { t.Errorf("unexpected diagnostics difference: %s", diff) } @@ -616,7 +616,8 @@ func TestBackendConfig_IAMEndpoint(t *testing.T) { pathString(cty.GetAttrPath("iam_endpoint")), pathString(cty.GetAttrPath("endpoints").GetAttr("iam")), ), - )}, + ), + }, }, "envvar": { vars: map[string]string{ @@ -660,7 +661,7 @@ func TestBackendConfig_IAMEndpoint(t *testing.T) { _, diags := testBackendConfigDiags(t, New(), backend.TestWrapConfig(config)) - if diff := cmp.Diff(diags, tc.expectedDiags, tfdiags.DiagnosticComparer); diff != "" { + if diff := cmp.Diff(diags, tc.expectedDiags, diagnosticComparer); diff != "" { t.Errorf("unexpected diagnostics difference: %s", diff) } }) @@ -736,7 +737,8 @@ func TestBackendConfig_S3Endpoint(t *testing.T) { pathString(cty.GetAttrPath("endpoint")), pathString(cty.GetAttrPath("endpoints").GetAttr("s3")), ), - )}, + ), + }, }, "envvar": { vars: map[string]string{ @@ -783,7 +785,7 @@ func TestBackendConfig_S3Endpoint(t *testing.T) { raw, diags := testBackendConfigDiags(t, New(), backend.TestWrapConfig(config)) b := raw.(*Backend) - if diff := cmp.Diff(diags, tc.expectedDiags, tfdiags.DiagnosticComparer); diff != "" { + if diff := cmp.Diff(diags, tc.expectedDiags, diagnosticComparer); diff != "" { t.Errorf("unexpected diagnostics difference: %s", diff) } @@ -943,7 +945,7 @@ func TestBackendConfig_EC2MetadataEndpoint(t *testing.T) { raw, diags := testBackendConfigDiags(t, New(), backend.TestWrapConfig(config)) b := raw.(*Backend) - if diff := cmp.Diff(diags, tc.expectedDiags, tfdiags.DiagnosticComparer); diff != "" { + if diff := cmp.Diff(diags, tc.expectedDiags, diagnosticComparer); diff != "" { t.Errorf("unexpected diagnostics difference: %s", diff) } @@ -1435,7 +1437,7 @@ func TestBackendConfig_PrepareConfigValidation(t *testing.T) { _, valDiags := b.PrepareConfig(populateSchema(t, b.ConfigSchema(), tc.config)) - if diff := cmp.Diff(valDiags, tc.expectedDiags, tfdiags.DiagnosticComparer); diff != "" { + if diff := cmp.Diff(valDiags, tc.expectedDiags, diagnosticComparer); diff != "" { t.Errorf("unexpected diagnostics difference: %s", diff) } }) @@ -2497,7 +2499,7 @@ func TestBackendConfigKmsKeyId(t *testing.T) { diags = diags.Append(confDiags) } - if diff := cmp.Diff(diags, tc.expectedDiags, tfdiags.DiagnosticComparer); diff != "" { + if diff := cmp.Diff(diags, tc.expectedDiags, diagnosticComparer); diff != "" { t.Fatalf("unexpected diagnostics difference: %s", diff) } @@ -2627,7 +2629,7 @@ func TestBackendSSECustomerKey(t *testing.T) { diags = diags.Append(confDiags) } - if diff := cmp.Diff(diags, tc.expectedDiags, tfdiags.DiagnosticComparer); diff != "" { + if diff := cmp.Diff(diags, tc.expectedDiags, diagnosticComparer); diff != "" { t.Fatalf("unexpected diagnostics difference: %s", diff) } @@ -3287,7 +3289,7 @@ func TestAssumeRole_PrepareConfigValidation(t *testing.T) { var diags tfdiags.Diagnostics validateNestedAttribute(assumeRoleSchema, config, path, &diags) - if diff := cmp.Diff(diags, tc.expectedDiags, tfdiags.DiagnosticComparer); diff != "" { + if diff := cmp.Diff(diags, tc.expectedDiags, diagnosticComparer); diff != "" { t.Errorf("unexpected diagnostics difference: %s", diff) } }) diff --git a/internal/backend/remote-state/s3/testing_test.go b/internal/backend/remote-state/s3/testing_test.go new file mode 100644 index 0000000000..13d3056ee0 --- /dev/null +++ b/internal/backend/remote-state/s3/testing_test.go @@ -0,0 +1,28 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: BUSL-1.1 + +package s3 + +import ( + "github.com/google/go-cmp/cmp" + "github.com/hashicorp/terraform/internal/tfdiags" +) + +var diagnosticComparer cmp.Option = cmp.Comparer(diagnosticComparerS3) + +// diagnosticComparerS3 is a Comparer function for use with cmp.Diff to compare two tfdiags.Diagnostic values +func diagnosticComparerS3(l, r tfdiags.Diagnostic) bool { + if l.Severity() != r.Severity() { + return false + } + if l.Description() != r.Description() { + return false + } + + lp := tfdiags.GetAttribute(l) + rp := tfdiags.GetAttribute(r) + if len(lp) != len(rp) { + return false + } + return lp.Equals(rp) +} diff --git a/internal/tfdiags/contextual.go b/internal/tfdiags/contextual.go index 7f2bd5e5df..3eedf82bfe 100644 --- a/internal/tfdiags/contextual.go +++ b/internal/tfdiags/contextual.go @@ -99,6 +99,9 @@ func GetAttribute(d Diagnostic) cty.Path { return nil } +var _ Diagnostic = &attributeDiagnostic{} +var _ ComparableDiagnostic = &attributeDiagnostic{} + type attributeDiagnostic struct { diagnosticBase attrPath cty.Path @@ -384,6 +387,9 @@ func WholeContainingBody(severity Severity, summary, detail string) Diagnostic { } } +var _ Diagnostic = &wholeBodyDiagnostic{} +var _ ComparableDiagnostic = &wholeBodyDiagnostic{} + type wholeBodyDiagnostic struct { diagnosticBase subject *SourceRange // populated only after ElaborateFromConfigBody diff --git a/internal/tfdiags/diagnostic_base.go b/internal/tfdiags/diagnostic_base.go index 6c0db08280..e942db7ad6 100644 --- a/internal/tfdiags/diagnostic_base.go +++ b/internal/tfdiags/diagnostic_base.go @@ -15,6 +15,13 @@ type diagnosticBase struct { address string } +var _ Diagnostic = &diagnosticBase{} + +// diagnosticBase doesn't implement ComparableDiagnostic because the lack of source data +// means separate diagnostics might be falsely identified as equal. This poses a user-facing +// risk if deduplication of diagnostics removes a diagnostic that's incorrectly been identified +// as a duplicate via comparison. + func (d diagnosticBase) Severity() Severity { return d.severity } diff --git a/internal/tfdiags/hcl.go b/internal/tfdiags/hcl.go index 3531237477..5db8c6ad1c 100644 --- a/internal/tfdiags/hcl.go +++ b/internal/tfdiags/hcl.go @@ -13,6 +13,7 @@ type hclDiagnostic struct { } var _ Diagnostic = hclDiagnostic{} +var _ ComparableDiagnostic = hclDiagnostic{} func (d hclDiagnostic) Severity() Severity { switch d.diag.Severity { diff --git a/internal/tfdiags/rpc_friendly.go b/internal/tfdiags/rpc_friendly.go index 223a21aa60..69ccee8305 100644 --- a/internal/tfdiags/rpc_friendly.go +++ b/internal/tfdiags/rpc_friendly.go @@ -15,6 +15,9 @@ type rpcFriendlyDiag struct { Context_ *SourceRange } +var _ Diagnostic = &rpcFriendlyDiag{} +var _ ComparableDiagnostic = &rpcFriendlyDiag{} + // rpcFriendlyDiag transforms a given diagnostic so that is more friendly to // RPC. //