-
Notifications
You must be signed in to change notification settings - Fork 13
Add ReuseServerTestCase for class-scoped server reuse #13
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
base: unstable
Are you sure you want to change the base?
Changes from 6 commits
6ae835f
88d0f96
eb44776
c4c0160
1336e18
3aba829
d4b3768
6e9aeaa
c687798
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should look at updating readme for this as well to show this new functionality |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| import logging | ||
| import subprocess | ||
| import time | ||
| import os | ||
|
|
@@ -728,3 +729,108 @@ def waitForReplicaOffsetToSyncUp(self, primary, replica): | |
| pinfo.get_primary_repl_offset(), | ||
| timeout=TEST_MAX_WAIT_TIME_SECONDS, | ||
| ) | ||
|
|
||
|
|
||
| class ReuseServerTestCase(ValkeyTestCase): | ||
| """Test case that reuses a single server across all tests in the class. | ||
|
|
||
| Instead of spawning a fresh server per test, one server is started on the | ||
| first create_server() call and reused for all subsequent tests. FLUSHALL + | ||
| CONFIG RESETSTAT run between tests to reset state. | ||
|
|
||
| Usage — just change your base class: | ||
|
|
||
| class MyModuleTestCase(ReuseServerTestCase): | ||
| ... # keep your existing setup_test exactly as-is | ||
|
|
||
| That's it. self.server, self.client, create_server() all work as before. | ||
| """ | ||
|
|
||
| def create_server( | ||
| self, | ||
| testdir=None, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any benefit to not require
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a fallback: if |
||
| bind_ip=None, | ||
| port=None, | ||
| server_path=None, | ||
| args="", | ||
| skip_teardown=False, | ||
| conf_file=None, | ||
| external_server=False, | ||
| wait_for_ping=True, | ||
| connect_client=True, | ||
| ): | ||
| if hasattr(self.__class__, "_shared_server") and self.__class__._shared_server: | ||
| return self.__class__._shared_server, self.__class__._shared_client | ||
|
|
||
| if testdir is None: | ||
| testdir = self.testdir | ||
| if server_path is None: | ||
| server_path = self.server_path | ||
|
|
||
| server, client = super().create_server( | ||
| testdir=testdir, | ||
| bind_ip=bind_ip, | ||
| port=port, | ||
| server_path=server_path, | ||
| args=args, | ||
| skip_teardown=skip_teardown, | ||
| conf_file=conf_file, | ||
| external_server=external_server, | ||
| wait_for_ping=wait_for_ping, | ||
| connect_client=connect_client, | ||
| ) | ||
| self.__class__._shared_server = server | ||
| self.__class__._shared_client = client | ||
| self.__class__._initial_config = client.config_get("*") | ||
| return server, client | ||
|
|
||
| def teardown(self): | ||
| if hasattr(self.__class__, "_shared_server") and self.__class__._shared_server: | ||
| client = self.__class__._shared_client | ||
| try: | ||
| client.execute_command("REPLICAOF", "NO", "ONE") | ||
| client.flushall() | ||
| client.execute_command("CONFIG", "RESETSTAT") | ||
| client.execute_command("SCRIPT", "FLUSH") | ||
| try: | ||
| client.execute_command("FUNCTION", "FLUSH") | ||
| except Exception: | ||
| pass | ||
| users = client.execute_command("ACL", "LIST") | ||
| for entry in users: | ||
| if isinstance(entry, bytes): | ||
| entry = entry.decode() | ||
| if not entry.startswith("user default "): | ||
| username = entry.split(" ")[1] | ||
| client.execute_command("ACL", "DELUSER", username) | ||
| client.execute_command( | ||
| "ACL", "SETUSER", "default", "reset", "on", "~*", "&*", "+@all" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Default user should also not need a password from my understanding |
||
| ) | ||
| if hasattr(self.__class__, "_initial_config"): | ||
| current = client.config_get("*") | ||
| for key, val in self.__class__._initial_config.items(): | ||
| if current.get(key) != val: | ||
| try: | ||
| client.config_set(key, val) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| except Exception: | ||
| logging.warning( | ||
| f"Could not reset config '{key}' — " | ||
| f"tearing down server for fresh restart" | ||
| ) | ||
| self.__class__._shared_server.exit() | ||
| self.__class__._shared_server = None | ||
| self.__class__._shared_client = None | ||
| return | ||
| except Exception: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this potentially leave behind process that won't be cleaned up? |
||
| logging.warning("Server unreachable during teardown — killing process") | ||
| self.__class__._shared_server.exit() | ||
| self.__class__._shared_server = None | ||
| self.__class__._shared_client = None | ||
|
|
||
| @pytest.fixture(autouse=True, scope="class") | ||
| def class_teardown(self, request): | ||
| yield | ||
| if hasattr(self.__class__, "_shared_server") and self.__class__._shared_server: | ||
| self.__class__._shared_server.exit() | ||
| self.__class__._shared_server = None | ||
| self.__class__._shared_client = None | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| """ | ||
| Demonstrates ReuseServerTestCase usage. | ||
|
|
||
| All tests in this class share ONE server. Between each test, FLUSHALL + CONFIG | ||
| RESETSTAT run automatically to give each test a clean slate without the cost of | ||
| restarting the server. | ||
|
|
||
| Tests run top-to-bottom in definition order (via pytest-order with | ||
| --order-scope=class). Some tests verify isolation from the previous test, | ||
| so ordering matters. | ||
| """ | ||
|
|
||
| import os | ||
| import pytest | ||
| from conftest import resource_port_tracker | ||
| from valkey_test_case import ReuseServerTestCase | ||
|
|
||
|
|
||
| class TestReuseServer(ReuseServerTestCase): | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the tests I think they should run top to bottom? Should we make a note that this is how the ordering works somewhere?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The tests already run top to bottom. I'll reference it explicitly in the README file and py file |
||
| """Verifies that server reuse works and tests are isolated.""" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lack of setup_test to call create_server() after redesigning the ReuseServerTestCase in the 2nd commit
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you're correct. After the redesign of the server reuse, the server is only created when
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add a test for resetting a config? |
||
|
|
||
| @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" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| self.server, self.client = self.create_server( | ||
| testdir=self.testdir, server_path=server_path | ||
| ) | ||
|
|
||
| def test_write_and_read(self): | ||
| """Basic write/read on the shared server.""" | ||
| self.client.set("greeting", "hello") | ||
| assert self.client.get("greeting") == b"hello" | ||
|
|
||
| def test_isolation_from_previous(self): | ||
| """Proves FLUSHALL cleaned up the previous test's data.""" | ||
| result = self.client.get("greeting") | ||
| assert result is None, "Key from previous test should not exist" | ||
|
|
||
| def test_server_still_alive(self): | ||
| """Proves the server survived across tests (no restart).""" | ||
| assert self.client.ping() is True | ||
|
|
||
| def test_multiple_keys(self): | ||
| """Write multiple keys, verify they all exist within this test.""" | ||
| for i in range(10): | ||
| self.client.set(f"key:{i}", f"value:{i}") | ||
| assert self.client.dbsize() == 10 | ||
|
|
||
| def test_previous_keys_gone(self): | ||
| """Proves the 10 keys from the previous test were flushed.""" | ||
| assert self.client.dbsize() == 0 | ||
|
|
||
| def test_config_change_is_restored(self): | ||
| """Proves configs modified during a test get restored for the next.""" | ||
| original = self.client.config_get("hz")["hz"] | ||
| self.client.config_set("hz", "50") | ||
| assert self.client.config_get("hz")["hz"] == "50" | ||
|
|
||
| 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}" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
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.
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
Uh oh!
There was an error while loading. Please reload this page.
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.
I think it should be fine. We don't have a teardown list as we will reuse the
_shared_serverunless it crashes somehow in previous test. We properly tear down the existing server inclass_teardown. Though it's good point that our overrideteardownfunction 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 yourteardownfunction, and properly creating a private method_reset_server_stateand call it insideteardown.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.
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.
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.
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. ;)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.
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
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.
Discussed offline with @zackcam.
@Fniakate8 There are some tests in bloom and search modules which call
create_serverinside the test itself, then we might not properly clean them up. We should properly clean up all the servers in the list inclass_teardownsimilar to the current teardown.