ankidroid / Anki-Android

AnkiDroid: Anki flashcards on Android. Your secret trick to achieve superhuman information retention.
GNU General Public License v3.0
8.51k stars 2.21k forks source link

[Bug] JaCoCo does not include Kotlin files #9432

Closed david-allison closed 2 years ago

mikehardy commented 3 years ago

ah fer the love o' ... :sweat_smile:

david-allison commented 3 years ago

https://medium.com/wandera-engineering/android-kotlin-code-coverage-with-jacoco-sonar-and-gradle-plugin-6-x-3933ed503a6e looked interesting as it's fairly close to our current implementation.

C:\GitHub\Anki-Android-Git\Anki-Android\AnkiDroid\build\intermediates\javac\amazonDebug\classes doesn't contain kotlin files.

These exist in C:\GitHub\Anki-Android-Git\Anki-Android\AnkiDroid\build\tmp\kotlin-classes\amazonDebug\com\ichi2\anki

Will need a good regex to handle nested classes (and determine which are necessary), as Kotlin produces a significant amount of them

github-actions[bot] commented 2 years ago

Hello 👋, this issue has been opened for more than 2 months with no activity on it. If the issue is still here, please keep in mind that we need community support and help to fix it! Just comment something like still searching for solutions and if you found one, please open a pull request! You have 7 days until this gets closed automatically

github-actions[bot] commented 2 years ago

Hello 👋, this issue has been opened for more than 2 months with no activity on it. If the issue is still here, please keep in mind that we need community support and help to fix it! Just comment something like still searching for solutions and if you found one, please open a pull request! You have 7 days until this gets closed automatically

gpauer commented 2 years ago

I like regex please assign to me :)

mikehardy commented 2 years ago

Yessss - code coverage can be useless or useful, historically here I think we have used in a way that is useful, it has been for me anyway. Would love to see it back up for Kotlin code

mikehardy commented 2 years ago

This is no intention to pressure or anything: I was just in codecov fixing up it's stale default branch reference so it points to main, and re-checked this. We are definitely not getting coverage for kotlin still - it did not magically fix itself ;-). Not the highest priority issue here but this is a nice development aide if it can get fixed up.

Tinkidinki commented 2 years ago

I have a couple of questions on this PR. Context: I'm trying to understand how we have decided what Java files to include for jacoco, in order to do the same for Kotlin

In the jacoco.gradle file, lines 100-102, we have:

  def fileFilter = ['**/R.class', '**/R$*.class', '**/BuildConfig.*', '**/Manifest*.*', '**/*Test*.*', 'android/**/*.*']
  def debugTree = fileTree(dir: "$project.buildDir/intermediates/javac/playDebug/classes", excludes: fileFilter)
  def mainSrc = "$project.projectDir/src/main/java"

1) What does **/R$*.class match? More specifically, what does the $ do here? According to this website, $ marks end of string, but it is followed by a * in the above code, so I'm a bit confused. To understand how the others work, I assume they follow the syntax explained in this answer.

2) Surprisingly, I didn't find files in $project.buildDir/intermediates/javac/playDebug/classes matching the exclude syntax, except the files starting with android. Which files match the other exclude globs?

3) A more general question: how did we decide which files jacoco must not see?

david-allison commented 2 years ago

Drop back to the history and this was added in 7acc79e14e601481b2082599d118a6888e3085f1 : https://github.com/ankidroid/Anki-Android/pull/4919

@mikehardy may have more context

1) R.class was the class providing resource values. I don't believe that this exists any more as Android no longer provides 'constant' IDs here.

Are you sure this is a Regex, and not a file system globbing. I'd expect

'match R' 'match $' 'match 0 or more characters' 'match .class'

2)

You're probably not building for 'playDebug'. They exist.

Screenshot 2022-05-07 at 20 28 57

3)

Arbitrary: Remove BuildConfig as it's generated, remove other generated files.

Then: Let's get everything and decide on a case-by-case basis. Do check with Mike as he set this up originally

mikehardy commented 2 years ago

I did set it up and I remember setting it up but I don't remember spending a lot of time on those file excludes, I was following a recipe and it was just part of the recipe. I would do whatever seems reasonable, where reasonable is "we want coverage of all the code we write, and we want to ignore code we did not write including auto-generated classes", that's the general rule

Tinkidinki commented 2 years ago

Also, @gpauer, may I work on this? We could discuss together and work, if you're interested!