You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2022/02/27 07:16:16 UTC
[GitHub] [maven-surefire] olamy opened a new pull request #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true, add an IT which show it looks to be fixed with 3.0.0-M6 but was failing with 3.0.0-M5
olamy opened a new pull request #478:
URL: https://github.com/apache/maven-surefire/pull/478
Signed-off-by: Olivier Lamy <ol...@apache.org>
Following this checklist to help us incorporate your
contribution quickly and easily:
- [ ] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/SUREFIRE) 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](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.
- [ ] 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] hboutemy commented on a change in pull request #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true
Posted by GitBox <gi...@apache.org>.
hboutemy commented on a change in pull request #478:
URL: https://github.com/apache/maven-surefire/pull/478#discussion_r823177122
##########
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:
it seems people don't agree on what `BUILD_FAILURE` and `BUILD_ERROR` mean or have as concrete result (which I personally don't know)
and I don't know why, instead of clarifying, the discussion becomes heated for no reason
--
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 a change in pull request #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true
Posted by GitBox <gi...@apache.org>.
olamy commented on a change in pull request #478:
URL: https://github.com/apache/maven-surefire/pull/478#discussion_r821112210
##########
File path: surefire-its/src/test/java/org/apache/maven/surefire/its/jiras/Surefire1426JvmCrashShouldNotBeIgnoredIT.java
##########
@@ -0,0 +1,50 @@
+package org.apache.maven.surefire.its.jiras;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import org.apache.maven.it.VerificationException;
+import org.apache.maven.surefire.its.fixture.SurefireJUnit4IntegrationTestCase;
+import org.junit.Test;
+
+import java.util.List;
+
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.is;
+
+/**
+ * Test https://issues.apache.org/jira/browse/SUREFIRE-1426
+ *
+ */
+public class Surefire1426JvmCrashShouldNotBeIgnoredIT
+ extends SurefireJUnit4IntegrationTestCase
+{
+ @Test
+ public void mavenShouldFail() throws VerificationException
+ {
+ List<String> logs = unpack( "surefire-1426-ignore-fail-jvm-crash" )
+ .maven()
+ .withFailure()
+ .debugLogging()
+ .executeTest()
+ .assertThatLogLine( containsString( "BUILD SUCCESS" ), is( 0 ) )
+ .verifyTextInLog( "BUILD FAILURE" )
+ .loadLogLines();
Review comment:
Lol
You guys really nitpicking...
--
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 a change in pull request #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true
Posted by GitBox <gi...@apache.org>.
olamy commented on a change in pull request #478:
URL: https://github.com/apache/maven-surefire/pull/478#discussion_r817166587
##########
File path: surefire-its/src/test/resources/surefire-1426-ignore-fail-jvm-crash/pom.xml
##########
@@ -0,0 +1,49 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ ~ Licensed to the Apache Software Foundation (ASF) under one
+ ~ or more contributor license agreements. See the NOTICE file
+ ~ distributed with this work for additional information
+ ~ regarding copyright ownership. The ASF licenses this file
+ ~ to you under the Apache License, Version 2.0 (the
+ ~ "License"); you may not use this file except in compliance
+ ~ with the License. You may obtain a copy of the License at
+ ~
+ ~ http://www.apache.org/licenses/LICENSE-2.0
+ ~
+ ~ Unless required by applicable law or agreed to in writing,
+ ~ software distributed under the License is distributed on an
+ ~ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ ~ KIND, either express or implied. See the License for the
+ ~ specific language governing permissions and limitations
+ ~ under the License.
+ -->
+
+<project xmlns="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">
+ <modelVersion>4.0.0</modelVersion>
+
+ <groupId>org.apache.maven.plugins.surefire</groupId>
+ <artifactId>SUREFIRE-1426</artifactId>
+ <version>1.0-SNAPSHOT</version>
+ <name>SUREFIRE-1426</name>
+
+ <properties>
+ <maven.compiler.source>1.8</maven.compiler.source>
+ <maven.compiler.target>1.8</maven.compiler.target>
+ </properties>
+
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-surefire-plugin</artifactId>
+ <version>${surefire.version}</version>
+ <configuration>
+ <argLine>-Dfile.encoding=UTF-8 -Duser.language=en -XFFOOOBEEER -Duser.region=US -showversion -Xmx6g -Xms2g -XX:+PrintGCDetails </argLine>
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`.
none with both `maven.test.failure.ignore` and jvm crash.
is there a real problem adding a new test and name it with the JIra ID?
> And why `-Xmx6g -Xms2g`? No idea!
why? the jvm doesn't even start because of a wrong parameter.
It could have been even `-Xmx128g -Xms64g` **THE JVM DO NOT START ON PURPOSE FOR THE 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.
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 change in pull request #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] slawekjaranowski commented on pull request #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on pull request #478:
URL: https://github.com/apache/maven-surefire/pull/478#issuecomment-1054735362
I'm afraid that jacoco not working in IT tests .... with current configuration ... maybe I'm wrong
--
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 #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true
Posted by GitBox <gi...@apache.org>.
olamy commented on pull request #478:
URL: https://github.com/apache/maven-surefire/pull/478#issuecomment-1054754133
> I'm afraid that jacoco not working in IT tests .... with current configuration ... maybe I'm wrong
I will have a look but yeah it looks not
--
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 #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true
Posted by GitBox <gi...@apache.org>.
olamy commented on pull request #478:
URL: https://github.com/apache/maven-surefire/pull/478#issuecomment-1054711444
> Agree ... surefire code is enough complexity, we should reduce complexity not increase.
>
> Next question how to test such code with coverage all decision branch?
there is an IT for this change.
And IT are supposed to be recording jacoco as well.
But strangly this doesn't look to go to it https://ci-maven.apache.org/job/Maven/job/maven-box/job/maven-surefire/view/change-requests/job/PR-478/lastBuild/jacoco/org.apache.maven.plugin.surefire/SurefireHelper/
that's weird.
I will review exactly how jacoco reports are merged or not.
--
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 change in pull request #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true
Posted by GitBox <gi...@apache.org>.
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 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ ~ Licensed to the Apache Software Foundation (ASF) under one
+ ~ or more contributor license agreements. See the NOTICE file
+ ~ distributed with this work for additional information
+ ~ regarding copyright ownership. The ASF licenses this file
+ ~ to you under the Apache License, Version 2.0 (the
+ ~ "License"); you may not use this file except in compliance
+ ~ with the License. You may obtain a copy of the License at
+ ~
+ ~ http://www.apache.org/licenses/LICENSE-2.0
+ ~
+ ~ Unless required by applicable law or agreed to in writing,
+ ~ software distributed under the License is distributed on an
+ ~ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ ~ KIND, either express or implied. See the License for the
+ ~ specific language governing permissions and limitations
+ ~ under the License.
+ -->
+
+<project xmlns="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">
+ <modelVersion>4.0.0</modelVersion>
+
+ <groupId>org.apache.maven.plugins.surefire</groupId>
+ <artifactId>SUREFIRE-1426</artifactId>
+ <version>1.0-SNAPSHOT</version>
+ <name>SUREFIRE-1426</name>
+
+ <properties>
+ <maven.compiler.source>1.8</maven.compiler.source>
+ <maven.compiler.target>1.8</maven.compiler.target>
+ </properties>
+
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-surefire-plugin</artifactId>
+ <version>${surefire.version}</version>
+ <configuration>
+ <argLine>-Dfile.encoding=UTF-8 -Duser.language=en -XFFOOOBEEER -Duser.region=US -showversion -Xmx6g -Xms2g -XX:+PrintGCDetails </argLine>
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@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
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] slawekjaranowski commented on a change in pull request #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on a change in pull request #478:
URL: https://github.com/apache/maven-surefire/pull/478#discussion_r821094322
##########
File path: surefire-its/src/test/java/org/apache/maven/surefire/its/jiras/Surefire1426JvmCrashShouldNotBeIgnoredIT.java
##########
@@ -0,0 +1,50 @@
+package org.apache.maven.surefire.its.jiras;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import org.apache.maven.it.VerificationException;
+import org.apache.maven.surefire.its.fixture.SurefireJUnit4IntegrationTestCase;
+import org.junit.Test;
+
+import java.util.List;
+
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.is;
+
+/**
+ * Test https://issues.apache.org/jira/browse/SUREFIRE-1426
+ *
+ */
+public class Surefire1426JvmCrashShouldNotBeIgnoredIT
+ extends SurefireJUnit4IntegrationTestCase
+{
+ @Test
+ public void mavenShouldFail() throws VerificationException
+ {
+ List<String> logs = unpack( "surefire-1426-ignore-fail-jvm-crash" )
+ .maven()
+ .withFailure()
+ .debugLogging()
+ .executeTest()
+ .assertThatLogLine( containsString( "BUILD SUCCESS" ), is( 0 ) )
+ .verifyTextInLog( "BUILD FAILURE" )
+ .loadLogLines();
Review comment:
Now variable `logs` and call `loadLogLines` are also not needed 😄
--
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 #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #478:
URL: https://github.com/apache/maven-surefire/pull/478#issuecomment-1054539564
You added one more branching and you call `throwException`.
Why?
It has to end up with the only one exception - `BUILD ERROR` with no other decision making.
So it is like this:
```
if ( isBooterError( firstForkException ) )
{
throwBuildError( reportParameters, result, firstForkException );
}
else if ( reportParameters.isTestFailureIgnore() )
{
log.error( createErrorMessage( reportParameters, result, firstForkException ) );
}
else
{
throwException( reportParameters, result, firstForkException ); // here is further decision making only!
}
private static void throwBuildFailure( SurefireReportParameters reportParameters, RunResult result, Exception firstForkException )
{
throw new MojoFailureException( createErrorMessage( reportParameters, result, firstForkException ), firstForkException );
}
private static void throwBuildError( SurefireReportParameters reportParameters, RunResult result, Exception firstForkException )
{
throw new MojoExecutionException( createErrorMessage( reportParameters, result, firstForkException ), firstForkException );
}
private static void throwException( SurefireReportParameters reportParameters, RunResult result,
Exception firstForkException )
throws MojoFailureException, MojoExecutionException
{
if ( isFatal( firstForkException ) || result.isInternalError() )
{
throwBuildError( createErrorMessage( reportParameters, result, firstForkException, firstForkException );
}
else
{
throwBuildFailure( createErrorMessage( reportParameters, result, firstForkException, firstForkException );
}
}
private static boolean isBooterError( Exception firstForkException )
{
return firstForkException instanceof SurefireBooterForkException;
}
```
--
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 a change in pull request #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true
Posted by GitBox <gi...@apache.org>.
olamy commented on a change in pull request #478:
URL: https://github.com/apache/maven-surefire/pull/478#discussion_r821246153
##########
File path: surefire-its/src/test/java/org/apache/maven/surefire/its/jiras/Surefire1426JvmCrashShouldNotBeIgnoredIT.java
##########
@@ -0,0 +1,50 @@
+package org.apache.maven.surefire.its.jiras;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import org.apache.maven.it.VerificationException;
+import org.apache.maven.surefire.its.fixture.SurefireJUnit4IntegrationTestCase;
+import org.junit.Test;
+
+import java.util.List;
+
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.is;
+
+/**
+ * Test https://issues.apache.org/jira/browse/SUREFIRE-1426
+ *
+ */
+public class Surefire1426JvmCrashShouldNotBeIgnoredIT
+ extends SurefireJUnit4IntegrationTestCase
+{
+ @Test
+ public void mavenShouldFail() throws VerificationException
+ {
+ List<String> logs = unpack( "surefire-1426-ignore-fail-jvm-crash" )
+ .maven()
+ .withFailure()
+ .debugLogging()
+ .executeTest()
+ .assertThatLogLine( containsString( "BUILD SUCCESS" ), is( 0 ) )
+ .verifyTextInLog( "BUILD FAILURE" )
+ .loadLogLines();
Review comment:
done
--
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] slawekjaranowski commented on a change in pull request #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on a change in pull request #478:
URL: https://github.com/apache/maven-surefire/pull/478#discussion_r821144864
##########
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:
I feel that it is not important if build stop with error or failure ... simply should be break.
--
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 a change in pull request #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true
Posted by GitBox <gi...@apache.org>.
olamy commented on a change in pull request #478:
URL: https://github.com/apache/maven-surefire/pull/478#discussion_r821089042
##########
File path: surefire-its/src/test/java/org/apache/maven/surefire/its/jiras/Surefire1426JvmCrashShouldNotBeIgnoredIT.java
##########
@@ -0,0 +1,51 @@
+package org.apache.maven.surefire.its.jiras;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import org.apache.maven.it.VerificationException;
+import org.apache.maven.surefire.its.fixture.SurefireJUnit4IntegrationTestCase;
+import org.junit.Test;
+
+import java.util.List;
+
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.is;
+
+/**
+ * Test https://issues.apache.org/jira/browse/SUREFIRE-1426
+ *
+ */
+public class Surefire1426JvmCrashShouldNotBeIgnoredIT
+ extends SurefireJUnit4IntegrationTestCase
+{
+ @Test
+ public void mavenShouldFail() throws VerificationException
+ {
+ List<String> logs = unpack( "surefire-1426-ignore-fail-jvm-crash" )
+ .maven()
+ .withFailure()
+ .debugLogging()
+ .executeTest()
+ .assertThatLogLine( containsString( "BUILD SUCCESS" ), is( 0 ) )
+ .verifyTextInLog( "BUILD FAILURE" )
+ .loadLogLines();
+ logs.stream().forEach( line -> System.out.println( line ) );
Review comment:
oops sorry it was a temporary debug
--
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 #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true
Posted by GitBox <gi...@apache.org>.
olamy commented on pull request #478:
URL: https://github.com/apache/maven-surefire/pull/478#issuecomment-1064657277
> @Tibor17 I hope I answered all your concerns and we can now merge this?
@Tibor17 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] Tibor17 commented on a change in pull request #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] olamy commented on a change in pull request #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true
Posted by GitBox <gi...@apache.org>.
olamy commented on a change in pull request #478:
URL: https://github.com/apache/maven-surefire/pull/478#discussion_r821314009
##########
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:
both exception generate `BUILD FAILURE` at the end (probably a maven core bug)
but anyway we need a build stop no need of a distinction really.
--
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 #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true
Posted by GitBox <gi...@apache.org>.
olamy commented on pull request #478:
URL: https://github.com/apache/maven-surefire/pull/478#issuecomment-1054680670
> You added one more branching and you call `throwException`.
>
> Why?
>
> It has to end up with the only one exception - `BUILD ERROR` with no other decision making.
>
> So it is like this:
>
> ```
> if ( isBooterError( firstForkException ) )
> {
> throwBuildError( reportParameters, result, firstForkException );
> }
> else if ( reportParameters.isTestFailureIgnore() )
> {
> log.error( createErrorMessage( reportParameters, result, firstForkException ) );
> }
> else
> {
> throwException( reportParameters, result, firstForkException ); // here is further decision making only!
> }
>
> private static void throwBuildFailure( SurefireReportParameters reportParameters, RunResult result, Exception firstForkException )
> {
> throw new MojoFailureException( createErrorMessage( reportParameters, result, firstForkException ), firstForkException );
> }
>
> private static void throwBuildError( SurefireReportParameters reportParameters, RunResult result, Exception firstForkException )
> {
> throw new MojoExecutionException( createErrorMessage( reportParameters, result, firstForkException ), firstForkException );
> }
>
> private static void throwException( SurefireReportParameters reportParameters, RunResult result,
> Exception firstForkException )
> throws MojoFailureException, MojoExecutionException
> {
> if ( isFatal( firstForkException ) || result.isInternalError() )
> {
> throwBuildError( createErrorMessage( reportParameters, result, firstForkException, firstForkException );
> }
> else
> {
> throwBuildFailure( createErrorMessage( reportParameters, result, firstForkException, firstForkException );
> }
> }
>
> private static boolean isBooterError( Exception firstForkException )
> {
> return firstForkException instanceof SurefireBooterForkException;
> }
> ```
frankly I find your proposal adding a lot more complexity (~40 lines of code) for no much gain.
--
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 #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true
Posted by GitBox <gi...@apache.org>.
olamy closed pull request #478:
URL: https://github.com/apache/maven-surefire/pull/478
--
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 a change in pull request #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true
Posted by GitBox <gi...@apache.org>.
olamy commented on a change in pull request #478:
URL: https://github.com/apache/maven-surefire/pull/478#discussion_r822350092
##########
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:
no need of distinction between both exception maven core doesn't handle for very long time https://github.com/apache/maven/pull/632
this mean we can simplify the surefire code even more
--
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] slawekjaranowski commented on a change in pull request #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on a change in pull request #478:
URL: https://github.com/apache/maven-surefire/pull/478#discussion_r821143465
##########
File path: surefire-its/src/test/java/org/apache/maven/surefire/its/jiras/Surefire1426JvmCrashShouldNotBeIgnoredIT.java
##########
@@ -0,0 +1,50 @@
+package org.apache.maven.surefire.its.jiras;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import org.apache.maven.it.VerificationException;
+import org.apache.maven.surefire.its.fixture.SurefireJUnit4IntegrationTestCase;
+import org.junit.Test;
+
+import java.util.List;
+
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.is;
+
+/**
+ * Test https://issues.apache.org/jira/browse/SUREFIRE-1426
+ *
+ */
+public class Surefire1426JvmCrashShouldNotBeIgnoredIT
+ extends SurefireJUnit4IntegrationTestCase
+{
+ @Test
+ public void mavenShouldFail() throws VerificationException
+ {
+ List<String> logs = unpack( "surefire-1426-ignore-fail-jvm-crash" )
+ .maven()
+ .withFailure()
+ .debugLogging()
+ .executeTest()
+ .assertThatLogLine( containsString( "BUILD SUCCESS" ), is( 0 ) )
+ .verifyTextInLog( "BUILD FAILURE" )
+ .loadLogLines();
Review comment:
It was my last comments ... only for having code clean.
It is test class so it is not critical.
I don't see any other issues.
--
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 #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true
Posted by GitBox <gi...@apache.org>.
olamy commented on pull request #478:
URL: https://github.com/apache/maven-surefire/pull/478#issuecomment-1062624043
@Tibor17 I hope I answered all your concerns and we can now merge 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] olamy commented on pull request #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true
Posted by GitBox <gi...@apache.org>.
olamy commented on pull request #478:
URL: https://github.com/apache/maven-surefire/pull/478#issuecomment-1079535461
done via https://github.com/apache/maven-surefire/pull/496
--
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 #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #478:
URL: https://github.com/apache/maven-surefire/pull/478#issuecomment-1054523389
Your thrown exception does not make sense, because there are two types, just notice that.
If it is a `BUILD FAILURE` then it has not to fail the build because you configured `maven.test.failure.ignore`.
Build failure must not happen if the failure is ignored.
--
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 change in pull request #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] olamy commented on a change in pull request #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true
Posted by GitBox <gi...@apache.org>.
olamy commented on a change in pull request #478:
URL: https://github.com/apache/maven-surefire/pull/478#discussion_r823296651
##########
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:
sample here https://github.com/olamy/maven-exception-plugin
so there is no difference between `throw MojoExecutionException` and `throw MojoFailureException`
we can definitely simplify the code managing this but should be part of another PR
--
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] slawekjaranowski commented on pull request #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on pull request #478:
URL: https://github.com/apache/maven-surefire/pull/478#issuecomment-1054688231
Agree ... surefire code is enough complexity, we should reduce complexity not increase.
Next question how to test such code with coverage all decision branch?
--
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 #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true
Posted by GitBox <gi...@apache.org>.
olamy commented on pull request #478:
URL: https://github.com/apache/maven-surefire/pull/478#issuecomment-1054670812
> Your thrown exception does not make sense, because there are two types, just notice that. If it is a `BUILD FAILURE` then it has not to fail the build because you configured `maven.test.failure.ignore`. Build failure must not happen if the failure is ignored.
please read the comment here https://github.com/apache/maven-surefire/pull/478#issuecomment-1054523389
--
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 change in pull request #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@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
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] slawekjaranowski commented on a change in pull request #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on a change in pull request #478:
URL: https://github.com/apache/maven-surefire/pull/478#discussion_r820842354
##########
File path: surefire-its/src/test/java/org/apache/maven/surefire/its/jiras/Surefire1426JvmCrashShouldNotBeIgnoredIT.java
##########
@@ -0,0 +1,51 @@
+package org.apache.maven.surefire.its.jiras;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import org.apache.maven.it.VerificationException;
+import org.apache.maven.surefire.its.fixture.SurefireJUnit4IntegrationTestCase;
+import org.junit.Test;
+
+import java.util.List;
+
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.is;
+
+/**
+ * Test https://issues.apache.org/jira/browse/SUREFIRE-1426
+ *
+ */
+public class Surefire1426JvmCrashShouldNotBeIgnoredIT
+ extends SurefireJUnit4IntegrationTestCase
+{
+ @Test
+ public void mavenShouldFail() throws VerificationException
+ {
+ List<String> logs = unpack( "surefire-1426-ignore-fail-jvm-crash" )
+ .maven()
+ .withFailure()
+ .debugLogging()
+ .executeTest()
+ .assertThatLogLine( containsString( "BUILD SUCCESS" ), is( 0 ) )
+ .verifyTextInLog( "BUILD FAILURE" )
+ .loadLogLines();
+ logs.stream().forEach( line -> System.out.println( line ) );
Review comment:
We don't need to print logs to stdout ... verifier store it in log file
--
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 change in pull request #478: [SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org