-
Notifications
You must be signed in to change notification settings - Fork 894
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
fix: fix cluster test #2612
fix: fix cluster test #2612
Conversation
tests/dragonfly/cluster_test.py
Outdated
@@ -1104,12 +1116,12 @@ async def list_counter(key, client: aioredis.RedisCluster): | |||
for counter in counters: | |||
counter.cancel() | |||
|
|||
# Push new config | |||
await push_config(json.dumps(await generate_config()), [node.admin_client for node in nodes]) | |||
|
|||
# Generate capture, capture ignores counter keys | |||
capture = await seeder.capture() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acually I think we should call capture before we issue SLOT-MIGRATION-FINALIZE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can capture before and after, consistency shouldn't break for outside observers. We could even capture 3 times and check it's the same every time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG. unfortunately, I've been working on the same area doing pretty much the same thing :|
I was doing some screenshots for a testing presentation and wanted to show this test as an example. But then I noticed it was wrong 😂 |
* fix: fix cluster test
The test didn't check anything because we pushed the config before actually generating the capture. Note that in my original #2496 this is not the case 🤨