fixes for debug flag on integration tests #418
Conversation
daddaman-amz
commented
Oct 16, 2025
daddaman-amz
commented
- Ensures debug flag ( or asan, tsan flags) are passed to oss integration tests
- Added --debug-on-crash flag for oss integration tests which would drop interactive gdb in the event of crash in the test.
daddaman-amz
force-pushed
the
build-integ-fix
branch
2 times, most recently
from
a5c0fac to
9721195
Compare
|
@allenss-amazon to help with review few issues we chatted about . |
| class DebugValkeyServerHandle(ValkeyServerHandle): | ||
| """ValkeyServerHandle that wraps valkey-server with gdb for crash debugging""" | ||
|
|
||
| def start(self, wait_for_ping=True, connect_client=True): | ||
| if self.server: | ||
| raise RuntimeError("Server already started") | ||
|
|
||
| # Build server command | ||
| server_args = [self.valkey_path] | ||
| if self.conf_file: | ||
| server_args.append(self.conf_file) | ||
| for k, v in self.args.items(): | ||
| server_args.extend([f"--{k.replace('_', '-')}", str(v)]) | ||
|
|
||
| # Wrap with gdb if debug-on-crash enabled | ||
| if os.environ.get("DEBUG_ON_CRASH") == "yes": | ||
| cmd = ["gdb", "--ex", "run", "--args"] + server_args | ||
| else: | ||
| cmd = server_args | ||
|
|
||
| self.server = subprocess.Popen(cmd, cwd=self.cwd) | ||
| # Standard connection handling | ||
| if connect_client: | ||
| self.wait_for_ready_to_accept_connections() |
There was a problem hiding this comment.
This duplicates the start code in the testing infrastructure, right? Is that necessary seems like it actually makes it more complicated.
There was a problem hiding this comment.
You're right about the duplication. We're overriding the start() method from ValkeyServerHandle to wrap the server command with gdb when the debug-on-crash flag is enabled.
There is some duplication but I felt it was simple approach for this use case to automatically start gdb when there is crash. I also noticed in your other PRs you mentioned about saving coredumps for test failures. If this debug-on-crash feature is redundant with your commit, I'm happy to remove it.
Let me know your preference
There was a problem hiding this comment.
What I don't like about overriding the start method is that essentially your duplicating it. Over time, the start method is going to accumulate many more parameters, etc. Most likely, the people that make those changes won't even know that there's a duplicate of start that would also need the same changes, leading to the situation where the debug option doesn't work the same as the non-debug option. I recently added the ability to run under valgrind, and faced the same problem but modified the underlying start routine. Is there a reason that this couldn't do the same? See https://github.com/valkey-io/valkey-test-framework/blob/f42f4aa754fd0e26c8b8e52e2a7abd698978856f/src/valkey_test_case.py#L228
daddaman-amz
force-pushed
the
build-integ-fix
branch
from
9721195 to
43fd99c
Compare
Signed-off-by: Manish Addanki <[email protected]>
Signed-off-by: Manish Addanki <[email protected]>
daddaman-amz
force-pushed
the
build-integ-fix
branch
from
43fd99c to
597c024
Compare
2 participants
