You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "chalmagr (via GitHub)" <gi...@apache.org> on 2023/02/03 05:25:50 UTC
[GitHub] [maven-surefire] chalmagr opened a new pull request, #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
chalmagr opened a new pull request, #516:
URL: https://github.com/apache/maven-surefire/pull/516
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:
- [x] Make sure there is a [JIRA issue](https://issues.apache.org/jira/projects/SUREFIRE/issues/SUREFIRE-2065) 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.
- [x] Each commit in the pull request should have a meaningful subject line and body.
- [x] 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.
- [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [x] Run `mvn clean install` to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
- [x] 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](https://www.apache.org/licenses/icla.pdf) 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](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [x] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
- [ ] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
--
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
[GitHub] [maven-surefire] olamy closed pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
Posted by "olamy (via GitHub)" <gi...@apache.org>.
olamy closed pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
URL: https://github.com/apache/maven-surefire/pull/516
--
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
[GitHub] [maven-surefire] Tibor17 commented on a diff in pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on code in PR #516:
URL: https://github.com/apache/maven-surefire/pull/516#discussion_r857087085
##########
maven-surefire-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(),
Review Comment:
@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.
--
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
[GitHub] [maven-surefire] br0nstein commented on pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
Posted by "br0nstein (via GitHub)" <gi...@apache.org>.
br0nstein commented on PR #516:
URL: https://github.com/apache/maven-surefire/pull/516#issuecomment-1398801924
@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!
--
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
[GitHub] [maven-surefire] andpab commented on pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
Posted by "andpab (via GitHub)" <gi...@apache.org>.
andpab commented on PR #516:
URL: https://github.com/apache/maven-surefire/pull/516#issuecomment-1418207507
@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.
--
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
[GitHub] [maven-surefire] jbonofre commented on pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #516:
URL: https://github.com/apache/maven-surefire/pull/516#issuecomment-1415240538
@olamy thanks, let me rebase, I will maybe create another PR if I can't reach the original PR author
--
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
[GitHub] [maven-surefire] Tibor17 commented on pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on PR #516:
URL: https://github.com/apache/maven-surefire/pull/516#issuecomment-1093484047
What happens if you run the command `$ mvn clean install`?
We have Mac in Github Actions and it does not any problems like this.
--
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
[GitHub] [maven-surefire] chalmagr commented on pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
Posted by GitBox <gi...@apache.org>.
chalmagr commented on PR #516:
URL: https://github.com/apache/maven-surefire/pull/516#issuecomment-1105256296
@Tibor17 the tests were all good within that hour. Please let me know any actions required to move forward on this one.
--
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
[GitHub] [maven-surefire] Tibor17 commented on pull request #516: Test Reports Inconsistencies with Parameterized and junit4
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on PR #516:
URL: https://github.com/apache/maven-surefire/pull/516#issuecomment-1092929095
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.
--
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
[GitHub] [maven-surefire] andpab commented on pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
Posted by "andpab (via GitHub)" <gi...@apache.org>.
andpab commented on PR #516:
URL: https://github.com/apache/maven-surefire/pull/516#issuecomment-1415556550
@jbonofre Can you consider my suggestion from above?
https://github.com/apache/maven-surefire/pull/516#discussion_r1060357685
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.
--
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
[GitHub] [maven-surefire] andpab commented on a diff in pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
Posted by 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
[GitHub] [maven-surefire] chalmagr commented on a diff in pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
Posted by GitBox <gi...@apache.org>.
chalmagr commented on code in PR #516:
URL: https://github.com/apache/maven-surefire/pull/516#discussion_r890146141
##########
maven-surefire-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(),
Review Comment:
I've updated the PR with just changes to the adapter and additional ITs for this jira
--
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
[GitHub] [maven-surefire] Tibor17 commented on a diff in pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on code in PR #516:
URL: https://github.com/apache/maven-surefire/pull/516#discussion_r857087085
##########
maven-surefire-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(),
Review Comment:
@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.
--
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
[GitHub] [maven-surefire] jbonofre commented on pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #516:
URL: https://github.com/apache/maven-surefire/pull/516#issuecomment-1415350303
I confirm the tests are failing even after rebase, I'm checking.
--
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
[GitHub] [maven-surefire] chalmagr commented on a diff in pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
Posted by GitBox <gi...@apache.org>.
chalmagr commented on code in PR #516:
URL: https://github.com/apache/maven-surefire/pull/516#discussion_r889652117
##########
maven-surefire-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(),
Review Comment:
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!
--
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
[GitHub] [maven-surefire] Tibor17 commented on pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on PR #516:
URL: https://github.com/apache/maven-surefire/pull/516#issuecomment-1099954619
@Tibor17 yes it normally takes over 1h. You may use `mvn clean install -P run-its` , that's enough.
--
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
[GitHub] [maven-surefire] chalmagr commented on pull request #516: Test Reports Inconsistencies with Parameterized and junit4
Posted by GitBox <gi...@apache.org>.
chalmagr commented on PR #516:
URL: https://github.com/apache/maven-surefire/pull/516#issuecomment-1093047652
> @chalmagr How can I reproduce the first issue with `ClassNotFoundException`?
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
```bash
$ 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"
```
```bash
$ 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...
--
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
[GitHub] [maven-surefire] chalmagr commented on pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
Posted by GitBox <gi...@apache.org>.
chalmagr commented on PR #516:
URL: https://github.com/apache/maven-surefire/pull/516#issuecomment-1159308557
@Tibor17 @br0nstein ping
--
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
[GitHub] [maven-surefire] chalmagr commented on pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
Posted by GitBox <gi...@apache.org>.
chalmagr commented on PR #516:
URL: https://github.com/apache/maven-surefire/pull/516#issuecomment-1097224464
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
--
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
[GitHub] [maven-surefire] jbonofre commented on pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #516:
URL: https://github.com/apache/maven-surefire/pull/516#issuecomment-1418231341
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.
--
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
[GitHub] [maven-surefire] olamy commented on pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
Posted by "olamy (via GitHub)" <gi...@apache.org>.
olamy commented on PR #516:
URL: https://github.com/apache/maven-surefire/pull/516#issuecomment-1415009310
close then reopen to trigger ci build
--
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
[GitHub] [maven-surefire] olamy commented on pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
Posted by "olamy (via GitHub)" <gi...@apache.org>.
olamy commented on PR #516:
URL: https://github.com/apache/maven-surefire/pull/516#issuecomment-1415252283
I pushed the branch pr-516-rebase-master
--
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
[GitHub] [maven-surefire] Tibor17 commented on pull request #516: Test Reports Inconsistencies with Parameterized and junit4
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on PR #516:
URL: https://github.com/apache/maven-surefire/pull/516#issuecomment-1092933166
@chalmagr
How can I reproduce the first issue with `ClassNotFoundException`?
--
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
[GitHub] [maven-surefire] chalmagr commented on pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
Posted by GitBox <gi...@apache.org>.
chalmagr commented on PR #516:
URL: https://github.com/apache/maven-surefire/pull/516#issuecomment-1097496230
@Tibor17 Run integration tests properly as well now. It usually takes ~1h to complete?
`install site site:stage -P reporting,run-its`
--
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
[GitHub] [maven-surefire] chalmagr closed pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
Posted by "chalmagr (via GitHub)" <gi...@apache.org>.
chalmagr closed pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
URL: https://github.com/apache/maven-surefire/pull/516
--
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
[GitHub] [maven-surefire] Tibor17 commented on a diff in pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on code in PR #516:
URL: https://github.com/apache/maven-surefire/pull/516#discussion_r857087085
##########
maven-surefire-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(),
Review Comment:
@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>]>(<class>)`. If this is wrong, then the problem is in provider and not in this class TSRL.
--
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
[GitHub] [maven-surefire] chalmagr commented on pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
Posted by GitBox <gi...@apache.org>.
chalmagr commented on PR #516:
URL: https://github.com/apache/maven-surefire/pull/516#issuecomment-1145594062
Hi - been unavailable for some time... will look into this shortly. Thanks!
--
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
[GitHub] [maven-surefire] Tibor17 commented on pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on PR #516:
URL: https://github.com/apache/maven-surefire/pull/516#issuecomment-1175171232
@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.
--
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
[GitHub] [maven-surefire] br0nstein commented on pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
Posted by GitBox <gi...@apache.org>.
br0nstein commented on PR #516:
URL: https://github.com/apache/maven-surefire/pull/516#issuecomment-1140517354
@chalmagr ping for update, thanks!
--
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
[GitHub] [maven-surefire] Tibor17 commented on a diff in pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on code in PR #516:
URL: https://github.com/apache/maven-surefire/pull/516#discussion_r857087085
##########
maven-surefire-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(),
Review Comment:
@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>]>(class)`. If this is wrong, then the problem is in provider and not in this class TSRL.
--
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
[GitHub] [maven-surefire] chalmagr commented on pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
Posted by GitBox <gi...@apache.org>.
chalmagr commented on PR #516:
URL: https://github.com/apache/maven-surefire/pull/516#issuecomment-1175088348
@Tibor17 are you still available to look into this? should someone else be pulled in?
--
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
[GitHub] [maven-surefire] olamy commented on pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
Posted by "olamy (via GitHub)" <gi...@apache.org>.
olamy commented on PR #516:
URL: https://github.com/apache/maven-surefire/pull/516#issuecomment-1415230939
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
--
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
[GitHub] [maven-surefire] jbonofre commented on pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #516:
URL: https://github.com/apache/maven-surefire/pull/516#issuecomment-1413719080
I'm having the same issue. Let me ping on #maven to get guidance and eventually this PR accepted. Thanks.
--
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
[GitHub] [maven-surefire] jbonofre commented on pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #516:
URL: https://github.com/apache/maven-surefire/pull/516#issuecomment-1415266713
@olamy Awesome, thanks buddy :)
--
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
[GitHub] [maven-surefire] chalmagr closed pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
Posted by GitBox <gi...@apache.org>.
chalmagr closed pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
URL: https://github.com/apache/maven-surefire/pull/516
--
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
[GitHub] [maven-surefire] chalmagr commented on a diff in pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
Posted by GitBox <gi...@apache.org>.
chalmagr commented on code in PR #516:
URL: https://github.com/apache/maven-surefire/pull/516#discussion_r889474392
##########
maven-surefire-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(),
Review Comment:
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
```java
...
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
```java
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
```java
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;
}
```
--
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
[GitHub] [maven-surefire] br0nstein commented on a diff in pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
Posted by GitBox <gi...@apache.org>.
br0nstein commented on code in PR #516:
URL: https://github.com/apache/maven-surefire/pull/516#discussion_r889499114
##########
maven-surefire-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(),
Review Comment:
Make sure you understand b2cce57a5e1dbd9a3a9e79c2eadd866e289e6f7f and especially e8f65b99aaaa7c04e1ad3770309cd626d3fe5ab0. 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](https://github.com/junit-team/junit5/blob/e69243ae0f3c68f07d075ac3901c98c375d040f0/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/TestTemplateInvocationTestDescriptor.java#L61) to methodName(ParameterDataType1, ..)[testInvocationNum] (e.g. "methodName(Long)[0]") and the [display name ](https://github.com/junit-team/junit5/blob/e69243ae0f3c68f07d075ac3901c98c375d040f0/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/TestTemplateInvocationTestDescriptor.java#L48)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 t
hey 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.
--
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
[GitHub] [maven-surefire] br0nstein commented on a diff in pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
Posted by GitBox <gi...@apache.org>.
br0nstein commented on code in PR #516:
URL: https://github.com/apache/maven-surefire/pull/516#discussion_r889499114
##########
maven-surefire-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(),
Review Comment:
Make sure you understand b2cce57a5e1dbd9a3a9e79c2eadd866e289e6f7f and especially e8f65b99aaaa7c04e1ad3770309cd626d3fe5ab0. I believe the fix for this issue should be in RunListenerAdapter. 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.
--
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
[GitHub] [maven-surefire] chalmagr commented on pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
Posted by GitBox <gi...@apache.org>.
chalmagr commented on PR #516:
URL: https://github.com/apache/maven-surefire/pull/516#issuecomment-1147062010
Please look at [Pull request 544](https://github.com/apache/maven-surefire/pull/544) since this one was closed after pushing master branch too early :) - will try to re-use now that I see the 'Reopen and comment' ...
--
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
[GitHub] [maven-surefire] jbonofre commented on pull request #516: [SUREFIRE-2065] Test Reports Inconsistencies with Parameterized and junit4
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #516:
URL: https://github.com/apache/maven-surefire/pull/516#issuecomment-1415562783
@andpab absolutely, I'm on it. I will keep you posted asap. Thanks !
--
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