Refactor TCL reference to support running tests with CMake

[zhijun42]

Historically, Valkey’s TCL test suite expected all binaries (src/valkey-server, src/valkey-cli, src/valkey-benchmark, etc.) to exist under the src/ directory. This PR enables Valkey TCL tests to run seamlessly after a CMake build — no manual symlinks or make build required.

CMake will copy all TCL test entrypoints (runtest, runtest-cluster, etc.) into the CMake build dir (e.g. cmake-build-debug). Now we can either do ./cmake-build-debug/runtest at the project root or ./runtest at the Cmake dir to run all tests.

A new CMake post-build target prints a friendly reminder after successful builds, guiding developers on how to run tests with their CMake binaries:

Hint: It is a good idea to run tests with your CMake-built binaries ;)
      ./cmake-build-debug/runtest

Build finished

A helper TCL script tests/support/set_executable_path.tcl is added to support this change, which gets called by all test entrypoints: runtest, runtest-cluster, runtest-sentinel.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.44%. Comparing base (7f8c5b6) to head (153a7e4).
⚠️ Report is 37 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #2816      +/-   ##
============================================
+ Coverage     72.40%   72.44%   +0.03%     
============================================
  Files           128      128              
  Lines         70270    70487     +217     
============================================
+ Hits          50880    51062     +182     
- Misses        19390    19425      +35     

see 33 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[ranshid]

I think this will probably solve the problem but I have some drawbacks:

1. I don't much like the use of symlinks if it can be avoided. For Example, the fact that we might leave them dangling when we decide to delete some cmake target directory should probably be fixed.
2. It feels like the real fix should be to optionally propagate the PATH of the binaries into the tcl runtest scripts. this sound like a better way to allow user defined selection of binaries?

[zhijun42]

I think this will probably solve the problem but I have some drawbacks:

1. I don't much like the use of symlinks if it can be avoided. For Example, the fact that we might leave them dangling when we decide to delete some cmake target directory should probably be fixed.
2. It feels like the real fix should be to optionally propagate the PATH of the binaries into the tcl runtest scripts. this sound like a better way to allow user defined selection of binaries?

Good point. Yeah propagating the path is the first solution that came to my mind when trying to fix the issue, but I guess I was being lazy to go with the symlinks approach. I just went ahead to refactor all TCL references to binary paths to allow pointing to CMake built executables.

[zhijun42]

Also updated the PR description to reflect the new changes.

[eifrah-aws]

Why not simply using the standard PATH environment variable?

[zuiderkwast]

If the full path to valkey-cli is in a variable, then we shouln't have the ../../../ here. It's quite confusing to mix the two and give an illusion that the variable can be a full path when it can only be some subset(?) of relative paths.

I like @eifrah-aws's idea of using the standard PATH variable. The runtest script could prepend to the PATH when it's invoking tclsh, such as PATH=$(pwd)/src:$PATH $TCLSH tests/test_helper.tcl "${@}". Then we can just write valkey-cli here.

Suggested change

	../../../$::VALKEY_CLI_BIN --cluster reshard \
	valkey-cli --cluster reshard \

[zhijun42]

@zuiderkwast @eifrah-aws Thanks for the suggestion. I'd like to discuss how to proceed. I think embedding the PATH variable into the runtest script like PATH=$(pwd)/src:$PATH $TCLSH tests/test_helper.tcl "${@}" doesn't work because we don't know beforehand whether the user runs the binary built from Make or CMake.

Do you envision having CMake execute a one-off command (only run once) export PATH=<my-cmake-build-dir>/bin:$PATH so that users can just simply do ./runtest at the project root? Or should the user do PATH=<my-cmake-build-dir>/bin:$PATH ./runtest every single time? And is it possible that some users might have multiple CMake directories (debug/release)? If so, how do we decide which version of the binary gets called?

I have an idea. Every time CMake finishes its build, it will write a file (overwrite if already exists), say .cmake_build_path (we can gitignore this local file), to indicate the current build path. The runtest script will detect if there's a valid .cmake_build_path file - if so, it will read from that file to find the CMake binaries; otherwise it will use the Make binaries. What do you guys think?

[zuiderkwast]

Ah, CMake doesn't copy tests into the build dir? If the tests were written in C (or another compiled language), CMake would have to build the tests into the build dir. The runtest script would be built/copied into the build dir too and the user would run it from there. That's what I've seen in other projects where the tests are written in C.

I guess it would work to let CMake copy the tests and the runtest script into the build dir and then runtest will just work when running if from within the build dir. If we don't want to copy all the tests, would it be an option to let CMake copy just the runtest script into the build dir and let the user run the tests from there and the runtest would pick up the correct path variables? CMake can then generate the .cmake_build_path or .cmake_tests_path etc. within the build dir that runtest can pick up before running tclsh?

The logs from the tests are generated under test/tmp/. Shouldn't these ideally be generated somewhere under the build dir too?

[zuiderkwast]

I'm fine with

VALKEY_BIN_DIR=cmake-build-debug/bin ./runtest

too, but in that case we should allow the VALKEY_BIN_DIR to be an absolute or relative path. ../../../$::VALKEY_CLI_BIN doesn't work with an absolute path.

The runtest script (or test_helper.tcl) can transform a relative path to an absolute path (check if it starts with a slash and prepend the current directory otherwise). Then the code here in the tests would become

                $::VALKEY_BIN_DIR/valkey-cli --cluster reshard \

or

                $::VALKEY_CLI_BIN --cluster reshard \

[zhijun42]

CMake doesn't copy tests into the build dir?

Only the tests written in C were copied, but not the TCL scripts.

would it be an option to let CMake copy just the runtest script into the build dir

I love this idea! Just went ahead and implemented this. Now we can either do ./cmake-build-debug/runtest at the project root or ./runtest at the Cmake dir to run all tests.

CMake will print a friendly hint after each build.

Hint: It is a good idea to run tests with your CMake-built binaries ;)
      ./cmake-build-debug/runtest

Shouldn't these tests logs ideally be generated somewhere under the build dir too?

I'm fine either way honestly and I think this is actually a less relevant topic and would require another round of efforts to refactor.

../../../$::VALKEY_CLI_BIN doesn't work with an absolute path.

I've fixed all references to relative bin paths in the TCL scripts.

[zhijun42]

Updated PR description correspondingly.

[zuiderkwast]

Great!

@eifrah-aws @ranshid do like this approach?

[zhijun42]

@eifrah-aws @ranshid How do you guys think of this?

[zuiderkwast]

This looks pretty good to me.

[zuiderkwast]

The double underscore looks like it's a reserved name or a magic function of some sort. Let's not use that. Instead we can use a name that more exactly explains what it does, like valkey_bin_path.

We don't currently have a check that the binaries are existing. If you just try ./runtest without building first (or after make clean), you currently get an exception like [exception]: Executing test client: Server executable file not found or not executable: src/valkey-server.. I guess a pretty error can be nicer but what if you just built the server using make valkey-server and only want to run one test suite that only uses the server? No need for e.g. valkey-check-aof then. Maybe we can check only for the server binary and ignore the rest?

With VALKEY_BIN_DIR, it will also be possible to run the tests against system installed binaries, like VALKEY_BIN_DIR=/usr/local/bin ./runtest. I don't think we need to imply a make vs cmake mistake. Let's just print out that the file is missing or not executable.

Summary:

Suggested change

	# Helper to build absolute paths
	proc __build_absolute_path {name} {
	set p [file join $::VALKEY_BIN_DIR $name]
	if {![file executable $p]} {
	error "Binary not found or not executable: $p \nDo you intend to run the binaries built from Make or CMake?"
	}
	return $p
	}
	set ::VALKEY_SERVER_BIN [__build_absolute_path "valkey-server"]
	set ::VALKEY_CLI_BIN [__build_absolute_path "valkey-cli"]
	set ::VALKEY_BENCHMARK_BIN [__build_absolute_path "valkey-benchmark"]
	set ::VALKEY_CHECK_AOF_BIN [__build_absolute_path "valkey-check-aof"]
	set ::VALKEY_CHECK_RDB_BIN [__build_absolute_path "valkey-check-rdb"]
	set ::VALKEY_SENTINEL_BIN [__build_absolute_path "valkey-sentinel"]
	# Helper to build absolute paths
	proc valkey_bin_path {name} {
	return [file join $::VALKEY_BIN_DIR $name]
	}
	set ::VALKEY_SERVER_BIN [valkey_bin_path "valkey-server"]
	set ::VALKEY_CLI_BIN [valkey_bin_path "valkey-cli"]
	set ::VALKEY_BENCHMARK_BIN [valkey_bin_path "valkey-benchmark"]
	set ::VALKEY_CHECK_AOF_BIN [valkey_bin_path "valkey-check-aof"]
	set ::VALKEY_CHECK_RDB_BIN [valkey_bin_path "valkey-check-rdb"]
	set ::VALKEY_SENTINEL_BIN [valkey_bin_path "valkey-sentinel"]
	if {![file executable $::VALKEY_SERVER_BIN]} {
	error "Binary not found or not executable: $::VALKEY_SERVER_BIN"
	}

[zhijun42]

The double underscore looks like it's a reserved name or a magic function of some sort.

I intended to use double underscore to show that this function is private to this helper TCL script only, and Valkey developers won't ever need it when writing TCL tests. I guess this convention comes from other programming languages. You're right, I don't need to overcomplicate this. Updated.

No need for e.g. valkey-check-aof then. Maybe we can check only for the server binary and ignore the rest?

I see that we only have 3 real executable bins, valkey-server, valkey-cli and valkey-benchmark , and all others are just symlinks, but you're right that a user might indeed just want valkey-server. Updated.

I don't think we need to imply a make vs cmake mistake.

I was worried that some users might not realize that we have Make/CMake dual stack and both builds can run TCL tests now. But I guess I might be over-worried. Removed this line.

[zuiderkwast]

LGTM.

A minor suggestion is that we can accept a relative path too. See below.

[zuiderkwast]

I suppose we can convert it to an absolute path if a relative path is given. Then we can drop the limitation.

Idea: It's absolute if it starts with / and otherwise it's relative. We can use this to convert it. Something like this:

if {[string index $::VALKEY_BIN_DIR 0] != "/"} {
    set ::VALKEY_BIN_DIR [file join [pwd] $::VALKEY_BIN_DIR]
}

[zhijun42]

Sounds good! TCL has build-in function file normalize to support that.