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

Truncate irrelevant parts of stacktrace #420

Merged
merged 7 commits into from
Sep 27, 2023

Conversation

socksy
Copy link
Contributor

@socksy socksy commented Jun 14, 2023

Resolves #58

This is directly based on @pesterhazy's PR #321

What I've added:

  • made the binding dynamic, making it possible to rebind in your tests.edn
  • added a line to the docs
  • added tests for both existing stacktrace filtering, and stacktrace truncating
  • removed orchestra from default stacktrace filtering, since people might use it in their own code (ala @tonsky's comment)

@pesterhazy
Copy link
Contributor

Very nice – I'll close my PR in favor of this one!

Currently failing the tests though:

FAIL in config.bindings/stacktrace-shortening (test/features/config/bindings.feature:110)
the output should not contain
expected: (not (str/includes? (:out m) output))
  actual: (not (not true))

@alysbrooks
Copy link
Member

Thanks for picking this up, @socksy! I think we can likely merge this after the failing test is fixed on CI. (Strangely, it passes when I run it locally, but I don't have time to dig in further, unfortunately.)

@socksy
Copy link
Contributor Author

socksy commented Jun 17, 2023

Yeah, it works for me locally when developing, so I figured this must somehow be a CI runner setup? Unfortunately I'm away from the keyboard for at least the next 2 weeks and unable to debug for a while.

Is the CI running a different command than when you test locally? Or is this maybe a Linux/Mac thing?

@socksy
Copy link
Contributor Author

socksy commented Jun 17, 2023

I also noticed that the CI error has

(Rest of stacktrace elided)

which is explicitly disabled in the tests.edn, for obvious reasons 😅. Can it be that this file is ignored when running the tests in CI?!

@alysbrooks
Copy link
Member

@socksy I think that's from within the test of eliding itself, so the stack eliding (elision?) is configured by the test rather than the overall project settings.

My suspicion is that this test might be inheriting a setting from the overall test suite that's only enabled on CI and is causing kaocha.ns to appear apart from the stacktrace itself because the error is coming from line 110 of the feature test.

@alysbrooks
Copy link
Member

Trying to mimic the CI invocation locally didn't reproduce the CI failure. A couple things I tried:

  • bin/kaocha --reporter documentation integration --plugin cloverage --codecov
  • bin/kaocha --plugin :kaocha.plugin/profiling --plugin :kaocha.plugin/capture-output --plugin :kaocha.plugin.alpha/info --plugin :kaocha.plugin/cloverage --print-env --reporter kaocha.report/documentation --codecov --focus 'config.bindings/stacktrace-shortening'

This is pretty strange, since the differences should be pretty marginal between my environment and CI if I pass all those flags, especially since I'm running a Linux system. The main difference remaining is that CI is a headless environment, but I'm pretty sure the only part of Kaocha that is affected by that is the notification plugin.

@alysbrooks
Copy link
Member

Turns out the missing ingredient was setting the CI environment variable. (You can also use --profile ci). I knew Kaocha pays attention to that flag, but I didn't realize any stacktrace stuff was affected by it. I'm not actually sure how to fix it, though. Getting closer, though!

pesterhazy and others added 3 commits September 18, 2023 19:15
On CI, we have plugins enabled that also list namespaces, so adding the
at ensures those aren't confused for elements of the stack trace.
"at" is only used on the first line, and the point of this filtering is
to prevent it from printing noise further along than the first line to
increase readibility of the entire stacktrace.

`kaocha.runner` seems to be a good bet to be running both locally and in
CI from my observations, and thus better than `kaocha.ns` which doesn't
appear to be in the stacktrace when not in CI?
@codecov
Copy link

codecov bot commented Sep 18, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.04% 🎉

Comparison is base (7dcb230) 75.10% compared to head (8ed006d) 75.15%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #420      +/-   ##
==========================================
+ Coverage   75.10%   75.15%   +0.04%     
==========================================
  Files          52       52              
  Lines        2804     2809       +5     
  Branches      289      290       +1     
==========================================
+ Hits         2106     2111       +5     
  Misses        511      511              
  Partials      187      187              
Flag Coverage Δ
integration 56.31% <100.00%> (+0.07%) ⬆️
unit 69.41% <60.00%> (-0.02%) ⬇️

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

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@socksy
Copy link
Contributor Author

socksy commented Sep 19, 2023

@alysbrooks wdyt? seems to finally pass CI :)

@plexus
Copy link
Member

plexus commented Sep 27, 2023

Thanks, @socksy

@plexus plexus merged commit 5579c24 into lambdaisland:main Sep 27, 2023
9 checks passed
@plexus
Copy link
Member

plexus commented Sep 27, 2023

Released in v1.87.1366

[lambdaisland/kaocha "1.87.1366"]                 ;; deps.edn
{lambdaisland/kaocha {:mvn/version "1.87.1366"}}  ;; project.clj

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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