Pytests overhaul

[dranikpg]

Big testing overhaul

A lot of new features that we introduce for replication, snapshotting, compression, serialization and cancellation don't have proper tests. Many of those features are also hard to test in general, under load and for corner cases.

So far pytests have made a good job in uncovering bugs, but they used only simple commands and a single database.

The new DflySeeder can issue command sequences that converge to some targeted number of keys and oscilliate upon reaching it. It supports all main data types (strings, lists, sets, hsets, zsets) and 10 incremental commands (but can be extended to any number).

It allows creating captures on the master instance (that is expected to work faultless) and then comparing them to the state on different instances, showing any some changes if needed.

Its designed to be efficient (fully async, parallel work on multiple dbs, pipelined requests) so python's performance is not the bottleneck.

Example:

# Create seeder with target number of keys (100k) of specified size (200) and work on 5 dbs
seeder = new DflySeeder(keys=100_000, value_size=200, dbcount=5)

# Stop when we are in 5% of target number of keys (i.e. above 95_000)
# because its probabilistic we might never reach exactly 100_000
await seeder.run(target_deviation=0.05) 

# Run 3 iterations (full batches) in stable state
await seeder.run(target_times=3)

# Create a capture
capture = await seeder.capture()

# Compare capture to replica on port 1112
assert await seeder.compare(capture, port=1112)

fixes #530

[romange]

can you add here what it does?

[boazsade]

Maybe we can use python test containers and not run them as sub processes (if I understand correctly, this is what it does here, spins DFs sub processes). They reject my PR for supporting DF, but you can start a DF container with the right parameters.

[dranikpg]

This is inconvenient (increased memory requirements) and requires re-building a container just for testing some change

[boazsade]

nitpick - yield f"key-{i}" f"value-{i}"
BTW why did you removed the "gen_test_data"?

[dranikpg]

Because its now the only place where it would be used... Maybe I should keep it though

[boazsade]

at what cases this is useful? I mean from what I know await will block the current task until the awaited function will return. So when the iteration will take place?

[dranikpg]

This is for waiting until an instance becomes available for queries (i.e. exits the LOADING state). Its indeed supposed to block all this time, because the test has nothing else to to except wait for the instance to be available for comparing data

[boazsade]

you have self.port, why not making this defaulted to self.port and not to 6379?

[dranikpg]

Stable state currently has issues with set and zset commands (for example spop pops different values), so I commented them out for now to let the tests run

[dranikpg]

This is inconvenient (increased memory requirements) and requires re-building a container just for testing some change

[dranikpg]

Because its now the only place where it would be used... Maybe I should keep it though

[dranikpg]

This is for waiting until an instance becomes available for queries (i.e. exits the LOADING state). Its indeed supposed to block all this time, because the test has nothing else to to except wait for the instance to be available for comparing data

[romange]

Totally. I even claim we should use ubuntu 20.04 as a test which version to assume.

…

On Sat, Dec 31, 2022, 11:58 Vladislav ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In tests/dragonfly/utility.py <#569 (comment)> : > + ('LPOP {k}', ValueType.LIST), + #('SADD {k} {val}', ValueType.SET), + #('SPOP {k}', ValueType.SET), + ('HSETNX {k} v0 {val}', ValueType.HSET), + ('HINCRBY {k} v1 1', ValueType.HSET), + #('ZPOPMIN {k} 1', ValueType.ZSET), + #('ZADD {k} 0 {val}', ValueType.ZSET) Stable state currently has issues with set and zset commands (for example spop pops different values), so I commented them out for now to let the tests run ------------------------------ In tests/dragonfly/__init__.py <#569 (comment)> : > @@ -99,6 +105,16 @@ def create(self, **kwargs) -> DflyInstance: self.instances.append(instance) return instance + def start_all(self, instances): This is inconvenient (increased memory requirements) and requires re-building a container just for testing some change ------------------------------ In tests/dragonfly/server_family_test.py <#569 (comment)> : > def test_scan(client): - try: - for key, val in gen_test_data(n=10, seed="set-test-key"): - res = client.set(key, val) - assert res is not None - cur, keys = client.scan(cursor=0, match=key, count=2) - assert cur == 0 - assert len(keys) == 1 - assert keys[0] == key - except Exception as e: - assert False, str(e) + def gen_test_data(): + for i in range(10): + yield "key-"+str(i), "value-"+str(i) Because its now the only place where it would be used... Maybe I should keep it though ------------------------------ In tests/dragonfly/utility.py <#569 (comment)> : > -def gen_test_data(n, start=0, seed=None): - for i in range(start, n): - yield "k-"+str(i), "v-"+str(i) + ("-"+str(seed) if seed else "") +async def wait_available_async(client: aioredis.Redis): This is for waiting until an instance becomes available for queries (i.e. exits the LOADING state). Its indeed supposed to block all this time, because the test has nothing else to to except wait for the instance to be available for comparing data ------------------------------ In tests/dragonfly/utility.py <#569 (comment)> : > + s = self.set_for_type(t) + + if s is None or len(s) == 0: + return None, None + + k = s.pop() + if not pop: + s.add(k) + + return k, t + + def generate_val(self, t: ValueType): + def rand_str(k=3, s=''): + return s.join(random.choices(string.ascii_letters, k=k)) + + if t == ValueType.STRING: I know it exists, but its Python 3.10, whereas most repos of most distros cover for now only Python 3.9 I would rather not use fancy new features so that its runnable with default python and doesn't require installing a new version just to do it — Reply to this email directly, view it on GitHub <#569 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4BFCBV3AQ37HLIWIXA3NDWP77VRANCNFSM6AAAAAATCAJGMY> . You are receiving this because you commented.Message ID: ***@***.***>

[dranikpg]

They seem to pass now consistently

It takes about 3 min to run them fully on my machine. I reduced the tests a little because if it fails, it usually tends to do so already on the medium sized ones. Otherwise testing will take eternity 😄

Some parts are commented out - those are the commands we don't support. It works on Redis though and once we support them, we'll just uncomment the parts (like SPOP for example)

[romange]

Vlad, it's an amazing addition to our tests and to our testing methodology!
You really raise the bar for our testing quality. I gave you a few readability comments.

[romange]

can you provide a full command instead of a single option?

[dranikpg]

What does full command mean? You can use it multiple times like --df logtostdout --df proactor_threads=2, I'll add this info

[dranikpg]

Pushed fixes to you comments and some more last minute ones