junit5 icon indicating copy to clipboard operation
junit5 copied to clipboard

Document benefits of `messageSupplier` in `Assertions`

Open manueljordan opened this issue 2 years ago • 14 comments

About JUnit 5 documentation in the 2.5. Assertions section, exists the following @Test method

    @Test
    void standardAssertions() {
        assertEquals(2, calculator.add(1, 1));
        assertEquals(4, calculator.multiply(2, 2),
                "The optional failure message is now the last parameter");
        assertTrue('a' < 'b', () -> "Assertion messages can be lazily evaluated -- "
                + "to avoid constructing complex messages unnecessarily.");
    }

Is not clear what is the advantage to use () -> "Assertion messages can be lazily evaluated -- ..." and not simply "Assertion messages can be lazily evaluated -- ..." It means: () -> "" vs ""

Therefore:

        assertTrue('a' < 'b', () -> "Assertion messages can be lazily evaluated -- "
                + "to avoid constructing complex messages unnecessarily.");
        assertTrue('a' < 'b', "Assertion messages can be lazily evaluated -- "
                + "to avoid constructing complex messages unnecessarily.");

What does Assertion messages can be lazily evaluated mean? The explanation would be valuable for a better understanding and to know explicitly when use mandatorily an approach over the other. Currently in the complete/full documentation only appears through a sample code the () -> "something" approach just twice, but not more. Some example(s) is/are appreciate showing the two approaches together and to understand clearly the difference of use of one over the other. Therefore I want to know when would be mandatory use the lambda approach. So far, the current official documentation does not cover this explicitly

I already wrote a question on SO about this question at:

The answer has sense, but if it can be covered and expanded from the official source documentation would be nice for the community

Thanks for your understanding

manueljordan avatar Feb 10 '23 20:02 manueljordan

Team decision: Change the documentation to contain a more meaningful example.

marcphilipp avatar Feb 17 '23 11:02 marcphilipp

@manueljordan Would you like to take a stab at this?

marcphilipp avatar Feb 17 '23 11:02 marcphilipp

@marcphilipp If I am able to do it, yes. But what could be the most adequate scenario to add it as an example? Who was the author of the addition of this "lazy approach"? He/She created that feature to solve/improve a situation. Having that info in hands would be a good starting point.

At https://github.com/junit-team/junit5-samples, a sample would be nice to be added, but under what directory?

BTW Does the current documentation include references to https://github.com/junit-team/junit5-samples to go deep, through those examples, to offer more details about any topic being covered? It for the developer, if he/she wants get more details.

Thanks for your understanding

manueljordan avatar Feb 17 '23 18:02 manueljordan

But what could be the most adequate scenario to add it as an example? Who was the author of the addition of this "lazy approach"? He/She created that feature to solve/improve a situation. Having that info in hands would be a good starting point.

It's the same rationale as in java.util.logging methods accepting a supplier.: creating a String can cause unnecessary overhead if it is never consumed. In logging that happens when the log level is not enabled. In assertions that happens when the assertion succeeds. Coming up with a descriptive example would be the job of whoever decides to tackle this issue. 🙂

At junit-team/junit5-samples, a sample would be nice to be added, but under what directory?

I don't think this minor feature justifies a sample in junit5-samples. Improving the snippet in the User Guide should be enough.

marcphilipp avatar Feb 18 '23 06:02 marcphilipp

I think the best approach is add something like:

assertTrue( condition, showErrorMessage());       //1
assertTrue( condition, () -> showErrorMessage()); //2

Where:

String showErrorMessage() {
    System.out.println("condition failed"):
    return "custom error message";
}

And indicate that:

  • For "1" does not matter if condition evaluates to true or false, so the showErrorMessage() method always is executed
  • For "2" only if condition evaluates to false just then the showErrorMessage() method is executed.

In this case is easily confirmed when System.out.println("condition failed") is executed or not, in the real life the showErrorMessage() could do an expensive work, so here use the lazy approach makes the difference.

Therefore the current example is not clear because instead to do a method call is established directly a String message instead.

manueljordan avatar Mar 15 '23 15:03 manueljordan

Here, "Assertion messages can be lazily evaluated", means that the message is not evaluated immediately when the assertion is made, if failed, the message will be printed.

assertTrue('a' < 'b', () -> "Assertion messages can be lazily evaluated -- " // print message if it fails, otherwise will not show it. + "to avoid constructing complex messages unnecessarily."); // indicates that using lazy evaluate to avoid complex // messages, here the messages itself is declarative

Lazily evaluating the assertion message can provide a performance benefit, especially if the message construction is complex or time-consuming, as it is only evaluated when the assertion fails.

In the example provided in the JUnit 5 documentation, the assertion message is a concatenation of multiple strings, which could be time-consuming to construct. By using a lambda expression to evaluate the message lazily, the string concatenation is only performed if the assertion fails.

However, it is worth noting that in cases where the assertion message is short and simple, there may be little to no performance benefit in using a lambda expression for lazily evaluating the message.

aliniko avatar Mar 26 '23 15:03 aliniko

@aliniko, your explanation would work nicely.

@manueljordan, invoking another method in the lambda expression is much better than the simple string concatenation example we currently have.

So, if I were to rewrite the example, I would:

  • have the lambda invoke a demonstrative method such as () -> generateFailureMessage('a, 'b)
  • not include an actual implementation for generateFailureMessage
  • include an explanation similar to what @aliniko proposed, explaining that the generateFailureMessage could be complex or time consuming

Anyone interested in submitting a PR which does that?

sbrannen avatar Mar 26 '23 16:03 sbrannen

I think is wise keep the current example and add the other one, the lambda method approach, it to show the clear difference with two examples. I would do this PR - but based on both explanations together, from Aliniko and my explanation (about "1" and "2") - Are you agree Sam?

manueljordan avatar Mar 26 '23 20:03 manueljordan

Hello

Is this issue open to be taken? I'd like to contribute to this project.

Thank you!

FdHerrera avatar Nov 22 '23 14:11 FdHerrera

@sbrannen @manueljordan Yes, I am interested in submitting the PR. And I also have some suggestions if you would like.

We can also write the tests as below to emphasize the fact that the 3rd parameter can be a plain expression or a lambda expression.

  • When used as a plain expression, the expression will be evaluated irrespective of the assertion result.
  • When used as a lambda expression, the expression will only be evaluated when the assertion result is failed and will help us save unnecessary computations when they are not needed.
@Test
void standardAssertions() {
   assertTrue('a' < 'b', "This expression will always evaluate irrespective of assertion result." + getMessage('a', 'b'));
   assertTrue('a' < 'b', () -> "This lambda will only evaluate if assertion fails." + getMessage('a', 'b'));
}

private String getMessage(char p1, char p2) {
   // It can be a complex operation, and must be evaluated, only when assertion fails, to avoid unncessary computation.
   return String.format("ASCII value of '%c' (%d) is greater than '%c' (%d) ", p1, (int)p1, p2, (int)p2));
}

ankitwasankar avatar Jan 07 '24 14:01 ankitwasankar

@ankitwasankar, since no one else has submitted a PR yet, feel free to submit a PR based on your above proposal.

sbrannen avatar Jan 07 '24 15:01 sbrannen

@sbrannen - Raised the PR, please review and let me know - https://github.com/junit-team/junit5/pull/3635

ankitwasankar avatar Jan 10 '24 16:01 ankitwasankar

Hey @sbrannen looks like this PR is still open, id be happy to finish it off and get it over the line. Although at this point having read the thread a few times im unclear what you would like to be done as I think its pretty clear? If you let me know i will update the PR! :) This would be my first open source contribution so excited to start my journey here!

HopeDwag avatar Apr 05 '24 16:04 HopeDwag