From a87457bb4ae077d96bea21de82a6bdc4e8bebed7 Mon Sep 17 00:00:00 2001 From: Ralph Weires Date: Tue, 21 Feb 2023 09:48:35 +0100 Subject: [PATCH 1/4] [SUREFIRE-2152] Handle JUnit5 templated-tests separately to set name/displayName Ensures that all JUnit5 templated tests have a unique name that includes their invocation ID (index) This change also covers an issue of SUREFIRE-2087 where certain tests (namely, tests defined through a @TestTemplate) still cause mixups of test invocations in relation with rerunFailingTestsCount, which can be mixed up by surefire if their name isn't unique for each distinct invocation. For an example description of this issue, see also: https://issues.apache.org/jira/browse/SUREFIRE-2087#comment-17690951 --- .../junitplatform/RunListenerAdapter.java | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/surefire-providers/surefire-junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/RunListenerAdapter.java b/surefire-providers/surefire-junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/RunListenerAdapter.java index cafc934d05..e6626ff08b 100644 --- a/surefire-providers/surefire-junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/RunListenerAdapter.java +++ b/surefire-providers/surefire-junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/RunListenerAdapter.java @@ -33,6 +33,8 @@ import java.util.Optional; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import java.util.stream.Stream; import org.apache.maven.surefire.api.report.OutputReportEntry; @@ -60,6 +62,8 @@ final class RunListenerAdapter implements TestExecutionListener, TestOutputReceiver, RunModeSetter { + private static final Pattern COMMA_PATTERN = Pattern.compile( "," ); + private final ClassMethodIndexer classMethodIndexer = new ClassMethodIndexer(); private final ConcurrentMap testStartTime = new ConcurrentHashMap<>(); private final ConcurrentMap failures = new ConcurrentHashMap<>(); @@ -362,6 +366,23 @@ private String[] toClassMethodName( TestIdentifier testIdentifier ) // param || m()[1] | [1] // param+displ || m()[1] | displ + // Override resulting methodDesc/methodDisp values again, for invocations of + // JUnit5 templated-tests (such as @ParameterizedTest/@RepeatedTest) + Integer templatedTestInvocationId = extractTemplatedInvocationId( testIdentifier ); + if ( templatedTestInvocationId != null ) + { + String simpleClassNames = COMMA_PATTERN.splitAsStream( methodSource.getMethodParameterTypes() ) + .map( s -> s.substring( 1 + s.lastIndexOf( '.' ) ).trim() ) + .collect( joining( ", " ) ); + + String methodSignature = methodName + '(' + simpleClassNames + ')'; + + String invocationIdStr = "[" + templatedTestInvocationId + "]"; + + methodDesc = methodSignature + invocationIdStr; + methodDisp = parentDisplay + display; + } + return new String[] {source[0], source[1], methodDesc, methodDisp}; } else if ( testSource.filter( ClassSource.class::isInstance ).isPresent() ) @@ -390,6 +411,39 @@ else if ( testSource.filter( ClassSource.class::isInstance ).isPresent() ) } } + private static final Pattern TEST_TEMPLATE_INVOCATION_MATCHER = + Pattern.compile( "\\[test-template-invocation:#([1-9][0-9]*)]" ); + + /** + * If the given test-id defines an invocation of a templated-test (such as a specific + * instance of a @ParameterizedTest or @RepeatedTest), returns the invocation-id of + * that instance (1, 2, ...) + * + *

Returns null if the given test-id doesn't seem to be a templated-test invocation, + * or if no invocation-id could be extracted. + */ + private Integer extractTemplatedInvocationId( TestIdentifier testId ) + { + /* + Note: with JUnit 5.8+, we could make this nicer using testId.getUniqueIdObject() + + # Segment lastSegment = testId.getUniqueIdObject().getLastSegment(); + # if (lastSegment.getType().equals(TestTemplateInvocationTestDescriptor.SEGMENT_TYPE)) { + # String invocationIdStr = lastSegment.getValue(); // #1, #2, ... + # if (invocationIdStr.startsWith("#")) { // assuming always true + # return Integer.valueOf(invocationIdStr.substring(1)); + # } + # } + */ + Matcher m = TEST_TEMPLATE_INVOCATION_MATCHER.matcher( testId.getUniqueId() ); + if ( m.find() ) + { + String group = m.group( 1 ); + return Integer.valueOf( group ); + } + return null; + } + /** * @return Map of tests that failed. */ From 46ec3baa79a1d98798ce8a6e1c65386a67eac854 Mon Sep 17 00:00:00 2001 From: Ralph Weires Date: Tue, 21 Feb 2023 09:48:35 +0100 Subject: [PATCH 2/4] [SUREFIRE-2152] Leave out suffix of templated tests, for StackTraceWriter It seems cleaner to strip the suffix of templated tests again here, since it is unrelated to the actual method signature, and might hence just cause confusion in context of a StackTraceWriter. --- .../surefire/junitplatform/RunListenerAdapter.java | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/surefire-providers/surefire-junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/RunListenerAdapter.java b/surefire-providers/surefire-junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/RunListenerAdapter.java index e6626ff08b..ed91ae0995 100644 --- a/surefire-providers/surefire-junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/RunListenerAdapter.java +++ b/surefire-providers/surefire-junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/RunListenerAdapter.java @@ -255,8 +255,14 @@ private SimpleReportEntry createReportEntry( TestIdentifier testIdentifier, { methodText = null; } - StackTraceWriter stw = - testExecutionResult == null ? null : toStackTraceWriter( className, methodName, testExecutionResult ); + String methodNameForSTW = methodName; + if ( methodNameForSTW != null && methodNameForSTW.contains( ")[" ) ) + { + // don't pass suffix of templated-tests ("[1] ...") on to STW, as it's not part of the method signature + methodNameForSTW = methodNameForSTW.substring( 0, methodNameForSTW.indexOf( ")[" ) + 1 ); + } + StackTraceWriter stw = testExecutionResult == null ? null + : toStackTraceWriter( className, methodNameForSTW, testExecutionResult ); return new SimpleReportEntry( runMode, classMethodIndexer.indexClassMethod( className, methodName ), className, classText, methodName, methodText, stw, elapsedTime, reason, systemProperties ); } From ff694485e6812e9f5d6f1c996abbe1f9824aec6f Mon Sep 17 00:00:00 2001 From: Ralph Weires Date: Tue, 21 Feb 2023 09:48:35 +0100 Subject: [PATCH 3/4] [SUREFIRE-2152] Include display-name of JUnit5 templated-tests in methodDesc This includes the displayName (e.g. value of name="..." of a @ParameterizedTest) in the method description / test identifier. Provides more information than merely the invocation ID in case of failing tests, also for the console reporter. This can (especially if the actual list of arguments are quite large and/or dynamically determined at runtime) help a lot to identify which actual cases are the problematic ones. --- .../surefire/junitplatform/RunListenerAdapter.java | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/surefire-providers/surefire-junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/RunListenerAdapter.java b/surefire-providers/surefire-junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/RunListenerAdapter.java index ed91ae0995..ec4f9ac62e 100644 --- a/surefire-providers/surefire-junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/RunListenerAdapter.java +++ b/surefire-providers/surefire-junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/RunListenerAdapter.java @@ -374,6 +374,9 @@ private String[] toClassMethodName( TestIdentifier testIdentifier ) // Override resulting methodDesc/methodDisp values again, for invocations of // JUnit5 templated-tests (such as @ParameterizedTest/@RepeatedTest) + // => Include the display-name of the actual invocation as well in the methodDesc of + // each invocation (also according to the 'name=' values of such tests), to have + // more context of what failed also e.g. from output of console reporter Integer templatedTestInvocationId = extractTemplatedInvocationId( testIdentifier ); if ( templatedTestInvocationId != null ) { @@ -384,8 +387,14 @@ private String[] toClassMethodName( TestIdentifier testIdentifier ) String methodSignature = methodName + '(' + simpleClassNames + ')'; String invocationIdStr = "[" + templatedTestInvocationId + "]"; - - methodDesc = methodSignature + invocationIdStr; + String displayForDesc = display; + if ( displayForDesc.startsWith( invocationIdStr ) ) + { + // a bit hacky, try to prevent including ID multiple times in most default cases + // "foo()[1][1] abc def" -> "foo()[1] abc def" + displayForDesc = displayForDesc.substring( invocationIdStr.length() ); + } + methodDesc = methodSignature + invocationIdStr + displayForDesc; methodDisp = parentDisplay + display; } From 606b33482d560410ba786de5074ead536b8b09f5 Mon Sep 17 00:00:00 2001 From: Ralph Weires Date: Tue, 21 Feb 2023 09:48:35 +0100 Subject: [PATCH 4/4] [SUREFIRE-2152] Adapt some tests, to make expectations match the code changes Since the name of templated tests now includes the displayName as suffix, the nameText is the same (and hence set to null) in some test-cases --- .../junitplatform/JUnitPlatformProviderTest.java | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/surefire-providers/surefire-junit-platform/src/test/java/org/apache/maven/surefire/junitplatform/JUnitPlatformProviderTest.java b/surefire-providers/surefire-junit-platform/src/test/java/org/apache/maven/surefire/junitplatform/JUnitPlatformProviderTest.java index b909b2c43a..877f40dc94 100644 --- a/surefire-providers/surefire-junit-platform/src/test/java/org/apache/maven/surefire/junitplatform/JUnitPlatformProviderTest.java +++ b/surefire-providers/surefire-junit-platform/src/test/java/org/apache/maven/surefire/junitplatform/JUnitPlatformProviderTest.java @@ -640,27 +640,27 @@ public void rerunParameterized() assertEquals( TestClass7.class.getName(), reportEntries.get( 0 ).getSourceName() ); assertNull( reportEntries.get( 0 ).getSourceText() ); - assertEquals( "testParameterizedTestCases(String, boolean)[1]", reportEntries.get( 0 ).getName() ); assertEquals( "testParameterizedTestCases(String, boolean)[1] Always pass, true", - reportEntries.get( 0 ).getNameText() ); + reportEntries.get( 0 ).getName() ); + assertEquals( null, reportEntries.get( 0 ).getNameText() ); assertEquals( TestClass7.class.getName(), reportEntries.get( 1 ).getSourceName() ); assertNull( reportEntries.get( 1 ).getSourceText() ); - assertEquals( "testParameterizedTestCases(String, boolean)[2]", reportEntries.get( 1 ).getName() ); assertEquals( "testParameterizedTestCases(String, boolean)[2] Always fail, false", - reportEntries.get( 1 ).getNameText() ); + reportEntries.get( 1 ).getName() ); + assertEquals( null, reportEntries.get( 1 ).getNameText() ); assertEquals( TestClass7.class.getName(), reportEntries.get( 2 ).getSourceName() ); assertNull( reportEntries.get( 2 ).getSourceText() ); - assertEquals( "testParameterizedTestCases(String, boolean)[2]", reportEntries.get( 2 ).getName() ); assertEquals( "testParameterizedTestCases(String, boolean)[2] Always fail, false", - reportEntries.get( 2 ).getNameText() ); + reportEntries.get( 2 ).getName() ); + assertEquals( null, reportEntries.get( 2 ).getNameText() ); assertEquals( TestClass7.class.getName(), reportEntries.get( 3 ).getSourceName() ); assertNull( reportEntries.get( 3 ).getSourceText() ); - assertEquals( "testParameterizedTestCases(String, boolean)[2]", reportEntries.get( 3 ).getName() ); assertEquals( "testParameterizedTestCases(String, boolean)[2] Always fail, false", - reportEntries.get( 3 ).getNameText() ); + reportEntries.get( 3 ).getName() ); + assertEquals( null, reportEntries.get( 3 ).getNameText() ); TestExecutionSummary summary = executionListener.summaries.get( 0 ); assertEquals( 2, summary.getTestsFoundCount() );