From 76ade177af96e1ee74a6b87d5b82bed8344ed7f6 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Fri, 28 Feb 2025 02:08:14 +0800 Subject: [PATCH] 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 | 337 +++++++++++++++++++--------------- 1 file changed, 192 insertions(+), 145 deletions(-) diff --git a/chanbackup/backupfile_test.go b/chanbackup/backupfile_test.go index 31fcfc721..e97d4ceba 100644 --- a/chanbackup/backupfile_test.go +++ b/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 -// 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. Finally, we check for noBackupArchive to ensure that the -// archive file is created when it's set to false, and not created when it's -// set to true. -func TestUpdateAndSwap(t *testing.T) { +// TestUpdateAndSwapWithArchive test that we're able to properly swap out old +// backups on disk with new ones. In addition, we check for noBackupArchive to +// ensure that the archive file is created when it's set to false, and not +// created when it's set to true. +func TestUpdateAndSwapWithArchive(t *testing.T) { t.Parallel() - tempTestDir := t.TempDir() - testCases := []struct { - fileName string - tempFileName string - - oldTempExists bool + name string 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( - tempTestDir, DefaultBackupFileName, - ), - tempFileName: filepath.Join( - tempTestDir, DefaultTempBackupFileName, - ), + name: "no archive file", noBackupArchive: true, - valid: true, }, - // Test with v set to false - should create + // Test with noBackupArchive set to false - should create // archive. { - fileName: filepath.Join( - tempTestDir, DefaultBackupFileName, - ), - tempFileName: filepath.Join( - tempTestDir, DefaultTempBackupFileName, - ), + name: "with archive file", noBackupArchive: false, - valid: true, }, } - for i, testCase := range testCases { - backupFile := NewMultiFile( - testCase.fileName, testCase.noBackupArchive, - ) - // To start with, we'll make a random byte slice that'll pose - // as our packed multi backup. - newPackedMulti, err := makeFakePackedMulti() - if err != nil { - t.Fatalf("unable to make test backup: %v", err) - } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + tempTestDir := t.TempDir() - // If the old temporary file is meant to exist, then we'll - // create it now as an empty file. - if testCase.oldTempExists { - f, err := os.Create(testCase.tempFileName) - if err != nil { - t.Fatalf("unable to create temp file: %v", err) + fileName := filepath.Join( + tempTestDir, DefaultBackupFileName, + ) + tempFileName := filepath.Join( + tempTestDir, DefaultTempBackupFileName, + ) + + backupFile := NewMultiFile(fileName, tc.noBackupArchive) + + // 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) + + // 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, + ) + + // 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 } - 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 - // gone. - assertBackupMatches(t, testCase.fileName, newPackedMulti) - assertFileDeleted(t, testCase.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() - if err != nil { - t.Fatalf("unable to make test backup: %v", err) - } - - // We'll then attempt to swap the old version for this new one. - err = backupFile.UpdateAndSwap(PackedMulti(newPackedMulti2)) - if err != nil { - t.Fatalf("unable to swap file: %v", err) - } - - // Once again, the file written on disk should have been - // properly swapped out with the new instance. - assertBackupMatches(t, testCase.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, testCase.tempFileName) - - // Now check if archive was created when noBackupArchive is - // false. - archiveDir := filepath.Join( - filepath.Dir(testCase.fileName), - DefaultChanBackupArchiveDirName, - ) - if !testCase.noBackupArchive { + // 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() - // 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. - require.NoDirExists(t, archiveDir) + // 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, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + tempTestDir := t.TempDir() + + 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) + }) } }