Feature request: infer additional check info for function calls

[jackfirth]

I have a lot of tests that look like this:

(test-case "customer-order-sales-tax should calculate tax correctly"
  (define order (customer-order #:price 10.00))
  (check-equal? (customer-order-sales-tax order) 0.50))

When this fails, I'll see an error message like this:

FAILURE
customer-order-sales-tax should calculate tax correctly
actual: 0.80
expected: 0.50

In DrRacket this isn't so bad since the source location info lets me jump to the failing check and see that order is (customer-order #:price 10.00). But when running tests from the terminal, the failure message doesn't give very much information.

I propose changing check-equal? (and perhaps other checks) to recognize when the actual expression is a "simple" function call and add check infos for the arguments automatically. So a test like this:

(test-case "f should calculate its result correctly"
  (define x 5)
  (define y 10)
  (define z 20)
  (check-equal? (f x y z) 42))

Should produce failures like this:

FAILURE
f should calculate its result correctly
x: 5
y: 10
z: 20
actual: -42
expected: 42

That is, for any check of the form (check-equal? (function:id arg:id ...) expected:expr), each arg value should be added to the check info stack under the name 'arg.

[mfelleisen]

Sounds like a good idea but check-equal? outside of test-case should stay the same. Please.

[samdphillips]

Sounds like a good idea but check-equal? outside of test-case should stay the same. Please.

Maybe this feature would be good to add as a different name initially that worked anywhere (fancier-check-equal?) before changing the existing behavior?

[jackfirth]

Sounds like a good idea but check-equal? outside of test-case should stay the same. Please.

Why? Not against the idea, just wondering if there's a specific problem you have in mind.

[shhyou]

My thought is that the behavior of the error message and the available information becomes fragile depending on the syntax at each use case. In comparison, check-equal? has been an ordinary function with uniform behavior.

I would feel less surprised if the API has a new name, perhaps with syntax

(check-app/infer-name f x1 x2 ... xn <expected>)

Additionally, this syntax could enclose the application with an exception handler.

[shhyou]

Just curious, doesn't your expect support something more powerful?

[jackfirth]

I would feel less surprised if the API has a new name, perhaps with syntax

(check-app/infer-name f x1 x2 ... xn <expected>)

Additionally, this syntax could enclose the application with an exception handler.

If it's a new name, it's pretty much useless, since I'll never remember to use it. Besides there's other places it would be nice, like (check-true (my-predicate input)) or (check-exn exn:fail:foo? (lambda () (my-function input))). We already automatically add some error information inferred from check usage sites like srclocs anyway, so I don't think it's any more of an offense against checks behaving like functions than what exists today.

[jackfirth]

Just curious, doesn't your expect support something more powerful?

It does, but after using expect for a while I gave up and went back to vanilla rackunit. It turns out that powerful error reporting capabilities aren't all that helpful if they require extra user work when writing test cases. I'd rather just write the same ordinary rackunit checks I always write and make rackunit show better failure information by default.

[mfelleisen]

I am just asking for backwards compatibility, the usual Racket guideline. So ``` (module+ test (check-equal? (f 1 2 3) 4) ) ``` should not change functional behavior though an improvement in effects (printing additional information) is okay.

[shhyou]

I would feel less surprised if the API has a new name, perhaps with syntax

(check-app/infer-name f x1 x2 ... xn <expected>)

Additionally, this syntax could enclose the application with an exception handler.

If it's a new name, it's pretty much useless, since I'll never remember to use it. Besides there's other places it would be nice, like (check-true (my-predicate input)) or (check-exn exn:fail:foo? (lambda () (my-function input))). We already automatically add some error information inferred from check usage sites like srclocs anyway, so I don't think it's any more of an offense against checks behaving like functions than what exists today.

Sure. My point is not against adding inferred information, but such inference should be uniform. In the best scenario, there should be no extra rules to demand the user memorize and write checks in a particular form just to obtain the extra information. I'd be happy if the error message is something like expression: (f x y z) so it will survive different usages and refactoring.

For example, if I temporary change y to (+ 1 2) or another function call, what is the output? What if f is a macro? What if I'm testing a curried function (((f x) y) z) or ((f x) (g x))?

---

Here is another thought. Given that those variables are defined locally, what if test-case reports the values of the local definitions?

[jackfirth]

I am just asking for backwards compatibility, the usual Racket guideline. So (module+ test (check-equal? (f 1 2 3) 4) ) should not change functional behavior though an improvement in effects (printing additional information) is okay.

Functional behavior there would be the same, yes.

[jackfirth]

Sure. My point is not against adding inferred information, but such inference should be uniform. In the best scenario, there should be no extra rules to demand the user memorize and write checks in a particular form just to obtain the extra information. I'd be happy if the error message is something like expression: (f x y z) so it will survive different usages and refactoring.

The expression isn't what's useful. The expression you can see by looking at the code, it's the value that you can't see.

I don't think users will have to memorize anything. This is a best-effort feature. If a user wants a guarantee that some check info will be included in the error message, they can still use with-check-info as they would today.

For example, if I temporary change y to (+ 1 2) or another function call, what is the output? What if f is a macro? What if I'm testing a curried function (((f x) y) z) or ((f x) (g x))?

I think it's only useful to report argument info when 1) there's an obvious name to use and 2) the exact literal value passed in isn't obvious looking at the check expression. I posit that there should be this behavior:

• In (check-equal? (f x (+ 1 2) z) 42), x and z argument info is added
• In (check-equal? (((f x) y) z) 42), no argument info is added
• In (check-equal? ((f x) (g x)) 42), no argument info is added
• If f is a macro, no argument info is added. But detecting whether f is a macro or a function is tricky due to things like contract-out. Cooperation with contract-out specifically might be desirable.

Here is another thought. Given that those variables are defined locally, what if test-case reports the values of the local definitions?

This would be nice too. I'm less sure on the details of this, because I have test cases like this:

(test-case "some-function"
  (define input (setup-input ...))
  
  (test-case "case one"
    (define other-input ...)
    (check-equal? (some-function input other-input) ...))

  (test-case "case two"
    (define other-input ...)
    (check-equal? (some-function input other-input) ...)))

...and these can nest quite deep, and some test cases might not use every local variable defined in the outer test case. Maybe only the innermost test cases matter? And maybe only for test cases that span a handful of lines? I'm far less confident making assertions about what's helpful here.

[shhyou]

I see! If it's not a rigid syntax match (check-equal? (function:id arg:id ...) expected:expr) but includes the names of all directly mentioned variables then I agree it's nice.

[samdphillips]

I threw together a little proof of concept of this. There is still some space for abstracting out how to wrap the existing checks. Also things like (check-true (lambda (x) x) fail, which is bad.

Does this look like the right approach to this feature?

https://gist.github.com/samdphillips/913a65998067967fb17c7fd567f01df8

[jackfirth]

@samdphillips That looks about right, although I actually recommend against adding info for expressions in the "expected" position. Focusing on the "actual" position seems like a better approach since the expected position is usually just literal data anyway. Showing too much info will dilute its value.

[AlexKnauth]

@samdphillips

Also things like (check-true (lambda (x) x) fail, which is bad.

There should be some check that the head of the s-expression isn't a special identifier, something like:

(define kernel? (literal-set->predicate kernel-literals))

(define (special-identifier? stx)
  (and (identifier? stx)
       (or (kernel? stx)
           (and (syntax-transforming?) (syntax-local-value stx (λ () #f)) #t))))

(define (extract-arguments stx)
  (syntax-case stx ()
    [(f t* ...)
     (not (special-identifier? #'f))
     (for/list ([t (in-syntax #'(t* ...))] #:when (identifier? t)) t)]
    [_ null]))

This could be improved by cooperating with the contract system, and possibly cooperating with other macros that are guaranteed to play well as functions, but this is at least a good conservative starting point.

[samdphillips]

Thanks @AlexKnauth. In a project where I was exploring this idea more I (very grossly) used free-identifier=? to check for only quote and lambda. I'll test it out with your suggestion.

@jackfirth That seems reasonable. I think it may tidy up the implementation a bit too.