[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true

2022-03-09 Thread GitBox 


Tibor17 commented on a change in pull request #478:
URL: https://github.com/apache/maven-surefire/pull/478#discussion_r822751341



##
File path: 
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireHelper.java
##
@@ -154,7 +155,11 @@ public static void reportExecution( 
SurefireReportParameters reportParameters, R
 return;
 }
 
-if ( reportParameters.isTestFailureIgnore() )
+if ( firstForkException instanceof SurefireBooterForkException )
+{
+throwException( reportParameters, result, firstForkException );

Review comment:
   @olamy 
   @slawekjaranowski 
   It is extremely important due to you want to interrupt the build with 
**ignoring the failure** but on the other side you let the build to say **BUILD 
FAILURE**. How come? You want to ignore it. Hopefully, we have another 
alternative **BUILD ERROR** which looks much more reasonable to the users and 
everyone I guess.
   
   Here matters what you do and how you do it. This is 50% of the behavior of 
`maven.test.failure.ignore` because yes you stopped the build, but the next 50% 
is how you present the information to the client.
   
   I will never use the same terminology in the text with "LOL" same way as 
Olivier openly does later in these comments, because this is disgusting. So 
what are we going to play, a game with ego, or we would patiently evaluate 
technical/business arguments?
   @hboutemy 
   Herve, pls participate as an independent moderator here. I expect from 
@olamy to be neutral and fully rational.




-- 
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-unsubscr...@maven.apache.org

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

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true

2022-03-07 Thread GitBox 


Tibor17 commented on a change in pull request #478:
URL: https://github.com/apache/maven-surefire/pull/478#discussion_r820982701



##
File path: 
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireHelper.java
##
@@ -154,7 +155,11 @@ public static void reportExecution( 
SurefireReportParameters reportParameters, R
 return;
 }
 
-if ( reportParameters.isTestFailureIgnore() )
+if ( firstForkException instanceof SurefireBooterForkException )
+{
+throwException( reportParameters, result, firstForkException );

Review comment:
   `throwException()` has two behaviors. You do not have possibility to 
switch between them. Here must be only one behavior, means `BUILD ERROR` and 
not `BUILD FAILURE`. I know that you saw my code proposal but you should think 
about it. This situation is the same situation with the existing situation  
`isFatal()` that we use nearby in this class. I think you want to have this 
change because it is an exception in the behavior of 
`-Dmaven.test.failure.ignore` which is okay if you ensure that `BUILD ERROR` 
happens while `SurefireBooterForkException` is caught in a combination with 
`maven.test.failure.ignore`.




-- 
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-unsubscr...@maven.apache.org

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

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true

2022-03-07 Thread GitBox 


Tibor17 commented on a change in pull request #478:
URL: https://github.com/apache/maven-surefire/pull/478#discussion_r820982701



##
File path: 
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireHelper.java
##
@@ -154,7 +155,11 @@ public static void reportExecution( 
SurefireReportParameters reportParameters, R
 return;
 }
 
-if ( reportParameters.isTestFailureIgnore() )
+if ( firstForkException instanceof SurefireBooterForkException )
+{
+throwException( reportParameters, result, firstForkException );

Review comment:
   `throwException()` has two behaviors. You do not have possibility to 
switch between them. Here must be only one behavior, means `BUILD ERROR` and 
not `BUILD FAILURE`. I know that you saw my code proposal but you should think 
about it. This situation is the same situation with the existing situation  
`isFatal()` that we use nearby in this class. I think you want to have this 
change because it is an exception in the behavior of 
`-Dmaven.test.failure.ignore` which is okay if you ensure that `BUILD ERROR` 
happens while `SurefireBooterForkException` is caught in a combination with 
activate `maven.test.failure.ignore`.




-- 
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-unsubscr...@maven.apache.org

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

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true

2022-03-07 Thread GitBox 


Tibor17 commented on a change in pull request #478:
URL: https://github.com/apache/maven-surefire/pull/478#discussion_r820982701



##
File path: 
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireHelper.java
##
@@ -154,7 +155,11 @@ public static void reportExecution( 
SurefireReportParameters reportParameters, R
 return;
 }
 
-if ( reportParameters.isTestFailureIgnore() )
+if ( firstForkException instanceof SurefireBooterForkException )
+{
+throwException( reportParameters, result, firstForkException );

Review comment:
   `throwException()` has two behaviors. You do not have possibility to 
switch between them. Here must be only one behavior, means `BUILD ERROR` and 
not `BUILD FAILURE`. I know that you saw my code proposal but you should think 
about it. This situation is the same situation with the existing situation  
`isFatal()` that we use nearby in this class. I think you want to have this 
change because it is an exception in the behavior of 
`-Dmaven.test.failure.ignore` which is okay if you ensure that `BUILD ERROR` 
happens with `SurefireBooterForkException` is caught.




-- 
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-unsubscr...@maven.apache.org

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

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true

2022-03-07 Thread GitBox 


Tibor17 commented on a change in pull request #478:
URL: https://github.com/apache/maven-surefire/pull/478#discussion_r820982701



##
File path: 
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireHelper.java
##
@@ -154,7 +155,11 @@ public static void reportExecution( 
SurefireReportParameters reportParameters, R
 return;
 }
 
-if ( reportParameters.isTestFailureIgnore() )
+if ( firstForkException instanceof SurefireBooterForkException )
+{
+throwException( reportParameters, result, firstForkException );

Review comment:
   `throwException()` has two behaviors. You do not have possibility to 
switch between them. Here must be only one behavior, means `BUILD ERROR` and 
not `BUILD FAILURE`. I know that you saw my code proposal but you should think 
about it. This situation is the same situation with the existing situation  
`isFatal()` that we use nearby in this class. I think you want to have this 
change because it is an exception in the behavior of 
`-Dmaven.test.failure.ignore` which is okay if you ensure that `BUILD ERROR` 
happens if `SurefireBooterForkException` is caught.




-- 
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-unsubscr...@maven.apache.org

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

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true

2022-03-07 Thread GitBox 


Tibor17 commented on a change in pull request #478:
URL: https://github.com/apache/maven-surefire/pull/478#discussion_r820982701



##
File path: 
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireHelper.java
##
@@ -154,7 +155,11 @@ public static void reportExecution( 
SurefireReportParameters reportParameters, R
 return;
 }
 
-if ( reportParameters.isTestFailureIgnore() )
+if ( firstForkException instanceof SurefireBooterForkException )
+{
+throwException( reportParameters, result, firstForkException );

Review comment:
   `throwException()` has two behaviors. You do not have possibility to 
switch between them. Here must be only one behavior, means `BUILD ERROR` and 
not `BUILD FAILURE`. I know that you saw my code proposal but you should think 
about it. This situation is the same situation with the existing situation  
`isFatal()` that we use nearby in this class. I think you want to have this 
change because it is an exception in the behavior of 
`-Dmaven.test.failure.ignore`.




-- 
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-unsubscr...@maven.apache.org

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

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true

2022-03-07 Thread GitBox 


Tibor17 commented on a change in pull request #478:
URL: https://github.com/apache/maven-surefire/pull/478#discussion_r820982701



##
File path: 
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireHelper.java
##
@@ -154,7 +155,11 @@ public static void reportExecution( 
SurefireReportParameters reportParameters, R
 return;
 }
 
-if ( reportParameters.isTestFailureIgnore() )
+if ( firstForkException instanceof SurefireBooterForkException )
+{
+throwException( reportParameters, result, firstForkException );

Review comment:
   throwException() has two behaviors. You do not have possibility to 
switch between them. Here must be only one behavior, means `BUILD ERROR` and 
not `BUILD FAILURE`. I know that you saw my code proposal but you should think 
about it. This situation is the same situation with the existing situation  
`isFatal()` that we use nearby in this class. I think you want to have this 
change because it is an exception in the behavior of 
`-Dmaven.test.failure.ignore`.




-- 
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-unsubscr...@maven.apache.org

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

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true

2022-03-01 Thread GitBox 


Tibor17 commented on a change in pull request #478:
URL: https://github.com/apache/maven-surefire/pull/478#discussion_r816873091



##
File path: 
surefire-its/src/test/resources/surefire-1426-ignore-fail-jvm-crash/pom.xml
##
@@ -0,0 +1,49 @@
+
+
+
+http://maven.apache.org/POM/4.0.0;
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance;
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 
http://maven.apache.org/xsd/maven-4.0.0.xsd;>
+  4.0.0
+
+  org.apache.maven.plugins.surefire
+  SUREFIRE-1426
+  1.0-SNAPSHOT
+  SUREFIRE-1426
+
+  
+1.8
+1.8
+  
+
+  
+
+  
+org.apache.maven.plugins
+maven-surefire-plugin
+${surefire.version}
+
+  -Dfile.encoding=UTF-8 -Duser.language=en -XFFOOOBEEER 
-Duser.region=US -showversion -Xmx6g -Xms2g -XX:+PrintGCDetails 

Review comment:
   There are multiple use cases for `maven.test.failure.ignore`.
   This test with `-XFFOOOBEEER` already exists in 
`Surefire735ForkFailWithRedirectConsoleOutputIT`. Of course it must not be 
modified. You can only add a new test method and add the system property 
`maven.test.failure.ignore`.
   And why `-Xmx6g -Xms2g`? No idea!




-- 
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-unsubscr...@maven.apache.org

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