Treat exceptions thrown by arguments to check as test failures, without changing function behavior

[AlexKnauth]

Just like #107, this PR has the goal of catching errors thrown by arguments and turning them into check failures. However unlike #107, this one does not change the behavior of checks when used as function-ids.

[AlexKnauth]

What are the errors

instantiate: unknown module
  module name: #<resolved-module-path:"/home/travis/.racket/snapshot/pkgs/scribble-lib/scribble/xref.rkt">
  compilation context...:
   /home/travis/.racket/snapshot/pkgs/racket-index/scribblings/main/local-redirect.scrbl

instantiate: unknown module
  module name: #<resolved-module-path:"/home/travis/racket/collects/syntax/apply-transformer.rkt">
  compilation context...:
   /home/travis/build/racket/rackunit/rackunit-typed/rackunit/main.rkt
   /home/travis/build/racket/rackunit/rackunit-typed/rackunit.rkt

about?

[bennn]

Good question. I think something's wrong with the Travis configuration but I haven't tried to figure it out.

The last two commits to master have the same CI failures, for example: https://travis-ci.org/racket/rackunit/builds/537037545?utm_source=github_status&utm_medium=notification

If the rackunit tests pass locally, I'm ok with merging this PR

[AlexKnauth]

The rackunit tests do not pass locally. The specific order errors might be because I'm adding the location and expression check-infos before everything else, since those are the two things that it can know before it evaluates the arguments.

I have two ideas for how to fix this, but I'm not sure which one to choose: (a) Add name before location and expression so that the order happens to remain the same (b) Evaluate the arguments within a check-info context, and then once we have the values, throw away that check-info context and let the function-call set up its own check-info context from nothing.

raco test: (submod ".../pkgs/rackunit-test/tests/rackunit/check-info-test.rkt" test)
raco test: @(test-responsible '(jay noel))
--------------------
define-check adds certain infos automatically in a specific order
FAILURE
location:   check-info-test.rkt:89:4
name:       check-equal?
actual:     '(location expression name params)
expected:   '(name location expression params)
--------------------
--------------------
define-check infos are added before custom infos
FAILURE
location:   check-info-test.rkt:98:4
name:       check-equal?
actual:     '(location expression name params custom1 custom2)
expected:   '(name location expression params custom1 custom2)
--------------------
--------------------
define-check infos are added before calling current-check-around
FAILURE
location:   check-info-test.rkt:117:4
name:       check-equal?
actual:     '(location expression name params custom)
expected:   '(name location expression params custom)
--------------------

[bennn]

Ok, (a) it is. Tests pass for me. LGTM

[sorawee]

Good to merge?

[samth]

I don't totally understand how well this avoids the concerns raised in #107. @AlexKnauth can you explain a little?

[bennn]

This PR evaluates args to a check-X form when its used directly (the common case) in the same testing context as the check itself:

Example program:

#lang racket/base
(require rackunit)

(define (f)
  (+ 41 "1"))

(check-eq? (f) 42)

On master the call to f throws a normal error:

+: contract violation
  expected: number?
  given: "1"
  ....

but with this PR, RackUnit catches the exception:

--------------------
ERROR
name:       check-eq?
location:   test.rkt:7:0

+: contract violation
  expected: number?
  given: "1"
--------------------

checks as functions

Using check-eq as a function gives the same answer on master and here:

#lang racket/base
(require rackunit)

(define (f)
  (+ 41 "1"))

(define chk check-eq?)

(chk (f) 42)
;; +: contract violation

[rfindler]

Does the exception printing just call error-display-handler?

And, while we're fixing things in rackunit, can we make something like this be a check failure:

(check-equal? (values 1 2) (values 1 3))

and this be a success?

(check-equal? (values 1 2) (values 1 2))

Robby

[sorawee]

The values variant is addressed in https://github.com/racket/rackunit/pull/73, I think.

[AlexKnauth]

@samth Re: the concerns raised in #107

I see one main concern voiced by both @bennn and @rmculpepper:

This changes the "checks are functions" idea that the docs suggest. In rackunit, the check forms are meant to act like functions.

I believe this PR addresses that by leaving make-check-func and the check-impl function alone, and in particular not changing the "calling convention" for how the macro-version of each check calls check-impl, so that no matter whether it's used as a macro or function, there's no difference in how the arguments are passed into the check-impl.

[AlexKnauth]

Is that enough to address the concerns?

[samth]

This looks ok to me.

[AlexKnauth]

Is it okay to merge this weekend if there are no additional concerns raised?