Add ReuseServerTestCase for class-scoped server reuse - #13
Conversation
e94d1d4 to
5006368
Compare
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>
de8c642 to
88d0f96
Compare
chinguyen21
left a comment
There was a problem hiding this comment.
Overall looks better a lot after the 2nd change. Just a few things to improve. Please check the comments
|
|
||
|
|
||
| class TestReuseServer(ReuseServerTestCase): | ||
| """Verifies that server reuse works and tests are isolated.""" |
There was a problem hiding this comment.
Lack of setup_test to call create_server() after redesigning the ReuseServerTestCase in the 2nd commit
There was a problem hiding this comment.
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().
| @@ -1,4 +1,4 @@ | |||
| valkey | |||
| pytest==6 | |||
| pytest==7.4.3 | |||
There was a problem hiding this comment.
Why do we need this change? Doesn't pytest6 work?
There was a problem hiding this comment.
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.
| def teardown(self): | ||
| if hasattr(self.__class__, '_shared_server') and self.__class__._shared_server: | ||
| client = self.__class__._shared_client | ||
| client.flushall() |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
|
|
||
| def create_server( | ||
| self, | ||
| testdir=None, |
There was a problem hiding this comment.
Is there any benefit to not require testdir?
There was a problem hiding this comment.
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.
482ecaa to
53af29f
Compare
|
I let workflows run seems like there are some formatting issues can run |
|
|
||
|
|
||
| @pytest.fixture(scope="class") | ||
| def class_port_tracker(request): |
There was a problem hiding this comment.
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>
53af29f to
c3685d4
Compare
This PR implements server reuse for test classes, reducing per-test overhead from (
FLUSHALL + CONFIG RESETSTAT).Whats Added
ReuseServerTestCase now inherits ValkeyTestCase and overrides create_server() to cache the server on first call instead of spawning a new one per test
Configs are snapshotted on server creation and restored between tests to prevent pollution
FLUSHALL + CONFIG RESETSTAT runs between each test for data isolation
Bumps pytest to 7.4.3
How to use
To adopt server reuse in your module, you need to:
Import ReuseServerTestCasefrom the frameworkChange your test base class to inherit from
ReuseServerTestCaseinstead ofValkeyTestCaseyour
existingsetup_test, create_server()calls, andself.server/self.clientassignments all work unchanged.How I tested with other modules
Bloom module — Changed
ValkeyBloomTestCaseBaseto inheritReuseServerTestCase.Rantest_bloom_basic.py(34 tests). All 34 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
JsonTestCaseto inheritReuseServerTestCase.Rantest_json_basic.py(171 tests). 170 passed, 1 failed due to a pre-existing missingSOURCE_DIRenv 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).
Test plan