Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chanbackup: fix test flake in TestUpdateAndSwap #9563

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

chanbackup: fix test flake in TestUpdateAndSwap #9563

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
317 changes: 182 additions & 135 deletions chanbackup/backupfile_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,160 +45,217 @@ 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(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
}

// 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()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, all the test cases share the same folder.


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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this case, we would have a file left on the disk. Since noBackupArchive is true, we won't create an archive for it.

// 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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now enter this test, with the leftover state from the previous test, in which a file was left on the disk.

// 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))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because a file was left on the disk from the previous test case, this call will create an archive.

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(newPackedMulti)
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(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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the two archives share the same filename most of the time, this would pass, otherwise it will fail.

Expand All @@ -208,24 +265,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
Loading