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

Stack trace printed when no test classes are run (Surefire 2.12.2+) #19

Closed
atomicknight opened this Issue Sep 5, 2012 · 22 comments

Comments

Projects
None yet
4 participants
@atomicknight
Contributor

atomicknight commented Sep 5, 2012

Version 2.12.2 of the maven-surefire-plugin includes an optimization that avoids forking the JVM if a fork mode of "once" is specified and there are no tests to run. Consequently, there is no guarantee that the Jacoco data file will be generated, leading to the following stack trace when the above conditions are met:

[ERROR] Unable to read execution data file /home/atomicknight/test/target/jacoco.exec: /home/atomicknight/test/target/jacoco.exec (The system cannot find the path specified)
java.io.FileNotFoundException: /home/atomicknight/test/target/jacoco.exec (The system cannot find the path specified)
at java.io.FileInputStream.open(Native Method)
at java.io.FileInputStream.(FileInputStream.java:120)
at org.jacoco.maven.ReportMojo.loadExecutionData(ReportMojo.java:251)
at org.jacoco.maven.ReportMojo.executeReport(ReportMojo.java:228)
at org.jacoco.maven.ReportMojo.execute(ReportMojo.java:217)
at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:101)
at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:209)
at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:153)
at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:145)
at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject(LifecycleModuleBuilder.java:84)
at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject(LifecycleModuleBuilder.java:59)
at org.apache.maven.lifecycle.internal.LifecycleStarter.singleThreadedBuild(LifecycleStarter.java:183)
at org.apache.maven.lifecycle.internal.LifecycleStarter.execute(LifecycleStarter.java:161)
at org.apache.maven.DefaultMaven.doExecute(DefaultMaven.java:320)
at org.apache.maven.DefaultMaven.execute(DefaultMaven.java:156)
at org.apache.maven.cli.MavenCli.execute(MavenCli.java:537)
at org.apache.maven.cli.MavenCli.doMain(MavenCli.java:196)
at org.apache.maven.cli.MavenCli.main(MavenCli.java:141)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
at java.lang.reflect.Method.invoke(Method.java:597)
at org.codehaus.plexus.classworlds.launcher.Launcher.launchEnhanced(Launcher.java:290)
at org.codehaus.plexus.classworlds.launcher.Launcher.launch(Launcher.java:230)
at org.codehaus.plexus.classworlds.launcher.Launcher.mainWithExitCode(Launcher.java:409)
at org.codehaus.plexus.classworlds.launcher.Launcher.main(Launcher.java:352)

I'm not sure how this should be addressed. The simplest solution is to just ignore the missing data file, but that might not be desirable in all situations.

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Sep 5, 2012

Member

Probably a simple one-line warning would be the best, as proposed here:

http://sourceforge.net/tracker/?func=detail&aid=3563431&group_id=177969&atid=883351

Member

marchof commented Sep 5, 2012

Probably a simple one-line warning would be the best, as proposed here:

http://sourceforge.net/tracker/?func=detail&aid=3563431&group_id=177969&atid=883351

@ghost ghost assigned Godin Sep 5, 2012

@atomicknight

This comment has been minimized.

Show comment
Hide comment
@atomicknight

atomicknight Sep 5, 2012

Contributor

Thanks for the feedback and for providing the link to the existing issue. I like the general concept, though I wonder if logging a warning might be too alarming, given that valid configurations would trigger this behavior.

Here's a possible alternative: instead of modifying the default behavior, introduce an "ignoreMissingDataFile" parameter that suppresses the stack trace and replaces it with an INFO message if the file is missing and the setting is set to "true." When set to "false" (the default), the current behavior remains unchanged (though I believe most plugins actually fail the build on error as opposed to just printing the stack trace).

What do you think? I can submit a pull request for this if that'd be helpful.

Contributor

atomicknight commented Sep 5, 2012

Thanks for the feedback and for providing the link to the existing issue. I like the general concept, though I wonder if logging a warning might be too alarming, given that valid configurations would trigger this behavior.

Here's a possible alternative: instead of modifying the default behavior, introduce an "ignoreMissingDataFile" parameter that suppresses the stack trace and replaces it with an INFO message if the file is missing and the setting is set to "true." When set to "false" (the default), the current behavior remains unchanged (though I believe most plugins actually fail the build on error as opposed to just printing the stack trace).

What do you think? I can submit a pull request for this if that'd be helpful.

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Sep 5, 2012

Member

What if we simply log an INFO message if the file does not exist? (we should still dump an ERROR if the file exists but can not be read).

Member

marchof commented Sep 5, 2012

What if we simply log an INFO message if the file does not exist? (we should still dump an ERROR if the file exists but can not be read).

@atomicknight

This comment has been minimized.

Show comment
Hide comment
@atomicknight

atomicknight Sep 5, 2012

Contributor

That seems reasonable. Would you like me to submit a pull request? Or is someone already working on it?

Contributor

atomicknight commented Sep 5, 2012

That seems reasonable. Would you like me to submit a pull request? Or is someone already working on it?

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Sep 5, 2012

Member

Of course feel free to submit pull request - this might speed-up resolution of this issue, however take a look on our conventions http://www.eclemma.org/jacoco/trunk/doc/conventions.html , what implies that patch must be well-formated and tested.

Member

Godin commented Sep 5, 2012

Of course feel free to submit pull request - this might speed-up resolution of this issue, however take a look on our conventions http://www.eclemma.org/jacoco/trunk/doc/conventions.html , what implies that patch must be well-formated and tested.

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Sep 6, 2012

Member

BTW, I can imagine another way to fix this issue: AgentMojo can create an empty jacoco.exec file, thus it will exist even if surefire wasn't executed. Guys, WDYT?

Member

Godin commented Sep 6, 2012

BTW, I can imagine another way to fix this issue: AgentMojo can create an empty jacoco.exec file, thus it will exist even if surefire wasn't executed. Guys, WDYT?

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Sep 6, 2012

Member

I prefer to separate concerns and go with the proposed fix. There might be scenarios where we want to use agent and reporting separately.

Member

marchof commented Sep 6, 2012

I prefer to separate concerns and go with the proposed fix. There might be scenarios where we want to use agent and reporting separately.

@atomicknight

This comment has been minimized.

Show comment
Hide comment
@atomicknight

atomicknight Sep 6, 2012

Contributor

Creating the empty file was actually the first approach I tried - it does work, but I think it's a bit questionable in that it creates additional pre-/post-conditions that aren't necessarily obvious at first glance. On the other hand, I can't really think of a use case in which the reporting goal would be used without executing the agent goal first. While I don't think the proposed fix is perfect, I think the separation of concerns argument is still fairly compelling.

Contributor

atomicknight commented Sep 6, 2012

Creating the empty file was actually the first approach I tried - it does work, but I think it's a bit questionable in that it creates additional pre-/post-conditions that aren't necessarily obvious at first glance. On the other hand, I can't really think of a use case in which the reporting goal would be used without executing the agent goal first. While I don't think the proposed fix is perfect, I think the separation of concerns argument is still fairly compelling.

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Sep 7, 2012

Member

Here is my vision:

  • Goal of ReportMojo - generate report based on existing dump file.
  • Goal of AgentMojo - prepare for collection of coverage.

And now main question here - do we want to stop ReportMojo immediately if dump not exists or not?
I prefer fail-fast approach, because nobody looks into logs, if CI job completed successfully. From here - proposed solution is not good one.

And I don't see how creation of file in AgentMojo breaks concept of separation, because this is exactly the job of AgentMojo - prepare for collection of coverage and this preparation may mean creation of a file.

But maybe I miss something?

Member

Godin commented Sep 7, 2012

Here is my vision:

  • Goal of ReportMojo - generate report based on existing dump file.
  • Goal of AgentMojo - prepare for collection of coverage.

And now main question here - do we want to stop ReportMojo immediately if dump not exists or not?
I prefer fail-fast approach, because nobody looks into logs, if CI job completed successfully. From here - proposed solution is not good one.

And I don't see how creation of file in AgentMojo breaks concept of separation, because this is exactly the job of AgentMojo - prepare for collection of coverage and this preparation may mean creation of a file.

But maybe I miss something?

@atomicknight

This comment has been minimized.

Show comment
Hide comment
@atomicknight

atomicknight Sep 7, 2012

Contributor

No, that seems reasonable. I've just been a wary of relying too heavily on side effects, though it's clear that the situation requires them to some degree (though the question is "to what degree?").

I do agree that fail-fast execution is desired here - this is why I consider the proposed solution to be imperfect. I'm really fine with either approach.

Contributor

atomicknight commented Sep 7, 2012

No, that seems reasonable. I've just been a wary of relying too heavily on side effects, though it's clear that the situation requires them to some degree (though the question is "to what degree?").

I do agree that fail-fast execution is desired here - this is why I consider the proposed solution to be imperfect. I'm really fine with either approach.

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Sep 7, 2012

Member

Ok, so @marchof we're waiting your decision ;)

Member

Godin commented Sep 7, 2012

Ok, so @marchof we're waiting your decision ;)

@atomicknight

This comment has been minimized.

Show comment
Hide comment
@atomicknight

atomicknight Sep 7, 2012

Contributor

As a side note, the missing file doesn't actually trigger a build failure - it just prints a stack trace and exits normally. So to actually get that fail-fast behavior, that logic should be modified as well.

Contributor

atomicknight commented Sep 7, 2012

As a side note, the missing file doesn't actually trigger a build failure - it just prints a stack trace and exits normally. So to actually get that fail-fast behavior, that logic should be modified as well.

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Sep 7, 2012

Member

@atomicknight good point!

Member

Godin commented Sep 7, 2012

@atomicknight good point!

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Sep 7, 2012

Member

On the other side - no need to create dump file by AgentMojo, if current project even not designed to contain tests (e.g. just to create package with application).

Member

Godin commented Sep 7, 2012

On the other side - no need to create dump file by AgentMojo, if current project even not designed to contain tests (e.g. just to create package with application).

@atomicknight

This comment has been minimized.

Show comment
Hide comment
@atomicknight

atomicknight Sep 10, 2012

Contributor

I didn't quite follow that last comment - could you elaborate?

Contributor

atomicknight commented Sep 10, 2012

I didn't quite follow that last comment - could you elaborate?

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Sep 10, 2012

Member

What is the exact situation we're targeting here? Is it

  • we don't have tests for a module at all
  • we want to create an coverage report anyways.
Member

marchof commented Sep 10, 2012

What is the exact situation we're targeting here? Is it

  • we don't have tests for a module at all
  • we want to create an coverage report anyways.
@atomicknight

This comment has been minimized.

Show comment
Hide comment
@atomicknight

atomicknight Sep 10, 2012

Contributor

For me, the situation is:

  • The module contains no tests
  • The Jacoco plugin executions are defined in a parent POM
  • We don't expect a coverage report even though the plugin runs
  • We don't want to see an error message/stack trace being printed during execution of the plugin
Contributor

atomicknight commented Sep 10, 2012

For me, the situation is:

  • The module contains no tests
  • The Jacoco plugin executions are defined in a parent POM
  • We don't expect a coverage report even though the plugin runs
  • We don't want to see an error message/stack trace being printed during execution of the plugin
@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Sep 11, 2012

Member

From the discussion I don't see a final solution that covers all issues raised here. So my proposal is to go with a slightly modified version of atomicknight's patch as a first step as it improves the current situation a bit:

  • We get a proper info log in case of missing exec files (done by the patch)
  • We log an error and break the build, if the exec file exists but cannot be read (to be done)

Can we agree on this and open new issues for the other aspects?

Member

marchof commented Sep 11, 2012

From the discussion I don't see a final solution that covers all issues raised here. So my proposal is to go with a slightly modified version of atomicknight's patch as a first step as it improves the current situation a bit:

  • We get a proper info log in case of missing exec files (done by the patch)
  • We log an error and break the build, if the exec file exists but cannot be read (to be done)

Can we agree on this and open new issues for the other aspects?

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Sep 11, 2012

Member

LGTM, so I'll do this today.

Member

Godin commented Sep 11, 2012

LGTM, so I'll do this today.

@atomicknight

This comment has been minimized.

Show comment
Hide comment
@atomicknight

atomicknight Sep 11, 2012

Contributor

Sounds good. I've updated the pull request to trigger a build failure when the execution data file cannot be read. I wasn't sure if it made sense to log an error in addition to throwing the exception (since the latter prints the message already), but I left it in anyway.

Contributor

atomicknight commented Sep 11, 2012

Sounds good. I've updated the pull request to trigger a build failure when the execution data file cannot be read. I wasn't sure if it made sense to log an error in addition to throwing the exception (since the latter prints the message already), but I left it in anyway.

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Jan 4, 2013

mvn -DskipTests clean install now prints just

[jacoco:report]
Skipping JaCoCo execution due to missing execution data file

which is good. An optimization could be to suppress this message—and also suppress any activity from prepare-agent—when skipTests (or maven.test.skip) is set.

jglick commented Jan 4, 2013

mvn -DskipTests clean install now prints just

[jacoco:report]
Skipping JaCoCo execution due to missing execution data file

which is good. An optimization could be to suppress this message—and also suppress any activity from prepare-agent—when skipTests (or maven.test.skip) is set.

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Jan 4, 2013

Member

@jglick Feel free to create separate issue.

Member

Godin commented Jan 4, 2013

@jglick Feel free to create separate issue.

@jacoco jacoco locked and limited conversation to collaborators Jan 11, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.