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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 5 commits into from
Closed

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

wants to merge 5 commits into from

Conversation

alemendoza-v
Copy link
Contributor

@alemendoza-v alemendoza-v commented Apr 21, 2023 •
edited
Loading

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

sbrannen
sbrannen requested changes Apr 22, 2023
View reviewed changes
Copy link
Member

@sbrannen sbrannen left a comment •
edited
Loading

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.

junit-jupiter-params/src/main/java/org/junit/jupiter/params/ParameterizedTestNameFormatter.java Outdated Show resolved Hide resolved
junit-jupiter-params/src/test/kotlin/ParameterizedTestNameFormatterIntegrationTests.kt Outdated Show resolved Hide resolved
junit-jupiter-params/src/test/kotlin/ParameterizedTestNameFormatterIntegrationTests.kt Outdated Show resolved Hide resolved
junit-jupiter-params/src/test/kotlin/ParameterizedTestNameFormatterIntegrationTests.kt Outdated Show resolved Hide resolved
@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?

sbrannen
sbrannen requested changes Apr 23, 2023
View reviewed changes
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.

junit-jupiter-params/src/main/java/org/junit/jupiter/params/ParameterizedTestNameFormatter.java Outdated Show resolved Hide resolved
...piter-params/src/test/java/org/junit/jupiter/params/ParameterizedTestNameFormatterTests.java Outdated Show resolved Hide resolved
junit-jupiter-params/src/test/kotlin/ParameterizedTestNameFormatterIntegrationTests.kt Outdated Show resolved Hide resolved
junit-jupiter-params/src/test/kotlin/ParameterizedTestNameFormatterIntegrationTests.kt Outdated Show resolved Hide resolved
@sbrannen
Copy link
Member

sbrannen commented Apr 23, 2023 •
edited
Loading

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 linked an issue Apr 24, 2023 that may be closed by this pull request
Apostrophe in display name breaks name generation for @ParameterizedTest #3235
Closed
@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
Polish contribution
7457144 
See #3264
sbrannen added a commit that referenced this pull request Apr 24, 2023
@sbrannen
Move release note for #3235/#3264 to 5.9.3
0a7c051
sbrannen pushed a commit that referenced this pull request Apr 24, 2023
@alemendoza-v @sbrannen
Do not parse {displayName} for @ParameterizedTest using MessageFormat
e575ea3 
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
Polish contribution
6c02c06 
See #3264
sbrannen added a commit that referenced this pull request Apr 24, 2023
@sbrannen
Move release note for #3235/#3264 to 5.9.3
468958d
@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
Reviewers

@sbrannen sbrannen sbrannen requested changes

Assignees

@sbrannen sbrannen

Labels
component: Jupiter component: Kotlin theme: parameterized tests type: bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Apostrophe in display name breaks name generation for @ParameterizedTest
2 participants
@alemendoza-v @sbrannen