Add ReuseServerTestCase for class-scoped server reuse by Fniakate8 · Pull Request #13 · valkey-io/valkey-test-framework

Fniakate8 · 2026-07-29T01:05:07Z

This PR implements server reuse for test classes, keeping one server alive across all tests in a class instead of spawning and killing a new process per test. Between tests the server is reset to a clean state so each test still starts as isolated.

What's Added

- ReuseServerTestCase inherits ValkeyTestCase and overrides create_server() to cache the server on the first call — subsequent calls return the cached instance

Configs are snapshotted on server creation and any changed values are restored between tests to prevent pollution
Between tests, _reset_server_state() restores full isolation:
- RESET on the shared connection (discards MULTI/WATCH, disables CLIENT TRACKING, restores RESP2/db0/READWRITE, etc.)
- CLIENT KILL TYPE normal to drop any connections a test spawned (SKIPME keeps the shared client alive)
- REPLICAOF NO ONE, FLUSHALL, CONFIG RESETSTAT, SCRIPT FLUSH, FUNCTION FLUSH
- SLOWLOG RESET, LATENCY RESET, ACL LOG RESET so per-test log checks start clean
- ACL reset (delete non-default users, reset default user to full permissions with nopass)
- Config restore against the initial snapshot
If the server becomes unreachable or a config can't be restored, it's torn down and a fresh one starts for the next test
Additional servers created during a test are tracked in server_list and cleaned up (the shared server is skipped)

How to use

To adopt server reuse in your module:

Import ReuseServerTestCase from the framework
Change your test base class to inherit from ReuseServerTestCase instead of ValkeyTestCase

Your existing setup_test, create_server() calls, and self.server/self.client assignments all work unchanged. Tests that need a fresh server (restart the server, change startup-only configs, do persistence operations)
should stay on ValkeyTestCase.

How I tested with other modules

Bloom module — Changed ValkeyBloomTestCaseBase to inherit ReuseServerTestCase. Ran test_bloom_basic.py (70 tests). All 70 passed. Tests that use CONFIG SET (like bf.bloom-memory-usage-limit, maxmemory) no longer
pollute later tests thanks to the automatic config restore.
JSON module — Changed JsonTestCase to inherit ReuseServerTestCase. Ran test_json_basic.py (171 tests). 170 passed, 1 failed due to a pre-existing missing SOURCE_DIR env var (unrelated to server reuse).

Server reuse helps most when module loading is expensive (like JSON) and tests are lightweight data operations. Bloom sees less improvement because its tests are compute-heavy (filling filters, sleeping for
expiration).

The framework's own tests/test_reuse_server.py also verifies the reset behavior directly: data isolation, config restore, and that a client connection + ACL user left behind in one test are gone in the next.

Test plan

Bloom: all 34 tests pass with only base class change
JSON: 170/171 pass with only base class change
Config-heavy tests no longer pollute subsequent tests
Connection state, spawned clients, ACL users, and logs are reset between tests
Tests that need a fresh server can stay on ValkeyTestCase

Instead of spawning a fresh valkey-server per test (~42ms), one server is started per class and FLUSHALL + CONFIG RESETSTAT (~5ms) run between tests to reset state. This gives ~5-7x speedup for test classes. Changes: - src/conftest.py: add class_port_tracker fixture (scope="class") - src/valkey_test_case.py: add ReuseServerTestCase class - tests/test_reuse_server.py: demo/test proving reuse and isolation Signed-off-by: Fanta Niakate <niakatf@amazon.com>

ReuseServerTestCase now inherits ValkeyTestCase and overrides create_server() to cache the server on first call. Modules only need to change their base class — no other code modifications required. Key changes: - Inherit ValkeyTestCase instead of ValkeyTestCaseBase so all fixtures (setup, port_tracker_fixture) work automatically - Override create_server() to return cached server on subsequent calls - Snapshot all configs on first creation and restore between tests to prevent config pollution across tests - FLUSHALL + CONFIG RESETSTAT between tests for data isolation - Bump pytest to 7.4.3 Tested against bloom (34/34 pass) and JSON (170/171 pass, 1 unrelated env var issue). JSON sees ~17x speedup (8s vs 138s). Signed-off-by: Fanta Niakate <niakatf@amazon.com>

chinguyen21

Overall looks better a lot after the 2nd change. Just a few things to improve. Please check the comments

chinguyen21 · 2026-07-30T20:52:27Z

+
+
+class TestReuseServer(ReuseServerTestCase):
+    """Verifies that server reuse works and tests are isolated."""


Lack of setup_test to call create_server() after redesigning the ReuseServerTestCase in the 2nd commit

you're correct. After the redesign of the server reuse, the server is only created when create_server() is first called. I'll add a setup_test fixture taht calls create_server().

chinguyen21 · 2026-07-30T20:53:21Z

@@ -1,4 +1,4 @@
 valkey
-pytest==6
+pytest==7.4.3


Why do we need this change? Doesn't pytest6 work?

The pytest 6 works but it throws deprecation warnings for the teardown(self) method and pytest 7 handles fixture scoping more reliably (like our class-scoped class_teardown fixture.) but for simplicity I'll remove it.

chinguyen21 · 2026-07-30T21:24:16Z

+    def teardown(self):
+        if hasattr(self.__class__, '_shared_server') and self.__class__._shared_server:
+            client = self.__class__._shared_client
+            client.flushall()


If the server somehow crashes during a test, this will raise an exception. Can we handle it? And make sure if exception happens, we should discard the shared server, so other tests can still run properly

Fixed. I wrapped the teardown in a try/except. If the server crashes, we can just discard _shared_server and _shared_client by setting them to None. The next test's create_server will see None and spin up a new server for concurrent tests.

chinguyen21 · 2026-07-30T21:56:32Z

+
+    def create_server(
+        self,
+        testdir=None,


Is there any benefit to not require testdir?

Added a fallback: if testdir isn't passed, it defaults to self.testdir (same approach as server_path). This way None never reaches the parent, and modules that already pass testdir=self.testdir still work the same.

zackcam · 2026-07-31T18:51:36Z

I let workflows run seems like there are some formatting issues can run black . to fix formatting issues.

zackcam · 2026-07-31T18:56:14Z

+
+
+@pytest.fixture(scope="class")
+def class_port_tracker(request):


Do we need this still? I saw we had an import for this but removed it

- Handle server crash in teardown: if flushall/config reset fails, discard the shared server so next test creates a fresh one - Default testdir to self.testdir when not passed (same pattern as server_path) so None never reaches the parent - Add setup_test fixture to test_reuse_server.py to call create_server() matching the pattern modules use - Remove unused class_port_tracker import from test file - Revert pytest version bump — not needed for server reuse Signed-off-by: Fanta Niakate <niakatf@amazon.com>

zackcam

Looking good! Still seems to have workflow failures with formatting hopefully should be easy to fix.
Do we also want to reset ACL? Script and function also might persist through? Potentially replication as well i.e made the server a replica should we check if we want to reset that?

zackcam · 2026-08-03T23:21:37Z

+from valkey_test_case import ReuseServerTestCase
+
+
+class TestReuseServer(ReuseServerTestCase):


For the tests I think they should run top to bottom? Should we make a note that this is how the ordering works somewhere?

The tests already run top to bottom. I'll reference it explicitly in the README file and py file

zackcam · 2026-08-03T23:22:05Z

+
+
+class TestReuseServer(ReuseServerTestCase):
+    """Verifies that server reuse works and tests are isolated."""


Can we add a test for resetting a config?

zackcam · 2026-08-03T23:23:36Z

+                    for key, val in self.__class__._initial_config.items():
+                        if current.get(key) != val:
+                            try:
+                                client.config_set(key, val)


As we catch here we silently could not reset a config. I think we should at least put a comment if this happens, or even this time actually tear down the server fully and restart one

zackcam · 2026-08-03T23:30:27Z

We should look at updating readme for this as well to show this new functionality

zackcam · 2026-08-03T23:31:21Z

+                                client.config_set(key, val)
+                            except Exception:
+                                pass
+            except Exception:


Could this potentially leave behind process that won't be cleaned up?

- Add ReuseServerTestCase usage section to README - Add note about test ordering (top-to-bottom via pytest-order) - Log warning and tear down server when config_set fails to restore - Call exit() on server before discarding when teardown hits an exception - Add test_config_change_is_restored and test_config_restored_after_previous Signed-off-by: Fanta Niakate <niakatf@amazon.com>

Signed-off-by: Fanta Niakate <niakatf@amazon.com>

…down ReuseServerTestCase.teardown() now also resets: - REPLICAOF NO ONE (restore to primary if made replica) - SCRIPT FLUSH (clear cached Lua scripts) - FUNCTION FLUSH (clear loaded functions, no-op on Valkey < 7) - ACL DELUSER for non-default users + reset default user permissions Also adds explicit `import logging` instead of relying on wildcard import. Signed-off-by: Fanta Niakate <niakatf@amazon.com>

zackcam · 2026-08-04T17:35:16Z

+        assert self.client.execute_command("GET K") is None
+```
+
+`ReuseServerTestCase` inherits `ValkeyTestCase`, so all existing fixtures, `create_server()` calls, and `self.server`/`self.client` assignments work unchanged. To adopt it in your module, just change the base class — no other code changes needed.


I think the create_server() might do some funky things, do we tear it down properly as we override the teardown list? I think we also might return early here and not actually create a new server

I think it should be fine. We don't have a teardown list as we will reuse the _shared_server unless it crashes somehow in previous test. We properly tear down the existing server in class_teardown. Though it's good point that our override teardown function is mainly doing the reset config.
@Fniakate8 We properly can add comment like Reset shared server state between tests instead of shutting it down. at the beginning of your teardown function, and properly creating a private method _reset_server_state and call it inside teardown.

I was thinking more on the lines if we call create server in a test then we will need to tear down the extra one we created.

The server cannot create a extra test, it just resets its configs, and if it can't reset it, it logs a warning, kills the server and creates a new one . But I just Extracted it into _reset_server_state() to make the intent more clear at a glance. ;)

In that case the readme isn't quite right then, as the create_server would not return a new server if called in a test. If we are adding it to be the same we should allow the functionality to allow the user to call create server in a test and have it return a new server and teardown at the end of that test

Discussed offline with @zackcam.
@Fniakate8 There are some tests in bloom and search modules which call create_server inside the test itself, then we might not properly clean them up. We should properly clean up all the servers in the list in class_teardown similar to the current teardown.

zackcam · 2026-08-04T17:39:25Z

+    def test_config_restored_after_previous(self):
+        """Proves the config changed in the previous test was reset."""
+        current = self.client.config_get("hz")["hz"]
+        assert current == "10", f"Expected hz=10 (default), got hz={current}"


Not against hardcoding this but if its easy might be better to do where we capture the value of the config at startup but again small nit this shouldnt change i think

zackcam · 2026-08-04T17:42:02Z

+                        username = entry.split(" ")[1]
+                        client.execute_command("ACL", "DELUSER", username)
+                client.execute_command(
+                    "ACL", "SETUSER", "default", "reset", "on", "~*", "&*", "+@all"


Default user should also not need a password from my understanding

zackcam · 2026-08-04T17:45:06Z

+
+    @pytest.fixture(autouse=True)
+    def setup_test(self, setup):
+        server_path = f"{os.path.dirname(os.path.realpath(__file__))}/.build/binaries/{os.environ['SERVER_VERSION']}/valkey-server"


For this if someone wants to run this without using build.sh we should default to checking unstable if they haven't set server version

…llback - Add nopass to ACL SETUSER default reset to ensure no password required - Extract _reset_server_state() from teardown for clarity - Add comment on create_server() explaining cached return behavior - Capture initial hz value in test instead of hardcoding - Fall back to "unstable" if SERVER_VERSION env var not set - Clarify README on caching and reset behavior Signed-off-by: Fanta Niakate <niakatf@amazon.com>

- Clean up additional servers in teardown() (skip shared server) - Add safety-net server_list cleanup in class_teardown with getattr - Add docstring to _reset_server_state explaining reset behavior - Update class docstring to accurately list all reset operations - Document additional server cleanup in README Signed-off-by: Fanta Niakate <niakatf@amazon.com>

chinguyen21

LGTM

zackcam · 2026-08-07T20:55:01Z

Sorry for churn found a couple more places we should reset which I missed before :( but code looks good to me now.

SLOWLOG RESET, LATENCY RESET, ACL LOG RESET should also be run to reset in case of log checks
We have a shared client so should issue a reset for it, this will do all these: https://valkey.io/commands/reset/
If we spawn a new client in a test should we kill it? can maybe issue client kills for everything but the shared one

Addresses mentor feedback on additional state to reset between tests: - RESET the shared connection first (discards MULTI/WATCH, disables CLIENT TRACKING, restores RESP2/db0/READWRITE, etc.) so later commands aren't queued inside a lingering MULTI - CLIENT KILL TYPE normal to drop any connections a test spawned (SKIPME yes default keeps the shared client alive) - SLOWLOG RESET, LATENCY RESET, ACL LOG RESET so per-test log checks start clean Add tests proving a spawned connection and an ACL user left behind in one test are gone in the next, and sync the class/method/README/test docstrings with the full set of reset operations. Signed-off-by: Fanta Niakate <niakatf@amazon.com>

Fniakate8 force-pushed the server-reuse-upstream branch 2 times, most recently from e94d1d4 to 5006368 Compare July 29, 2026 01:14

Fanta Niakate added 2 commits July 30, 2026 17:45

Fniakate8 force-pushed the server-reuse-upstream branch from de8c642 to 88d0f96 Compare July 30, 2026 17:50

chinguyen21 reviewed Jul 30, 2026

View reviewed changes

Fniakate8 force-pushed the server-reuse-upstream branch from 482ecaa to 53af29f Compare July 30, 2026 23:59

zackcam reviewed Jul 31, 2026

View reviewed changes

Fniakate8 force-pushed the server-reuse-upstream branch from 53af29f to c3685d4 Compare July 31, 2026 21:06

Fniakate8 force-pushed the server-reuse-upstream branch from 1ecb724 to eb44776 Compare August 3, 2026 23:32

zackcam reviewed Aug 3, 2026

View reviewed changes

Fanta Niakate added 3 commits August 4, 2026 00:04

Fix test_reuse_server to use built binary path from SERVER_VERSION

1336e18

Signed-off-by: Fanta Niakate <niakatf@amazon.com>

zackcam reviewed Aug 4, 2026

View reviewed changes

Fanta Niakate added 2 commits August 4, 2026 19:45

Fniakate8 force-pushed the server-reuse-upstream branch from db2c9b8 to 6e9aeaa Compare August 4, 2026 23:57

chinguyen21 approved these changes Aug 5, 2026

View reviewed changes

zackcam approved these changes Aug 10, 2026

View reviewed changes



		class TestReuseServer(ReuseServerTestCase):
		"""Verifies that server reuse works and tests are isolated."""



		@pytest.fixture(scope="class")
		def class_port_tracker(request):

		from valkey_test_case import ReuseServerTestCase


		class TestReuseServer(ReuseServerTestCase):

Uh oh!

Conversation

Fniakate8 commented Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chinguyen21 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fniakate8 Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fniakate8 Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zackcam commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zackcam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fniakate8 Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinguyen21 Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinguyen21 left a comment

Choose a reason for hiding this comment

Uh oh!

zackcam commented Aug 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Fniakate8 commented Jul 29, 2026 •

edited

Loading

Fniakate8 Jul 30, 2026 •

edited

Loading

Fniakate8 Jul 30, 2026 •

edited

Loading

zackcam commented Jul 31, 2026 •

edited

Loading

Fniakate8 Aug 3, 2026 •

edited

Loading

chinguyen21 Aug 4, 2026 •

edited

Loading