Skip to content

Fix unit test for TLS auto reload#3094

Merged
zuiderkwast merged 3 commits into
valkey-io:unstablefrom
yang-z-o:tls_cert_auto_reload
Jan 23, 2026
Merged

Fix unit test for TLS auto reload#3094
zuiderkwast merged 3 commits into
valkey-io:unstablefrom
yang-z-o:tls_cert_auto_reload

Conversation

@yang-z-o

@yang-z-o yang-z-o commented Jan 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor 
 % ./runtest --single tests/unit/tls.tcl --tls 
Cleanup: may take some time... OK
Starting test server at port 21079
[ready]: 95019
Testing unit/tls
Port 21111 was already busy, trying another port...
Port 21113 was already busy, trying another port...
[ok]: TLS: Not accepting non-TLS connections on a TLS port (0 ms)
[ok]: TLS: Verify tls-auth-clients behaves as expected (31 ms)
[ok]: TLS: Verify tls-protocols behaves as expected (16 ms)
[ok]: TLS: Verify tls-ciphers behaves as expected (27 ms)
[ok]: TLS: Verify tls-prefer-server-ciphers behaves as expected (21 ms)
[ok]: TLS: Verify tls-cert-file is also used as a client cert if none specified (1178 ms)
[ok]: TLS: switch between tcp and tls ports (25 ms)
[ok]: TLS: Working with an encrypted keyfile (12 ms)
[ok]: TLS: Auto-authenticate using tls-auth-clients-user (CN) (12 ms)
[ok]: TLS: Auto-reload detects changes (2700 ms)
[ok]: TLS: Auto-reload skips unchanged materials (1903 ms)
[ok]: TLS: Auto-reload interval validation (6 ms)
[ok]: TLS: Auto-reload with CA cert directory (0 ms)
[ok]: Check for memory leaks (pid 95029) (667 ms)
[1/1 done]: unit/tls (7 seconds)

                   The End

Execution time of different units:
  7 seconds - unit/tls

Test Summary: 14 passed, 0 failed

\o/ All tests passed without errors!

Cleanup: may take some time... OK

@yang-z-o
Fix unit test for TLS auto reload
87e61f5 
Signed-off-by: Yang Zhao <zymy701@gmail.com>
@yang-z-o yang-z-o mentioned this pull request Jan 22, 2026
Closed
@codecov

codecov Bot commented Jan 22, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.20%. Comparing base (1a2ed04) to head (57ecdbc).
⚠️ Report is 4 commits behind head on unstable.

Additional details and impacted files 
@@             Coverage Diff              @@
##           unstable    #3094      +/-   ##
============================================
- Coverage     74.40%   74.20%   -0.20%     
============================================
  Files           129      129              
  Lines         71011    71013       +2     
============================================
- Hits          52833    52695     -138     
- Misses        18178    18318     +140

see 26 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.

@zuiderkwast zuiderkwast added the run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) label Jan 22, 2026
@github-actions github-actions Bot removed the run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) label Jan 22, 2026
@yang-z-o

yang-z-o commented Jan 22, 2026

Copy link
Copy Markdown
Contributor Author

Verifying the fix for the flaky tests in progress on my fork: yang-z-o#2

@enjoy-binbin

enjoy-binbin commented Jan 22, 2026

Copy link
Copy Markdown
Member

There are some tests are failing, can you take a look?

@yang-z-o

yang-z-o commented Jan 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Yes, I’m making fixes for the failing CI jobs.
I’m currently verifying them in my fork with additional CI runs before pushing, since I can’t re-add the tag on this PR.
Progress is here: yang-z-o#2

@madolson

madolson commented Jan 22, 2026

Copy link
Copy Markdown
Member

@yang-z-o I invited you to the valkey org with permissions to add the tag for when you update this PR.

@madolson madolson mentioned this pull request Jan 22, 2026
Merged
@yang-z-o
Skip TLS modules
ed8bcc4 
Signed-off-by: Yang Zhao <zymy701@gmail.com>
@yang-z-o yang-z-o added the run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) label Jan 22, 2026
@github-actions github-actions Bot removed the run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) label Jan 22, 2026
zuiderkwast
zuiderkwast reviewed Jan 22, 2026
View reviewed changes
Comment thread tests/unit/tls.tcl Outdated
@zuiderkwast zuiderkwast added the run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) label Jan 22, 2026
zuiderkwast
zuiderkwast reviewed Jan 22, 2026
View reviewed changes
Comment thread tests/unit/tls.tcl Outdated
zuiderkwast
zuiderkwast reviewed Jan 22, 2026
View reviewed changes
Comment thread tests/unit/tls.tcl Outdated
@github-actions github-actions Bot removed the run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) label Jan 22, 2026
@yang-z-o
Skip better & reduce wait
57ecdbc 
Signed-off-by: Yang Zhao <zymy701@gmail.com>
@yang-z-o yang-z-o added the run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) label Jan 22, 2026
@github-actions github-actions Bot removed the run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) label Jan 22, 2026
@zuiderkwast zuiderkwast added the run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) label Jan 22, 2026
zuiderkwast
zuiderkwast approved these changes Jan 22, 2026
View reviewed changes

@zuiderkwast zuiderkwast left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@madolson do you want to take a look too?

I'm curious if anyone has managed to run the tests with TLS on macOS. I haven't and we don't seem to do it in the CI and Daily jobs.

Comment thread tests/unit/tls.tcl
file copy -force $orig_server_key $temp_key

# Ensure cleanup happens even if test fails
try {

@zuiderkwast zuiderkwast Jan 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yang-z-o Have you managed to run these tests on macOS now?

  • AFAIK, macOS only has TCL 8.5, at least that's what we run in the CI jobs for macos. I don't know if there is a 8.6 version available.
  • try was added in TCL 8.6 and doesn't exist in TCL 8.5.
  • I don't know how to get TCL with TLS in macOS. I would like to know, because I have a mac too...

@yang-z-o yang-z-o Jan 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I had to use Tcl with OpenSSL via Homebrew, instead of using the built-in Tcl with LibreSSL.

@zuiderkwast zuiderkwast removed the run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) label Jan 22, 2026
@zuiderkwast

zuiderkwast commented Jan 22, 2026

Copy link
Copy Markdown
Contributor

The TLS tests are passing in the CI. I saw it, but I accidentally started them by adding the label again and then I cancelled them. Sorry for that.

@zuiderkwast zuiderkwast added the run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) label Jan 22, 2026
@github-actions github-actions Bot removed the run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) label Jan 23, 2026
@zuiderkwast zuiderkwast merged commit 18257cd into valkey-io:unstable Jan 23, 2026
118 of 147 checks passed
@zuiderkwast

zuiderkwast commented Jan 23, 2026

Copy link
Copy Markdown
Contributor

Thank you!

We should drop our support for TCL 8.5 and the TCL version detection for it in the runtest script.

We currently run 8.5 in the CI job for macos, but now if we have try-finally that requires 8.6 and it's possible to install 8.6 in macos, we should drop the support for 8.5 and write some instructions in tests/README.md about how to install the right TCL versions.

harrylin98 pushed a commit to harrylin98/valkey_forked that referenced this pull request Feb 19, 2026
@yang-z-o
Fix unit test for TLS auto reload (valkey-io#3094)
ceb0e11 
Signed-off-by: Yang Zhao <zymy701@gmail.com>
@yang-z-o yang-z-o deleted the tls_cert_auto_reload branch March 4, 2026 05:09
hpatro pushed a commit to hpatro/valkey that referenced this pull request Mar 5, 2026
@yang-z-o @hpatro
Fix unit test for TLS auto reload (valkey-io#3094)
6a68506 
Signed-off-by: Yang Zhao <zymy701@gmail.com>
Signed-off-by: Harkrishn Patro <bunty.hari@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zuiderkwast zuiderkwast zuiderkwast approved these changes
@enjoy-binbin enjoy-binbin enjoy-binbin approved these changes

Assignees

@yang-z-o yang-z-o

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@yang-z-o @enjoy-binbin @madolson @zuiderkwast