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

assertTrue()/assertFalse() should include an error message by default #1030

Closed
kluever opened this issue Nov 10, 2014 · 17 comments
Closed

assertTrue()/assertFalse() should include an error message by default #1030

kluever opened this issue Nov 10, 2014 · 17 comments

Comments

@kluever
Copy link
Contributor

kluever commented Nov 10, 2014

assertTrue(falseMethod()) gives:
junit.framework.AssertionFailedError
assertEquals(true, falseMethod()) gives:
junit.framework.AssertionFailedError: expected: but was:

Can we add default error messages for assertTrue(boolean)/assertFalse(boolean)?

The easiest thing to do is probably just to call assertEquals(true, condition), etc.

@marcphilipp
Copy link
Member

Would such default error messages be helpful? You'd have to look at the test code to figure out what went wrong anyway, wouldn't you?

BTW Please use org.junit.Assert instead of junit.framework.Assert.

@kluever
Copy link
Contributor Author

kluever commented Nov 11, 2014

Let's say I write this test:

User user = ...;
assertTrue(user.isHappy());
assertFalse(user.isSad());

The test fails with an AssertionError with no message. There's no way to tell without inspecting the code and inspecting line numbers which assertion failed.

Why is more data a bad thing?

@marcphilipp
Copy link
Member

More data is not a bad thing. In this case, I can only see a tiny bit of more data, though.

While I'm not against this feature request, I think there a better ways to formulate assertions nowadays, e.g. Hamcrest or Truth.

Nevertheless, I think if someone would submit a pull request for this issue, we would probably merge it... ;-)

@junit-team/junit-committers Any objections?

@kluever
Copy link
Contributor Author

kluever commented Nov 11, 2014

I certainly don't disagree with that :-)
https://github.com/google/truth/commits?author=kluever

But we've had some folks at Google who were reluctant to use assertTrue/assertFalse because of this. But sure, I'll send a pull request for this.

@marcphilipp
Copy link
Member

😄

@stefanbirkner
Copy link
Contributor

You can provide a message to assertTrue and assertFalse.

assertTrue("The user is not happy, but she should.", user.isHappy());
assertFalse("The user is sad, but she should not.", user.isSad());

This message is shown in the stack trace.

java.lang.AssertionError: The user is not happy, but she should.
java.lang.AssertionError: The user is sad, but she should not.

@kluever
Copy link
Contributor Author

kluever commented Nov 11, 2014

@stefanbirkner Yes, I'm well aware of that, but why not show a reasonable error message for the default case too?

We already do this for assertEquals(expected, actual)...this is so you don't have to write:
assertEquals("Expected " + expected +" but was " + actual, expected, actual);

@kcooney
Copy link
Member

kcooney commented Nov 11, 2014

While I agree having a better message for a failure from assertTrue() could be useful, I am not sure if "expected <true> but was <false>" is better

I believe @dsaff had some historical context.

@dsaff
Copy link
Member

dsaff commented Nov 18, 2014

Just to be clear about the scope of the problem: "Expected but was " is 'much better' because of a stylistic preference for some message rather than none, not because it conveys actual additional information, correct?

The original design thought was that making up an error message is a cognitive distraction from the matter at hand. If I say assertTrue(list.isEmpty()), I'm thinking of the state of list, not of a comparison test between booleans. Thus, JUnit has nothing useful to say, and should say nothing.

That's the reason behind Chesterton's fence.

There's still no really interesting data given in this case mentioned above:

assertTrue(user.isHappy());
assertFalse(user.isSad());

The reason that assertEquals has a more informative error message is because it has something more informative to say, namely what the actual value and expected values are, which are often not known until runtime.

JUnit is a heavily used library, and even minor changes like this, when made to core methods, tend to break someone's test somewhere, where someone is depending on the current behavior of JUnit. I'm fine with doing that if the new behavior is different in a useful way, but I'm not convinced of the usefulness by the argument so far.

@kevinb9n
Copy link

A consequence of the current state is that a number of reasonable and intelligent developers have a personal practice of always writing assertEquals(true, expression()) instead of assertTrue(expression()). Plus, of course, asking reviewees to do the same, putting it in their style guides, etc. This makes us sad, because we like code simplicity, but I don't think we can claim that that number is very high.

I also think the answer to David's opening question (last comment) is a clear "yes". The only case in which any actual information is provided is when a test method contains exactly one messageless assertTrue and exactly one messageless assertFalse. That's not very common, and there's a stack trace, and they could have supplied a message... it doesn't add up to much. I think it has to do with the messageless exception just appearing too bare.

@marcphilipp
Copy link
Member

@junit-team/junit-committers What are we going to do about this issue?

@dsaff
Copy link
Member

dsaff commented Dec 9, 2014

Well, I'm in favor of closing it. Kevin, Kurt, are either of you enthusiastic enough to be counsel for the defense?

@kluever
Copy link
Contributor Author

kluever commented Dec 9, 2014

I'm in favor of closing it.

@mlgiter
Copy link

mlgiter commented Apr 3, 2018

I do vote up this issue. Other frameworks support showing actual value capability and it's essential. I just couldn't believe JUnit is that inflexible.

@ChaminW
Copy link

ChaminW commented May 28, 2019

I do vote up this issue since it's important to have a more informative error message for assertTrue and assertFalse. My colleagues and I are reluctant to use assertTrue/assertFalse because of this.

@stefanbirkner
Copy link
Contributor

@ChaminW You can use assertTrue("Some more specific message.", value); for more informative error message.

@BMomani
Copy link

BMomani commented Nov 24, 2022

I know its closed but let's vote in emoji
leave 👍 if you are in favor of the change (doing the requested ch)
leave 👎 if you are in favor of the decision

@kcooney kcooney closed this as not planned Won't fix, can't repro, duplicate, stale Feb 28, 2023
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

No branches or pull requests

9 participants