states/remote: use t.Run in table-based tests

These tests were originally written long before Go supported subtests explicitly, but now that we have t.Run we can avoid the prior problem that one test failing would mask all of the others that followed it. Now we'll always run all of them, potentially collecting more errors in a single run so we can have more context to debug with and potentially fix them all in a single step rather than one by one.
hashicorp · Jul 21, 2022 · 69aa0a2 · 69aa0a2
1 parent ad5ac89
commit 69aa0a2
Showing 1 changed file with 92 additions and 86 deletions.
diff --git a/internal/states/remote/state_test.go b/internal/states/remote/state_test.go
@@ -245,28 +245,30 @@ func TestStatePersist(t *testing.T) {
 
 	// Run tests in order.
 	for _, tc := range testCases {
-		s, cleanup := tc.mutationFunc(mgr)
-
-		if err := mgr.WriteState(s); err != nil {
-			t.Fatalf("failed to WriteState for %q: %s", tc.name, err)
-		}
-		if err := mgr.PersistState(); err != nil {
-			t.Fatalf("failed to PersistState for %q: %s", tc.name, err)
-		}
-
-		if tc.isRequested(t) {
-			// Get captured request from the mock client log
-			// based on the index of the current test
-			if logIdx >= len(mockClient.log) {
-				t.Fatalf("request lock and index are out of sync on %q: idx=%d len=%d", tc.name, logIdx, len(mockClient.log))
+		t.Run(tc.name, func(t *testing.T) {
+			s, cleanup := tc.mutationFunc(mgr)
+
+			if err := mgr.WriteState(s); err != nil {
+				t.Fatalf("failed to WriteState for %q: %s", tc.name, err)
 			}
-			loggedRequest := mockClient.log[logIdx]
-			logIdx++
-			if diff := cmp.Diff(tc.expectedRequest, loggedRequest); len(diff) > 0 {
-				t.Fatalf("incorrect client requests for %q:\n%s", tc.name, diff)
+			if err := mgr.PersistState(); err != nil {
+				t.Fatalf("failed to PersistState for %q: %s", tc.name, err)
+			}
+
+			if tc.isRequested(t) {
+				// Get captured request from the mock client log
+				// based on the index of the current test
+				if logIdx >= len(mockClient.log) {
+					t.Fatalf("request lock and index are out of sync on %q: idx=%d len=%d", tc.name, logIdx, len(mockClient.log))
+				}
+				loggedRequest := mockClient.log[logIdx]
+				logIdx++
+				if diff := cmp.Diff(tc.expectedRequest, loggedRequest); len(diff) > 0 {
+					t.Fatalf("incorrect client requests for %q:\n%s", tc.name, diff)
+				}
 			}
-		}
-		cleanup()
+			cleanup()
+		})
 	}
 	logCnt := len(mockClient.log)
 	if logIdx != logCnt {
@@ -395,37 +397,39 @@ func TestWriteStateForMigration(t *testing.T) {
 	logIdx := 0
 
 	for _, tc := range testCases {
-		sf := tc.stateFile(mgr)
-		err := mgr.WriteStateForMigration(sf, tc.force)
-		shouldError := tc.expectedError != ""
-
-		// If we are expecting and error check it and move on
-		if shouldError {
-			if err == nil {
-				t.Fatalf("test case %q should have failed with error %q", tc.name, tc.expectedError)
-			} else if err.Error() != tc.expectedError {
-				t.Fatalf("test case %q expected error %q but got %q", tc.name, tc.expectedError, err)
+		t.Run(tc.name, func(t *testing.T) {
+			sf := tc.stateFile(mgr)
+			err := mgr.WriteStateForMigration(sf, tc.force)
+			shouldError := tc.expectedError != ""
+
+			// If we are expecting and error check it and move on
+			if shouldError {
+				if err == nil {
+					t.Fatalf("test case %q should have failed with error %q", tc.name, tc.expectedError)
+				} else if err.Error() != tc.expectedError {
+					t.Fatalf("test case %q expected error %q but got %q", tc.name, tc.expectedError, err)
+				}
+				return
+			}
+
+			if err != nil {
+				t.Fatalf("test case %q failed: %v", tc.name, err)
+			}
+
+			// At this point we should just do a normal write and persist
+			// as would happen from the CLI
+			mgr.WriteState(mgr.State())
+			mgr.PersistState()
+
+			if logIdx >= len(mockClient.log) {
+				t.Fatalf("request lock and index are out of sync on %q: idx=%d len=%d", tc.name, logIdx, len(mockClient.log))
 			}
-			continue
-		}
-
-		if err != nil {
-			t.Fatalf("test case %q failed: %v", tc.name, err)
-		}
-
-		// At this point we should just do a normal write and persist
-		// as would happen from the CLI
-		mgr.WriteState(mgr.State())
-		mgr.PersistState()
-
-		if logIdx >= len(mockClient.log) {
-			t.Fatalf("request lock and index are out of sync on %q: idx=%d len=%d", tc.name, logIdx, len(mockClient.log))
-		}
-		loggedRequest := mockClient.log[logIdx]
-		logIdx++
-		if diff := cmp.Diff(tc.expectedRequest, loggedRequest); len(diff) > 0 {
-			t.Fatalf("incorrect client requests for %q:\n%s", tc.name, diff)
-		}
+			loggedRequest := mockClient.log[logIdx]
+			logIdx++
+			if diff := cmp.Diff(tc.expectedRequest, loggedRequest); len(diff) > 0 {
+				t.Fatalf("incorrect client requests for %q:\n%s", tc.name, diff)
+			}
+		})
 	}
 
 	logCnt := len(mockClient.log)
@@ -551,43 +555,45 @@ func TestWriteStateForMigrationWithForcePushClient(t *testing.T) {
 	logIdx := 0
 
 	for _, tc := range testCases {
-		// Always reset client to not be force pushing
-		mockClient.force = false
-		sf := tc.stateFile(mgr)
-		err := mgr.WriteStateForMigration(sf, tc.force)
-		shouldError := tc.expectedError != ""
-
-		// If we are expecting and error check it and move on
-		if shouldError {
-			if err == nil {
-				t.Fatalf("test case %q should have failed with error %q", tc.name, tc.expectedError)
-			} else if err.Error() != tc.expectedError {
-				t.Fatalf("test case %q expected error %q but got %q", tc.name, tc.expectedError, err)
+		t.Run(tc.name, func(t *testing.T) {
+			// Always reset client to not be force pushing
+			mockClient.force = false
+			sf := tc.stateFile(mgr)
+			err := mgr.WriteStateForMigration(sf, tc.force)
+			shouldError := tc.expectedError != ""
+
+			// If we are expecting and error check it and move on
+			if shouldError {
+				if err == nil {
+					t.Fatalf("test case %q should have failed with error %q", tc.name, tc.expectedError)
+				} else if err.Error() != tc.expectedError {
+					t.Fatalf("test case %q expected error %q but got %q", tc.name, tc.expectedError, err)
+				}
+				return
+			}
+
+			if err != nil {
+				t.Fatalf("test case %q failed: %v", tc.name, err)
+			}
+
+			if tc.force && !mockClient.force {
+				t.Fatalf("test case %q should have enabled force push", tc.name)
+			}
+
+			// At this point we should just do a normal write and persist
+			// as would happen from the CLI
+			mgr.WriteState(mgr.State())
+			mgr.PersistState()
+
+			if logIdx >= len(mockClient.log) {
+				t.Fatalf("request lock and index are out of sync on %q: idx=%d len=%d", tc.name, logIdx, len(mockClient.log))
+			}
+			loggedRequest := mockClient.log[logIdx]
+			logIdx++
+			if diff := cmp.Diff(tc.expectedRequest, loggedRequest); len(diff) > 0 {
+				t.Fatalf("incorrect client requests for %q:\n%s", tc.name, diff)
 			}
-			continue
-		}
-
-		if err != nil {
-			t.Fatalf("test case %q failed: %v", tc.name, err)
-		}
-
-		if tc.force && !mockClient.force {
-			t.Fatalf("test case %q should have enabled force push", tc.name)
-		}
-
-		// At this point we should just do a normal write and persist
-		// as would happen from the CLI
-		mgr.WriteState(mgr.State())
-		mgr.PersistState()
-
-		if logIdx >= len(mockClient.log) {
-			t.Fatalf("request lock and index are out of sync on %q: idx=%d len=%d", tc.name, logIdx, len(mockClient.log))
-		}
-		loggedRequest := mockClient.log[logIdx]
-		logIdx++
-		if diff := cmp.Diff(tc.expectedRequest, loggedRequest); len(diff) > 0 {
-			t.Fatalf("incorrect client requests for %q:\n%s", tc.name, diff)
-		}
+		})
 	}
 
 	logCnt := len(mockClient.log)