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.

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

test: move ReplicaofRejectOnLoad test from pytest into unit tests #4410

Merged
merged 1 commit into from
Jan 7, 2025

Conversation

BorysTheDev
Copy link
Contributor

@BorysTheDev BorysTheDev commented Jan 6, 2025

added test to check that replicaof can't run during the LOADING state

@BorysTheDev BorysTheDev requested a review from adiholden January 6, 2025 09:10
@@ -821,6 +821,14 @@ TEST_F(DflyEngineTest, StreamMemInfo) {
EXPECT_GT(stream_mem_second, 0);
}

TEST_F(DflyEngineTest, ReplicaofRejectOnLoad) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The idea of pytest below checks that:

  1. on server start we are in loading state
  2. once we move from loading state to active as we finished loading the file the replica of command success

This unit test is very limited

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we have a lot of tests where we start replica in active state. I don't see any reason to do the same

Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not mind adding an additional test.

@romange
Copy link
Collaborator

romange commented Jan 6, 2025

@BorysTheDev what's the reason it takes 5 minutes? It's indeed a lot

@BorysTheDev
Copy link
Contributor Author

@BorysTheDev what's the reason it takes 5 minutes? It's indeed a lot

I didn't check but I think because of debug populate 8000000 -> save -> load operations

@adiholden
Copy link
Collaborator

@BorysTheDev what's the reason it takes 5 minutes? It's indeed a lot

I didn't check but I think because of debug populate 8000000 -> save -> load operations

The debug populate is the most time consuming
We added so many keys here to gauranty the time it takes to load the data is long enough to catch the fail on loading.
Instead of populating 8m keys I believe we can feel the database in another way
f.e add sets with many elements using static seeder

@romange
Copy link
Collaborator

romange commented Jan 6, 2025

Let's maybe reduce it to 4000000 ? I do not want to optimize this too much, it's not important. This PR,
#4408 touches this test, I can make the change.

@BorysTheDev
Copy link
Contributor Author

BorysTheDev commented Jan 6, 2025

Let's maybe reduce it to 4000000 ? I do not want to optimize this too much, it's not important. This PR, #4408 touches this test, I can make the change.

I suggest to have 3 tests instead of this one:

  1. loading state during loading (pytest) - slow
  2. reject replicaof during loading state (unit test) - fast
  3. success replicaof after loading file (pytest) - fast

@BorysTheDev BorysTheDev force-pushed the move_ReplicaofRejectOnLoad_test branch from a21a118 to 0ffdb61 Compare January 6, 2025 11:13
@BorysTheDev BorysTheDev merged commit ed1436b into main Jan 7, 2025
9 checks passed
@BorysTheDev BorysTheDev deleted the move_ReplicaofRejectOnLoad_test branch January 7, 2025 08:20
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.

3 participants