Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SUREFIRE-2001] Sometimes the plugin prints an internal stack trace on BUILD FAILURE #486

Closed
wants to merge 1 commit into from

Conversation

Tibor17
Copy link
Contributor

@Tibor17 Tibor17 commented Mar 9, 2022

[INFO] --- maven-surefire-plugin:3.0.0-M6-SNAPSHOT:test (default-test) @ dummy ---
[INFO] Surefire report directory: C:\vcs\release\RELEASE-3.0.0-M1\surefire-its\target\Surefire943ReportContentIT_test_noParallel\target\surefire-reports
[INFO] Using configured provider org.apache.maven.surefire.junitcore.JUnitCoreProvider
[INFO] parallel='none', perCoreThreadCount=true, threadCount=4, useUnlimitedThreads=false, threadCountSuites=0, threadCountClasses=0, threadCountMethods=0, parallelOptimized=true
[INFO] 
[INFO] -------------------------------------------------------
[INFO]  T E S T S
[INFO] -------------------------------------------------------
[INFO] Running org.sample.module.My1Test
[ERROR] Tests run: 3, Failures: 1, Errors: 0, Skipped: 1, Time elapsed: 2.041 s <<< FAILURE! - in org.sample.module.My1Test
[ERROR] org.sample.module.My1Test.fails  Time elapsed: 1.016 s  <<< FAILURE!
java.lang.AssertionError: Always fails
	at org.sample.module.My1Test.fails(My1Test.java:34)

[INFO] Running org.sample.module.My2Test
[ERROR] Tests run: 3, Failures: 1, Errors: 0, Skipped: 1, Time elapsed: 1.993 s <<< FAILURE! - in org.sample.module.My2Test
[ERROR] org.sample.module.My2Test.fails  Time elapsed: 1.01 s  <<< FAILURE!
java.lang.AssertionError: Always fails
	at org.sample.module.My2Test.fails(My2Test.java:33)

[INFO] Running org.sample.module.My3Test
[ERROR] Tests run: 2, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 1.996 s <<< FAILURE! - in org.sample.module.My3Test
[ERROR] org.sample.module.My3Test.fails  Time elapsed: 1.008 s  <<< FAILURE!
java.lang.AssertionError: Always fails
	at org.sample.module.My3Test.fails(My3Test.java:32)

[INFO] Running org.sample.module.My4Test
[WARNING] Tests run: 1, Failures: 0, Errors: 0, Skipped: 1, Time elapsed: 0.001 s - in org.sample.module.My4Test
[INFO] Running org.sample.module.My5Test
[ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0 s <<< FAILURE! - in org.sample.module.My5Test
[ERROR] org.sample.module.My5Test  Time elapsed: 0 s  <<< ERROR!
java.lang.RuntimeException: always fails before class
	at org.sample.module.My5Test.failsOnBeforeClass(My5Test.java:30)

[INFO] 
[INFO] Results:
[INFO] 
[ERROR] Failures: 
[ERROR]   My1Test.fails:34 Always fails
[ERROR]   My2Test.fails:33 Always fails
[ERROR]   My3Test.fails:32 Always fails
[ERROR] Errors: 
[ERROR]   My5Test.failsOnBeforeClass:30 Runtime always fails before class
[INFO] 
[ERROR] Tests run: 10, Failures: 3, Errors: 1, Skipped: 3
[INFO] 
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  15.603 s
[INFO] Finished at: 2022-03-08T01:15:32+01:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:3.0.0-M6-SNAPSHOT:test (default-test) on project dummy: There are test failures.
[ERROR] 
[ERROR] Please refer to C:\vcs\release\RELEASE-3.0.0-M1\surefire-its\target\Surefire943ReportContentIT_test_noParallel\target\surefire-reports for the individual test results.
[ERROR] Please refer to dump files (if any exist) [date].dump, [date]-jvmRun[N].dump and [date].dumpstream.
[ERROR] -> [Help 1]
org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:3.0.0-M6-SNAPSHOT:test (default-test) on project dummy: There are test failures.

Please refer to C:\vcs\release\RELEASE-3.0.0-M1\surefire-its\target\Surefire943ReportContentIT_test_noParallel\target\surefire-reports for the individual test results.
Please refer to dump files (if any exist) [date].dump, [date]-jvmRun[N].dump and [date].dumpstream.
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:215)
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:156)
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:148)
    at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject (LifecycleModuleBuilder.java:117)
    at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject (LifecycleModuleBuilder.java:81)
    at org.apache.maven.lifecycle.internal.builder.singlethreaded.SingleThreadedBuilder.build (SingleThreadedBuilder.java:56)
    at org.apache.maven.lifecycle.internal.LifecycleStarter.execute (LifecycleStarter.java:128)
    at org.apache.maven.DefaultMaven.doExecute (DefaultMaven.java:305)
    at org.apache.maven.DefaultMaven.doExecute (DefaultMaven.java:192)
    at org.apache.maven.DefaultMaven.execute (DefaultMaven.java:105)
    at org.apache.maven.cli.MavenCli.execute (MavenCli.java:957)
    at org.apache.maven.cli.MavenCli.doMain (MavenCli.java:289)
    at org.apache.maven.cli.MavenCli.main (MavenCli.java:193)
    at sun.reflect.NativeMethodAccessorImpl.invoke0 (Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke (NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke (DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke (Method.java:498)
    at org.codehaus.plexus.classworlds.launcher.Launcher.launchEnhanced (Launcher.java:282)
    at org.codehaus.plexus.classworlds.launcher.Launcher.launch (Launcher.java:225)
    at org.codehaus.plexus.classworlds.launcher.Launcher.mainWithExitCode (Launcher.java:406)
    at org.codehaus.plexus.classworlds.launcher.Launcher.main (Launcher.java:347)
Caused by: org.apache.maven.plugin.MojoFailureException: There are test failures.

Please refer to C:\vcs\release\RELEASE-3.0.0-M1\surefire-its\target\Surefire943ReportContentIT_test_noParallel\target\surefire-reports for the individual test results.
Please refer to dump files (if any exist) [date].dump, [date]-jvmRun[N].dump and [date].dumpstream.
    at org.apache.maven.plugin.surefire.SurefireHelper.throwException (SurefireHelper.java:275)
    at org.apache.maven.plugin.surefire.SurefireHelper.reportExecution (SurefireHelper.java:163)
    at org.apache.maven.plugin.surefire.SurefirePlugin.handleSummary (SurefirePlugin.java:536)
    at org.apache.maven.plugin.surefire.AbstractSurefireMojo.executeAfterPreconditionsChecked (AbstractSurefireMojo.java:1176)
    at org.apache.maven.plugin.surefire.AbstractSurefireMojo.execute (AbstractSurefireMojo.java:925)
    at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo (DefaultBuildPluginManager.java:137)
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:210)
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:156)
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:148)
    at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject (LifecycleModuleBuilder.java:117)
    at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject (LifecycleModuleBuilder.java:81)
    at org.apache.maven.lifecycle.internal.builder.singlethreaded.SingleThreadedBuilder.build (SingleThreadedBuilder.java:56)
    at org.apache.maven.lifecycle.internal.LifecycleStarter.execute (LifecycleStarter.java:128)
    at org.apache.maven.DefaultMaven.doExecute (DefaultMaven.java:305)
    at org.apache.maven.DefaultMaven.doExecute (DefaultMaven.java:192)
    at org.apache.maven.DefaultMaven.execute (DefaultMaven.java:105)
    at org.apache.maven.cli.MavenCli.execute (MavenCli.java:957)
    at org.apache.maven.cli.MavenCli.doMain (MavenCli.java:289)
    at org.apache.maven.cli.MavenCli.main (MavenCli.java:193)
    at sun.reflect.NativeMethodAccessorImpl.invoke0 (Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke (NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke (DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke (Method.java:498)
    at org.codehaus.plexus.classworlds.launcher.Launcher.launchEnhanced (Launcher.java:282)
    at org.codehaus.plexus.classworlds.launcher.Launcher.launch (Launcher.java:225)
    at org.codehaus.plexus.classworlds.launcher.Launcher.mainWithExitCode (Launcher.java:406)
    at org.codehaus.plexus.classworlds.launcher.Launcher.main (Launcher.java:347)
[ERROR] 
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException

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.

@olamy
Copy link
Member

olamy commented Mar 9, 2022

this code could/must be simplified as Maven Core doesn;t make any difference between the 2 exceptions for very long time (i.e first version of maven 3.x)
it's always BUILD FAILURE at the end of the build for both exceptions.

@Tibor17
Copy link
Contributor Author

Tibor17 commented Mar 9, 2022

@olamy
A typical political benefit for your own PR.
You can see the purpose in compiler as behavior.
Again, non technical comment from @olamy , there is nothing to simplify, but rather respect the Maven Plugin API as it is.

@Tibor17 Tibor17 requested a review from hboutemy March 9, 2022 21:35
Comment on lines +271 to +273
throw firstForkException == null
? new MojoExecutionException( msg )
: new MojoExecutionException( msg, firstForkException );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the difference
MojoExecutionException( msg ) and MojoExecutionException( msg, null )
for both getCause() will return null

@Tibor17 Tibor17 removed the request for review from eolivelli March 9, 2022 21:52
@olamy
Copy link
Member

olamy commented Mar 9, 2022

My point is: What is the difference in the output result of a Maven build between MojoFailureException and MojoExecutionException? Please provide some ITs or an example I will be happy to understand

@olamy
Copy link
Member

olamy commented Mar 10, 2022

sample here https://github.com/olamy/maven-exception-plugin

the technical comment is all this code can be removed as there is no difference between throw MojoExecutionException and throw MojoFailureException

@Tibor17
Copy link
Contributor Author

Tibor17 commented Mar 12, 2022

@slawekjaranowski
@hboutemy
@olamy
What's the difference between these constructors?
What's the difference between these two exceptions.

The purpose of both exceptions (MojoExecutionException and MojoFailureException) is written with the example of m-compiler-p in the Javadoc.
Briefly speaking, the MojoFailureException is related to a config error, e.g. user error as for instance you configure the parameter parallel on the provider which does not support the parallel exec. On the other hand the MojoExecutionException is the error of the plugin itself.

Basically, this is wrong question and should not be given to me because I did not create this API for Plugins and I am not the author of SurefireHelper.java. This code existed here for years. I remember these calls of exceptions maybe 8 years. It means that the author of these exceptions knew that they should be called with purpose.

One can be wondering why we alter the constructors even if the second parameter is null. I am wondering about it too, but the most important is the result. I tried to call all constructors but this actually works properly and the message is as expected too. Pls give me a hint if you can, I appreciate it.
The fact is that the clients are wondering why there is the stack trace on the console, and some users are asking these questions on the StackOverlow. These questions are bad for us especially in case of BUILD FAILURE because the stack trace gives a bad impression to the users that the the problem is in the plugin but the problem is not in the plugin in real! And I saw the colleagues of mine in the companies that they are not scrolling up to see the test error, there is no reason to print the stack trace if the firstForkException is null. Sometimes the newbies users do not understand that they should scroll higher a bit to see that the bottom is not important, but their practices come from they daily experiences of another tools where only exceptions are important to see and so the people sometimes filter out all relevant messages on the console, so they have this selective read abilities. So the stack trace is annoying if it is irrelevant to see for them.

These questions regarding existence of exceptions should not be given to me as I am not the author of the Maven Plugin API. These exceptions were here always. I do not see any reason why we should not use them. We have always used them, so I am only preventing showing stack trace when should not be shown on the console. The exceptions have been used for many years, even before when I entered the ASF. It means that the exceptions have certain purpose for the author. The questions could be given to @krosenvold or @agudian as well, who were our colleagues and they are in the same situation as me or you or anybody else, which means that we use the API still the same way for years and we respect the API. I can see another reason why these questions are rised up, and they are not very technical, and I have to say that the same is elaborated in Olivier's PR and Olivier does not want to accept my arguments that the exceptions are two, we have to respect the API and the purpose, and the most imporant argument is that it is very silly to report BUILD FAILURE if -Dmaven.test.failure.ignore=true - try to read it because it is really funny to ignore my argument which explains that the users wants to ignore failures in the build but we finish the build with FAILURE. That's the reason why I recommended to Olivier to throw a specific exception as an error and not a failure. Why I want to recommend it? Because the developers make mistakes in the future and it is better to show them that MojoExecutionException is intended in the particular IF statement. Removing the calls of MojoExecutionException would be maybe an ego benefit in Olivier's PR but definitelly it would not be the rightest right decision.

Mostly if the firstForkException is not null the plugin throws BUILD ERROR and the stack trace makes sense because it is the real internal error. There is one more situation and it is the timeout. It is not a typical failure due to the JVMs have been timed out and stopped - the JVM was stopped abruptly - with an exit error code.

@olamy
Copy link
Member

olamy commented Mar 13, 2022

@slawekjaranowski @hboutemy @olamy What's the difference between these constructors? What's the difference between these two exceptions.

The purpose of both exceptions (MojoExecutionException and MojoFailureException) is written with the example of m-compiler-p in the Javadoc.

Please provide a link.

Briefly speaking, the MojoFailureException is related to a config error, e.g. user error as for instance you configure the parameter parallel on the provider which does not support the parallel exec. On the other hand the MojoExecutionException is the error of the plugin itself.

what is the difference for the end user if term of build result and output?

Basically, this is wrong question and should not be given to me because I did not create this API for Plugins and I am not the author of SurefireHelper.java. This code existed here for years. I remember these calls of exceptions maybe 8 years. It means that the author of these exceptions knew that they should be called with purpose.

I do not point any fingers on anybody. I'm just saying this code not worth it and can be simplify as it doesn;t bring any value for end user. The result is same.
so if we can simplify a bit the surefire code that will be great.
The code exists because it has been created with maven2 but doesn't have any sense anymore now with maven3
So again it can be simplified and I'm not blaming anybody.

One can be wondering why we alter the constructors even if the second parameter is null. I am wondering about it too, but the most important is the result. I tried to call all constructors but this actually works properly and the message is as expected too. Pls give me a hint if you can, I appreciate it. The fact is that the clients are wondering why there is the stack trace on the console, and some users are asking these questions on the StackOverlow. These questions are bad for us especially in case of BUILD FAILURE because the stack trace gives a bad impression to the users that the the problem is in the plugin but the problem is not in the plugin in real! And I saw the colleagues of mine in the companies that they are not scrolling up to see the test error, there is no reason to print the stack trace if the firstForkException is null. Sometimes the newbies users do not understand that they should scroll higher a bit to see that the bottom is not important, but their practices come from they daily experiences of another tools where only exceptions are important to see and so the people sometimes filter out all relevant messages on the console, so they have this selective read abilities. So the stack trace is annoying if it is irrelevant to see for them.

please provide some example projects as I don't really understand your point here. and especially in a user point of view.

These questions regarding existence of exceptions should not be given to me as I am not the author of the Maven Plugin API. These exceptions were here always. I do not see any reason why we should not use them. We have always used them, so I am only preventing showing stack trace when should not be shown on the console. The exceptions have been used for many years, even before when I entered the ASF. It means that the exceptions have certain purpose for the author. The questions could be given to @krosenvold or @agudian as well, who were our colleagues and they are in the same situation as me or you or anybody else, which means that we use the API still the same way for years and we respect the API. I can see another reason why these questions are rised up, and they are not very technical, and I have to say that the same is elaborated in Olivier's PR and Olivier does not want to accept my arguments that the exceptions are two, we have to respect the API and the purpose,

again please read my arguments below or have a look at the sample project provided, APIs

and the most imporant argument is that it is very silly to report BUILD FAILURE if -Dmaven.test.failure.ignore=true - try to read it because it is really funny to ignore my argument which explains that the users wants to ignore failures in the build but we finish the build with FAILURE. That's the reason why I recommended to Olivier to throw a specific exception as an error and not a failure. Why I want to recommend it? Because the developers make mistakes in the future and it is better to show them that MojoExecutionException is intended in the particular IF statement. Removing the calls of MojoExecutionException would be maybe an ego benefit in Olivier's PR but definitelly it would not be the rightest right decision.

it's not related to this PR and would be better to add comments here #478
But anyway I will comment here a bit with some real use case and user experience.
The goal of -Dmaven.test.failure.ignore=true is to ignore test failure and print a BUILD SUCCESS at the end even if some tests has failed. As stated in the documentation, This option is explicitly NOT RECOMMENDED.
The use case is to have a build which run all the the tests and some tooling will give a global result of tests which has been runned.
Still giving a BUILD SUCCESS even if some tests has failed but some CI such Jenkins collect the results and can eventually mark the result as unstable or fail but you need the help of another tooling.
But in some misconfiguration the tests are not even running so the proposed change #478 .
not being able to run the tests is a very different case than running tests and have test failure.
Use case (and I have encountered this real life problem), huge build with a lot modules, this build is configured with the ignore option and run on Jenkins with a matrix of 3 os and 6 jdks. (different profiles for each jdk with some different jvm args)
With this option activated and some wrong jvm arguments, one of the modules can totally fail in this big matrix of build but nobody will notice it as the result is always success and the tests didn't even run. So even Jenkins will not collect anything.

Mostly if the firstForkException is not null the plugin throws BUILD ERROR and the stack trace makes sense because it is the real internal error. There is one more situation and it is the timeout. It is not a typical failure due to the JVMs have been timed out and stopped - the JVM was stopped abruptly - with an exit error code.

please provide example with some real difference in a user point of view or final build output.

@Tibor17
Copy link
Contributor Author

Tibor17 commented Mar 13, 2022

@olamy
@slawekjaranowski
@hboutemy
We should discuss one extra topic in the mailing list dev@maven.apache.org with a topic related to the findings we have observed in this PR. I am convinced that the issue with BUILD FAILURE/ERROR of the exceptions MojoExecutionException/MojoFailureException is not a problem of this plugin itself. I have talked about it with @hboutemy and we launched the project [1] given by @olamy . If you strictly follow the Javadoc of these exceptions in the API 3.2.5, you would see a difference between the text and the current behavior of Maven Core 3.x. We should agree on what would be done with this misconception, whether we will adapt the Javadoc in favor of Maven Core or opposite, plus some technical details...
I don't want to be the one who takes the responsibility, we all should inform the team about the difference between the reality and Javadoc. Since it looks like a trivial issue, it can be discussed in the mailing list and the steps can be defining a fix. Pls let me open the discussion, feel free to provide your opinions and fix proposals. Thx
[1]: https://github.com/olamy/maven-exception-plugin

@slawekjaranowski
Copy link
Member

Never mind if we use one or two exceptions - this change add code, like:

throw firstForkException == null
                ? new MojoExecutionException( msg )
                : new MojoExecutionException( msg, firstForkException );

IMHO it is not necessary the same result is:

throw new MojoExecutionException( msg, firstForkException );

we needn't check if cause is null or not

@Tibor17
Copy link
Contributor Author

Tibor17 commented Mar 13, 2022

@slawekjaranowski
I know what you mean and I agree with you.
I only want to say that we should go step by step. First, the Javadoc should be fixed in the API due to we forgot it as it seems.

@slawekjaranowski
Copy link
Member

So @Tibor17 what step you achieve by this PR?

I see next branch of decision in code (next "if" ) without any result.
I don't see any tests which confirm that something change in expected way.

Maybe I don't see something else with bigger context.

@hboutemy
Copy link
Member

@slawekjaranowski

IMHO it is not necessary the same result is:

do you mean that it's not necessarily the same result? (= I don't agree)
or it is not necessary, because it gives the same result? (= here, I agree)

@slawekjaranowski
Copy link
Member

It is not necessary, because it gives the same result.

Stack Trace is the same for both constructor.

MojoExecutionException( msg )
MojoExecutionException( msg, null )

Checking - if cause has null value or not is redundant.

@hboutemy
Copy link
Member

@slawekjaranowski I see we agree

@Tibor17 this PR does nothing (than adding more lines of code),

then IMHO it can/should be simply dropped
and instead you should describe in SUREFIRE-2001 Jira issue what problem is being reported: once the problem is described, we'll see about solutions

Copy link
Member

@hboutemy hboutemy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to define what problem is being worked on

@Tibor17
Copy link
Contributor Author

Tibor17 commented Mar 26, 2022

The ITs use -e so they print stacktrace. I did not notice that, sorry for the mistake.

@Tibor17 Tibor17 closed this Mar 26, 2022
@asfgit asfgit deleted the SUREFIRE-2001 branch March 27, 2022 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants