Skip to content

Commit

Permalink
chanbackup: fix test flake in TestUpdateAndSwap
Browse files Browse the repository at this point in the history 
To make it easier to debug, we break the old test into smaller ones and
fix a flake caused by uncleaned test dir.
  • Loading branch information
@yyforyongyu
yyforyongyu committed Feb 27, 2025
1 parent e3d9fcb commit 2593aba
Showing 1 changed file with 190 additions and 135 deletions.
325 changes: 190 additions & 135 deletions chanbackup/backupfile_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,160 +45,225 @@ func assertFileDeleted(t *testing.T, filePath string) {
}
}

// 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()

testCases := []struct {
name string
noBackupArchive bool
}{
// Test with noBackupArchive set to true - should not create
// archive.
{
name: "no archive file",
noBackupArchive: true,
},

// Test with noBackupArchive set to false - should create
// archive.
{
name: "with archive file",
noBackupArchive: 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, 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(
PackedMulti(newPackedMulti),

Check failure on line 95 in chanbackup/backupfile_test.go

View workflow job for this annotation

GitHub Actions / lint code 

unnecessary conversion (unconvert)
)
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(
PackedMulti(newPackedMulti2),

Check failure on line 113 in chanbackup/backupfile_test.go

View workflow job for this annotation

GitHub Actions / lint code 

unnecessary conversion (unconvert)
)
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
}

// Otherwise we expect an archive to be created.
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)
})
}
}

// 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.
// has been removed.
func TestUpdateAndSwap(t *testing.T) {
t.Parallel()

tempTestDir := t.TempDir()

testCases := []struct {
fileName string
tempFileName string
// Check that when the main file name is blank, an error is returned.
backupFile := NewMultiFile("", false)

oldTempExists bool
noBackupArchive bool
err := backupFile.UpdateAndSwap(PackedMulti(nil))
require.ErrorIs(t, err, ErrNoBackupFileExists)

valid bool
testCases := []struct {
name string
oldTempExists 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,
),
name: "remove old temp file",
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,
name: "swap out file",
oldTempExists: false,
},
}

// Test with noBackupArchive set to true - should not create
// archive.
{
fileName: filepath.Join(
tempTestDir, DefaultBackupFileName,
),
tempFileName: filepath.Join(
tempTestDir, DefaultTempBackupFileName,
),
noBackupArchive: true,
valid: true,
},
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
tempTestDir := t.TempDir()

// Test with v set to false - should create
// archive.
{
fileName: filepath.Join(
fileName := filepath.Join(
tempTestDir, DefaultBackupFileName,
),
tempFileName: filepath.Join(
)
tempFileName := filepath.Join(
tempTestDir, DefaultTempBackupFileName,
),
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)
}
backupFile := NewMultiFile(fileName, false)

// 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)
}
require.NoError(t, f.Close())
// 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)

// TODO(roasbeef): mock out fs calls?
}
// 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())

// 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)
// TODO(roasbeef): mock out fs calls?
}

// 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)
}
// With our backup created, we'll now attempt to swap
// out this backup, for the old one.
err = backupFile.UpdateAndSwap(
PackedMulti(newPackedMulti),

Check failure on line 230 in chanbackup/backupfile_test.go

View workflow job for this annotation

GitHub Actions / lint code 

unnecessary conversion (unconvert)
)
require.NoError(t, 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, fileName, newPackedMulti)
assertFileDeleted(t, tempFileName)

// 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()
require.NoError(t, err)

// 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),
)
require.NoError(t, 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, fileName, newPackedMulti2)

// 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 {
// 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)
Expand All @@ -208,24 +273,14 @@ 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
}

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

})
}
}

Expand Down

0 comments on commit 2593aba

Please sign in to comment.