Add meta checks

[jackfirth]

Closes #47

This PR adds the following checks:

• (check-fail <pred-or-exn> <thunk>) - asserts that <thunk> evaluates a failing check and that the check failure exception satisfies <pred-or-exn> in the same way as check-exn
• (check-fail* <thunk>) - like check-fail, but without caring about the specifics of the thrown failure
• (check-fail/info <check-info> <thunk>) - asserts that <thunk> evaluates a failing check and that the check failure exception contains an info that is equal? to <check-info>
• (check-error <pred-or-exn> <thunk>) - asserts that <thunk> evaluates a check that raises an error instead of failing and that the raised value satisfies <pred-or-exn>

I went with check-fail/info instead of adding an info case to check-fail to avoid confusion over whether check-error has an info-accepting variant. The existence of check-fail/info and nonexistence of check-error/info is a clear signal in documentation search results and playing at the REPL that wouldn't be present if check-fail/info was merged with check-fail.

The only addition from the proposed API is check-error, which I realized I'd need to test to the contract failure cases of these checks.

[AlexKnauth]

Why is check-error called check-error?

[jackfirth]

Symmetry with the output of checks, since check failures print "FAILURE" while check errors print "ERROR". If you have a suggestion for a less ambiguous name, I'm all ears.

[bennn]

How does check-error differ from check-exn?

I tried a quick example and check-exn works how I'd expect:

#lang racket
(module+ test
  (require rackunit)
  (check-exn exn:fail:contract?
    (lambda () (check-true (+ "asdf" 2)))))

$ raco test test.rkt
raco test: (submod "test.rkt" test)
1 test passed

Reminds me though, it would be good to have a check-exn helper to test "throws a contract error that blames a certain party"

[jackfirth]

That example works because (+ "asdf" 2") is evaluated before being passed as an argument to check-true, so the error isn't thrown from within the body of check-true. Try this example:

(check-exn (lambda (v) (equal? v 'oops))
  (lambda () (check-pred (lambda (v) (raise 'oops)) 'foo)))

[bennn]

I think (predicate/c pred-or-msg) will do the right thing (and also check (procedure-arity-includes? pred-or-msg 1))

(or use contract-out)

[bennn]

can you add a procedure-arity-includes? check here?

[bennn]

These fail-check should print failure somehow. (Do they?)

I'd prefer they mirror check-exn, and also say "Wrong failure raised" in both cases

[jackfirth]

Done. They now print the message and the failure value itself, just like check-exn.

[bennn]

This stops at the first unexpected info, right?

I think it'd be better to print all the unexpecteds.

[jackfirth]

I hadn't thought about if the info stack contains multiple infos with the expected name but different values. This needs to be reworked to deal with that.

[bennn]

(predicate/c pred-or-msg)

[jackfirth]

That's a higher order contract, so I don't think it would work here.

[bennn]

same here, add procedure-arity-includes?

[bennn]

eventually = this pull request?

[jackfirth]

By "real check contracts", I mean #55. I don't plan on implementing those in this PR.

[bennn]

predicate/c
(next line too)

[jackfirth]

I'd rather not add the complexity of real higher order contracts here. Additionally, the function used as a predicate can return non-boolean values which predicate/c doesn't allow.

[bennn]

ok. then (and (procedure? pred-or-msg) (procedure-arity-includes? pred-or-msg 1))
?

[jackfirth]

Done

[bennn]

use negate ?

I don't really care but racket/function's already imported

[jackfirth]

Done

[jackfirth]

@bennn Added docs and addressed all your comments (I think), this PR is ready for a final review.

[bennn]

good timing :)

[bennn]

it it's => if it's

[jackfirth]

Done.

[bennn]

Should get the same output with @examples[#:label #f ....

[jackfirth]

Done. Didn't add #:label #f because using the label seems appropriate. They are examples, after all.

[bennn]

if it => if

Also "checks that the raised value" should refer to fail-exn-predicate.

(The sentence is a little long & awkward ... maybe start a new sentence after "passing or failing"? or end it with "checks that the raised value satisfies the predicate. Satisfies means ....")

[bennn]

can you add a procedure-arity-includes? check here?

[bennn]

same here, add procedure-arity-includes?

[AlexKnauth]

Would it be better if check-error were renamed to either check-check-error or check-fail-error?

[jackfirth]

@AlexKnauth How about check-fail/error? According to scheme pronunciation rules, that's "check fail with error" which seems accurate.

[jackfirth]

@bennn For some reason I can't respond directly to your comments about procedure-arity-includes?. I added the arity check in contract-pred-or-msg! so I don't think it needs to be tested in the cond expression.

[AlexKnauth]

(Re: check-fail/error) That makes sense to me.

[rmculpepper]

I think this should be a new module, maybe rackunit/meta. That would make it clearer that these are not general-purpose checks, but rather specialized for testing other checks.

I'd prefer if check-fail, check-fail*, and check-fail/info were combined into one form. (I disagree with your rationale for keeping them separate.) I propose:

;; check-fail : (Treeof FailurePred) (-> Any) -> Void
;; where FailurePred is one of
;; - (exn:test:check -> Boolean)  -- exn must satisfy predicate
;; - Regexp                       -- exn message must match
;; - Check-Info                    -- exn must contain check-info

More descriptive, less ambiguous names would be check-check-fail and check-check-error... but I could understand not liking them. check-fail/exn is not quite right, since we consider failures distinct from errors. But check-error is bad, because it's too similar to check-exn. (I believe there's a library somewhere that defines check-error as a macro that wraps the expression to check as a thunk.)

[jackfirth]

@rmculpepper Thanks for taking a look at this! Here are my thoughts:

A rackunit/meta module is a good idea, but why stop there? Should there be rackunit/base, rackunit/check, rackunit/suite, or other modules for splitting up the contents of rackunit? I'd find it strange to only have rackunit/meta provided from a special module while nothing else in RackUnit's non-UI code is modularized. What about if rackunit provides the meta checks and we open a separate issue for finer-grained modules?

To me, the (Treeof FailurePred) argument API is too complex. It presents multiple ways of doing the same thing, e.g. these are all nearly equivalent:

(test-begin
  (check-fail (list #rx"foo" pred) thnk))

(test-begin
  (check-fail #rx"foo" thnk)
  (check-fail pred thnk))

(test-begin
  (check-fail (list (list #rx"foo") (list (list pred))) thnk))

One of these could be preferred, but will they have subtle differences that a user has to remember? What about the extra location info provided by separate checks? And when using only a single predicate / regex / info, does it needs to be wrapped in a list to be considered a tree? Can you mix predicates / regexes / infos in the same tree? Is the order important? Is the tree traversed depth-first or breadth-first? Why is it a tree and not a list? If multiple parts of the tree fail, which message is shown? These are all questions I'd have to ask myself as a user of this API, and I'd likely forget the answers to some of them frequently and have to consult the docs. Not to mention there's the increased implementation and contract complexity. I don't see the benefits of this approach outweighing the costs.

[bennn]

I like @rmculpepper 's suggestions.

• :+1: for rackunit/meta, because it distinguishes "RackUnit for the meta-tester" from "RackUnit for the normal user".
  • it's fine with me to add rackunit/base etc. as long as (require rackunit) still gives the same imports.
• :+1: for having one form
• :+1: for accepting a tree. The plot library has a similar (very convenient) API for renderers. I think it should check everything in the tree & report all errors. Order should be unspecified --- if you want to meta-meta-test then only supply non-list arguments

[jackfirth]

Re rackunit/meta: I will move these exports to rackunit/meta and not export them from rackunit. This PR will not attempt to add a rackunit/base module or make any changes to the current exports of rackunit.

As for the tree API:

A tree API wouldn't quite be a single form, because it would still need separate forms for normal failures and errors. A third form that that supplied an empty tree by default would also be needed if we wanted to match the simplicity of check-fail*.

A tree of renderers in plot makes sense because there would be a lot of duplication otherwise, and there's no semantic changes that can occur as a result of the order of renderers in the tree. Changing the order of values in the tree supplied to a tree-based meta check form would change which error is shown, and I don't see any circumstance where the message argument to such a check would apply to all leaves of a tree.

A tree API also makes it harder to distinguish between different kinds of failures. Each leaf of the tree would have the same check location information. DrRacket's integration with the location information makes it easy to instantly see exactly which cases failed; I consider it bad practice for a rackunit API to encourage users to subvert that feature. Maybe if we had some way to point to different leaves of the tree in the location message?

[bennn]

I still like the tree API

• I don't mind losing check-fail*
• RackUnit can test everything in the tree (so changing order only changes the orders of errors, not "which errors are shown")
• to distinguish kinds of errors, you can split one check-fail* into multiple (check-exn has the same "problem" --- a user can write one big predicate that tests lots of things & is hard to debug)

[jackfirth]

I'd like to get this feature in and it seems I'm the only one doubting a tree API, so I shall defer to your judgment. Tree API it is!

With multiple errors, how should the checks report failures? Multiple failures for one check is something I've been working on in racket-expect and it can be complex enough that I'd rather avoid implementing it entirely here.

[bennn]

I think just normal messages separated by whitespace would be OK. Example:

--------------------
FAILURE
name:       check-equal?
location:   foo.rkt:4:0
actual:     1
expected:   0

name:       check-equal?
location:   foo.rkt:5:0
actual:     1
expected:   2
--------------------

But since name & location are always the same, maybe each failed check in the tree could register a unique check-info key & all these keys could be printed on failure.

[jackfirth]

Just printing like this would be fine with me:

--------------------
FAILURE
name:       check-fail?
location:   foo.rkt:4:0
actual:     1
expected:   0
actual:     1
expected:   2
--------------------

Anything more complicated I'd defer to a separate package designed with multiple failures in mind.

[bennn]

Can simplify 1 more step and only print actual 1 time?
So check-fail prints one actual & multiple expecteds

[jackfirth]

This is ready for another review.

Now, the only export of rackunit/meta is check-fail. It accepts a tree of predicates, regexps, and check info values. Here is a sample use and its output:

(define (fail-foo)
  (with-check-info (['foo 1])
    (fail-check "some message from foo")))

(define (has-bar-info? exn)
  (member 'bar (map check-info-name (exn:test:check-stack exn))))

(check-fail (list has-bar-info?
                  #rx"message from bar"
                  (make-check-info 'bar 1))
            fail-foo)

--------------------
FAILURE
name:            check-fail
location:        unsaved-editor:13:0
actual:          (exn:test:check "some message from foo" #<continuation-mark-set> ...)
actual-message:  "some message from foo"
actual-info:
  foo:             1
expected:
  predicate:       #<procedure:has-bar-info?>
  message:         #rx"message from bar"
  info:            (check-info 'bar 1)
--------------------

The check-fail/error form is no longer needed because #75 means check failures evaluated in thunks given to check-exn will raise their exceptions normally and not log failures (hooray!)

See the meta-test.rkt module for a more substantial example of how one would test custom rackunit checks.

[bennn]

About the error message; I like how the expected looks, but it's hard for me to line it up with the "actual".

Can you change the error message to something like this:

--------------------
FAILURE
name:            check-fail
location:        unsaved-editor:13:0
actual:
  exn:                 (exn:test:check "some message from foo" #<continuation-mark-set> ...)
  message:             "some message from foo"
  info:                ((check-info 'foo 1))
expected:
  exn-predicate:       #<procedure:has-bar-info?>
  message:             #rx"message from bar"
  info:                (check-info 'bar 1)
--------------------

And also sort the expected data, so all predicates, messages, and infos are grouped together?

[bennn]

check information stack => @tech{check-info stack}

[jackfirth]

Done

[bennn]

treeof needs a for-label require ... not sure which one

[jackfirth]

It's in plot somewhere, but I'm wary of adding a dependency on the plot package's docs to rackunit-doc. Maybe treeof should be moved out of the plot package?

[bennn]

I think the wording here is too low-level (sounds too much like an algorithm to me).

suggestion: change this line & the list to say

satisfies every assertion in assertion-tree. A failure can satisfy an assertion in one of three ways, depending on the type of the assertion:

• a failure f satisfies a predicate p if (p f) is a true value
• a failure satisfies a regular expression r if r matches the failure's message,
• a failure satisfies a check-info structure if the failure's check stack contains the expected value

... okay I don't like what I wrote very much either. Maybe it'd be better to have a deftech for the (treeof ....) contract and define "satisfies" in that?

Anyway, what's important to me is that the 3 cases for an "assertion" have 3 clear meanings. (I don't think the 4-point list & focus on "satisfying a tree" does that.)

[jackfirth]

I like what you wrote and definitely agree about having three list cases instead of four. I used slightly different wording though.

A deftech seems out of place here, especially since it would only be used in this one check.

[bennn]

@itemlist[ .... ]

(same for the examples below)

[jackfirth]

Done

[bennn]

replace "failing" with "calling fail-check"?

Because I think:

• "failing" is a technical term, it really means to throw an exn:test:check
• "failing" isn't described in the docs (anyway, someone writing their first define-check will have trouble with the current error message)

[jackfirth]

Done, very good idea.

[bennn]

Can this go in a common place?

[jackfirth]

I've written this in half a dozen different codebases so really I'd prefer to see it added to racket/list or some list-more package. But I'll put it in rackunit/private/util-list for now (not util, because rackunit/private/check duplicates this and it can't depend on util).

[bennn]

I guess any/c should be any

[jackfirth]

The predicates have to return one value so I think any/c is appropriate here.

[bennn]

boolean? => any

[jackfirth]

Changed to any/c (see earlier comment about any vs any/c)

[bennn]

Add some tests with silly lists?

like '(((#rx"hello") ()) #rx"world") ... just non-flat variations on the tests you already have

[jackfirth]

Done. I think one test that uses all three kinds of assertions in a single silly list ought to be enough; more tests than that seem redundant.

[jackfirth]

Sorting and grouping infos in the failure message is now implemented.

[bennn]

Cool, I like the new docs

[bennn]

I wish the actual and expected info fields looked more similar:

actual:
  exn:        (exn:test:check "some message from foo" #<continuation-mark-set> ...)
  message:    "some message from foo"
  info:
    foo:        1
expected:
  predicate:  #<procedure:has-bar-info?>
  message:    #rx"message from bar"
  info:       (check-info 'bar 1)

Because, what if foo isn't a check-info struct, but something else that prints the same way?

I'd like to do (nested-info (map pretty-info (exn:test:check-stack raised))), but that's a contract error.

[jackfirth]

Do you mean what if foo is a check-info struct who's value is not 1, but something that prints the same such as (string-info "1")? Calling exn:test:check-stack will always return a list of check-info values modulo unreasonable shenanigans.

[bennn]

I didn't know that about exn:test:check-stack.

Okay though, my real complaint is that foo: 1 and (check-info 'bar 1) look too different. I'd rather the first looked like (check-info 'foo 1).

[bennn]

I don't think the docs really need this note.

But maybe, add a reminder note not to abuse to tree API? (A check should test "1 thing".)

[bennn]

I think these thunks are more time/space expensive than just doing (define exn-info (make....

[jackfirth]

The optimizer should be able to inline them, so I don't think that's worth worrying about.