pesterhazy commented 1 year ago

Fixes https://github.com/lambdaisland/kaocha/issues/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[bot] commented 1 year ago

Codecov Report

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

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

```diff @@ 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%)` | :arrow_up: | | unit | `69.52% <66.66%> (+0.01%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://codecov.io/gh/lambdaisland/kaocha/pull/321?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [src/kaocha/stacktrace.clj](https://codecov.io/gh/lambdaisland/kaocha/pull/321/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL2thb2NoYS9zdGFja3RyYWNlLmNsag==) | `87.80% <100.00%> (+1.69%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

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

plexus commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Closing in favor of https://github.com/lambdaisland/kaocha/pull/420

lambdaisland / kaocha

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

Codecov Report

before

after