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

Do not parse {displayName} placeholder for @ParameterizedTest using MessageFormat #3264

Conversation

alemendoza-v
Copy link
Contributor

@alemendoza-v alemendoza-v commented Apr 21, 2023

Overview

The @ParameterizedTest annotation was breaking name generation by not allowing ' in the method names for Kotlin-based tests or in custom display names in geneeral. It also made it so that placeholders were not being replaced correctly.

To solve it, instead of replacing the DISPLAY_NAME_PLACEHOLDER before the MessageFormat was being evaluated, the DISPLAY_NAME_PLACEHOLDER is now replaced with another temporary placeholder, which is then replaced after the MessageFormat is evaluated.

Solves: #3235


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


Definition of Done

  • There are no TODOs left in the code
  • Method preconditions are checked and documented in the method's Javadoc
  • Coding conventions (e.g. for logging) have been followed
  • Change is covered by automated tests including corner cases, errors, and exception handling
  • Change is documented in the Release Notes

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.

Thanks for submitting the PR.

I've requested a few minor changes.

After making those changes, please add an entry in the Release Notes.

@alemendoza-v
Copy link
Contributor Author

@sbrannen Sure I'll get to work on these changes. Should the Release Notes entry include the basic change? Something like "Bug Fix: @ParameterizedTest now accepts single apostrophes inside the method names in Kotlin tests"?

@alemendoza-v
Copy link
Contributor Author

@sbrannen Also, should the new constant be inside the Interface or inside the ParameterizedTest class?

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.

Thanks for making the previously requested changes.

I've requested a few additional changes after the latest review.

@sbrannen
Copy link
Member

sbrannen commented Apr 23, 2023

FYI: changes to the Javadoc and User Guide are not needed for this change, so I've updated the Definition of Done accordingly.

@sbrannen
Copy link
Member

should the new constant be inside the Interface or inside the ParameterizedTest class?

It's good as a private constant in ParameterizedTestNameFormatter as you have it now, since it is an internal implementation detail local to exactly that one class.

@sbrannen
Copy link
Member

Should the Release Notes entry include the basic change? Something like "Bug Fix: @ParameterizedTest now accepts single apostrophes inside the method names in Kotlin tests"?

There's no need to prefix it with "Bug Fix:". There's a dedicated section for bug fixes in JUnit Jupiter.

Regarding the content, it should be more generic in nature, explaining that the {displayName} placeholder is no longer parsed using MessageFormat, which means that a @DisplayName or Kotlin test method name can contain apostrophes and MessageFormat elements (such as {data}).

See what you come up with, and if we deem it necessary we will polish the wording when merging the PR.

@alemendoza-v
Copy link
Contributor Author

@sbrannen Thanks for reviewing, I'll get to work on the changes and the release notes entry :)

@sbrannen sbrannen self-assigned this Apr 24, 2023
@sbrannen sbrannen changed the title Solve Apostrophe in display name breaks name generation for @ParameterizedTest Do not parse {displayName} placeholder for @ParameterizedTest using MessageFormat Apr 24, 2023
@sbrannen sbrannen closed this in 7b2688c Apr 24, 2023
sbrannen added a commit that referenced this pull request Apr 24, 2023
sbrannen added a commit that referenced this pull request Apr 24, 2023
sbrannen pushed a commit that referenced this pull request Apr 24, 2023
Prior to this commit, if a @ParameterizedTest used the {displayName}
placeholder to generate a display name and the value of the displayName
contained an apostrophe (') or something resembling a MessageFormat
element (such as {data}), JUnit threw an exception due to failure to
parse the display name.

This is applicable to method names in Kotlin-based tests or custom
display names in general.

To fix this bug, instead of replacing the DISPLAY_NAME_PLACEHOLDER
before the MessageFormat is evaluated, the DISPLAY_NAME_PLACEHOLDER is
now replaced with another temporary placeholder, which is then replaced
after the MessageFormat has been evaluated.

Closes #3235
Closes #3264
sbrannen added a commit that referenced this pull request Apr 24, 2023
sbrannen added a commit that referenced this pull request Apr 24, 2023
@sbrannen
Copy link
Member

This has been merged into main in 7b2688c and revised in 7457144.

In addition, since this PR was finalized before the 5.9.3 release, I backported this to 5.9.x for inclusion in the upcoming 5.9.3 release.

Thanks

@alemendoza-v
Copy link
Contributor Author

Thank you!

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

Successfully merging this pull request may close these issues.

Apostrophe in display name breaks name generation for @ParameterizedTest
2 participants