valkey icon indicating copy to clipboard operation
valkey copied to clipboard

Refactor TCL reference to support running tests with CMake

Open zhijun42 opened this issue 1 month ago • 5 comments

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.

zhijun42 avatar Nov 07 '25 07:11 zhijun42

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 72.44%. Comparing base (7f8c5b6) to head (153a7e4). :warning: Report is 39 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

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Nov 07 '25 08:11 codecov[bot]

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?

ranshid avatar Nov 10 '25 15:11 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?

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 avatar Nov 11 '25 01:11 zhijun42

Also updated the PR description to reflect the new changes.

zhijun42 avatar Nov 11 '25 01:11 zhijun42

Why not simply using the standard PATH environment variable?

eifrah-aws avatar Nov 12 '25 08:11 eifrah-aws