-
Notifications
You must be signed in to change notification settings - Fork 36
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
Generate better default messages for isTrue()
.
#117
base: master
Are you sure you want to change the base?
Conversation
Why print "expected true" when we could print the value(s) that caused the problem? The code already exists; we just have to use it.
I doubt this should be enabled by default. |
How big is a "big test suite"? I have a suite of 650 tests and can't measure any difference in compile time. |
If you're sure it needs to be optional, I'd suggest making it opt-out rather than opt-in. Premature optimization and all that: if we can't demonstrate a performance issue, shouldn't functionality win? The problem with opt-in is that people might not realize they need to opt in until it's already too late. I sometimes use randomized tests, on the basis that over time they'll test a wide range of situations. But this tripped me up a couple times, because I'd get the "expected true" message with no indication of what the random values had been. Luckily, I'm currently testing simple and straightforward mathematical constructs, and it has never taken too long to reproduce the errors. However, I'd argue that as project size increases, it becomes more likely to have one-in-a-million edge cases that you wouldn't be able to reproduce just by trying again. |
This time I documented it!
I'm hoping the code sample also helps demonstrate why it's useful. |
Added!
I did some profiling this time around. In my test suite of 870 assertions, this code adds 0.05s-0.06s to the build time, about 1.5% of the total. And there's no reason that percentage would change significantly, so if some enormous test suite took 100 seconds to compile, this macro would bring it to ~101.5 seconds. |
Instead of defining `UTEST_FAILURE_REDUCE_DETAIL` to disable the behavior, you set `UTEST_DETAILED_MESSAGES` to enable it.
I still don't feel like a 1.5% increase is a big deal, but I've gone ahead and made it opt-in. Hopefully now this can be merged? |
I fixed the merge conflict. Can we get this merged? In case it helps, here's the new documentation describing the new opt-in approach:
|
Why print "expected true" when we could print the value(s) that caused the problem? The code already exists; we just have to use it. With this change,
Assert.isTrue(1 + 1 > 4)
will print "Failed: 1 + 1 > 4. Values: 2 > 4".For simplicity, this only applies if you pass exactly one argument. If you pass a custom message or
PosInfos
(or even type outnull
as the message), it'll fall back to the old behavior.