[GitHub] [maven-surefire] andpab commented on a diff in pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4

[GitBox <gi...@apache.org>]

andpab commented on code in PR #516:
URL: https://github.com/apache/maven-surefire/pull/516#discussion_r1060357685


##########
surefire-providers/surefire-junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/RunListenerAdapter.java:
##########
@@ -314,28 +313,26 @@ private String[] toClassMethodName( TestIdentifier testIdentifier )
             boolean needsSpaceSeparator = isNotBlank( parentDisplay ) && !display.startsWith( "[" );
             String methodDisplay = parentDisplay + ( needsSpaceSeparator ? " " : "" ) + display;
 
-            String simpleClassNames = COMMA_PATTERN.splitAsStream( methodSource.getMethodParameterTypes() )
-                    .map( s -> s.substring( 1 + s.lastIndexOf( '.' ) ) )
-                    .collect( joining( "," ) );
-
-            boolean hasParams = isNotBlank( methodSource.getMethodParameterTypes() );

Review Comment:
   You're not only changing the behaviour for parameterized tests, but also for normal tests which is why you then have to adjust a lot of test expectations. That's not particularly nice.
   
   I would suggest an approach that also addresses the other root cause of the problem which is that the logic for determining `hasParams` only looks at `methodSource.getMethodParameterTypes()`. That's not correct for JUnit 4 Parameterized tests since they do not have a method parameter. In JUnit 4 the parameterization is handled on a level further up.
   
   Consider something like this:
   
   ```java
   boolean hasParameterizedParent =
       collectAllTestIdentifiersInHierarchy( testIdentifier )
           .filter( identifier -> !identifier.getSource().isPresent() )
           .map( TestIdentifier::getLegacyReportingName )
           .anyMatch( legacyReportingName -> legacyReportingName.matches( "^\\[.+]$" ) );
   
   boolean parameterized = isNotBlank( methodSource.getMethodParameterTypes() ) || hasParameterizedParent;
   String methodDesc = parameterized ? description : methodName;
   ```
   This code relies on the fact that for JUnit 4 parameterized tests there is a container-type test identifier with the legacyReportingName `[<index>]` or `[<displayName>]` that has an empty source field in the test identifier hierarchy between the test method and the test class.
   
   The rest of the code could stay as it was to keep the behaviour for non-parameterized tests the same - except for `simpleClassNames` and `methodSign` which are not needed anymore. 
   
   Then you don't have to change any expectations of existing tests. You can leave them as they are and only add your new tests.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org