Fix diagnostic comparison issues in s3 backend tests (#37509) (#37838)

* 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

Author: SarahFrench

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)
 			}
 
@@ -3281,7 +3283,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)
 			}
 		})

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)
+}

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

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
 }

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 {

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.
 //