Apostrophe in display name breaks name generation for @ParameterizedTest

[TWiStErRob]

Steps to reproduce

class Tests {
	@BeforeEach 
	fun setUp(info: TestInfo) {
		System.out.println(info.displayName)
	}

	@MethodSource("methodSource")
	@ParameterizedTest(name = "{displayName} - {0}") // BUG
	fun `displayName'Zero`(param: String) {
		// displayNameZero(String) - {0}
		// displayNameZero(String) - {0}
	}

	@MethodSource("methodSource")
	@ParameterizedTest(name = "{0} - {displayName}") 
	fun `zero'DisplayName`(param: String) {
		// foo - zeroDisplayName(String)
		// bar - zeroDisplayName(String)
	}

	@MethodSource("methodSource")
	@ParameterizedTest(name = "{displayName} - {0}") 
	fun displayNameZero(param: String) {
		// displayNameZero(String) - foo
		// displayNameZero(String) - bar
	}

	@MethodSource("methodSource")
	@ParameterizedTest(name = "{0} - {displayName}") 
	fun zeroDisplayName(param: String) {
		// foo - zeroDisplayName(String)
		// bar - zeroDisplayName(String)
	}

	companion object {

		@JvmStatic
		private fun methodSource(): Array<String> =
			arrayOf(
				"foo",
				"bar"
			)
	}
}

1. Clone https://github.com/TWiStErRob/repros/tree/main/junit/jupiter-params-displayName-vs-apostrophe
2. gradlew test
3. Review report at build/reports/tests/test/index.html

Note: the repro project is a bit more complex than the above:

• TestK is a pure Kotlin version
• TestJ is a pure Java version
• Test is the problematic version with Kotlin apostrophes.
  I made them really similar so it's easy to compare their outputs.

Expected

No {0} anywhere.
It works correctly without apostrophes:
build/reports/tests/test/classes/com.example.MyTestK.html

Actual

When the method name contains ', the {0} is not substituted.
It fails formatting the name when apostrophes are added:
build/reports/tests/test/classes/com.example.MyTest.html

Context

• Used versions (Jupiter/Vintage/Platform): 5.9.2
• Build Tool/IDE: Gradle / IDEA, but likely irrelevant.

Deliverables

I think the problem might be around ParameterizedTestNameFormatter, but didn't debug it.

[sbrannen]

JUnit uses java.text.MessageFormat to process display names for parameterized tests.

Here's an excerpt from the User Guide:

Please note that name is a MessageFormat pattern. Thus, a single quote (') needs to be represented as a doubled single quote ('') in order to be displayed.

That means if you renamed your test method to displayName''Zero, it should work.

I say "should", because I have not tested it myself (since I don't use Kotlin).

Please let us know if that works for you.

[TWiStErRob]

Yep that works as expected, but imagine test names like fun `foo shouldn''t bar`(baz: Qux) 🤢. Of course in case of contractions it's easy to expand to "should not", but there are cases when you don't have an option, like foo 'bar' baz, although that might "just work" because the quotation is closed.

Could the name be pre-escaped somehow for MessageFormat, since using that replacement tool is just an implementation detail?

in order to be displayed.

I think that part of the user guide should be more explicit that if you have a ' in display name, things will break other than "not showing".

[sbrannen]

Yep that works as expected,

Glad to hear that works with Kotlin.

Thanks for confirming. 👍

but there are cases when you don't have an option, like foo 'bar' baz, although that might "just work" because the quotation is closed.

I'm not so sure that would work, but feel free to try it and report back.

Could the name be pre-escaped somehow for MessageFormat, since using that replacement tool is just an implementation detail?

Interesting idea. We'll discuss it amongst the team.

I think that part of the user guide should be more explicit that if you have a ' in display name, things will break other than "not showing".

Indeed, the documentation should be improved, both in User Guide and in the Javadoc.

[TWiStErRob]

Confirmed that using apostrophe-enclosed text becomes raw and doesn't break the formatter:

(Note that ideally these apostrophes would stay in the rendered test names, so generally escaping "user input" could be a way. Unless someone wants to have foo '{0}' bar {0} → foo {0} bar actualparam which wouldn't be possible afterwards 😥)

[marcphilipp]

Team Decision: Ensure replacing {displayName} in the name pattern works if it contains '.

Implementation hint: Try replacing {displayName} after evaluating the MessageFormat.

[marcphilipp]

@TWiStErRob Would you be interested in submitting a PR for this?

[TWiStErRob]

Happy to let anyone take a stab, please go ahead!

(I might do it in the future, but I'll comment here if/when I start.)

[alemendoza-v]

Hello, i'm interested in solving this issue, could someone help me get started? I'm new to contributing :)

[marcphilipp]

@alemendoza-v Sure! The relevant code is in ParameterizedTestNameFormatter (look for DISPLAY_NAME_PLACEHOLDER). Unit tests should be added to the corresponding test class and we should also have an "integration" test as a new test class in junit-jupiter-params/src/test/kotlin. Please let me know if you need any additional pointers.

[alemendoza-v]

@alemendoza-v Sure! The relevant code is in ParameterizedTestNameFormatter (look for DISPLAY_NAME_PLACEHOLDER). Unit tests should be added to the corresponding test class and we should also have an "integration" test as a new test class in junit-jupiter-params/src/test/kotlin. Please let me know if you need any additional pointers.

Thanks! I'm assuming i'll have to install Kotlin right? I've only worked with Java before

[marcphilipp]

You only need JDK 17. The Gradle build will take care of everything else when you import the project into IntelliJ IDEA.

[alemendoza-v]

Perfect, I was planning on using vscode but ill use intelliJ to work

[alemendoza-v]

@TWiStErRob Hello, I think I found a possible solution, If the ' are replaced to '' it would allow single apostrophes, do you think that would solve the issue?

Doing that I got this output using the MyTest.kt example file with the new changes, this is the desired output right?

[sbrannen]

If the ' are replaced to '' it would allow single apostrophes, do you think that would solve the issue?

That would solve the issue regarding single apostrophes; however, it would not be a general purpose solution.

Please see the tip in #3235 (comment):

Try replacing {displayName} after evaluating the MessageFormat.

Doing that should theoretically cover all use cases.

[alemendoza-v]

If the ' are replaced to '' it would allow single apostrophes, do you think that would solve the issue?

That would solve the issue regarding single apostrophes; however, it would not be a general purpose solution.

Please see the tip in #3235 (comment):

Try replacing {displayName} after evaluating the MessageFormat.

Doing that should theoretically cover all use cases.

Does the evaluation of the MessageFormat happen when the makeReadable() function is called or when a new instance is created?

[sbrannen]

Does the evaluation of the MessageFormat happen when the makeReadable() function is called or when a new instance is created?

The MessageFormat is evaluated when format() is invoked: format.format(humanReadableArguments).

The displayName is replaced in prepareMessageFormatPattern().

[alemendoza-v]

Does the evaluation of the MessageFormat happen when the makeReadable() function is called or when a new instance is created?

The MessageFormat is evaluated when format() is invoked: format.format(humanReadableArguments).

The displayName is replaced in prepareMessageFormatPattern().

Thanks!

[alemendoza-v]

I have a problem, to replace {displayName} after evaluating the MessageFormat, I have to find a way to exclude {displayName} from the pattern right? Since MessageFormat does not allow {displayName} to be part of the pattern. Then I should replace it?

[alemendoza-v]

@sbrannen OK so I think I found the solution.

In the prepareMessageFormatPattern() function, instead of replacing the placeholder with the actual display name, I used a new place holder that does not include braces.

String result = pattern//
			.replace(DISPLAY_NAME_PLACEHOLDER, "__DISPLAY_NAME__")//
			.replace(INDEX_PLACEHOLDER, String.valueOf(invocationIndex));

This allows the MessageFormat object to actually be created.

Then the formatSafely() function works as normal, but before returning the string, it replaces that new placeholder with the display name:

String formatted = format.format(humanReadableArguments);
return formatted.replace("__DISPLAY_NAME__",  this.displayName);

Using the tests provided by @TWiStErRob I get the following output names:

Is this the right approach?

[alemendoza-v]

Should I créate a PR for it?

[sbrannen]

@alemendoza-v,

I assumed that {displayName} would temporarily have to be replaced with something that doesn't contain curly brackets. So, yes, that appears to be the correct approach.

Feel free to submit a PR.

If you would like to get it included in JUnit 5.10, please do so in the coming days or week since we are now finalizing the feature set for 5.10.

In your PR, please include Java-based tests in addition to Kotlin-based tests.