chanbackup: fix test flake in TestUpdateAndSwap
#9563
Conversation
yyforyongyu
added
flake fix
no-itest
no-changelog
size/micro
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
To make it easier to debug, we break the old test into smaller ones and fix a flake caused by uncleaned test dir.
yyforyongyu
force-pushed
the
fix-unit-test
branch
from
2593aba to
587020a
Compare
There was a problem hiding this comment.
Thanks for the refactor. That's definitely nice to have!
But I think the core issue is that we use time.Now() for the file name:
Line 197 in 84f039d
So whenever this test runs exactly when the system time changes from xx:xx:59 to xx:xy:00, I think we'll still have the same problem.
IMO we could use a clock.Clock in the MultiFile struct that we can then control in the unit test.
There was a problem hiding this comment.
Left a more detailed analysis in the comments @guggero. You are right the time.Now created the issue, and it's the source of the flake due to how the test was written previously - it created an uncleaned state for the following test, and the test was difficult to understand.
Unless we want to test the creation of multiple archive files, I don't think we need to touch the actual code, plus I think the flake is gone,
Test TestUpdateAndSwap passed, count: 1170
Test TestUpdateAndSwap passed, count: 1171
| func TestUpdateAndSwap(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| tempTestDir := t.TempDir() |
There was a problem hiding this comment.
Previously, all the test cases share the same folder.
|
|
||
| // Test with noBackupArchive set to true - should not create |
There was a problem hiding this comment.
After this case, we would have a file left on the disk. Since noBackupArchive is true, we won't create an archive for it.
|
|
||
| // Test with v set to false - should create |
There was a problem hiding this comment.
We now enter this test, with the leftover state from the previous test, in which a file was left on the disk.
|
|
||
| // With our backup created, we'll now attempt to swap out this | ||
| // backup, for the old one. | ||
| err = backupFile.UpdateAndSwap(PackedMulti(newPackedMulti)) |
There was a problem hiding this comment.
Because a file was left on the disk from the previous test case, this call will create an archive.
| archiveDir := filepath.Join( | ||
| filepath.Dir(fileName), | ||
| DefaultChanBackupArchiveDirName, | ||
| ) | ||
| files, err := os.ReadDir(archiveDir) | ||
| require.NoError(t, err) | ||
| require.Len(t, files, 1) |
There was a problem hiding this comment.
Since the two archives share the same filename most of the time, this would pass, otherwise it will fail.
To make it easier to debug, we break the old test into smaller ones and fix a flake caused by uncleaned test dir.
Flake found in this build, which can be reproduced locally via
make flakehunter-unit pkg=chanbackup case=TestUpdateAndSwap,