- Notifications
You must be signed in to change notification settings - Fork 37
Generate better default messages for isTrue().#117
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
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading. Please reload this page.
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.
RealyUniqueName commented Nov 8, 2022
I doubt this should be enabled by default. |
player-03 commented Nov 8, 2022
How big is a "big test suite"? I have a suite of 650 tests and can't measure any difference in compile time. |
player-03 commented Nov 8, 2022
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. |
player-03 commented Mar 16, 2023
This time I documented it!
I'm hoping the code sample also helps demonstrate why it's useful. |
player-03 commented Mar 16, 2023
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.
player-03 commented Dec 16, 2023
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? |
player-03 commented Jun 23, 2024
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 outnullas the message), it'll fall back to the old behavior.