Skip to content

Improved Tcl stacktraces#3612

Open
zuiderkwast wants to merge 4 commits into
valkey-io:unstablefrom
zuiderkwast:tcl-stacktraces
Open

Improved Tcl stacktraces#3612
zuiderkwast wants to merge 4 commits into
valkey-io:unstablefrom
zuiderkwast:tcl-stacktraces

Conversation

@zuiderkwast
Copy link
Copy Markdown
Contributor

@zuiderkwast zuiderkwast commented May 5, 2026

Generate stack traces with filenames and absolute line numbers.

Examples, with exceptions inserted in the test suite:

% ./runtest --single tests/unit/auth.tcl
Cleanup: may take some time... OK
Starting test server at port 21079
[ready]: 48510
Testing unit/auth
[ok]: AUTH fails if there is no password configured server side (0 ms)
[exception]: Executing test client: invalid command name "dummy_undefined_function".
 in dummy_undefined_function at tests/unit/auth.tcl:6
 in start_server at tests/support/server.tcl:766
 in execute_test_file at tests/unit/auth.tcl:1
 in execute_test_file at tests/test_helper.tcl:146
 in test_client_main at tests/test_helper.tcl:645
 in test_client_main at tests/test_helper.tcl:1062
 in catch at tests/test_helper.tcl:1062
 in if at tests/test_helper.tcl:1061

% ./runtest --single tests/unit/auth.tcl
Cleanup: may take some time... OK
Starting test server at port 21079
[ready]: 52538
Testing unit/auth
[exception]: Executing test client: Bad protocol, 'N' as reply type byte.
 in valkey::__dispatch__ at tests/support/valkey.tcl:116
 in $fd at tests/unit/auth.tcl:4
 in test at tests/support/test.tcl:262
 in test at tests/unit/auth.tcl:2
 in start_server at tests/support/server.tcl:766
 in execute_test_file at tests/unit/auth.tcl:1
 in execute_test_file at tests/test_helper.tcl:146
 in test_client_main at tests/test_helper.tcl:645
 in test_client_main at tests/test_helper.tcl:1062
 in catch at tests/test_helper.tcl:1062
 in if at tests/test_helper.tcl:1061

Generate stack traces with filenames and absolute line numbers.

Examples:

```
% ./runtest --single tests/unit/auth.tcl
Cleanup: may take some time... OK
Starting test server at port 21079
[ready]: 48510
Testing unit/auth
[ok]: AUTH fails if there is no password configured server side (0 ms)
[exception]: Executing test client: invalid command name "dummy_undefined_function".
 in dummy_undefined_function at tests/unit/auth.tcl:6
 in start_server at tests/support/server.tcl:766
 in execute_test_file at tests/unit/auth.tcl:1
 in execute_test_file at tests/test_helper.tcl:146
 in test_client_main at tests/test_helper.tcl:645
 in test_client_main at tests/test_helper.tcl:1062
 in catch at tests/test_helper.tcl:1062
 in if at tests/test_helper.tcl:1061

% ./runtest --single tests/unit/auth.tcl
Cleanup: may take some time... OK
Starting test server at port 21079
[ready]: 52538
Testing unit/auth
[exception]: Executing test client: Bad protocol, 'N' as reply type byte.
 in valkey::__dispatch__ at tests/support/valkey.tcl:116
 in $fd at tests/unit/auth.tcl:4
 in test at tests/support/test.tcl:262
 in test at tests/unit/auth.tcl:2
 in start_server at tests/support/server.tcl:766
 in execute_test_file at tests/unit/auth.tcl:1
 in execute_test_file at tests/test_helper.tcl:146
 in test_client_main at tests/test_helper.tcl:645
 in test_client_main at tests/test_helper.tcl:1062
 in catch at tests/test_helper.tcl:1062
 in if at tests/test_helper.tcl:1061
```

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
The previous implementation reconstructed args from parsed opts + result,
which broke 'return {*}$args' patterns where multiple positional arguments
are passed (as seen in the failure in the scripting tests).

Now we only inject/modify -level in the original args list and pass
everything else through unchanged to orig_return.

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Copy link
Copy Markdown
Contributor

@sarthakaggarwal97 sarthakaggarwal97 left a comment

Choose a reason for hiding this comment

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

I support this idea!

@zuiderkwast zuiderkwast marked this pull request as draft May 6, 2026 10:26
- Only override return on Tcl 8.6+ where tailcall is available.
  This avoids the -level bump hack that broke package loading and
  other Tcl internals using uplevel + return -code error.
- On Tcl 8.5, return is not overridden; only error is overridden.
  return -code error traces degrade to $::errorInfo on 8.5.
- error override now calls orig_error directly instead of going
  through return -code error, making it work on all Tcl versions.

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Tcl's return treats args as a dict with an optional trailing result
value (odd element). Use this directly instead of manual flag parsing.

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
@codecov
Copy link
Copy Markdown

codecov Bot commented May 6, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.65%. Comparing base (8891441) to head (bdb6b78).
⚠️ Report is 5 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #3612      +/-   ##
============================================
- Coverage     76.90%   76.65%   -0.26%     
============================================
  Files           162      162              
  Lines         80612    80614       +2     
============================================
- Hits          61996    61792     -204     
- Misses        18616    18822     +206     

see 24 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 marked this pull request as ready for review May 6, 2026 14:39
@zuiderkwast zuiderkwast added the run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) label May 6, 2026
@zuiderkwast zuiderkwast requested a review from madolson May 6, 2026 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants