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

UniqueId.parse fails for methods with array type parameters #810

Closed
1 task
jlink opened this issue Apr 27, 2017 · 9 comments
Closed
1 task

UniqueId.parse fails for methods with array type parameters #810

jlink opened this issue Apr 27, 2017 · 9 comments

Comments

@jlink
Copy link
Contributor

jlink commented Apr 27, 2017

Bug description

String idString = uniqueIdFromMethodDescriptorWithArrayParameter.toString();
UniqueId id = UniqueId.parse(idString);

will fail with PreconditionViolation since array types are described with an [ in front but [ chars are currently forbidden as segment values.

Suggested Solution

Change representation of array types in ReflectionUtils.findMethod(clazz, methodName, parameterTypeNames) and StringUtils.nullSafeToString(classes).

Alternative: Don't use [ as separator char in unique IDs. But some 3rd party tools might already have committed to the current format.

Deliverables

  • Encode segmentType and value in constructor and decode in parse.
@jlink
Copy link
Contributor Author

jlink commented Apr 27, 2017

Just thought of a 3rd alternative: Escape/unescape forbidden characters. Seems like a valid approach.

@sbrannen sbrannen added this to the 5.0 M5 milestone Apr 27, 2017
@sbrannen sbrannen changed the title Bug: UniqueId.parse cannot handle unique IDs for methods with array type parameters UniqueId.parse cannot handle unique IDs for methods with array type parameters Apr 27, 2017
@sormuras
Copy link
Member

Alternative: Don't use [ as separator char in unique IDs.

+1

It is still time for breaking changes. Introducing mangling of IDs is something we can do after GA.

@jlink
Copy link
Contributor Author

jlink commented Apr 27, 2017

Using a different separator might break another engine's values. Escaping does not have any of those disadvantages.

@sbrannen
Copy link
Member

sbrannen commented Apr 28, 2017

Good catch! 👍

We've been generating unique IDs such as [engine:junit-jupiter]/[class:org.junit.jupiter.engine.extension.ArrayTests$PrimitiveArrayMethodInjectionTestCase]/[method:primitiveArray([I)] for quite some time, but we obviously never attempted to parse the string representation back into a UniqueId object.

@sbrannen
Copy link
Member

sbrannen commented Apr 28, 2017

FYI: I reproduced this error with a test case run by JUnit Jupiter on master.

public class ArrayTests extends AbstractJupiterTestEngineTests {

	@Test
	void executeTestsForPrimitiveArrayMethodInjectionCases() {
		ExecutionEventRecorder eventRecorder = executeTestsForClass(PrimitiveArrayMethodInjectionTestCase.class);

		assertEquals(1, eventRecorder.getTestStartedCount(), "# tests started");
		assertEquals(1, eventRecorder.getTestSuccessfulCount(), "# tests succeeded");
		assertEquals(0, eventRecorder.getTestFailedCount(), "# tests failed");

		eventRecorder.getExecutionEvents().stream().map(ExecutionEvent::getTestDescriptor).distinct().skip(2).map(
			TestDescriptor::getUniqueId).forEach(id -> {
				System.err.println(id);
				UniqueId.parse(id.toString());
			});
	}

	@ExtendWith(PrimitiveArrayParameterResolver.class)
	private static class PrimitiveArrayMethodInjectionTestCase {

		@Test
		void primitiveArray(int... ints) {
			assertArrayEquals(new int[] { 1, 2, 3 }, ints);
		}
	}

}

The resulting exception is:

org.junit.platform.commons.util.PreconditionViolationException: type or value 'primitiveArray([I)' must not contain '['
	at org.junit.platform.commons.util.Preconditions.condition(Preconditions.java:285)
	at org.junit.platform.engine.UniqueIdFormat.checkDoesNotContain(UniqueIdFormat.java:88)
	at org.junit.platform.engine.UniqueIdFormat.checkAllowed(UniqueIdFormat.java:82)
	at org.junit.platform.engine.UniqueIdFormat.createSegment(UniqueIdFormat.java:75)
	at java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:193)
	at java.util.Spliterators$ArraySpliterator.forEachRemaining(Spliterators.java:948)
	at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:481)
	at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:471)
	at java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:708)
	at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:499)
	at org.junit.platform.engine.UniqueIdFormat.parse(UniqueIdFormat.java:65)
	at org.junit.platform.engine.UniqueId.parse(UniqueId.java:51)
	at org.junit.jupiter.engine.extension.ArrayTests.lambda$0(ArrayTests.java:41)
	at java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:184)
	at java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:193)
	at java.util.stream.SliceOps$1$1.accept(SliceOps.java:204)
	at java.util.stream.DistinctOps$1$2.accept(DistinctOps.java:175)
	at java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:193)
	at java.util.Spliterators$ArraySpliterator.forEachRemaining(Spliterators.java:948)
	at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:481)
	at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:471)
	at java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:151)
	at java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:174)
	at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:418)
	at org.junit.jupiter.engine.extension.ArrayTests.executeTestsForPrimitiveArrayMethodInjectionCases(ArrayTests.java:39)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:303)
	at org.junit.jupiter.engine.execution.ExecutableInvoker.invoke(ExecutableInvoker.java:114)
	at org.junit.jupiter.engine.descriptor.MethodTestDescriptor.lambda$3(MethodTestDescriptor.java:171)
	at org.junit.jupiter.engine.execution.ThrowableCollector.execute(ThrowableCollector.java:40)
	at org.junit.jupiter.engine.descriptor.MethodTestDescriptor.invokeTestMethod(MethodTestDescriptor.java:168)
	at org.junit.jupiter.engine.descriptor.MethodTestDescriptor.execute(MethodTestDescriptor.java:115)
	at org.junit.jupiter.engine.descriptor.MethodTestDescriptor.execute(MethodTestDescriptor.java:1)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.lambda$0(HierarchicalTestExecutor.java:81)
	at org.junit.platform.engine.support.hierarchical.SingleTestExecutor.executeSafely(SingleTestExecutor.java:66)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.execute(HierarchicalTestExecutor.java:76)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.lambda$0(HierarchicalTestExecutor.java:91)
	at org.junit.platform.engine.support.hierarchical.SingleTestExecutor.executeSafely(SingleTestExecutor.java:66)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.execute(HierarchicalTestExecutor.java:76)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.lambda$0(HierarchicalTestExecutor.java:91)
	at org.junit.platform.engine.support.hierarchical.SingleTestExecutor.executeSafely(SingleTestExecutor.java:66)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.execute(HierarchicalTestExecutor.java:76)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.execute(HierarchicalTestExecutor.java:51)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestEngine.execute(HierarchicalTestEngine.java:43)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:170)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:154)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:90)
	at org.junit.platform.launcher.Launcher.execute(Launcher.java:96)

@sbrannen
Copy link
Member

The most robust solution would be to use a format similar to JSON, where the segmentType and value are wrapped in quotes (or some other character), and any occurrence of the wrapping character within a supplied segmentType and value is escaped.

That should solve all problems, future and present.

@sbrannen sbrannen changed the title UniqueId.parse cannot handle unique IDs for methods with array type parameters UniqueId.parse fails for methods with array type parameters May 1, 2017
@marcphilipp
Copy link
Member

We could also URI encode/decode /, [, ], : etc. in each segment.

@marcphilipp
Copy link
Member

Team decision: Use URI encoding for segmentType and value.

sormuras added a commit that referenced this issue Jun 13, 2017
sormuras added a commit that referenced this issue Jun 14, 2017
Prior to this commit a type or value string containing a meta-character
used by the string representation syntax broke the parsing of an unique
id string.
Now, the parser uses 3 characters (defaults to `]/[`) to split the
segments of the unique id string representation. This improvement does
handle a lot more cases than the old implementation, but it can still
yield format exceptions when the type or value string contains exactly
the 3 characters in order.

Fixes #810
@sbrannen
Copy link
Member

in progress by @sormuras

sormuras added a commit that referenced this issue Jun 14, 2017
sormuras added a commit that referenced this issue Jun 14, 2017
sormuras added a commit that referenced this issue Jun 15, 2017
Prior to this commit segment strings could contain characters that are
used by the toString-representation syntax of UniqueIdFormat. Now these
"forbidden" (reserved) characters are URL encoded, as well as decoded
when parsing unique ids.

Fixes #810
sormuras added a commit that referenced this issue Jun 15, 2017
Prior to this commit segment strings could contain characters that are
used by the toString-representation syntax of UniqueIdFormat. Now these
"forbidden" (reserved) characters are URL encoded, as well as decoded
when parsing unique ids.

Fixes #810
sormuras added a commit that referenced this issue Jun 15, 2017
Prior to this commit segment strings could contain characters that are
used by the toString-representation syntax of UniqueIdFormat. Now these
"forbidden" (reserved) characters are URL encoded, as well as decoded
when parsing unique ids.

Fixes #810
sormuras added a commit that referenced this issue Jun 15, 2017
Prior to this commit segment strings could contain characters that are
used by the toString-representation syntax of UniqueIdFormat. Now these
"forbidden" (reserved) characters are URL encoded, as well as decoded
when parsing unique ids.

Fixes #810
sormuras added a commit that referenced this issue Jun 15, 2017
Prior to this commit segment strings could contain characters that are
used by the toString-representation syntax of UniqueIdFormat. Now these
"forbidden" (reserved) characters are URL encoded, as well as decoded
when parsing unique ids.

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

No branches or pull requests

4 participants