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

[Feature-Request] Instrumented Test Coverage and Possible Report Merge #15

Open
zameelpichen opened this issue Apr 26, 2023 · 21 comments
Open

Comments

@zameelpichen
Copy link

Does this plugin also cover instrumented test reports using JaCoCo?

also, is an integration of both UI tests and unit tests possible?

reference from 2019 (Jurassic?) : https://medium.com/android-news/unified-code-coverage-reports-for-both-unit-and-ui-tests-e7c954a4e8ac

paradigm-wise it looks like ui coverage and unit coverage should remain separate, but i have only dipped my shoe in this water, not dived in yet.

appreciate your thoughts in advance

@gmazzo
Copy link
Owner

gmazzo commented Apr 26, 2023

You mean the AndroidTest variant right? Tests run on an emulator? I don't think it's going to work by default as the plugins is only aggregating UnitTest variants (that has coverage enabled). But in theory it should be possible if AGP supports to generate the JaCoCo coverage file (.exec) for it.

@zameelpichen
Copy link
Author

zameelpichen commented Apr 26, 2023

Yes the AndroidtTest variant.

But in the article they seem to have achieved it. (I havent tested it)

  1. What do you think about this being a useful feature for this plugin?
  2. Is it wrong to integrate both cov reports? (Any paradigm-wise or other reasons)

(trying to build a thesis for the 2nd point, so any thoughts, debateable points, opinions, are all highly welcome!)

@zameelpichen
Copy link
Author

found this, they have tried to merge it. (i have not tested)

https://github.com/ChristopherBull/demo-merge-android-coverage

@bddckr
Copy link

bddckr commented Apr 27, 2023

This is what I'm currently doing for supporting GMD tests (the only instrumented tests I run myself currently):

extensions.getByType(AndroidComponentsExtension::class).onVariants { variant ->
    executionDataConfiguration.configure {
        afterEvaluate {
            // We do reflection here because we want to stick with only using the
            // public AGP API dependency. Definitely hacky, but it's better than
            // accidentally using the non-public dependency APIs everywhere else in
            // this Gradle build.
            val expectedTaskNamePart =
                "${variant.buildType!!.replaceFirstChar(Char::titlecaseChar)}AndroidTest"
            tasks.matching {
                    it.javaClass.superclass.simpleName == "ManagedDeviceInstrumentationTestTask" && it.name.contains(
                        expectedTaskNamePart
                    )
                }.forEach { task ->
                    val coverageDirectory =
                        task.javaClass.getMethod("getCoverageDirectory")
                            .invoke(task) as DirectoryProperty
                    outgoing.artifact(coverageDirectory.file("coverage.ec")) {
                        type = ArtifactTypeDefinition.BINARY_DATA_TYPE
                    }
                }
        }
    }
}

By depending on the task output like that, I've seen it not even requiring manual dependency setup since the task dependencies are recognized by Gradle (getCoverageDirectory is annotated to be an output directory).

Basically: JaCoCo is fine with the ec files as well. Hopefully, that helps!

@zameelpichen zameelpichen changed the title [Feature-Req/Usage-Guide] Does this plugin also cover instrumented test reports? [Feature-Request] Instrumented Test Coverage and Possible Report Merge May 3, 2023
@zameelpichen
Copy link
Author

zameelpichen commented May 3, 2023

#15 (comment)

in theory it should be possible if AGP supports to generate the JaCoCo coverage file (.exec) for it.

#15 (comment)

JaCoCo is fine with the ec files

Summary :
So the question now is, does AGP support to generate the JaCoCo coverage file (.ec) for instrumented tests.

@gmazzo I think this feature would turn this plugin into a go to place for full test coverage setting up.

Looking forward to more thoughts

@zameelpichen
Copy link
Author

jacoco/jacoco#1208
Seems like this will take a while. I wonder if there are other options than JaCoCo. Or if anyone figured out to do it with JaCoCo.

Key issue is that @composable are being considered as functions to be unit tested, and therefore the code coverage report is very very very low. Meaning the report is unusable without filtering composables.

@gmazzo
Copy link
Owner

gmazzo commented May 3, 2023

jacoco/jacoco#1208 Seems like this will take a while. I wonder if there are other options than JaCoCo. Or if anyone figured out to do it with JaCoCo.

Key issue is that @composable are being considered as functions to be unit tested, and therefore the code coverage report is very very very low. Meaning the report is unusable without filtering composables.

Yeah, @Compsable (and any Kotlin code manipulation) is another story. At the end JaCoCo only understands bytecode and it has some rules to match it against the original source code. With Java it was easy because it usually is 1:1 relationship, but Kotlin does a lot of boilerplate magic plus its compiler plugins. It's impossible to JaCoCo to keep track of every implementation.
That's why they are working on Kover for this

@bddckr
Copy link

bddckr commented May 3, 2023

So the question now is, does AGP support to generate the JaCoCo coverage file (.ec) for instrumented tests.

That's what it already does. Sorry, that wasn't clear in my original comment.

To clarify: Instrumentation tests via AGP produce ec files. Those files can be given to the JaCoCo plugin task just like this plugin already gives it the unit tests' exec files.

So except for Compose stuff, you can get full code coverage reports for all tests, if this plugin adds instrumentation test support as well.

@zameelpichen
Copy link
Author

zameelpichen commented May 4, 2023

Alright!!

So I Confirmed, jacoco ignores most of composable lines when added with a custom annotation that has "Generated" in its annotation class name.

However, it still counted inner declarations like Row, Columns. Thus its useless to add the annotation, also it impacts readability to add such an annotation to say 106 composables

...

Also confirmed, kover can ignore annotations like @composable

...

I guess if we want instrumented test coverage, then ignoring @composable may not be needed

But kover dont have instrumented test coverage yet. (1.5 yr old now, complexity is still being calculated) Kotlin/kotlinx-kover#96

...

Thus, having JaCoCo instrumented coverage in this plugin makes even more sense!!

@gmazzo
Copy link
Owner

gmazzo commented May 4, 2023

I've been doing some exploration about supporting AndroidTest. It seems doable, but discovered a few issues that I'm trying to overcome.

Basically, with the current implemention, just enabling buildType.enableAndroidTestCovegare breaks the report. That's because the public API on AGP replaces the CLASSES artifact with the instrumented one.

I'd prefer not to rely on implemention details to do this to avoid breaking changes bumping AGP.

Let me see what I can do

@gmazzo
Copy link
Owner

gmazzo commented May 4, 2023

I have some progress but I've found some blocking issues with AGP implementation:

  • ScopedArtifact.CLASSES artifact gets replaced with JaCoCo instrumented classes when BuildType.enableAndroidTestCoverage = true is enabled (making aggregated JaCoCo task to fail)
  • The internal InternalScopedArtifact.PRE_JACOCO_TRANSFORMED_CLASSES artifact (which should be the output of CLASSES can't be accessed without reflection, which is very likely to break when bumping AGP.
  • And the most important one: createDebugUnitTestCoverageReport is empty when run on a com.android.library module. This issue is reproducible with both 8.0.0 and 8.0.1 even without this plugin. It seems to be fix on 8.1.0-beta01

As I don't want to rely on internal API or worse, to hardcode paths, I'm fine claiming this plugin is (for now) incompatible with AndroidTest variants and BuildType.enableAndroidTestCoverage = true.

I think it's better not to support this until the coverage API is stable from their side.

@zameelpichen
Copy link
Author

zameelpichen commented May 4, 2023

Ok! Thank you for looking into it. Will visit back once their side is ready. ⚡⚡👍👍

@bddckr
Copy link

bddckr commented May 5, 2023

@gmazzo Wanna raise a feature request with the AGP folks about making PRE_JACOCO_TRANSFORMED_CLASSES public? I'd love to upvote it. I have identified all the same that you found here (but I didn't care about the createDebugUnitTestCoverageReport issue as with this plugin's help, we create our own "full coverage report"), but I haven't had time to articulate a proper feature request for them.

Going by experience, they tend never to open anything up until the case has been made for it. They've been quite receptive to these API change requests as part of their push to move everyone off the gradle artifact in favor of the gradle-api artifact (see this).

@gmazzo
Copy link
Owner

gmazzo commented May 5, 2023

I'm going to park this for now until it's a bit more mature. The fact the the whole coverage report is broken makes me think they are not putting much focus on this right now.

I was actually reluctant to publish this code as a plugin assuming it will be eventually introduced officially by them at some point. So far I don't see it coming.

If you cant to make the case with them, feel free. I can contribute as well.

This is the WIP PR for implementing it:
#16

It access internal (so it will probably break between versions) but regardless of that it works. Still need to research how to implement correctly this part because the files has to be computed lazily as they are not know at configuration time (on AndroidTest):

            codeCoverageExecData.all file@{
                files(this).asFileTree.forEach { file ->
                    outgoing.artifact(file) {
                        type = ArtifactTypeDefinition.BINARY_DATA_TYPE
                        builtBy(this@file)
                    }
                }
            }

@bddckr
Copy link

bddckr commented May 7, 2023

A related plugin ran into the same issue (class files are instrumented and there's no public API to access the uninstrumented classes). An issue has been opened with AGP, which I suggest we upvote:
NeoTech-Software/Android-Root-Coverage-Plugin#82 (comment)

@gmazzo
Copy link
Owner

gmazzo commented Jun 23, 2023

Let's follow the progress of https://issuetracker.google.com/u/0/issues/281266702, it may be done depending on what it's decided there

@bddckr
Copy link

bddckr commented Nov 5, 2023

It looks like AGP 8.3 alpha 11 introduces the fix needed for this.

Things have changed in this plugin since that draft PR was opened, so it's not that easy for me to update that branch and check if it works. Can you update the PR and target alpha 11? 🙏

@gmazzo
Copy link
Owner

gmazzo commented Nov 10, 2023

I've updated the working branch #16 , but still is no mature yet and too much hacking is needed.

For instance, androidTest could run on one or many emulators, or via a managed devices now. And there is no safe task to collect all its results without failing (in case your project does not uses real devices/emulators).

There is no API also to get binary data from tests results.

I'll revisit this in the future, once it's more stable, or hopefully it is finally integrated into AGP itself

@ArcherEmiya05
Copy link

ArcherEmiya05 commented Apr 4, 2024

They removed the documentation about android-reporting plugin this month. Anyway may I ask here if it is possible to aggregate test reporting similar to this testAggregatedReport without JaCoCo? I tried to achieve something similar to it but I ended up having duplicate report on each test case.

I noticed that without setting these properties in each module when using JaCoCo, I am getting similar issue.

debug {
            enableUnitTestCoverage = true
        }
        release {
            aggregateTestCoverage = false
        }

@gmazzo
Copy link
Owner

gmazzo commented Apr 5, 2024

If I have some time, I may try to revisit this. But there are several structural issues to have this done properly, starting from the fact the official JaCoCo task Gradle does not support modules (while the JaCoCo ant tool used internally does) and combined with the Android architecture where the same class may be different between variants + multiple runs on UI devices, is making this overwhelming complex to hack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants