[SUREFIRE-1426] Fork crash doesn't fail build with -Dmaven.test.failure.ignore=true

[olamy]

Signed-off-by: Olivier Lamy olamy@apache.org

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

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

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

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

• I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004

• In any other case, please file an Apache Individual Contributor License Agreement.

[Tibor17]

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.

[Tibor17]

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;
   }

[olamy]

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 #478 (comment)

[olamy]

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.

[slawekjaranowski]

Agree ... surefire code is enough complexity, we should reduce complexity not increase.

Next question how to test such code with coverage all decision branch?

[olamy]

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.

[slawekjaranowski]

I'm afraid that jacoco not working in IT tests .... with current configuration ... maybe I'm wrong

[olamy]

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

[olamy]

@Tibor17 I hope I answered all your concerns and we can now merge this?

[olamy]

@Tibor17 I hope I answered all your concerns and we can now merge this?

@Tibor17 ping

[olamy]

done via #496