Skip to content
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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

player-03
Copy link
Contributor

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 out null as the message), it'll fall back to the old behavior.

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
Copy link
Member

I doubt this should be enabled by default.
Rebuilding expressions for every test method could affect compilation times for big test suites.
Also having it for isTrue, but skipping isFalse looks inconsistent. But isFalse would require a different message.

@player-03
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

This time I documented it!

utest can generate detailed failure messages for Assert.isTrue and Assert.> isFalse, but only if cond is either a comparison operator or function call. It also doesn't apply if a custom msg is already provided.

  function testSum() {
    var x = 3;
    Assert.isTrue(2 + 2 == x + x); // Failed: 2 + 2 == x + x. Values: 4 == 6
    Assert.isTrue(2 + 3 >= x + x); // Failed: 2 + 3 >= x + x. Values: 5 >= 6
    Assert.isTrue(2 + 3 >= x + x, "custom"); // custom

    var array = [1, 2, 4, 8];
    Assert.isTrue(array.contains(x)); // Failed: array.contains(x). Values: [1,2,4,8].contains(3)
  }

This behavior can be disabled using -D UTEST_FAILURE_REDUCE_DETAIL, or by adding UTEST_FAILURE_REDUCE_DETAIL to the environment variables at compile time. In that case, utest falls back to a generic failure message.

  function testSum() {
    var x = 3;
    Assert.isTrue(2 + 2 == x + x); // expected true
    Assert.isTrue(2 + 3 >= x + x); // expected true

    var array = [1, 2, 4, 8];
    Assert.isTrue(array.contains(x)); // expected true
  }

I'm hoping the code sample also helps demonstrate why it's useful.

@player-03
Copy link
Contributor Author

But isFalse would require a different message.

Added!

Rebuilding expressions for every test method could affect compilation times for big test suites.

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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:

Normally, when Assert.isTrue and Assert.isFalse fail, they only print "expected false" or "expected true", making it hard to diagnose the issue. To make diagnosis easier, utest offers the -D UTEST_DETAILED_MESSAGES option. Setting this makes the default messages more informative, while leaving custom messages untouched.

  function testSum() {
    var x = 3;
    Assert.isTrue(2 + 2 == x + x); // Failed: 2 + 2 == x + x. Values: 4 == 6
    Assert.isTrue(2 + 3 >= x + x); // Failed: 2 + 3 >= x + x. Values: 5 >= 6
    Assert.isFalse(2 + 4 <= x + x); // Failed: 2 + 4 <= x + x should be false. Values: 6 <= 6
    Assert.isTrue(2 + 3 >= x + x, "my custom message"); // my custom message

    var array = [1, 2, 4, 8];
    Assert.isTrue(array.contains(x)); // Failed: array.contains(x). Values: [1,2,4,8].contains(3)
    Assert.isFalse(array.contains(2), "didn't expect 2"); // didn't expect 2

    // Currently, only binary operators and function calls are supported.
    // Other expressions such as array access will fall back to the default.
    var bools = [true, false];
    Assert.isFalse(bools[0]); // expected false
    Assert.isTrue(bools[1]); // expected true
  }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants