check-compile-time-exn implementation for compile time exception testing #114

tommymchugh · 2020-01-04T07:42:02Z

Adds:
check-compile-time-exn
check-syntax-exn
check-not-compile-time-exn
check-not-syntax-exn

As some interest was expressed in providing handy wrappers for syntax/macro-testing's convert-compile-time-error and convert-syntax-error for thunk procedures in check-exn and check-not-exn.

Uses define-syntax instead of define-check for definition as wrappers are ignored when using define-check for expression.

Note: also adds || true to the Travis yaml file to allow the build to continue running the test suite even though there seems to be an existing dependency bug in rackunit-gui before this PR. Can remove if needed.

rackunit-doc/rackunit/scribblings/check.scrbl

sorawee · 2020-01-04T08:24:49Z

The documentation should specify the contract for exn-predicate (via #:contracts of defform), but there shouldn't be any contract for the second operand that is currently named thunk.

The documentation however should specify that the second operand is expanded in the expression context.

rackunit-doc/rackunit/scribblings/check.scrbl

sorawee · 2020-01-04T08:43:27Z

Note that I reviewed the PR directly from Github, so I haven't actually tested it yet.

One thing I concern is whether it reports a correct source location when a test fails. You might want to take a look at check-match which is another check that is defined using define-syntax and see how the source location is dealt with there.

tommymchugh · 2020-01-04T08:44:27Z

Sounds great, thanks so much for the detailed review! Will fix these things up and report back!

tommymchugh · 2020-01-04T12:10:51Z

New commit resolves documentation recommendations and adds source location to failed tests.

rmculpepper · 2020-01-04T18:06:00Z

I would expect a form named check-compile-time-exn to check that the exception is actually raised at compile time. You can do this with

(convert-compile-time-error (lambda () (#%expression EXPR)))

You can run the expression by applying the resulting thunk (if one is returned), but I'm not sure the expression needs to be run. If it is run, and if it raises a run time exception, should that be caught? (If so, it still should not be confused with a compile-time exception.)

sorawee · 2020-01-04T18:57:02Z

rackunit-doc/rackunit/scribblings/check.scrbl

@@ -176,6 +176,27 @@ entirely.
 ]
 }

+@defform[(check-compile-time-exn exn-predicate body)
+         #:contracts ([exn-predicate (or/c (-> any/c any/c) regexp?)]
+                      [body (-> any/c)])


Sorry if I was being unclear. I think:

1.body should be renamed to expr; and
2. It should not be in #:contracts.

For the first one, there's a convention to use the name body as described here. Because this operand only allows an expression, it should be named expr instead.

For the second one, I would hope that it is possible to write (check-compile-time-exn #rx"foo" 1), but 1 would not satisfy (-> any/c) as it's not a function. Switching from (-> any/c) to any/c wouldn't work either because (I would hope that) it's possible to write (check-compile-time-exn #rx"foo" (values 1 2 3)), but again, (values 1 2 3) would not satisfy any/c. So just leave it out from #:contracts.

tommymchugh · 2020-01-05T02:09:23Z

@rmculpepper @sorawee Thanks for the feedback! Will play around with this and get back with some fixes!

jackfirth · 2020-01-06T04:40:10Z

API feedback:

The check-syntax-exn form doesn't seem necessary. What's the intended benefit over check-compile-time-exn?
Often I want to check for compile time exceptions in code that spans multiple expressions. With the current API, I'd have to write this:

(check-compile-time-exn exn:fail:syntax?
  (begin
    (thing)
    (other-thing)
    (more-things)))

...but I'd like to omit the begin wrapper. Could we make check-compile-time-exn accept a sequence of expressions rather than a single expression?

Instead of having the check accept a predicate, could we make the check return the exception? That way you can assert multiple things about the exception without shoving the assertions into a predicate:

(test-case "hypothetical static type checker"
  (define e
    (check-compile-time-exn
      (: foo String)
      (define foo 42)))
  (check-pred exn:fail:syntax:type? e)
  (check-equal? (exn:fail:syntax:type-expected-type e) String))

Could we toss in a rackunit check for asserting that a syntax error points to specific expressions? I'm imagining something like this:

(define e
  (check-compile-time-exn
    (define good 42)
    (define bad)
    (define also-good 42)))
(check-pred exn:fail:syntax? e)
(check-syntax-error-source e #'(define bad))

Also, thank you so much for working on this! I think it will make a huge difference in the quality of everyone's macros.

tommymchugh · 2020-01-06T05:20:01Z

Thanks for the Great feedback for API design!! I'm working on some of the other feedback and will definitely add these to it, as I totally agree with you on these different points. Should have an update with a more detailed response to these points and the others in the PR ASAP!

sorawee · 2020-01-06T06:07:32Z

@jackfirth: what context should these forms be expanded in? One of your examples ends with a define statement, which is illegal in both internal definition context and expression context. Perhaps you want the behavior of block? Which, for the purpose of compile-time expansion, is equivalent to (lambda () body ... (void))?

jackfirth · 2020-01-06T06:10:09Z

@sorawee Expanding the forms inside a block sounds like a good idea. There definitely shouldn't be a requirement that the last form is an expression.

tommymchugh · 2020-01-06T06:11:26Z

Yeah that’s how I interpreted as there could be multiple statements that each are checked for compile time errors, last statement checked need not be an expression like block

On Mon, Jan 6, 2020 at 12:07 AM sorawee ***@***.***> wrote: @jackfirth <https://github.com/jackfirth>: what context should these forms be expanded in? One of your examples ends with a define statement, which is illegal in both internal definition context and expression context. Perhaps you want the behavior of block <https://docs.racket-lang.org/reference/block.html>? Which, for the purpose of compile-time expansion, is equivalent to (lambda () body ... (void))? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#114?email_source=notifications&email_token=AAMRJXMR3HSIXGURFR6HUELQ4LDCNA5CNFSM4KCU2ZL2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIEQOLQ#issuecomment-571017006>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMRJXNHWTPF27RMZK4IWOTQ4LDCNANCNFSM4KCU2ZLQ> .

-- *Thomas McHugh* *Northwestern University* Undergraduate Student Email: [email protected] Phone: 847-922-4801

sorawee · 2020-01-06T07:52:05Z

In that case, I would document check-compile-time-exn as:

@defform[(check-compile-time-exn exn-predicate-expr defn-or-expr ...)

with a contract for exn-predicate-expr.

sorawee · 2020-01-06T07:53:08Z

Do we want to require that there is at least one defn-or-expr? ~~My thought is that it should not require that since you might want to test a base case of macro expansion which expands to empty begin.~~

jackfirth · 2020-01-06T07:57:46Z

In that case, I would document check-compile-time-exn as:
@defform[(check-compile-time-exn exn-predicate-expr defn-or-expr ...)
with a contract for exn-predicate-expr.

Just to repeat what I said earlier, I think it shouldn't accept a predicate. Agreed on naming the body forms defn-or-expr though.

Do we want to require that there is at least one defn-or-expr?

Definitely. Only use case I can think of for allowing zero would be for a macro that expands into a use of check-compile-time-exn. In that case, the macro can just wrap the forms in a begin instead.

tommymchugh · 2020-01-06T07:58:10Z

@sorawee yeah I think that makes sense, I can't think of a point to having no defn-or-expr so requiring makes sense

tommymchugh · 2020-01-06T07:59:48Z

I agree with @jackfirth with having it without a predicate, returning the exception makes sense. Do we think it makes sense to return #f if no exception is generated by the syntax?

jackfirth · 2020-01-06T08:00:49Z

@tommymchugh If no exception is raised, the check should fail entirely. Failed checks raise exceptions, so it shouldn't need to return anything at all.

tommymchugh · 2020-01-06T08:02:27Z

@jackfirth Oh yeah thats right, sounds good to me

tommymchugh · 2020-01-06T08:06:59Z

@sorawee's updated docs would be
@defform[(check-compile-time-exn defn-or-expr ...) exn:fail?]

Accounting for removal of predicate and return type of exception unless failure.
Going back to @rmculpepper's comment, it makes sense to me for check to fail if there is a runtime exception to separate this check from check-exn

I also think scrapping check-syntax-exn makes sense, no real difference in this scenario

jackfirth · 2020-01-06T08:17:23Z

I don't think the expression should be evaluated at all, so even if it would throw a runtime error that shouldn't matter. For example this:

(define e
  (check-compile-time-exn
    (error "runtime kaboom!")
    (define compile-time-kaboom!)))

...should succeed and e should be set to the syntax error that (define compile-time-kaboom!) caused. If no compile-time error is raised at all, then the check should fail without evaluating the body. So this test case:

(tets-case "compile-time exception test"
  (define was-run? (box #f))
  (check-compile-time-exn (set-box! was-run? #t))

...should fail (because (set-box! was-run? #t) compiles without error), but was-run? should still be false (because (set-box! was-run? #t), though compiled, was never actually executed).

tommymchugh · 2020-01-06T08:44:04Z

So just to summarize mainly for my notes while I'm implementing here's overview of what I got.

API Design Spec

Add: check-compile-time-exn
@defform[(check-compile-time-exn defn-or-expr ...+)]

Checks for compile time exceptions in a block-inspired series of definitions or expressions. Does not run or check for runtime errors, but fails if there is no compile time exception. Returns the first identified compile time exception, and does not continue checking the rest of the definitions or expressions.

Add: check-not-compile-time-exn
@defform[(check-not-compile-time-exn defn-or-expr ...+)]

Checks that there are no compile time exceptions. Fails if there are any found. Does not run so does not check for runtime exceptions.

Add: check-syntax-error-source

@defproc[(check-syntax-error-source [exn-predicate exn:fail:syntax?]
                                    [stx-expr syntax?]) void?]

Confirms that stx-expr is the defn-or-expr causing the resulting exception from check-compile-time-exn

jackfirth · 2020-01-06T10:23:33Z

Design spec is a great idea! Here's my nitpicks:

Use ...+ in check-compile-time-exn and check-not-compile-time-exn instead of ..., since they require at least one form.
Remove the exn:fail? from check-compile-time-exn, since defform doesn't actually allow contracts on the result like that (because macros might not return results). Also, there's other kinds of values that can get raised at compile time anyway. For example this test case should pass:

(test-case "raising a non-exception value at compile-time"
  (define v
    (check-compile-time-exn
      (define-syntax foo (raise "just a plain string!"))))
  (check-pred string? v)
  (check-equal? v "just a plain string!"))

For check-syntax-error-source, it should accept any exn:fail:syntax exception. You shouldn't have to use it with only check-compile-time-exn. For example, this test is perfectly cromulent:

(test-case "manual syntax error test"
  (define e
    (make-exn:fail:syntax
      "I made this error all by myself!"
      (current-continuation-marks)
      (list #'(uh-oh-spaghettios))))
  (check-syntax-error-source e #'(uh-oh-spaghettios)))

The stx-expr given to check-syntax-error-source shouldn't have to be completely equal? to any of the syntax objects inside the exn:fail:syntax-exprs field. That's too restrictive, because syntax objects aren't equal unless their scopes, properties, and a whole bunch of other hidden metadata are all equal. This check shouldn't really care about any of that, instead it should extract only the datums from both the expected stx-expr and each of the syntax objects in the exn:fail:syntax-exprs field, using syntax->datum. The check should pass as long as the datums are equal.

tommymchugh · 2020-01-07T22:55:09Z

Just pushing this to update everyone, have compile time only checking working. Next steps for me are:

Return exception when found
Implement check-syntax-error-source
Update docs and tests

tommymchugh · 2020-01-07T23:07:38Z

Example:

(check-compile-time-exn (define a) (+ 1 2) (lambda ) (- 2 3)) ;; Will Pass
(check-compile-time-exn (error "I'll never be called") (+ 1 2) (- 2 3)) ;; Will Fail

jackfirth · 2020-01-09T08:05:31Z

rackunit-lib/rackunit/private/test.rkt

@@ -173,5 +177,11 @@
 (define-shortcut (test-exn pred thunk)
  (check-exn pred thunk))

+(define-shortcut (test-compile-time-exn pred thunk)


I think these shortcut forms should be skipped, actually. They won't work quite right for check-compile-time-exn, since it accepts a variable number of body forms instead of a single thunk. The shortcuts really only work for checks that behave much more like functions: notice there isn't a test-match shortcut either, since check-match is similarly unusual.

jackfirth · 2020-01-09T08:09:59Z

rackunit-lib/rackunit/private/check.rkt

+(define-syntax (check-compile-time-exn stx)
+  (syntax-parse stx
+       [(_ body ...+)
+        (with-syntax ([loc (datum->syntax #f 'loc stx)])


You can use the #:with pattern directive to simplify this a bit:

(syntax-parse stx [(_ body ...+) #:with loc (datum->syntax #f 'loc stx) (syntax/loc stx ...)])

jackfirth · 2020-01-09T08:12:12Z

rackunit-lib/rackunit/private/check.rkt

+
+(define-syntax (check-compile-time-exn stx)
+  (syntax-parse stx
+       [(_ body ...+)


You can use the expr syntax class to prevent body from matching bare keywords:

(syntax-parse stx [(_ body:expr ...+) ...])

That way, a usage like (check-compile-time-exn #:foo) won't be allowed.

jackfirth · 2020-01-09T08:14:34Z

rackunit-lib/rackunit/private/check.rkt

@@ -139,7 +142,7 @@
               (procedure-arity-includes? thunk 0))
    (raise-arguments-error name "thunk must be a procedure that accepts 0 arguments" "thunk" thunk)))

-(define-check (check-exn raw-pred thunk)
+(define-check (check-exn-helper raw-pred thunk location)


I think it would be better to not try and reuse the check-exn code. Checks are kind of hard to reuse, since they're sensitive to the source location of their usage site and they have some slightly whacky semantics. Plus there's no need to try and reuse the regex/predicate parts of check-exn.

jackfirth · 2020-01-09T08:23:24Z

rackunit-test/tests/rackunit/check-test.rkt

@@ -434,6 +435,20 @@
    (check-exn exn:fail:contract?
               (lambda ()
                 (check-not-exn (lambda (x) x)))))
+
+ ;; Verify compile time exceptions are now 


Here's some more test cases:

(test-case "check-compile-time-exn should allow multiple body forms" (check-compile-time-exn (define foo 1) (define bar 2) (define kaboom))) (test-case "check-compile-time-exn should allow raising non-exception values" (check-compile-time-exn (define-syntax foo (raise "not an exception")))) (test-case "check-compile-time-exn should not evaluate its body" (define evaluated? (box #f)) (check-compile-time-exn (set-box! evaluated? #t) (define kaboom)) (check-false (unbox evaluated?))) (test-case "check-compile-time-exn should return the raised value" (define raised-value (check-compile-time-exn (define-syntax foo (raise 42))) (check-equal? raised-value 42))

sorawee · 2020-01-09T08:52:12Z

Another very minor nit: note that indentations are off in some of your code. You might want to use DrRacket or racket-mode in Emacs to auto-indent the code for you.

tommymchugh · 2020-01-11T05:39:44Z

Thanks for the comments! Will have some solid time this weekend to work on this so will keep you all updated!

sorawee · 2020-11-28T03:01:31Z

We just switched from Travis to GHA, so you should discard any changes to .travis.yml to avoid the conflict.

sorawee · 2021-01-02T16:46:39Z

@jackfirth reading this again, I disagree that check-compile-time-exn should return an exception. The name check- suggests that it should be a check -- an assertion that the test should pass. If you want it to return an exception, then perhaps the name with-compile-time-exn is more appropriate?

That is to say, rackunit would provide check-compile-time-exn (which accepts a predicate), and perhaps there should be rackunit/util that provides with-compile-time-exn.

@tommymchugh do you still want to continue working on the PR? -- I notice that you deleted your forked repo. This is a great feature addition, so if you don't want to continue, would you mind if someone else takes it over? And in any case, thank you for your work so far!

tommymchugh · 2021-01-02T17:07:59Z

Happy to take this on. I have a new year's resolution to get back to some of the open source work I had that piled up during the pandemic :). I'll reassociate myself with this PR and write up the changes.

…

On Sat, Jan 2, 2021 at 10:46 AM sorawee ***@***.***> wrote: @jackfirth <https://github.com/jackfirth> reading this again, I disagree that check-compile-time-exn should return an exception. The name check- suggests that it should be a check form -- an assertion that the test should pass. If you want it to return an exception, then perhaps the name with-compile-time-exn is more appropriate? That is to say, rackunit would provide check-compile-time-exn (which accepts a predicate), and perhaps there should be rackunit/util that provides with-compile-time-exn. @tommymchugh <https://github.com/tommymchugh> do you still want to continue working on the PR? -- I notice that you deleted your forked repo. This is a great feature addition, so if you don't want to continue, would you mind if someone else takes it over? And in any case, thank you for your work so far! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#114 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMRJXKNB3TNOWGWRMFUXQDSX5EXXANCNFSM4KCU2ZLQ> .

tommymchugh · 2021-01-04T00:00:02Z

@sorawee Moving to #131 because this new implementation has significant changes.

added compile time and syntax error checking

32fa162

tommymchugh mentioned this pull request Jan 4, 2020

RackUnit check-exn fails to catch raised syntax error when error not called directly racket/racket#2996

Closed

sorawee requested changes Jan 4, 2020

View reviewed changes

rackunit-doc/rackunit/scribblings/check.scrbl Outdated Show resolved Hide resolved

rackunit-doc/rackunit/scribblings/check.scrbl Outdated Show resolved Hide resolved

sorawee reviewed Jan 4, 2020

View reviewed changes

rackunit-doc/rackunit/scribblings/check.scrbl Outdated Show resolved Hide resolved

sorawee reviewed Jan 4, 2020

View reviewed changes

rackunit-doc/rackunit/scribblings/check.scrbl Outdated Show resolved Hide resolved

added source locaton and doc updates

59a0ca7

replaced any with any/c

338cb67

sorawee reviewed Jan 4, 2020

View reviewed changes

Removed syntax-exn

7f2478b

jackfirth added the enhancement label Jan 6, 2020

tommymchugh changed the title ~~Added compile time and syntax error unit test check procedures~~ check-compile-time-exn implementation for compile time exception testing Jan 6, 2020

Updated check-(not-)compile-time-exn to spec

d9e7178

jackfirth reviewed Jan 9, 2020

View reviewed changes

tommymchugh mentioned this pull request Jan 3, 2021

Compile time check implementation #131

Open

tommymchugh closed this Jan 4, 2021

check-compile-time-exn implementation for compile time exception testing #114

check-compile-time-exn implementation for compile time exception testing #114

Conversation

tommymchugh commented Jan 4, 2020 • edited Loading

sorawee commented Jan 4, 2020 • edited Loading

sorawee commented Jan 4, 2020

tommymchugh commented Jan 4, 2020

tommymchugh commented Jan 4, 2020

rmculpepper commented Jan 4, 2020

sorawee Jan 4, 2020

Choose a reason for hiding this comment

tommymchugh commented Jan 5, 2020

jackfirth commented Jan 6, 2020 • edited Loading

tommymchugh commented Jan 6, 2020

sorawee commented Jan 6, 2020

jackfirth commented Jan 6, 2020

tommymchugh commented Jan 6, 2020 via email

sorawee commented Jan 6, 2020

sorawee commented Jan 6, 2020 • edited Loading

jackfirth commented Jan 6, 2020

tommymchugh commented Jan 6, 2020

tommymchugh commented Jan 6, 2020 • edited Loading

jackfirth commented Jan 6, 2020

tommymchugh commented Jan 6, 2020

tommymchugh commented Jan 6, 2020 • edited Loading

jackfirth commented Jan 6, 2020 • edited Loading

tommymchugh commented Jan 6, 2020 • edited Loading

jackfirth commented Jan 6, 2020

tommymchugh commented Jan 7, 2020 • edited Loading

tommymchugh commented Jan 7, 2020 • edited Loading

jackfirth Jan 9, 2020

Choose a reason for hiding this comment

jackfirth Jan 9, 2020

Choose a reason for hiding this comment

jackfirth Jan 9, 2020

Choose a reason for hiding this comment

jackfirth Jan 9, 2020

Choose a reason for hiding this comment

jackfirth Jan 9, 2020

Choose a reason for hiding this comment

sorawee commented Jan 9, 2020

tommymchugh commented Jan 11, 2020

sorawee commented Nov 28, 2020

sorawee commented Jan 2, 2021 • edited Loading

tommymchugh commented Jan 2, 2021 via email

tommymchugh commented Jan 4, 2021

tommymchugh commented Jan 4, 2020 •

edited

Loading

sorawee commented Jan 4, 2020 •

edited

Loading

jackfirth commented Jan 6, 2020 •

edited

Loading

sorawee commented Jan 6, 2020 •

edited

Loading

tommymchugh commented Jan 6, 2020 •

edited

Loading

tommymchugh commented Jan 6, 2020 •

edited

Loading

jackfirth commented Jan 6, 2020 •

edited

Loading

tommymchugh commented Jan 6, 2020 •

edited

Loading

tommymchugh commented Jan 7, 2020 •

edited

Loading

tommymchugh commented Jan 7, 2020 •

edited

Loading

sorawee commented Jan 2, 2021 •

edited

Loading