racket / rackunit

Other
18 stars 32 forks source link

Print the exception in the check-info with error-display-handler #157

Closed sorawee closed 2 years ago

sorawee commented 2 years ago

@giuliano from Discord raised an issue that check-not-exn interferes with errortrace. Specifically, rackunit replaces the original exception with a new one, and stuffs the original exception inside a check-info named 'exception. While this interference is by design, it gives a bad user experience because users can no longer trace back to the original exception location.

This PR solves the problem by recognizing the check-info and print its associating value using error-display-handler, which allows the errortracer to operate on the original exception.

For example, consider:

#lang errortrace racket

(define (f)
  (add1 (/ 1 0)))

(module+ test
  (require rackunit)
  (check-not-exn (thunk (f))))

The program outputs the following before the PR:

raco test: (submod "test.rkt" test)
--------------------

  errortrace...:
   /Applications/Racket v8.5/share/pkgs/rackunit-lib/rackunit/private/check.rkt:135:16: (module+ test (require rackunit) (check-not-exn (thunk (f))))
name:               check-not-exn
location:           test.rkt:8:2
params:             '(#<procedure:...yground/test.rkt:8:17>)
message:            "Exception raised"
exception-message:  "/: division by zero"
exception:
  #(struct:exn:fail:contract:divide-by-zero "/: division by zero" #<continuation-mark-set>)
--------------------
1/1 test failures

But it outputs the following after the PR:

raco test: (submod "test.rkt" test)
--------------------
FAILURE
  message:
  errortrace...:
   /Users/soraweep/projects/racket/extra-pkgs/rackunit/rackunit-lib/rackunit/private/check.rkt:135:16: (module+ test (require rackunit) (check-not-exn (thunk (f))))
name:               check-not-exn
location:           test.rkt:8:2
params:             '(#<procedure:...s/soraweep/test.rkt:8:17>)
message:            "Exception raised"
exception-message:  "/: division by zero"
exception:
  /: division by zero
    errortrace...:
     /Users/soraweep/test.rkt:4:8: (/ 1 0)
     /Users/soraweep/test.rkt:4:2: (add1 (/ 1 0))
     /Users/soraweep/projects/racket/extra-pkgs/rackunit/rackunit-lib/rackunit/private/check.rkt:135:16: (module+ test (require rackunit) (check-not-exn (thunk (f))))
    context...:
     /Users/soraweep/test.rkt:3:0: f
     /Users/soraweep/projects/racket/extra-pkgs/rackunit/rackunit-lib/rackunit/private/check.rkt:122:34
     /Users/soraweep/projects/racket/extra-pkgs/rackunit/rackunit-lib/rackunit/private/check.rkt:70:4
     body of (submod "/Users/soraweep/test.rkt" test)
     /Users/soraweep/projects/racket/pkgs/compiler-lib/compiler/commands/test.rkt:191:16
--------------------
1/1 test failures

Note that the output after the PR incorporates changes from https://github.com/racket/errortrace/pull/30

sorawee commented 2 years ago

Questions:

  1. Should we still print #(struct:exn:fail:contract:divide-by-zero "/: division by zero" #<continuation-mark-set>)?
  2. Should "/: division by zero" until the end of the stack be further indented (say, by two spaces)?
sorawee commented 2 years ago

Any opinion? @AlexKnauth @jackfirth @samth

sorawee commented 2 years ago

All pending questions are resolved, so this should now be ready for actual review + merge. A test is also added in the latest commit.

rfindler commented 2 years ago

Questions:

1. Should we still print `#(struct:exn:fail:contract:divide-by-zero "/: division by zero" #<continuation-mark-set>)`?

Dunno if this was answered, but I think that if the value raised answers true to exn? then it would be nice to just print what the error-display-handler prints. If it doesn't, however, it might be nice to indicate that, and then print the value.

sorawee commented 2 years ago

@rfindler yes, that is exactly what my PR does.

rfindler commented 2 years ago

Thank you! Great!

jackfirth commented 2 years ago

Looks good to me. It's been a long time since I've touched this code though.

samth commented 2 years ago

@sorawee can you squash the commits here with a message that's up to date?

sorawee commented 2 years ago

Done!