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

Add stacktrace option to test runner output #471

Draft
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

stefan-toubia
Copy link
Contributor

What has Changed?

  • Add exception stacktrace to test runner output for tests that failed in error
  • Add calva option outputStackTraceOnTest to enable outputting stacktrace

Details

This is still WIP, but here's my first pass at adding stacktrace to the test runner output. This prints stacktraces in the output pane for each test that had an error.

This uses cider's test-stacktrace command, which has been added to NReplSession. This works by requesting the stacktrace for each error individually, I'm not sure if there would be a performance hit if for example you run all tests and there are many errors. I believe that emacs gives only shows each stacktrace on demand if you want to see it.

What I have so far accomplishes the primary goal of getting a stacktrace for a test error, but there is room for improvement on how the stacktrace is output. VS code's output window is a bit limited, and a lot of stacktraces could potentially overwhelm the output. It would be nice if sections of the output were collapsable. I've thought of a few alternate approaches:

  • Write the stacktraces out to temp files in the workspace folder and put a clickable link in the output.
  • Write the stacktraces out to the REPL webview. I'm not sure how easy this would be, but the new stacktrace formatting in the REPL webview is very nice and would be great to reuse. Ideally there would be a link in the test output that could trigger the REPL window to show the stacktrace, but this is the part that I'm not sure is possible. This would be a better solution than linking to a log file.
  • Use the VS Code Diagnostic output to show the stacktrace on the Peek Problem link, similar to how the test failures are already working. I haven't played with this yet, but I think it should be implemented anyway. It looks like it should be easy to add. Maybe this would allow for querying the stacktrace on demand instead of all at once. I need to read up on this.

Output

(deftest error-test
  (is (= 1 (/ 1 0)) "Try dividing by 0"))
ERROR in hello.hello-test/error-test (line 15):
Try dividing by 0
expected: (= 1 (/ 1 0))
   error: java.lang.ArithmeticException: Divide by zero (Numbers.java)
            Numbers.java: 188 clojure.lang.Numbers/divide
            Numbers.java:3901 clojure.lang.Numbers/divide
          hello_test.clj:  15 hello.hello_test$fn__6529/invokeStatic
          hello_test.clj:  14 hello.hello_test$fn__6529/invoke
                test.clj: 203 cider.nrepl.middleware.test$test_var$fn__6746/invoke
                test.clj: 203 cider.nrepl.middleware.test$test_var/invokeStatic
                test.clj: 195 cider.nrepl.middleware.test$test_var/invoke
                test.clj: 218 cider.nrepl.middleware.test$test_vars$fn__6750$fn__6755/invoke
                test.clj: 687 clojure.test$default_fixture/invokeStatic
                test.clj: 683 clojure.test$default_fixture/invoke
                test.clj: 218 cider.nrepl.middleware.test$test_vars$fn__6750/invoke
                test.clj: 687 clojure.test$default_fixture/invokeStatic
                test.clj: 683 clojure.test$default_fixture/invoke
                test.clj: 215 cider.nrepl.middleware.test$test_vars/invokeStatic
                test.clj: 209 cider.nrepl.middleware.test$test_vars/invoke
                test.clj: 231 cider.nrepl.middleware.test$test_ns/invokeStatic
                test.clj: 222 cider.nrepl.middleware.test$test_ns/invoke
                test.clj: 242 cider.nrepl.middleware.test$test_var_query/invokeStatic
                test.clj: 235 cider.nrepl.middleware.test$test_var_query/invoke
                test.clj: 280 cider.nrepl.middleware.test$handle_test_var_query_op$fn__6794$fn__6795/invoke
                AFn.java: 152 clojure.lang.AFn/applyToHelper
                AFn.java: 144 clojure.lang.AFn/applyTo
                core.clj: 665 clojure.core$apply/invokeStatic
                core.clj:1973 clojure.core$with_bindings_STAR_/invokeStatic
                core.clj:1973 clojure.core$with_bindings_STAR_/doInvoke
             RestFn.java: 425 clojure.lang.RestFn/invoke
                test.clj: 272 cider.nrepl.middleware.test$handle_test_var_query_op$fn__6794/invoke
                AFn.java:  22 clojure.lang.AFn/run
             session.clj: 171 nrepl.middleware.session$session_exec$main_loop__1036$fn__1040/invoke
             session.clj: 170 nrepl.middleware.session$session_exec$main_loop__1036/invoke
                AFn.java:  22 clojure.lang.AFn/run
             Thread.java: 748 java.lang.Thread/run

Closes #424

My Calva PR Checklist

I have:

  • Read How to Contribute.
  • Made sure I am directing this pull request at the dev branch. (Or have specific reasons to target some other branch.)
  • Made sure I am changed the default PR base branch, so that it is not master. (Sorry for the nagging.)
  • Tested the VSIX built from the PR (well, if this is a PR that changes the source code.) You'll find the artifacts by clicking Show all checks in the CI section of the PR page, and then Details on the ci/circleci: build test. (For now you'll need to opt in to the CircleCI New Experience UI to see the Artifacts tab, because bug.)
    • Tested the particular change
    • Figured if the change might have some side effects and tested those as well.
    • Smoke tested the extension as such.
  • Referenced the issue I am fixing/addressing in a commit message for the pull request.
  • Updated the [Unreleased] entry in CHANGELOG.md, linking the issue(s) that the PR is addressing.

The Calva Team PR Checklist:

Before merging we (at least one of us) have:

  • Made sure the PR is directed at the dev branch (unless reasons).
  • Read the source changes.
  • Given feedback and guidance on source changes, if needed. (Please consider noting extra nice stuff as well.)
  • Tested the VSIX built from the PR (well, if this is a PR that changes the source code.)
    • Tested the particular change
    • Figured if the change might have some side effects and tested those as well.
    • Smoke tested the extension as such.
  • If need be, had a chat within the team about particular changes.

Ping @PEZ, @kstehn, @cfehse

@PEZ
Copy link
Collaborator

PEZ commented Nov 16, 2019

Thanks!

What I think would make the most sense is for the test runner to use the setting calva.sendAsyncOutputTo and if it is set to output to the REPL Window, we would send all output there and also always output stack-traces the way we do there today. (Including being initially collapsed). And then we wouldn't need making it optional, instead.

Since the file references will probably not be clickable in the output window, stack traces are less useful there.

@stefan-toubia
Copy link
Contributor Author

Sounds like a good idea! Got a little busy this week but I've gotten most of that implemented, I'll be getting it up soon.

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.

2 participants