[GitHub] [maven-surefire] Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5
Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5 URL: https://github.com/apache/maven-surefire/pull/245#issuecomment-548968926 @Col-E I think they call it "legacy" in junit5 becuse it's not related to legacy version of JUnit5 but the `description` of methods might be similar to the JUnit4. See the JavaDoc. But here I also did not see the returned value. It would be first interesting to debug it, put the breakpoint and see the string. Feel free to show the string how it looks like before we make a real change. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [maven-surefire] Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5
Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5 URL: https://github.com/apache/maven-surefire/pull/245#issuecomment-548970053 @Col-E Regarding the change in `String[2]` we talked about before, we have to be careful and run the integration tests for JUnit5. This means `*Platform*IT` and see if we broke something. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [maven-surefire] Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5
Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5 URL: https://github.com/apache/maven-surefire/pull/245#issuecomment-548968926 @Col-E I think they call it "legacy" in junit5 becuse it's not related to legacy version of JUnit5 but the `description` of methods similar to the JUnit4. See the JavaDoc. But here I also did not see the returned value. It would be first interesting to debug it, put the breakpoint and see the string. Feel free to show he string how it looks like before we make a real change. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [maven-surefire] Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5
Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5 URL: https://github.com/apache/maven-surefire/pull/245#issuecomment-548968926 @Col-E I think they call it "legacy" in junit5 becuse it's not related to legacy version of JUnit5 but the `description` of methods similar to the JUnit4. See the JavaDoc. But here I also did not see the returned value. It would be first interesting to debug it, put the breakpoint and see the string. Feel free to show the string how it looks like before we make a real change. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [maven-surefire] Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5
Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5 URL: https://github.com/apache/maven-surefire/pull/245#issuecomment-548942695 @Col-E It is fix for this PR. It is new code. It was not before, right? So we have to report issue and make new PR the same way as we did it before. And then the next issue would go with ParmeterizedTest only and not rerun saying that the Parameterize test should have different name for every combination of parameters. If we use `getLegacyReportingName()` instead of pure method name in the String[2] then integration test will pass. I mean this test from @Seijan was not IT. It was unit test but we need to have real IT and we have to check the XML report as well as the number of calls of the same method and we do not have to employ the rerun because it was fixed previously by you. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [maven-surefire] Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5
Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5 URL: https://github.com/apache/maven-surefire/pull/245#issuecomment-548782829 @Col-E @Seijan @marcphilipp What would be the best text representation of each run of param.test? Although the code has only one Java method, we should represent each combination of parameters as if it was a separate method. So we know that the class name is `pkg.MyTest`, method name is called `test` and the parameter is `Hello [1]`. And my question is what is the best `description` of the method in the report. What a string. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [maven-surefire] Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5
Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun
failing tests for JUnit5
URL: https://github.com/apache/maven-surefire/pull/245#issuecomment-548552528
@Seijan
@marcphilipp
I have a trivial test and I have several observations with ParameterizedTest.
```
@ParameterizedTest
@ValueSource(strings = { "Hello" })
void test(String world) {
if (i++ == 0) {
throw new RuntimeException();
}
}
```
This `discoveryRequest` does not find our test to run which is this line in
our code:
`launcher.execute( discoveryRequest, adapter )`
It looks like this in debugger. Does it have good values?
The ID of the method run is this:
`[engine:junit-jupiter]/[class:pkg.ParamTest]/[test-template:test(java.lang.String)]/[test-template-invocation:#1]`
Should we filter the method in the request like we do it in JUnit4 and use
`test[1]` instead of `test`?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-surefire] Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5
Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun
failing tests for JUnit5
URL: https://github.com/apache/maven-surefire/pull/245#issuecomment-548552528
@Seijan
@marcphilipp
I have a trivial test and I have several observations with ParameterizedTest.
```
@ParameterizedTest
@ValueSource(strings = { "Hello" })
void test(String world) {
if (i++ == 0) {
throw new RuntimeException();
}
}
```
This `discoveryRequest` does not find our test to run which is this line in
our code:
`launcher.execute( discoveryRequest, adapter )`
It looks like this in debugger. Does it have good values?
The ID of the method is this:
`[engine:junit-jupiter]/[class:pkg.ParamTest]/[test-template:test(java.lang.String)]/[test-template-invocation:#1]`
Should we filter the method in the request like we do it in JUnit4 and use
`test[1]` instead of `test`?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-surefire] Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5
Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun
failing tests for JUnit5
URL: https://github.com/apache/maven-surefire/pull/245#issuecomment-548552528
@Seijan
@marcphilipp
I have a trivial test and I have several observations with ParameterizedTest.
```
@ParameterizedTest
@ValueSource(strings = { "Hello" })
void test(String world) {
if (i++ == 0) {
throw new RuntimeException();
}
}
```
This `discoveryRequest` does not find our test to run which is this line in
our code:
`launcher.execute( discoveryRequest, adapter )`
It looks like this in debugger. Does it have good values?
The ID of the method is this:
`[engine:junit-jupiter]/[class:pkg.ParamTest]/[test-template:test(java.lang.String)]/[test-template-invocation:#1]`
Should we filter the method in the request like we do it in JUnit4 and use
filter the test method `test[1]` instead of `test`?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-surefire] Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5
Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5 URL: https://github.com/apache/maven-surefire/pull/245#issuecomment-543712421 @Col-E I think I understand the problem. We consider the `adapter` been `stateless` in every loop. In reality we modify the collection of failures from the first set of failure and the adapter is `stateful`. https://github.com/apache/maven-surefire/pull/245/commits/fe17751602f597d7b694ec8a65fc389aee0bac25#diff-b3a2904e92be87f11f2622fbfd122e31R152 So my proposal is to grap the collection of failures and make a copy, and then iterate over the elements. I hope I got it right. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [maven-surefire] Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5
Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5 URL: https://github.com/apache/maven-surefire/pull/245#issuecomment-537677972 @Col-E sorry that I have so many requirements but we have 527 new lines and this may worse the code coverage. Would you please add unit tests in `JUnitPlatformProviderTest` and cover new lines in `RunListenerAdapter` and `JUnitPlatformProvider`? In `JUnitPlatformProviderTest` we have tests which execute realistic JUnit5 executor so we do not need any mock. Thx 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [maven-surefire] Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5
Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5 URL: https://github.com/apache/maven-surefire/pull/245#issuecomment-537478286 @Col-E > then this should be resolved? yes, this should be the trick. Thx 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [maven-surefire] Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5
Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5 URL: https://github.com/apache/maven-surefire/pull/245#issuecomment-537425160 In the logs I saw this test project `junit-platform-rerun-failing-tests` but you have to use the assumption in every IT you added in this PR due to Surefire is compiled with Java 7 and JRE is between 7 - 13. This assumption skips the execution for JRE 7. Notice that JUnit5 libraries are compiled with Java 8. I saw the ITs in the commit https://github.com/apache/maven-surefire/pull/245/commits/2558e288edfa92171f9ad44645b10c9a0b071a5f Did you introduce another IT in the next 6 commits too? Thx 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [maven-surefire] Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5
Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun
failing tests for JUnit5
URL: https://github.com/apache/maven-surefire/pull/245#issuecomment-536989553
@jon-bell
@Col-E
Sorry for the delay. Let's continue, guys!
So I have compared these two methods, namely:
`private String[] toClassMethodName( TestIdentifier testIdentifier )`
`public String[] toClassMethodNameWithoutPlan( TestIdentifier testIdentifier
)`
The solution is very simple: `add a new boolean parameter in the old method
"toClassMethodName"`.
The above methods are cca 90% similar and that breaks the reusability.
Our problem is only this diff:
```
(old toClassMethodName)
String[] source = testPlan.getParent( testIdentifier )
.map( this::toClassMethodName )
.map( s -> new String[] { s[0], s[1] } )
.orElse( new String[] { realClassName, realClassName } );
```
with
```
(new method toClassMethodNameWithoutPlan)
String[] source = new String[] {realClassName, realClassName};
```
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-surefire] Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5
Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun
failing tests for JUnit5
URL: https://github.com/apache/maven-surefire/pull/245#issuecomment-536994438
One can see that the method must alter the behavior . In practice we do it
easily the way that we have algorithm in one method
`toClassMethodName(TestIdentifier, boolean)` because of 90% reusability called
in the accessor methods:
```
private toClassMethodName(TestIdentifier )
{
return toClassMethodName( testIdentifier, false );
}
```
and another public method:
```
toClassMethodNameWithoutPlan( testIdentifier )
{
return toClassMethodName( testIdentifier, true );
}
```
In our case `toClassMethodName(TestIdentifier, boolean)` is the old method
with new parameter - boolean. Pls check if the difference between those two
methods is what I used in the previous comment because I was too fast to
compare in detail. Thx
true: real class/method names without plan
false: with display names if possible
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-surefire] Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5
Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun
failing tests for JUnit5
URL: https://github.com/apache/maven-surefire/pull/245#issuecomment-536994438
One can see that the method must alter the behavior . In practice we do it
easily the way that we have algorithm in one method because of 90% reusability
and we introduce a private method `toClassMethodName(TestIdentifier, boolean)`
which is called in the accessor methods:
```
private toClassMethodName(TestIdentifier )
{
return toClassMethodName( testIdentifier, false );
}
```
and another public method:
```
toClassMethodNameWithoutPlan( testIdentifier )
{
return toClassMethodName( testIdentifier, true );
}
```
true: real class/method names without plan
false: with display names if possible
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-surefire] Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5
Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun
failing tests for JUnit5
URL: https://github.com/apache/maven-surefire/pull/245#issuecomment-536989553
@jon-bell
@Col-E
Sorry for the delay. Let's continue, guys!
So I have compare these two methods, namely:
`private String[] toClassMethodName( TestIdentifier testIdentifier )`
`public String[] toClassMethodNameWithoutPlan( TestIdentifier testIdentifier
)`
The solution is very simple: `add a new boolean parameter in the old method
"toClassMethodName"`.
The above methods are cca 90% similar and that breaks the reusability.
Our problem is only this diff:
```
(old toClassMethodName)
String[] source = testPlan.getParent( testIdentifier )
.map( this::toClassMethodName )
.map( s -> new String[] { s[0], s[1] } )
.orElse( new String[] { realClassName, realClassName } );
```
with
```
(new method toClassMethodNameWithoutPlan)
String[] source = new String[] {realClassName, realClassName};
```
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-surefire] Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5
Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun
failing tests for JUnit5
URL: https://github.com/apache/maven-surefire/pull/245#issuecomment-536989553
@jon-bell
@Col-E
Sorry for the delay. Let's continue, guys!
So I have compare these two methods, namely:
`private String[] toClassMethodName( TestIdentifier testIdentifier )`
`public String[] toClassMethodNameWithoutPlan( TestIdentifier testIdentifier
)`
(btw, hide the information/methoid as much as possible, thus use `private`
at first).
The solution is very simple: `add a new boolean parameter in the old method
"toClassMethodName"`.
The above methods are cca 90% similar and that breaks the reusability.
Our problem is only this diff:
```
(old toClassMethodName)
String[] source = testPlan.getParent( testIdentifier )
.map( this::toClassMethodName )
.map( s -> new String[] { s[0], s[1] } )
.orElse( new String[] { realClassName, realClassName } );
```
(new method toClassMethodNameWithoutPlan)
```
String[] source = new String[] {realClassName, realClassName};
```
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-surefire] Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5
Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun
failing tests for JUnit5
URL: https://github.com/apache/maven-surefire/pull/245#issuecomment-536989553
@jon-bell
@Col-E
Sorry for the delay. Let's continue, guys!
So I have compare these two methods, namely:
`private String[] toClassMethodName( TestIdentifier testIdentifier )`
`public String[] toClassMethodNameWithoutPlan( TestIdentifier testIdentifier
)`
(btw, hide the information/method as much as possible, thus use `private` at
first).
The solution is very simple: `add a new boolean parameter in the old method
"toClassMethodName"`.
The above methods are cca 90% similar and that breaks the reusability.
Our problem is only this diff:
```
(old toClassMethodName)
String[] source = testPlan.getParent( testIdentifier )
.map( this::toClassMethodName )
.map( s -> new String[] { s[0], s[1] } )
.orElse( new String[] { realClassName, realClassName } );
```
with
```
(new method toClassMethodNameWithoutPlan)
String[] source = new String[] {realClassName, realClassName};
```
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-surefire] Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5
Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5 URL: https://github.com/apache/maven-surefire/pull/245#issuecomment-523143928 > (JUnit 4+ providers and JUnit 5+ providers since 3.0.0-M4) > Is the remark for clarifying support as of 3.0.0-M4? yes, for both, that's ok and enough as you have mentioned. The reason is that the users sometimes are not aware that it is not the version they use in the company but the latest with a praticular feature. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [maven-surefire] Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5
Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5 URL: https://github.com/apache/maven-surefire/pull/245#issuecomment-523138627 @Col-E Additionally, we have to update the chart `maven-surefire-plugin/src/site/apt/examples/featurematrix.apt.vm` and update the line `re-run count` and type `Y` with a new remark `*3`. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [maven-surefire] Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5
Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5 URL: https://github.com/apache/maven-surefire/pull/245#issuecomment-522944865 @jon-bell Generally, it looks good. I have left only few comments. Pls open the MOJO classes. You will see `rerunFailingTestsCount`. Extend the JavaDoc and add a new support for JUnit5 with all known limitations if any exist. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [maven-surefire] Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5
Tibor17 edited a comment on issue #245: Surefire-1584: Add option to rerun failing tests for JUnit5 URL: https://github.com/apache/maven-surefire/pull/245#issuecomment-522658458 @jon-bell I don't like this huge number of commits and merge commits. Can you make only one commit on the top of master HEAD? There are two authors. Did you pull some commits also from other pull requests? Important: pls run the build `mvn install -P run-its` and setup your IDEA or Eclipse to the code style (there are config files for Ides) https://maven.apache.org/developers/conventions/code.html Do not change white spaces and code style against origin/master. Only add/remove/modify code. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
