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

Document benefits of messageSupplier in Assertions #3153

Closed
manueljordan opened this issue Feb 10, 2023 · 14 comments · Fixed by #3938
Closed

Document benefits of messageSupplier in Assertions #3153

manueljordan opened this issue Feb 10, 2023 · 14 comments · Fixed by #3938

Comments

@manueljordan
Copy link

manueljordan commented Feb 10, 2023

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

@marcphilipp
Copy link
Member

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

@marcphilipp
Copy link
Member

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

@manueljordan
Copy link
Author

@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

@marcphilipp
Copy link
Member

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.

@manueljordan
Copy link
Author

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.

@aliniko
Copy link

aliniko commented Mar 26, 2023

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.

@sbrannen
Copy link
Member

@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?

@manueljordan
Copy link
Author

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?

@FdHerrera
Copy link

Hello

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

Thank you!

@ankitwasankar
Copy link

@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));
}

@sbrannen sbrannen changed the title Improve documentation: What does "Assertion messages can be lazily evaluated" mean? Document what "Assertion messages can be lazily evaluated" means Jan 7, 2024
@sbrannen
Copy link
Member

sbrannen commented Jan 7, 2024

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

@sbrannen sbrannen closed this as completed Jan 7, 2024
@sbrannen sbrannen changed the title Document what "Assertion messages can be lazily evaluated" means Document benefit of messageSupplier in Assertions Jan 7, 2024
@sbrannen sbrannen reopened this Jan 8, 2024
@ankitwasankar
Copy link

@sbrannen - Raised the PR, please review and let me know - #3635

@sbrannen sbrannen changed the title Document benefit of messageSupplier in Assertions Document benefits of messageSupplier in Assertions Jan 11, 2024
@HopeDwag
Copy link

HopeDwag commented Apr 5, 2024

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!

@marcphilipp marcphilipp linked a pull request Apr 15, 2024 that will close this issue
6 tasks
@chris-carneiro
Copy link
Contributor

Dusting off this issue. I'm offering my contribution here, trying to include a bit of everyone's vision:

image image

If the result somewhat matches what's expected, I'd be happy to open a new PR for review.

chris-carneiro added a commit to chris-carneiro/junit5 that referenced this issue Aug 28, 2024
chris-carneiro added a commit to chris-carneiro/junit5 that referenced this issue Aug 28, 2024
chris-carneiro added a commit to chris-carneiro/junit5 that referenced this issue Aug 28, 2024
chris-carneiro added a commit to chris-carneiro/junit5 that referenced this issue Aug 28, 2024
chris-carneiro added a commit to chris-carneiro/junit5 that referenced this issue Sep 15, 2024
chris-carneiro added a commit to chris-carneiro/junit5 that referenced this issue Sep 17, 2024
chris-carneiro added a commit to chris-carneiro/junit5 that referenced this issue Sep 17, 2024
@marcphilipp marcphilipp added this to the 5.12 M1 milestone Sep 17, 2024
@marcphilipp marcphilipp modified the milestones: 5.12 M1, 5.11.1 Sep 23, 2024
marcphilipp pushed a commit that referenced this issue Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment