Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Truncate stacktrace once the first kaocha ns is encountered #321

Closed
wants to merge 1 commit into from

Conversation

pesterhazy
Copy link
Contributor

Fixes #58

This PR stops printing stacktraces when the first in a list of sentinels (kaocha stackframes) is encountered. Everything after that point is noise, as it doesn't pertain to the current project.

I had to scroll up on every test failure because the whole trace didn't fit in my terminal. This PR works for my small project so far, but haven't tested it extensively.

Marked as WIP because I'm not sure about a few design questions:

  • Should this be default? Optional (bring-your-own-stacktrace-printer)?
  • What if you're working on kaocha itself?
  • This is slightly "clever" logic. Should I add a unit test to the function?

@codecov
Copy link

codecov bot commented Oct 13, 2022

Codecov Report

Base: 75.47% // Head: 75.52% // Increases project coverage by +0.04% 🎉

Coverage data is based on head (dcf913d) compared to base (7e77da6).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #321      +/-   ##
==========================================
+ Coverage   75.47%   75.52%   +0.04%     
==========================================
  Files          51       51              
  Lines        2732     2737       +5     
  Branches      255      255              
==========================================
+ Hits         2062     2067       +5     
  Misses        510      510              
  Partials      160      160              
Flag Coverage Δ
integration 57.10% <100.00%> (+0.07%) ⬆️
unit 69.52% <66.66%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/kaocha/stacktrace.clj 87.80% <100.00%> (+1.69%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@plexus
Copy link
Member

plexus commented Oct 13, 2022

On a quick skim it seems reasonable. Could you maybe paste in an example here of the output before and after this patch?

Would there be a way to opt-out of this behavior? With stuff like this (see also e.g. output-capturing, or the elision behavior we have now) I think it's important to be able to opt-out completely in case there's a concern that this cleverness is getting in people's way.

@pesterhazy
Copy link
Contributor Author

before

76 lines

ERROR in unit (pico/app/control_test.clj:18)
Failed loading tests:
Exception: clojure.lang.Compiler$CompilerException: Syntax error compiling at (pico/app/control_test.clj:18:3).
#:clojure.error{:phase :compile-syntax-check, :line 18, :column 3, :source "pico/app/control_test.clj"}
 at clojure.lang.Compiler.analyze (Compiler.java:6825)
    ...
    kaocha.ns$required_ns.invokeStatic (ns.clj:13)
    kaocha.ns$required_ns.invoke (ns.clj:11)
    kaocha.type.ns$eval5680$fn__5681.invoke (ns.clj:29)
    ...
    kaocha.testable$load.invokeStatic (testable.clj:94)
    kaocha.testable$load.invoke (testable.clj:75)
    kaocha.testable$load_testables.invokeStatic (testable.clj:152)
    kaocha.testable$load_testables.invoke (testable.clj:144)
    kaocha.load$load_test_namespaces.invokeStatic (load.clj:49)
    kaocha.load$load_test_namespaces.doInvoke (load.clj:37)
    ...
    kaocha.type.clojure.test$eval7102$fn__7103.invoke (test.clj:17)
    ...
    kaocha.testable$load.invokeStatic (testable.clj:94)
    kaocha.testable$load.invoke (testable.clj:75)
    kaocha.testable$load_testables.invokeStatic (testable.clj:152)
    kaocha.testable$load_testables.invoke (testable.clj:144)
    kaocha.api$test_plan.invokeStatic (api.clj:56)
    kaocha.api$test_plan.invoke (api.clj:49)
    kaocha.api$run$fn__5358.invoke (api.clj:101)
    ...
    kaocha.api$run.invokeStatic (api.clj:99)
    kaocha.api$run.invoke (api.clj:86)
    kaocha.runner$run$fn__5421.invoke (runner.clj:132)
    ...
    kaocha.runner$run.invokeStatic (runner.clj:130)
    kaocha.runner$run.invoke (runner.clj:73)
    kaocha.runner$_main_STAR_.invokeStatic (runner.clj:176)
    kaocha.runner$_main_STAR_.doInvoke (runner.clj:144)
    ...
    kaocha.runner$_main.invokeStatic (runner.clj:187)
    kaocha.runner$_main.doInvoke (runner.clj:185)
    ...
Caused by: java.lang.RuntimeException: No such var: x/invalid
 at clojure.lang.Util.runtimeException (Util.java:221)
    ...
    kaocha.ns$required_ns.invokeStatic (ns.clj:13)
    kaocha.ns$required_ns.invoke (ns.clj:11)
    kaocha.type.ns$eval5680$fn__5681.invoke (ns.clj:29)
    ...
    kaocha.testable$load.invokeStatic (testable.clj:94)
    kaocha.testable$load.invoke (testable.clj:75)
    kaocha.testable$load_testables.invokeStatic (testable.clj:152)
    kaocha.testable$load_testables.invoke (testable.clj:144)
    kaocha.load$load_test_namespaces.invokeStatic (load.clj:49)
    kaocha.load$load_test_namespaces.doInvoke (load.clj:37)
    ...
    kaocha.type.clojure.test$eval7102$fn__7103.invoke (test.clj:17)
    ...
    kaocha.testable$load.invokeStatic (testable.clj:94)
    kaocha.testable$load.invoke (testable.clj:75)
    kaocha.testable$load_testables.invokeStatic (testable.clj:152)
    kaocha.testable$load_testables.invoke (testable.clj:144)
    kaocha.api$test_plan.invokeStatic (api.clj:56)
    kaocha.api$test_plan.invoke (api.clj:49)
    kaocha.api$run$fn__5358.invoke (api.clj:101)
    ...
    kaocha.api$run.invokeStatic (api.clj:99)
    kaocha.api$run.invoke (api.clj:86)
    kaocha.runner$run$fn__5421.invoke (runner.clj:132)
    ...
    kaocha.runner$run.invokeStatic (runner.clj:130)
    kaocha.runner$run.invoke (runner.clj:73)
    kaocha.runner$_main_STAR_.invokeStatic (runner.clj:176)
    kaocha.runner$_main_STAR_.doInvoke (runner.clj:144)
    ...
    kaocha.runner$_main.invokeStatic (runner.clj:187)
    kaocha.runner$_main.doInvoke (runner.clj:185)
    ...
1 tests, 1 assertions, 1 errors, 0 failures.

after

12 lines

ERROR in unit (pico/app/control_test.clj:18)
Failed loading tests:
Exception: clojure.lang.Compiler$CompilerException: Syntax error compiling at (pico/app/control_test.clj:18:3).
#:clojure.error{:phase :compile-syntax-check, :line 18, :column 3, :source "pico/app/control_test.clj"}
 at clojure.lang.Compiler.analyze (Compiler.java:6825)
    ...
(Rest of stacktrace elided)
Caused by: java.lang.RuntimeException: No such var: x/invalid
 at clojure.lang.Util.runtimeException (Util.java:221)
    ...
(Rest of stacktrace elided)
1 tests, 1 assertions, 1 errors, 0 failures.

@pesterhazy
Copy link
Contributor Author

Would there be a way to opt-out of this behavior?

Totally agree, for some cases - e.g. working on kaocha itself - it might be preferable to see the full trace.

What type of option would you prefer? Plugin? Command-line switch? Any existing switch to model this on?

One possibility is to have two printers:

  • kaocha.stacktrace/print-full-cause-trace (behavior as in main)
  • kaocha.stacktrace/print-short-cause-trace (behavior as in this PR)

Then the user can select a printer using a command line switch, defaulting to kaocha.stacktrace/print-short-cause-trace

This would also allow the user to supply their own printer, without having to modify kaocha code.

@pesterhazy
Copy link
Contributor Author

With the optional support, we could do "staged rollout": it could start out as optional and then be promoted to default when people like it

@alysbrooks
Copy link
Member

It seems like a unit test would be awkward with the current architecture since it's printing to the terminal. Or did you just want to test sentinel-element?.

Using a printer system would be an elegant solution if users want to write their own stacktrace printer. I'm not sure how often they do. (But perhaps if we had good documentation, they would?)

@pesterhazy
Copy link
Contributor Author

Closing in favor of #420

@pesterhazy pesterhazy closed this Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

Filter out all kaocha-related stacktrace elements by default
3 participants