chanbackup: fix test flake in TestUpdateAndSwap

To make it easier to debug, we break the old test into smaller ones and
fix a flake caused by uncleaned test dir.

chanbackup/backupfile_test.go

@@ -45,160 +45,101 @@ func assertFileDeleted(t *testing.T, filePath string) {
 	}
 }
 
-// TestUpdateAndSwap test that we're able to properly swap out old backups on
+// TestUpdateAndSwapWithArchive test that we're able to properly swap out old
-// disk with new ones. Additionally, after a swap operation succeeds, then each
+// backups on disk with new ones. In addition, we check for noBackupArchive to
-// time we should only have the main backup file on disk, as the temporary file
+// ensure that the archive file is created when it's set to false, and not
-// has been removed. Finally, we check for noBackupArchive to ensure that the
+// created when it's set to true.
-// archive file is created when it's set to false, and not created when it's
+func TestUpdateAndSwapWithArchive(t *testing.T) {
-// set to true.
-func TestUpdateAndSwap(t *testing.T) {
 	t.Parallel()
 
-	tempTestDir := t.TempDir()
-
 	testCases := []struct {
-		fileName     string
+		name            string
-		tempFileName string
-
-		oldTempExists   bool
 		noBackupArchive bool
-
-		valid bool
 	}{
-		// Main file name is blank, should fail.
-		{
-			fileName: "",
-			valid:    false,
-		},
-
-		// Old temporary file still exists, should be removed. Only one
-		// file should remain.
-		{
-			fileName: filepath.Join(
-				tempTestDir, DefaultBackupFileName,
-			),
-			tempFileName: filepath.Join(
-				tempTestDir, DefaultTempBackupFileName,
-			),
-			oldTempExists: true,
-			valid:         true,
-		},
-
-		// Old temp doesn't exist, should swap out file, only a single
-		// file remains.
-		{
-			fileName: filepath.Join(
-				tempTestDir, DefaultBackupFileName,
-			),
-			tempFileName: filepath.Join(
-				tempTestDir, DefaultTempBackupFileName,
-			),
-			valid: true,
-		},
-
 		// Test with noBackupArchive set to true - should not create
 		// archive.
 		{
-			fileName: filepath.Join(
+			name:            "no archive file",
-				tempTestDir, DefaultBackupFileName,
-			),
-			tempFileName: filepath.Join(
-				tempTestDir, DefaultTempBackupFileName,
-			),
 			noBackupArchive: true,
-			valid:           true,
 		},
 
-		// Test with v set to false - should create
+		// Test with noBackupArchive set to false - should create
 		// archive.
 		{
-			fileName: filepath.Join(
+			name:            "with archive file",
-				tempTestDir, DefaultBackupFileName,
-			),
-			tempFileName: filepath.Join(
-				tempTestDir, DefaultTempBackupFileName,
-			),
 			noBackupArchive: false,
-			valid:           true,
 		},
 	}
-	for i, testCase := range testCases {
+
-		backupFile := NewMultiFile(
+	for _, tc := range testCases {
-			testCase.fileName, testCase.noBackupArchive,
+		t.Run(tc.name, func(t *testing.T) {
+			tempTestDir := t.TempDir()
+
+			fileName := filepath.Join(
+				tempTestDir, DefaultBackupFileName,
+			)
+			tempFileName := filepath.Join(
+				tempTestDir, DefaultTempBackupFileName,
 			)
 
-		// To start with, we'll make a random byte slice that'll pose
+			backupFile := NewMultiFile(fileName, tc.noBackupArchive)
-		// as our packed multi backup.
+
+			// To start with, we'll make a random byte slice that'll
+			// pose as our packed multi backup.
 			newPackedMulti, err := makeFakePackedMulti()
-		if err != nil {
+			require.NoError(t, err)
-			t.Fatalf("unable to make test backup: %v", err)
-		}
 
-		// If the old temporary file is meant to exist, then we'll
+			// With our backup created, we'll now attempt to swap
-		// create it now as an empty file.
+			// out this backup, for the old one.
-		if testCase.oldTempExists {
+			err = backupFile.UpdateAndSwap(newPackedMulti)
-			f, err := os.Create(testCase.tempFileName)
+			require.NoError(t, err)
-			if err != nil {
-				t.Fatalf("unable to create temp file: %v", err)
-			}
-			require.NoError(t, f.Close())
-
-			// TODO(roasbeef): mock out fs calls?
-		}
-
-		// With our backup created, we'll now attempt to swap out this
-		// backup, for the old one.
-		err = backupFile.UpdateAndSwap(PackedMulti(newPackedMulti))
-		switch {
-		// If this is a valid test case, and we failed, then we'll
-		// return an error.
-		case err != nil && testCase.valid:
-			t.Fatalf("#%v, unable to swap file: %v", i, err)
-
-		// If this is an invalid test case, and we passed it, then
-		// we'll return an error.
-		case err == nil && !testCase.valid:
-			t.Fatalf("#%v file swap should have failed: %v", i, err)
-		}
-
-		if !testCase.valid {
-			continue
-		}
 
 			// If we read out the file on disk, then it should match
-		// exactly what we wrote. The temp backup file should also be
+			// exactly what we wrote. The temp backup file should
-		// gone.
+			// also be gone.
-		assertBackupMatches(t, testCase.fileName, newPackedMulti)
+			assertBackupMatches(t, fileName, newPackedMulti)
-		assertFileDeleted(t, testCase.tempFileName)
+			assertFileDeleted(t, tempFileName)
 
-		// Now that we know this is a valid test case, we'll make a new
+			// Now that we know this is a valid test case, we'll
-		// packed multi to swap out this current one.
+			// make a new packed multi to swap out this current one.
 			newPackedMulti2, err := makeFakePackedMulti()
-		if err != nil {
+			require.NoError(t, err)
-			t.Fatalf("unable to make test backup: %v", err)
-		}
 
-		// We'll then attempt to swap the old version for this new one.
+			// We'll then attempt to swap the old version for this
-		err = backupFile.UpdateAndSwap(PackedMulti(newPackedMulti2))
+			// new one.
-		if err != nil {
+			err = backupFile.UpdateAndSwap(newPackedMulti2)
-			t.Fatalf("unable to swap file: %v", err)
+			require.NoError(t, err)
-		}
 
 			// Once again, the file written on disk should have been
 			// properly swapped out with the new instance.
-		assertBackupMatches(t, testCase.fileName, newPackedMulti2)
+			assertBackupMatches(t, fileName, newPackedMulti2)
 
-		// Additionally, we shouldn't be able to find the temp backup
+			// Additionally, we shouldn't be able to find the temp
-		// file on disk, as it should be deleted each time.
+			// backup file on disk, as it should be deleted each
-		assertFileDeleted(t, testCase.tempFileName)
+			// time.
+			assertFileDeleted(t, tempFileName)
 
-		// Now check if archive was created when noBackupArchive is
+			// Now check if archive was created when noBackupArchive
-		// false.
+			// is false.
 			archiveDir := filepath.Join(
-			filepath.Dir(testCase.fileName),
+				filepath.Dir(fileName),
 				DefaultChanBackupArchiveDirName,
 			)
-		if !testCase.noBackupArchive {
+
+			// When noBackupArchive is true, no new archive file
+			// should be created.
+			//
+			// NOTE: In a real environment, the archive directory
+			// might exist with older backups before the feature is
+			// disabled, but for test simplicity (since we clean up
+			// the directory between test cases), we verify the
+			// directory doesn't exist at all.
+			if tc.noBackupArchive {
+				require.NoDirExists(t, archiveDir)
+				return
+			}
+
+			// Otherwise we expect an archive to be created.
 			files, err := os.ReadDir(archiveDir)
 			require.NoError(t, err)
 			require.Len(t, files, 1)
@@ -208,24 +149,130 @@ func TestUpdateAndSwap(t *testing.T) {
 			archiveFile := filepath.Join(
 				archiveDir, files[0].Name(),
 			)
-			// The archived content should match the previous
+
-			// backup (newPackedMulti) that was just swapped out.
+			// The archived content should match the previous backup
+			// (newPackedMulti) that was just swapped out.
 			assertBackupMatches(t, archiveFile, newPackedMulti)
 
 			// Clean up the archive directory.
 			os.RemoveAll(archiveDir)
+		})
+	}
+}
 
-			continue
+// TestUpdateAndSwap test that we're able to properly swap out old backups on
+// disk with new ones. Additionally, after a swap operation succeeds, then each
+// time we should only have the main backup file on disk, as the temporary file
+// has been removed.
+func TestUpdateAndSwap(t *testing.T) {
+	t.Parallel()
+
+	// Check that when the main file name is blank, an error is returned.
+	backupFile := NewMultiFile("", false)
+
+	err := backupFile.UpdateAndSwap(PackedMulti(nil))
+	require.ErrorIs(t, err, ErrNoBackupFileExists)
+
+	testCases := []struct {
+		name          string
+		oldTempExists bool
+	}{
+		// Old temporary file still exists, should be removed. Only one
+		// file should remain.
+		{
+			name:          "remove old temp file",
+			oldTempExists: true,
+		},
+
+		// Old temp doesn't exist, should swap out file, only a single
+		// file remains.
+		{
+			name:          "swap out file",
+			oldTempExists: false,
+		},
 	}
 
-		// When noBackupArchive is true, no new archive file should be
+	for _, tc := range testCases {
-		// created. Note: In a real environment, the archive directory
+		t.Run(tc.name, func(t *testing.T) {
-		// might exist with older backups before the feature is
+			tempTestDir := t.TempDir()
-		// disabled, but for test simplicity (since we clean up the
-		// directory between test cases), we verify the directory
-		// doesn't exist at all.
-		require.NoDirExists(t, archiveDir)
 
+			fileName := filepath.Join(
+				tempTestDir, DefaultBackupFileName,
+			)
+			tempFileName := filepath.Join(
+				tempTestDir, DefaultTempBackupFileName,
+			)
+
+			backupFile := NewMultiFile(fileName, false)
+
+			// To start with, we'll make a random byte slice that'll
+			// pose as our packed multi backup.
+			newPackedMulti, err := makeFakePackedMulti()
+			require.NoError(t, err)
+
+			// If the old temporary file is meant to exist, then
+			// we'll create it now as an empty file.
+			if tc.oldTempExists {
+				f, err := os.Create(tempFileName)
+				require.NoError(t, err)
+				require.NoError(t, f.Close())
+
+				// TODO(roasbeef): mock out fs calls?
+			}
+
+			// With our backup created, we'll now attempt to swap
+			// out this backup, for the old one.
+			err = backupFile.UpdateAndSwap(newPackedMulti)
+			require.NoError(t, err)
+
+			// If we read out the file on disk, then it should match
+			// exactly what we wrote. The temp backup file should
+			// also be gone.
+			assertBackupMatches(t, fileName, newPackedMulti)
+			assertFileDeleted(t, tempFileName)
+
+			// Now that we know this is a valid test case, we'll
+			// make a new packed multi to swap out this current one.
+			newPackedMulti2, err := makeFakePackedMulti()
+			require.NoError(t, err)
+
+			// We'll then attempt to swap the old version for this
+			// new one.
+			err = backupFile.UpdateAndSwap(newPackedMulti2)
+			require.NoError(t, err)
+
+			// Once again, the file written on disk should have been
+			// properly swapped out with the new instance.
+			assertBackupMatches(t, fileName, newPackedMulti2)
+
+			// Additionally, we shouldn't be able to find the temp
+			// backup file on disk, as it should be deleted each
+			// time.
+			assertFileDeleted(t, tempFileName)
+
+			// Now check if archive was created when noBackupArchive
+			// is false.
+			archiveDir := filepath.Join(
+				filepath.Dir(fileName),
+				DefaultChanBackupArchiveDirName,
+			)
+			files, err := os.ReadDir(archiveDir)
+			require.NoError(t, err)
+			require.Len(t, files, 1)
+
+			// Verify the archive contents match the previous
+			// backup.
+			archiveFile := filepath.Join(
+				archiveDir, files[0].Name(),
+			)
+
+			// The archived content should match the previous backup
+			// (newPackedMulti) that was just swapped out.
+			assertBackupMatches(t, archiveFile, newPackedMulti)
+
+			// Clean up the archive directory.
+			os.RemoveAll(archiveDir)
+		})
 	}
 }