[SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4 #516

chalmagr · 2022-04-07T20:58:18Z

Changing the run listener to use the nameText value (for parameterized scenarios) to prevent invalid flake test reports (see https://github.com/chalmagr/surefire-flaky-report)

Following this checklist to help us incorporate your
contribution quickly and easily:

Make sure there is a JIRA issue filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
Each commit in the pull request should have a meaningful subject line and body.
Format the pull request title like [SUREFIRE-XXX] - Fixes bug in ApproximateQuantiles,
where you replace SUREFIRE-XXX with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the
commit message.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Run mvn clean install to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
You have run the integration tests successfully (mvn -Prun-its clean install).

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.

Tibor17 · 2022-04-08T14:34:51Z

We should separate this problem in two.
I can see the first problem with ClassNotFoundException, and the second issue with junit4.
Let me reproduce the issues with your project.

Tibor17 · 2022-04-08T14:39:21Z

@chalmagr
How can I reproduce the first issue with ClassNotFoundException?

chalmagr · 2022-04-08T16:08:51Z

@chalmagr How can I reproduce the first issue with ClassNotFoundException?

@Tibor17 This issue happens when I try to build the maven-surefire project in my laptop - I don't have the problem when running the tests in the other test project. If I run with -DskipTests the project builds completely fine

$ mvn --version
Apache Maven 3.6.3 (cecedd343002696d0abb50b32b541b8a6ba2883f)
Maven home: ...
Java version: 1.8.0_92, vendor: Oracle Corporation, runtime: /Library/Java/JavaVirtualMachines/jdk1.8.0_92.jdk/Contents/Home/jre
Default locale: en_US, platform encoding: UTF-8
OS name: "mac os x", version: "10.16", arch: "x86_64", family: "mac"

$ mvn clean install site site:stage -P reporting,run-its
...
[INFO] Reactor Summary for Apache Maven Surefire 3.0.0-M7-SNAPSHOT:
[INFO]
[INFO] Apache Maven Surefire .............................. SUCCESS [ 20.238 s]
[INFO] Surefire Shared Utils .............................. SUCCESS [  6.816 s]
[INFO] SureFire Logger API ................................ SUCCESS [ 16.375 s]
[INFO] SureFire API ....................................... FAILURE [  5.184 s]
...
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:3.0.0-M4:test (default-test) on project surefire-api: There are test failures.
[ERROR]
[ERROR] Please refer to /Users/chalmagr/projects/maven-surefire/surefire-api/target/surefire-reports for the individual test results.
[ERROR] Please refer to dump files (if any exist) [date].dump, [date]-jvmRun[N].dump and [date].dumpstream.
[ERROR] The forked VM terminated without properly saying goodbye. VM crash or System.exit called?
[ERROR] Command was /bin/sh -c cd /Users/chalmagr/projects/maven-surefire/surefire-api && /Library/Java/JavaVirtualMachines/jdk1.8.0_92.jdk/Contents/Home/jre/bin/java -Xms32m -Xmx144m -XX:SoftRefLRUPolicyMSPerMB=50 -Djava.awt.headless=true -Djdk.net.URLClassPath.disableClassPathURLCheck=true '-javaagent:/Users/chalmagr/.m2/repository/org/jacoco/org.jacoco.agent/0.8.7/org.jacoco.agent-0.8.7-runtime.jar=destfile=/Users/chalmagr/projects/maven-surefire/surefire-api/target/jacoco.exec,includes=**/failsafe/*:**/failsafe/**/*:**/surefire/*:**/surefire/**/*,excludes=**/HelpMojo.class:**/shadefire/**/*:org/jacoco/**/*:com/vladium/emma/rt/*' org.apache.maven.shadefire.surefire.booter.ForkedBooter /Users/chalmagr/projects/maven-surefire/surefire-api/target/surefire 2022-04-08T10-40-14_147-jvmRun1 surefire5745983445816295292tmp surefire_0369998306054510421tmp
...

The log file contains the below

# Created at 2022-04-08T10:40:14.589
objc[11846]: Class JavaLaunchHelper is implemented in both /Library/Java/JavaVirtualMachines/jdk1.8.0_92.jdk/Contents/Home/jre/bin/java (0x10ab474c0) and /Library/Java/JavaVirtualMachines/jdk1.8.0_92.jdk/Contents/Home/jre/lib/libinstrument.dylib (0x10ad154e0). One of the two will be used. Which one is undefined.

# Created at 2022-04-08T10:40:14.751
Error: Could not find or load main class org.apache.maven.shadefire.surefire.booter.ForkedBooter

Hope this helps...

Tibor17 · 2022-04-08T23:40:58Z

What happens if you run the command $ mvn clean install?
We have MacOS in Github Actions and it does not any problems like this.
bwt, libinstrument.dylib ... this sounds like instrumentation and maybe that's the reason why some instrumentation tool has changed the package of ForkedBooter. I have updated the version of internal plugin, pls check it out now.

chalmagr · 2022-04-12T21:13:47Z

Pulled the changes from master and now clean install seems to run fine - looking into some test failures and will attempt to run with the integration test profiles as well and post an update

chalmagr · 2022-04-13T02:50:15Z

@Tibor17 Run integration tests properly as well now. It usually takes ~1h to complete?

install site site:stage -P reporting,run-its

Tibor17 · 2022-04-15T08:17:30Z

@Tibor17 yes it normally takes over 1h. You may use mvn clean install -P run-its , that's enough.

chalmagr · 2022-04-21T14:04:27Z

@Tibor17 the tests were all good within that hour. Please let me know any actions required to move forward on this one.

Tibor17 · 2022-04-24T08:14:20Z

...urefire-common/src/main/java/org/apache/maven/plugin/surefire/report/TestSetRunListener.java

@@ -312,7 +312,7 @@ private void addTestMethodStats()
        for ( WrappedReportEntry reportEntry : detailsForThis.getReportEntries() )
        {
            TestMethodStats methodStats =
-                new TestMethodStats( reportEntry.getClassMethodName(), reportEntry.getReportEntryType(),
+                new TestMethodStats( reportEntry.getReportClassMethodName(), reportEntry.getReportEntryType(),


@chalmagr I do not think this is right because source/name is always extracted from JUnit4 Description which encodes both strings as follows name(source) and JUnit ensures that the descrption is unique. Typically, junit encodes name as method[<id>]. If this is wrong, then the problem is in provider and not in this class TSRL.

I may need to dive a bit more and verify, but seems that even though JUnit is providing the source as you describe we are removing the (source) piece and splitting it between class and method

org.apache.maven.surefire.common.junit4.JUnit4RunListener#testFailure

... ClassMethod classMethod = toClassMethod( failure.getDescription() ); long testRunId = classMethodIndexer.indexClassMethod( classMethod.getClazz(), classMethod.getMethod() ); ReportEntry report = withException( runMode, testRunId, classMethod.getClazz(), null, classMethod.getMethod(), null, stackTrace ); ...

org.apache.maven.surefire.common.junit4.JUnit4ProviderUtil#toClassMethod

public static ClassMethod toClassMethod( Description description ) { String clazz = extractClassName( description.getDisplayName() ); ... String method = extractMethodName( description.getDisplayName() ); return new ClassMethod( clazz, method ); }

org.apache.maven.surefire.api.util.internal.TestClassMethodNameUtils#extractClassName

public static String extractClassName( String displayName ) { String clazz = displayName; if ( displayName.endsWith( ")" ) ) { int paren = displayName.lastIndexOf( '(' ); if ( paren != -1 ) { clazz = displayName.substring( paren + 1, displayName.length() - 1 ); } } return clazz; }

Make sure you understand b2cce57 and especially e8f65b9. I believe the fix for this issue should be in RunListenerAdapter.toClassMethodName. The logic there is hard to reason about without comments describing intent but ultimately I think you'll want methodDesc to be set to the legacy reporting name (given by junit in the testidentifier directly) for these junit4 parameterized tests, not methodSource.getMethodName() as it is today. It's using the method name because displayName and legacyReportingName are found to be equal (both e.g. methodName[0]).

When running junit5 parameterized tests, this does not repro because junit creates the testidentifier setting the legacy reporting name to methodName(ParameterDataType1, ..)[testInvocationNum] (e.g. "methodName(Long)[0]") and the display name if not overridden (@ParameterizedTest(name="blah")) to [testInvocationNum] argList e.g. "[0] 100" if the given param is 100L and it's the first parameterized case. Since the code finds them unequal it sets methodDesc to the legacyReportingName. The basic issue here is that the handling for parameterized tests makes an implicit assumption that they will always have a different legacy reporting name from display name which isn't the case for junit 4 parameterized tests without the @parameters name overridden.

Right... Was looking into my previous thought and was able to confirm that it was indeed ok and wasn't really affecting it as it depends on the provider... eventually ended up in this RunListenerAdapter class as well, and the logic there is quite overwhelming and a bit scary :) I have made changes to that class only now (without all the other changes) and it does seem to be ok, as suggested.

I've added a junit4 table with the different scenarios so that it has some examples on it. There is a different issue I found (when miss-using the junit4 displayName to not contain the parameter; causing same problem.. it is possible to avoid it, but requires messing with the UniqueID .. probably not the best since it may change... anyways, when attempting 2nd run on the failed tests (even with proper handling), junit4 fails to run only the failed and re-runs all the ones that match the 'displayName' ... but again, since this is some very odd case that I hope nobody is using (And should be discouraged I guess by some warnings when not adding {0} or anything to it)

Will run some more tests and see if I can update this PR to use the newly created branch with less changes or if I should overwrite my branch's history ... will update this PR soon

Thanks!

I've updated the PR with just changes to the adapter and additional ITs for this jira

Tibor17

Pls answer 5cbec32#r857087085

br0nstein · 2022-05-29T20:11:59Z

@chalmagr ping for update, thanks!

chalmagr · 2022-06-03T05:19:49Z

Hi - been unavailable for some time... will look into this shortly. Thanks!

chalmagr · 2022-06-06T05:29:12Z

Reopening PR after reseting master branch to remove my changes and added suggested changes from comments

…how as flaky on junit4

chalmagr · 2022-06-17T23:53:04Z

@Tibor17 @br0nstein ping

chalmagr · 2022-07-05T13:53:26Z

@Tibor17 are you still available to look into this? should someone else be pulled in?

Tibor17 · 2022-07-05T15:07:13Z

@chalmagr
@br0nstein
I have returned back from my trip and I want to continue on my local changes, and my local changes are the solution for your problem and we can proove my fix with your IT but the change in RunListenerAdapter is the last change of all changes and the most probable fix would appear in TestSetRunListener + StatelessXmlReporter + M6 release. So I will show you my PR, and there you will see why the stateful JUnit5 reports were chaotic in rerun and parallel exec too.

andpab · 2023-01-03T08:25:46Z

...junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/RunListenerAdapter.java

-                    .map( s -> s.substring( 1 + s.lastIndexOf( '.' ) ) )
-                    .collect( joining( "," ) );
-
-            boolean hasParams = isNotBlank( methodSource.getMethodParameterTypes() );


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:

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.

br0nstein · 2023-01-20T18:52:08Z

@Tibor17 any progress update on the PR you mentioned above to fix this an alternate way? We are unable to upgrade surefire due to this bug as we use parameterized tests extensively with junit-vintage-engine, so would appreciate a fix!

jbonofre · 2023-02-02T13:08:50Z

I'm having the same issue. Let me ping on #maven to get guidance and eventually this PR accepted. Thanks.

olamy · 2023-02-03T05:25:36Z

close then reopen to trigger ci build

olamy · 2023-02-03T07:34:02Z

some tests are failing. we would need a green build before merging.
Please first rebase from master then we will see what's happening.
Thanks

jbonofre · 2023-02-03T07:39:20Z

@olamy thanks, let me rebase, I will maybe create another PR if I can't reach the original PR author

olamy · 2023-02-03T07:45:45Z

I pushed the branch pr-516-rebase-master https://github.com/apache/maven-surefire/tree/pr-516-rebase-master

jbonofre · 2023-02-03T07:53:32Z

@olamy Awesome, thanks buddy :)

jbonofre · 2023-02-03T08:35:01Z

I confirm the tests are failing even after rebase, I'm checking.

andpab · 2023-02-03T10:01:55Z

@jbonofre Can you consider my suggestion from above?

#516 (comment)

That should solve the issues with failing tests. I think it's the better fix for this bug since it does not require changing any other test expectations and it's a more focused change. I can make an alternative pull request if you want me to. I would keep the additional tests from @chalmagr but use my fix.

jbonofre · 2023-02-03T10:03:52Z

@andpab absolutely, I'm on it. I will keep you posted asap. Thanks !

andpab · 2023-02-05T18:23:51Z

@jbonofre @olamy I filed a new pull request here: #608

It combines chalmagr's ITs from this PR with my fix proposal from the comment above. All rebased to master of course.

All tests should be successful there.

jbonofre · 2023-02-05T18:53:11Z

Thanks I gonna take a look. I was about to create a new one including your comment. If your PR is ok, I will with @olamy to propose a new surefire release to vote. Thanks again! I will work on your PR tonight my time.

chalmagr changed the title ~~Test Reports Inconsistencies with Parameterized and junit4~~ [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4 Apr 8, 2022

chalmagr force-pushed the master branch from 1790ff1 to 18beabc Compare April 8, 2022 17:57

Tibor17 reviewed Apr 24, 2022

View reviewed changes

Tibor17 requested changes Apr 24, 2022

View reviewed changes

chalmagr closed this Jun 6, 2022

chalmagr force-pushed the master branch from 5f97ffc to 157d2d9 Compare June 6, 2022 05:21

chalmagr mentioned this pull request Jun 6, 2022

[SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4 #544

Closed

8 tasks

chalmagr reopened this Jun 6, 2022

chalmagr requested a review from Tibor17 June 6, 2022 13:54

[SUREFIRE-2065] Use legacy name to avoid retried test to make tests s…

1486066

…how as flaky on junit4

chalmagr force-pushed the master branch from fd4c123 to 1486066 Compare June 6, 2022 17:36

andpab suggested changes Jan 3, 2023

View reviewed changes

olamy closed this Feb 3, 2023

olamy reopened this Feb 3, 2023

andpab mentioned this pull request Feb 5, 2023

[SUREFIRE-2065] Fix parameterized JUnit4 test reporting #608

Merged

8 tasks

chalmagr closed this Feb 9, 2023

Tibor17 mentioned this pull request Feb 27, 2023

Test Indexes and Reporters #616

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4 #516

[SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4 #516

chalmagr commented Apr 7, 2022 •

edited

Loading

Tibor17 commented Apr 8, 2022

Tibor17 commented Apr 8, 2022

chalmagr commented Apr 8, 2022 •

edited

Loading

Tibor17 commented Apr 8, 2022 •

edited

Loading

chalmagr commented Apr 12, 2022

chalmagr commented Apr 13, 2022

Tibor17 commented Apr 15, 2022

chalmagr commented Apr 21, 2022

Tibor17 Apr 24, 2022 •

edited

Loading

chalmagr Jun 4, 2022

br0nstein Jun 4, 2022 •

edited

Loading

chalmagr Jun 5, 2022

chalmagr Jun 6, 2022

Tibor17 left a comment

br0nstein commented May 29, 2022

chalmagr commented Jun 3, 2022

chalmagr commented Jun 6, 2022 •

edited

Loading

chalmagr commented Jun 17, 2022

chalmagr commented Jul 5, 2022

Tibor17 commented Jul 5, 2022

andpab Jan 3, 2023

br0nstein commented Jan 20, 2023

jbonofre commented Feb 2, 2023

olamy commented Feb 3, 2023

olamy commented Feb 3, 2023

jbonofre commented Feb 3, 2023

olamy commented Feb 3, 2023 •

edited

Loading

jbonofre commented Feb 3, 2023

jbonofre commented Feb 3, 2023

andpab commented Feb 3, 2023

jbonofre commented Feb 3, 2023

andpab commented Feb 5, 2023

jbonofre commented Feb 5, 2023

[SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4 #516

[SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4 #516

Conversation

chalmagr commented Apr 7, 2022 • edited Loading

Tibor17 commented Apr 8, 2022

Tibor17 commented Apr 8, 2022

chalmagr commented Apr 8, 2022 • edited Loading

Tibor17 commented Apr 8, 2022 • edited Loading

chalmagr commented Apr 12, 2022

chalmagr commented Apr 13, 2022

Tibor17 commented Apr 15, 2022

chalmagr commented Apr 21, 2022

Tibor17 Apr 24, 2022 • edited Loading

Choose a reason for hiding this comment

chalmagr Jun 4, 2022

Choose a reason for hiding this comment

br0nstein Jun 4, 2022 • edited Loading

Choose a reason for hiding this comment

chalmagr Jun 5, 2022

Choose a reason for hiding this comment

chalmagr Jun 6, 2022

Choose a reason for hiding this comment

Tibor17 left a comment

Choose a reason for hiding this comment

br0nstein commented May 29, 2022

chalmagr commented Jun 3, 2022

chalmagr commented Jun 6, 2022 • edited Loading

chalmagr commented Jun 17, 2022

chalmagr commented Jul 5, 2022

Tibor17 commented Jul 5, 2022

andpab Jan 3, 2023

Choose a reason for hiding this comment

br0nstein commented Jan 20, 2023

jbonofre commented Feb 2, 2023

olamy commented Feb 3, 2023

olamy commented Feb 3, 2023

jbonofre commented Feb 3, 2023

olamy commented Feb 3, 2023 • edited Loading

jbonofre commented Feb 3, 2023

jbonofre commented Feb 3, 2023

andpab commented Feb 3, 2023

jbonofre commented Feb 3, 2023

andpab commented Feb 5, 2023

jbonofre commented Feb 5, 2023

chalmagr commented Apr 7, 2022 •

edited

Loading

chalmagr commented Apr 8, 2022 •

edited

Loading

Tibor17 commented Apr 8, 2022 •

edited

Loading

Tibor17 Apr 24, 2022 •

edited

Loading

br0nstein Jun 4, 2022 •

edited

Loading

chalmagr commented Jun 6, 2022 •

edited

Loading

olamy commented Feb 3, 2023 •

edited

Loading