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 #3635

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ankitwasankar
Copy link

@ankitwasankar ankitwasankar commented Jan 10, 2024

Overview

Documented the benefits of MessageSupplier in assertion, as per the discussion on issue #3153


I hereby agree to the terms of the JUnit Contributor License Agreement.


Definition of Done

@sbrannen sbrannen changed the title Document benefits of MessageSupplier in Assertions Issue: #3153 Document benefits of MessageSupplier in Assertions Jan 11, 2024
Copy link
Member

@sbrannen sbrannen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's take a different approach.

I actually like the original text in the assertion failure messages (perhaps because I wrote them), and I think we should keep that content.

So, let's move that text to a new paragraph before the example code in the User Guide, and let's incorporate some of the text you wrote in the same paragraph -- in other words, combine the existing text with your text to come up with something more comprehensive.

After doing that, let's go back to an example similar to the one you proposed in #3153 (comment). Having plain strings in both variants (String and Supplier<String>) is what was wrong with the example to begin with. Using a lambda expression or method reference for the messageSupplier will make it much easier to explain the benefits.

@sbrannen sbrannen changed the title Document benefits of MessageSupplier in Assertions Document benefits of messageSupplier in Assertions Jan 11, 2024
@ankitwasankar
Copy link
Author

@sbrannen - thank you for the review comment, I have updated the code.
Please let me know if you think something needs to be added or removed.

@marcphilipp marcphilipp linked an issue Apr 15, 2024 that may be closed by this pull request
@marcphilipp marcphilipp requested a review from sbrannen May 2, 2024 14:38
Copy link
Member

@sbrannen sbrannen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ankitwasankar,

I think you misunderstood what I was requesting, so I'll restate that here.

I like the original text in the assertion failure messages (perhaps because I wrote them), and I think we should keep that content.

By "content" I mean the text/explanation which should retain mostly intact but in the "Assertions" section of the User Guide, not in the included AssertionsDemo code as inline comments.

So, let's move that text to a new paragraph before the example code in the User Guide, and let's incorporate some of the text you wrote in the same paragraph -- in other words, combine the existing text with your text to come up with something more comprehensive.

I'd still like to see this.

After doing that, let's go back to an example similar to the one you proposed in #3153 (comment). Having plain strings in both variants (String and Supplier) is what was wrong with the example to begin with.

That last part is the most important. There is no point in having a static string returned from a lambda expression.

The whole reason we support a messageSupplier is to avoid unnecessary creation of dynamic strings.

Please note as well that the concatenation of two static strings does not constitute a dynamic strings since the compiler will likely convert them to a single static string during compilation.

Using a lambda expression or method reference for the messageSupplier will make it much easier to explain the benefits.

Again, that's what we're trying to convey here, and your getMessage() example in #3153 (comment) achieves that.

I think we can also remove the assertWithMessageSupplier() test method and include all "standard assertions" in the existing standardAssertions() method.

Please make the necessary changes to achieve the above.

Thanks!

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.

Document benefits of messageSupplier in Assertions
2 participants