chore: fix test_snapshot #4607
chore: fix test_snapshot #4607
Conversation
tests/dragonfly/snapshot_test.py
Outdated
|
|
||
| types = ["STRING", "LIST", "SET", "HASH", "ZSET", "JSON"] | ||
| # If we add stream type, assert memory_after[counter] >= 0.5 * value fails | ||
| seeder = StaticSeeder(**self.SEEDER_ARGS, types=types) |
There was a problem hiding this comment.
I am not really happy with this nor do I think it's the best solution. Read the above 1, 2, 3 items from the list.
Both (1) and (3) make sense but I am trying to wrap my head around why (2) is useful ? Specifically:
- Memory counters after loading from snapshot is similar to before creating a snapshot
And then if you look on what we assert:
# Unfortunately memory usage sometimes depends on order of insertion / deletion, so
# it's usually not exactly the same. For the test to be stable we check that it's
# at least 50% that of the original value.
assert memory_after[counter] >= 0.5 * value
Why do we consider this test "stable" at 50% and not 60% or 20% ? It seems like a made up number. Furthermore, I don't really see the value of it? Let's take for instance the failure that we see here, because streams seem to consume less size after they loaded from the snapshot. Is this really a regerssion ?
In other words, is there a golden ration that we are aiming ? If so why ? Otherwise maybe we should consider removing (2) alltogether from this test ?
@adiholden Any comments ?
There was a problem hiding this comment.
I agree its not stable, but I do want to make sure we do not miss counting when we load from sanpshot.
I would change this to > 0 instead > 0.5 * value
There was a problem hiding this comment.
Oh so it's there for correctness. Why this is not a unit test then ? 😄
I will 1) change this to >0 2) at some point when I get some free bandwidth I will replace that part with a unit.
tests/dragonfly/snapshot_test.py
Outdated
| # at least 50% that of the original value. | ||
| assert memory_after[counter] >= 0.5 * value | ||
| # Counters should be non zero. | ||
| assert memory_after[counter] >= 0 |
There was a problem hiding this comment.
| assert memory_after[counter] >= 0 | |
| assert memory_after[counter] > 0 |
| # at least 50% that of the original value. | ||
| assert memory_after[counter] >= 0.5 * value | ||
| # Counters should be non zero. | ||
| assert memory_after[counter] > 0 |
There was a problem hiding this comment.
please update the comment of step 2 of checks
The issue is the
STREAMdata type consumes way less memory after the snapshot load and triggers the assertionassert memory_after[counter] >= 0.5 * value. The solution is to assert that the counter is non zero (that is that after loading the snapshot, df properly incremented the counters).Resolves #4606