-
-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move test-log logic for checks and tests inside their "around" parameters #75
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good
(raise-type-error 'current-check-around "procedure" v))))) | ||
(define handler (current-check-handler)) | ||
(with-handlers ([(λ (_) #t) handler]) (thunk)) | ||
(void)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does Typed Racket not depend on the return type of check-around
?
(Or are you going to change TR before merging this?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checks created with define-check
always returned void, so this shouldn't change the types. That's still the case according to the contract so these void expressions can be removed. I'll do that before merging.
(parameterize | ||
([current-check-handler raise] | ||
[current-check-around check-around]) | ||
(parameterize ([current-check-around plain-check-around]) | ||
(void) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing it causes some tests to fail because (test-begin)
was never a syntax error, but (parameterize ([param v]))
is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, maybe change to (void expr ....)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out that doesn't work because the expr
expressions actually aren't expressions, they could be definitions. I've added a comment and renamed the pattern variable to hopefully make this less confusing.
Test logging is handled exclusively in current-check-around now
81398ee
to
609551e
Compare
@bennn I'll merge this later today unless you've got any more comments. Thanks for the review! |
Closes #70
Closes #9
Closes #14
Closes #58
This PR moves the
test-log!
logic of checks and test cases into thecurrent-check-around
andcurrent-test-case-around
parameters, respectively. Additionally, checks now parameterizecurrent-check-around
inside the body ofdefine-check
so that nested checks are evaluated as normal functions. This allows checks to be imperatively composed; they can be chained together in series just like normal exception-throwing validation functions. This involves several subtle changes to the effects and evaluation of checks:Single nested check
This now reports one test passing, and uses the default check info added by
check-zero
. Previously, it would report two tests passing.Multiple nested checks
The passing use reports one test passing, and the failing use reports one test failing with the check failure thrown by the first
check-equal?
expression. Previously, the passing use reported two tests passing and the failing use reported two tests failing and one test passing, as each of the nested checks would raise and swallow a check failure leading the outer check to believe nothing went wrong.Multiple nested checks in test case
The passing test now reports one test passing, and the failing test reports one test failing. The passing test previously reported three tests passing, and the failing test previously reported one test passing and one test failing. It also only evaluated the first check inside
check-zero-twice
.Multiple checks in a test case
The passing test now reports one test passing, and the failing test now reports one test failing. Previously the passing test reported two tests passing and the failing test reported one test passing and two tests failing.
Check in a test suite
For all test suites, calling
run-tests
on a suite now causestest-log!
to report the same number of passing and failing tests asrun-tests
displays in its summary message. Previously,test-log!
would report significantly more tests thanrun-tests
claimed.Other changes
test-case
expressions instead oftest-suite
definitions provided and combined inall-rackunit-tests.rkt
rackunit/log
tests now have slightly better failure messagestest-log!
now need to be explicitly made by the user offold-test-results
orfoldts-test-results
. This also allows rackunit test suites to be run without interacting withrackunit/log
at all (which the GUI runner probably wants to do anyway)Future work
This doesn't add any documentation of how rackunit checks behave when composed. It now mostly works "as expected", but without docs and tests guaranteeing this behavior it's not safe for users to rely on.
There are corresponding changes that need to be made in Typed Racket's rackunit wrapper code that duplicates rackunit's test case and test suite macros.