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

Jacoco doesn't cover a class that is an injection by constructor #32254

Closed
mpumd opened this issue Mar 30, 2023 · 15 comments
Closed

Jacoco doesn't cover a class that is an injection by constructor #32254

mpumd opened this issue Mar 30, 2023 · 15 comments
Labels
kind/bug Something isn't working triage/invalid This doesn't seem right

Comments

@mpumd
Copy link

mpumd commented Mar 30, 2023

Describe the bug

Hi guys

Today we have a coverage problem with jacoco. An ApplicationScope bean that use an injection by constructor isn't cover by jacoco.

Expected behavior

Jacoco should cover all classes, whatever the way of dependency injection.

Actual behavior

No response

How to Reproduce?

You have an example project by file zip file just at the end of this message.

Output of uname -a or ver

MSYS_NT-10.0-19045 3.3.6-bec3d608-341.x86_64 2023-02-22 08:29 UTC x86_64 Msys

Output of java -version

Java version: 17.0.6, vendor: BellSoft, runtime: scoop\apps\liberica17-full-jdk\current

GraalVM version (if different from Java)

No response

Quarkus version or git rev

2.16.5.Final

Build tool (ie. output of mvnw --version or gradlew --version)

Apache Maven 3.9.1 (2e178502fcdbffc201671fb2537d0cb4b4cc58f8)

Additional information

jacoco.zip

Thanks !

@mpumd mpumd added the kind/bug Something isn't working label Mar 30, 2023
@quarkus-bot quarkus-bot bot added the area/arc Issue related to ARC (dependency injection) label Mar 30, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented Mar 30, 2023

/cc @Ladicek (arc), @manovotn (arc), @mkouba (arc)

@vsevel
Copy link
Contributor

vsevel commented Mar 30, 2023

when we use the injection by constructor, we see:

[INFO] --- jacoco-maven-plugin:0.8.8:report (report) @ getting-started ---
[INFO] Loading execution data file E:\tmp\getting-started-jacoco\target\jacoco.exec
[INFO] Analyzed bundle 'getting-started' with 3 classes
[WARNING] Classes in bundle 'getting-started' do not match with execution data. For report generation the same class files must be used as at runtime.
[WARNING] Execution data for class org/acme/MyBean does not match.

and /target/site/jacoco/org.acme/MyBean.java.html#L11 has 0 coverage.

instead of:

[INFO] --- jacoco-maven-plugin:0.8.8:report (report) @ getting-started ---
[INFO] Loading execution data file E:\tmp\getting-started-jacoco\target\jacoco.exec
[INFO] Analyzed bundle 'getting-started' with 3 classes

@mkouba mkouba removed the area/arc Issue related to ARC (dependency injection) label Mar 30, 2023
@mkouba
Copy link
Contributor

mkouba commented Mar 30, 2023

I'm not a jacoco expert but if you use constructor injection for an @ApplicationScoped bean that does not declare a no-args constructor then the class is transformed and a no-args constructor is added - that might be the reason why you get the warning.

Speaking of coverage - does jacoco hanlde CDI at all. I mean the bean is not instantiated directly in the code, so it would need to analyze the injection points (e.g. GreetingResource#myBean) and deduce that the injected bean is used...

@vsevel
Copy link
Contributor

vsevel commented Mar 30, 2023

you are right @mkouba: a default constructor with package or public visibility makes jacoco happy (does not work with private).

@manovotn
Copy link
Contributor

you are right @mkouba: a default constructor with package or public visibility makes jacoco happy (does not work with private).

Martin is right, the CDI specification requires that every bean with normal scope (which is, among others, @ApplicationScoped) has a non-private no-args constructor. This is for purposes of client proxy creation. Quarkus can bypass this rule by letting you not declare it and then generating it later during build automagically. Hence the jacoco complaint.

In fact, there is more bytecode tempering Quarkus can do (and not just in CDI), so technically you should feed jacoco the sources after Quarkus finishes build, but I am not sure that's possible 🤔

@vsevel
Copy link
Contributor

vsevel commented Mar 30, 2023

In fact, there is more bytecode tempering Quarkus can do (and not just in CDI), so technically you should feed jacoco the sources after Quarkus finishes build, but I am not sure that's possible

hmm. that would be nice. especially because jacoco fails on the class silently (just a warn in the build logs).
the only reason we spotted it was because we failed the coverage quality gate on sonar.

@manovotn
Copy link
Contributor

In fact, there is more bytecode tempering Quarkus can do (and not just in CDI), so technically you should feed jacoco the sources after Quarkus finishes build, but I am not sure that's possible

hmm. that would be nice. especially because jacoco fails on the class silently (just a warn in the build logs). the only reason we spotted it was because we failed the coverage quality gate on sonar.

Hm, you can dump the generated classes, so that might be a start if you want to play with it, see https://quarkus.io/guides/writing-extensions#dump-the-generated-classes-to-the-file-system
(the link is about writing extensions but it nicely describes the options)

@Ladicek
Copy link
Contributor

Ladicek commented Mar 30, 2023

I'd like to point out that there is no source code that could be matched with transformed bytecode. If the coverage tool tries to match source code with bytecode, that will inevitably fail in presence of bytecode transformations and there's nothing we can do about that.

@geoand
Copy link
Contributor

geoand commented Mar 30, 2023

IIRC, Jacoco skips over code if it has the synthetic modifier, but I was also under the impression that Arc does add that modifier when it adds a no-args constructor

@geoand
Copy link
Contributor

geoand commented Mar 30, 2023

This seems to indicate that we do add the proper modifier.

@geoand
Copy link
Contributor

geoand commented Mar 30, 2023

Maybe the fact that it's public is causing the problem? Can the constructor be made package private @mkouba @manovotn ?

geoand added a commit to geoand/quarkus that referenced this issue Mar 30, 2023
The idea here is that hopefully it convince code coverage
tools to ignore it

Relates to: quarkusio#32254
@geoand
Copy link
Contributor

geoand commented Mar 30, 2023

In any case, I opened: #32267. Would anyone be able to try it on their application?

@manovotn
Copy link
Contributor

IIRC, Jacoco skips over code if it has the synthetic modifier, but I was also under the impression that Arc does add that modifier when it adds a no-args constructor

Interesting, didn't know that.

Maybe the fact that it's public is causing the problem? Can the constructor be made package private @mkouba @manovotn ?

Yes, that should work. CDI needs any non-private.

@vsevel
Copy link
Contributor

vsevel commented Apr 6, 2023

does not seem to happen when using the quarkus jacoco extension

@vsevel vsevel closed this as completed Apr 6, 2023
@geoand geoand added the triage/invalid This doesn't seem right label Apr 6, 2023
@geoand
Copy link
Contributor

geoand commented Apr 6, 2023

Thanks for the update @vsevel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working triage/invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

6 participants