Skip to content

fix: RdbTest.LoadStream3 incorrect file usage #4242

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.

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

Merged
merged 1 commit into from
Dec 2, 2024
Merged

Conversation

BorysTheDev
Copy link
Contributor

problem: RdbTest.LoadStream3 used incorrect file
fix: change file name to correct

Also, I've moved RdbTest.SnapshotTooBig to the end of the file to check the reproducibility of #4215, because it tests OOM error and I don't have other ideas for now

@BorysTheDev BorysTheDev requested a review from kostasrim December 2, 2024 14:28
@@ -668,15 +659,24 @@ TEST_F(RdbTest, LoadStream2) {
}

TEST_F(RdbTest, LoadStream3) {
auto ec = LoadRdb("RDB_TYPE_STREAM_LISTPACKS_2.rdb");
auto ec = LoadRdb("RDB_TYPE_STREAM_LISTPACKS_3.rdb");
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was my bug, It looks I haven't saved it before commit when I wrote it

TEST_F(RdbTest, LoadStream2) {
auto ec = LoadRdb("RDB_TYPE_STREAM_LISTPACKS_2.rdb");
ASSERT_FALSE(ec) << ec.message();
auto res = Run({"XINFO", "STREAM", "mystream"});
EXPECT_THAT(
ASSERT_THAT(
Copy link
Contributor

Choose a reason for hiding this comment

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

I am still not sure why this fails and I don't see how moving SnapshotTooBig below will have any effect or it will somehow stop reproducing since on each test case we initialize everything from the start so i don't see how changing maxmemory of one test case can affect another 🤷

Out of curiosity did you try running the test in a loop in a similar machine as the one it failed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. But let's merge this fix and I will run the test in a loop.

@BorysTheDev BorysTheDev merged commit 935ae86 into main Dec 2, 2024
9 checks passed
@BorysTheDev BorysTheDev deleted the fix_LoadStream3Test branch December 2, 2024 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants