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

support for Kotlin #39

Merged
merged 1 commit into from
Feb 24, 2019
Merged

support for Kotlin #39

merged 1 commit into from
Feb 24, 2019

Conversation

rabidaudio
Copy link
Contributor

Based on the discussion on #37 I made some minor changes to support Kotlin coverage.

I think it might need a little work and maybe a test, but I haven't worked on a Gradle plugin before so I'm not really sure how to write a test for it.

Also, it breaks the logging of classDirectories because it needs to use two file trees. There may be a way around this but I couldn't figure it out short of printing every file.

@SebRut
Copy link

SebRut commented Apr 14, 2018

Could this be re-run/manually evaluated as it seems like the CI failed because of an unrelated connection issue.

Copy link

@SebRut SebRut left a comment

Choose a reason for hiding this comment

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

Feature implementation seems suitable while some changes are unnecessary IMO (see comments).

@@ -45,6 +45,10 @@ class JacocoAndroidPlugin implements Plugin<ProjectInternal> {
plugin
}

private static boolean hasKotlin(PluginContainer plugins) {
Copy link

Choose a reason for hiding this comment

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

Is that method really necessary for one call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's really not. I was just trying to match the style of these (also only used once) but I have no problem in-lining it.

@@ -100,7 +113,6 @@ class JacocoAndroidPlugin implements Plugin<ProjectInternal> {
private void logTaskAdded(JacocoReport reportTask) {
logger.info("Added $reportTask")
logger.info(" executionData: $reportTask.executionData.asPath")
logger.info(" classDirectories: $reportTask.classDirectories.dir.path")
Copy link

Choose a reason for hiding this comment

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

Why was this removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we combine 2 file trees we can no longer reference the class directory path, as it ends up instead listing all the files in the tree, instead of the directory. I wouldn't be surprised if there's a better way to handle this, but I couldn't find one.

project.fileTree(dir: classesDir, excludes: project.jacocoAndroidUnitTestReport.excludes)
if (kotlin) {

Choose a reason for hiding this comment

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

This if-else statement might cause some problems in mixed Java/Kotlin projects. Don't you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it? It uses both java and kotlin trees. Unless the same classes up in both places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our project is mixed and I haven't noticed any issues

@peterfortuin
Copy link

I checked out the pull request branch, build it and applied it to my Kotlin app and my coverage is still reporting 0%. I would expect (after reading the code) that this pull request would fix that for me. But it doesn't.

@rabidaudio
Copy link
Contributor Author

@peterfortuin If you have zero coverage, it sounds like a configuration problem. We still have some problems with running within Android Studio but running the gradle task works with the correct coverage.

This is our configuration, if it is helpful:

apply plugin: 'jacoco-android'

android {
    testOptions.unitTests.all {
        jacoco { includeNoLocationClasses = true }
    }
}

jacoco {
    toolVersion = "0.8.2"
}

jacocoAndroidUnitTestReport {
    csv.enabled false
    html.enabled true
    xml.enabled true

    excludes += ['jdk.internal.*']
}

@arturdm arturdm merged commit baad577 into arturdm:master Feb 24, 2019
@arturdm
Copy link
Owner

arturdm commented Feb 26, 2019

Fixes #37
Fixes #26

Released as part of 0.1.4. Thank you for contributing!

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

Successfully merging this pull request may close these issues.

5 participants