Fix part of #5343: Exempt targets incompatible with code coverage #5480

BenHenning · 2024-08-08T01:50:02Z

Explanation

Fixes part of #5343

This PR prepares for enabling code coverage support in CI by:

Introducing a new source file test exemption property: incompatibility with code coverage (see below for more insight into why this is needed).
Updating the existing test file exemption textproto to include all of the current source files whose corresponding tests do not run correctly with the recently introduced code coverage tooling.
Adding several test declarations that were missed in previous PRs (future Bazel work may include a script that checks for test file declarations to try and reduce the likelihood of this happening in the future).

At a high-level, there are several reasons why a test may fail to run in coverage:

It isn't modularized (this fails for reasons not yet fully understood--see investigation findings in [Feature Request]: Introduce support for code coverage & enforcement (GSoC'24 4.1) #5343), though some unmodularized tests do seem to pass (looking at you, SpotlightFragmentTest).
Its source file is either missing from offline instrumentation, or it simply fails to be resolved. This either results in a runtime failure during the test, or the source file being missing from its coverage.dat file.
Other JaCoCo runtime issues that may imply an incorrect instrumentation filter due to attempting to reinstrument an already instrumented dependency class. A multi-hour search for related problems and an analysis of code coverage support in both Bazel and rules_kotlin did not yield any additional insight into why this situation can happen.
Test files that are incorrectly named or in the wrong location (this PR doesn't make an effort to move these).
And yet other unknown failures that result in instrumentation failure or a failure to generate a coverage.dat file.

There are probably many investigations needed to fix all of these problems, so it makes sense to at least denylist all tests that that are for sure known to fail. This at least sets up the team to get some value from CI-run code coverage, though hard enforcement probably can't be enabled until these stability problems are addressed.

Note that a custom script was written for the purpose of actually performing this analysis, and that can be seen here:

package org.oppia.android.scripts.coverage

import com.google.protobuf.TextFormat
import org.oppia.android.scripts.common.CommandExecutor
import org.oppia.android.scripts.common.CommandExecutorImpl
import org.oppia.android.scripts.common.RepositoryFile
import org.oppia.android.scripts.common.ScriptBackgroundCoroutineDispatcher
import org.oppia.android.scripts.proto.TestFileExemptions
import java.io.File
import java.io.FileInputStream
import java.util.concurrent.TimeUnit

fun main(vararg args: String) {
  val repoRoot = File(args[0]).absoluteFile.normalize()
  // Skip non-Kotlin files, tests, this file, and already exempted files.
  val testFileExemptionList = loadTestFileExemptionsProto()
    .testFileExemptionList
    .filter { it.testFileNotRequired || it.sourceFileIsIncompatibleWithCodeCoverage }
    .mapTo(mutableSetOf()) { it.exemptedFilePath }
  val sourceFiles = RepositoryFile.collectSearchFiles(repoPath = repoRoot.path, expectedExtension = ".kt").filterNot { it.nameWithoutExtension.endsWith("Test") }.filterNot { it.nameWithoutExtension == "RunCoverageForAll" }.distinct()
  val nonExemptedSourceFiles = sourceFiles.filter { it.toRelativeString(repoRoot) !in testFileExemptionList }.sorted()
  println("Found ${sourceFiles.size} source files on which to run tests, but only ${nonExemptedSourceFiles.size} need to be checked (the others should be exempted).")
  val startTime = System.currentTimeMillis()
  val coverageResults = ScriptBackgroundCoroutineDispatcher().use { scriptBgDispatcher ->
    val commandExecutor = CommandExecutorImpl(scriptBgDispatcher, processTimeout = 10, processTimeoutUnit = TimeUnit.MINUTES)
    nonExemptedSourceFiles.withIndex().associate { (index, sourceFile) ->
      val sourcePath = sourceFile.toRelativeString(repoRoot)
      val timeSpentMs = (System.currentTimeMillis() - startTime).toFloat()
      val averageTimeMs = timeSpentMs / index.toFloat()
      val estRemainingTimeMs = averageTimeMs * (nonExemptedSourceFiles.size - index)
      if (index > 0) {
        print("- $sourcePath (${index + 1}/${nonExemptedSourceFiles.size})...~${estRemainingTimeMs / 60000.0f}m left")
      } else print("- $sourcePath (${index + 1}/${nonExemptedSourceFiles.size})...")
      sourcePath to commandExecutor.runCoverageForSourceFile(repoRoot, sourceFile).also { result ->
        println("\r- $sourcePath (${index + 1}/${nonExemptedSourceFiles.size}) -> ${result.summary}")
      }
    }
  }
  println()

  println("Found coverage results for ${coverageResults.size} files. High-level results:")
  val exemptions = coverageResults.values.filter { it == RunCoverageResult.Exempted }
  val failures = coverageResults.filter { (_, result) -> result.isFailure }
  val successes = coverageResults.values.filterIsInstance<RunCoverageResult.RanWithCoverage>()
  val averageCoverageRatio = successes.map { it.ratio }.average()
  println("- ${exemptions.size} source files had additional exemptions for code coverage.")
  println("- ${failures.size} source files failed to compute coverage. By category:")
  failures.values.groupBy { it.summary }.forEach { (summary, failuresGroup) ->
    println("  - ${summary.replaceFirstChar { if (it.isLowerCase()) it.titlecase() else it.toString() }}: ${failuresGroup.size}/${failures.size} failures.")
  }
  println("- Average coverage precentage is (for ${successes.size}/${coverageResults.size} passing tests): ${(averageCoverageRatio * 100.0).toFloat()}%.")
  println()

  println("Recomputed test file exemptions textproto:")
  println()
  println()
  val newExemptions = failures.map { (sourcePath, _) ->
    TestFileExemptions.TestFileExemption.newBuilder().apply {
      this.exemptedFilePath = sourcePath
      this.sourceFileIsIncompatibleWithCodeCoverage = true
    }.build()
  }
  val oldExemptionsProto = loadTestFileExemptionsProto()
  val allExemptions = (oldExemptionsProto.testFileExemptionList + newExemptions).sortedBy { it.exemptedFilePath }
  val updatedExemptionsProto = oldExemptionsProto.toBuilder().apply {
    clearTestFileExemption()
    addAllTestFileExemption(allExemptions)
  }.build()
  TextFormat.printer().print(updatedExemptionsProto, System.out)
  println()
}

private fun CommandExecutor.runCoverageForSourceFile(repoRoot: File, sourceFile: File): RunCoverageResult {
  val result = executeCommand(repoRoot, "bazel", "run", "//scripts:run_coverage", repoRoot.path, sourceFile.toRelativeString(repoRoot), "format=MARKDOWN")
  val outputLines = result.output
  return when {
    result.exitCode == 1 && outputLines.any { "Coverage data not found for the file" in it } -> RunCoverageResult.CoverageNotGenerated
    result.exitCode == 1 && outputLines.any { "No appropriate test file found" in it } -> RunCoverageResult.MissingTestFile
    result.exitCode == 1 && outputLines.any { "Cannot process instrumented class" in it } -> RunCoverageResult.FailsToInstrument
    result.exitCode == 1 && outputLines.any { "tried to access private method" in it } -> RunCoverageResult.CannotAccessJacocoInit
    result.exitCode == 0 && outputLines.any { it.startsWith("This file is exempted from having a test file") } -> RunCoverageResult.Exempted
    result.exitCode == 0 && outputLines.any { it.startsWith("Computed Coverage Ratio") } -> {
      val computedRatioLine = outputLines.single { it.startsWith("Computed Coverage Ratio") }
      val ratioStr = computedRatioLine.substringAfter(":").trim()
      RunCoverageResult.RanWithCoverage(ratioStr.toDouble())
    }
    else -> {
      println()
      println("Failed to run command (exit code ${result.exitCode}):")
      println("  ${result.command.joinToString(separator = " ")}")
      println()
      println("Output:")
      println("#".repeat(80))
      println()
      result.output.forEach(::println)
      println()
      println("#".repeat(80))
      println()
      error("Failed.")
    }
  }
}

private fun loadTestFileExemptionsProto(): TestFileExemptions {
  val protoBinaryFile = File("scripts/assets/test_file_exemptions.pb")
  val builder = TestFileExemptions.getDefaultInstance().newBuilderForType()

  // This cast is type-safe since proto guarantees type consistency from mergeFrom(),
  // and this method is bounded by the generic type T.
  @Suppress("UNCHECKED_CAST")
  val protoObj: TestFileExemptions =
    FileInputStream(protoBinaryFile).use {
      builder.mergeFrom(it)
    }.build() as TestFileExemptions
  return protoObj
}

private sealed class RunCoverageResult {
  abstract val isFailure: Boolean
  abstract val summary: String

  object Exempted : RunCoverageResult() {
    override val isFailure = false
    override val summary = "exempted"
  }

  // Seems to happen to some unmodularized tests.
  object CoverageNotGenerated : RunCoverageResult() {
    override val isFailure = true
    override val summary = "coverage.dat was not generated"
  }

  // Test files needs to be added, renamed, or moved.
  object MissingTestFile : RunCoverageResult() {
    override val isFailure = true
    override val summary = "missing test file"
  }

  // Seems to happen to some unmodularized tests.
  object FailsToInstrument : RunCoverageResult() {
    override val isFailure = true
    override val summary = "fails to instrument"
  }

  // Unclear what the root cause is for these cases; possibly an incorrect instrumentation_filter is
  // being used.
  object CannotAccessJacocoInit : RunCoverageResult() {
    override val isFailure = true
    override val summary = "cannot access private method jacocoInit in unnamed module"
  }

  data class RanWithCoverage(val ratio: Double) : RunCoverageResult() {
    override val isFailure = false
    override val summary: String get() = "coverage ran (${(ratio * 100.0).toFloat()}%)"
  }
}

With its corresponding library Bazel declaration:

kt_jvm_library(
    name = "run_coverage_for_all_lib",
    testonly = True,
    srcs = [
        "RunCoverageForAll.kt",
    ],
    visibility = ["//scripts:oppia_script_binary_visibility"],
    deps = [
        "//scripts/src/java/org/oppia/android/scripts/common:bazel_client",
        "//scripts/src/java/org/oppia/android/scripts/common:repository_file",
        "//scripts/src/java/org/oppia/android/scripts/proto:script_exemptions_java_proto",
    ],
)

and its binary declaration:

kt_jvm_binary(
    name = "run_coverage_for_all",
    testonly = True,
    data = TEST_FILE_EXEMPTION_ASSETS,
    main_class = "org.oppia.android.scripts.coverage.RunCoverageForAllKt",
    runtime_deps = ["//scripts/src/java/org/oppia/android/scripts/coverage:run_coverage_for_all_lib"],
)

This script was a significant help in simplifying the work to determining the list of exempted files. It also seems stable: running it with the latest exemption list correctly only runs valid tests.

The script also produced some high-level outputs which are interesting to note:

1274 total source files exist for potential code coverage (excludes non-Kotlin files, tests, and the RunCoverageForAll script itself).
After omitting exemptions (i.e. those tests which the team has exempted from requiring test files), 387 remaining source files were expected to have 1 or more corresponding tests.
Of those, 230 failed to have code coverage computed (and thus needed to be exempted in this PR). Here's the failure category break-down:
- 115/230 failures occurred from coverage.dat missing after running bazel coverage.
- 32/230 failures occurred from the test file not actually existing where it was expected to be.
- 81/230 failures occurred from a failure to instrument one or more of the classes needed for runtime.
- 2/230 failures occurred from a JaCoCo access issue (likely indicating some significant build configuration issue somewhere in the toolchain).
The average code coverage among the remaining 157 properly passing source files was: 85.15078%. This isn't fully representative since ~60% of source files couldn't be properly tested for coverage, but it's still much higher than expected.

Note to reviewers: scripts/assets/test_file_exemptions.textproto was automatically generated using the script above, and it was sorted by file path to make it a bit cleaner.

Finally, please note that DirectoryManagementUtilTest had one source file change due to a compile-time build warning that wasn't caught before (since the test wasn't being built in Bazel with Kotlin warnings-as-errors enabled).

Essential Checklist

The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
Any changes to scripts/assets files have their rationale included in the PR explanation.
The PR follows the style guide.
The PR does not contain any unnecessary code changes from Android Studio (reference).
The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

N/A -- This is only affecting build and test infrastructure.

PR description will contain much more context, but this also includes adding missing test declarations and some other fixes in existing code.

BenHenning

Self-reviewed.

scripts/src/java/org/oppia/android/scripts/common/BazelClient.kt

scripts/src/java/org/oppia/android/scripts/coverage/RunCoverage.kt

scripts/src/java/org/oppia/android/scripts/proto/script_exemptions.proto

BenHenning · 2024-08-08T02:00:43Z

PTAL @adhiamboperes for codeowners and @Rd4dev for code coverage project interoperability.

Note that this PR has a few changes that @Rd4dev should bring into her PRs, so this PR will have a few additional clean-ups before it's ready to submit. I will ask for re-review once those are done, but please do review it before then if you can.

scripts/src/java/org/oppia/android/scripts/coverage/RunCoverage.kt

github-actions · 2024-08-08T03:38:51Z

APK & AAB differences analysis

Note that this is a summarized snapshot. See the CI artifacts for detailed differences.

Dev

Expand to see flavor specifics

Universal APK

APK file size: 19 MiB (old), 19 MiB (new), 4 bytes (Added)

APK download size (estimated): 17 MiB (old), 17 MiB (new), 3 bytes (Removed)

Method count: 259120 (old), 259120 (new), 0 (No change)

Features: 2 (old), 2 (new), 0 (No change)

Permissions: 6 (old), 6 (new), 0 (No change)

Resources: 6788 (old), 6788 (new), 0 (No change)

Anim: 43 (old), 43 (new), 0 (No change)
Animator: 26 (old), 26 (new), 0 (No change)
Array: 15 (old), 15 (new), 0 (No change)
Attr: 922 (old), 922 (new), 0 (No change)
Bool: 9 (old), 9 (new), 0 (No change)
Color: 954 (old), 954 (new), 0 (No change)
Dimen: 1043 (old), 1043 (new), 0 (No change)
Drawable: 378 (old), 378 (new), 0 (No change)
Id: 1271 (old), 1271 (new), 0 (No change)
Integer: 37 (old), 37 (new), 0 (No change)
Interpolator: 11 (old), 11 (new), 0 (No change)
Layout: 378 (old), 378 (new), 0 (No change)
Menu: 3 (old), 3 (new), 0 (No change)
Mipmap: 1 (old), 1 (new), 0 (No change)
Plurals: 10 (old), 10 (new), 0 (No change)
Raw: 2 (old), 2 (new), 0 (No change)
String: 848 (old), 848 (new), 0 (No change)
Style: 831 (old), 831 (new), 0 (No change)
Xml: 6 (old), 6 (new), 0 (No change)

Lesson assets: 111 (old), 111 (new), 0 (No change)

AAB differences

Expand to see AAB specifics

Supported configurations:

hdpi (same)
ldpi (same)
mdpi (same)
tvdpi (same)
xhdpi (same)
xxhdpi (same)
xxxhdpi (same)

Base APK

APK file size: 18 MiB (old), 18 MiB (new), 4 bytes (Added)
APK download size (estimated): 17 MiB (old), 17 MiB (new), 11 bytes (Removed)

Configuration hdpi

APK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change)
APK download size (estimated): 18 KiB (old), 18 KiB (new), 0 bytes (No change)

Configuration ldpi

APK file size: 49 KiB (old), 49 KiB (new), 0 bytes (No change)
APK download size (estimated): 14 KiB (old), 14 KiB (new), 0 bytes (No change)

Configuration mdpi

APK file size: 45 KiB (old), 45 KiB (new), 0 bytes (No change)
APK download size (estimated): 14 KiB (old), 14 KiB (new), 0 bytes (No change)

Configuration tvdpi

APK file size: 86 KiB (old), 86 KiB (new), 0 bytes (No change)
APK download size (estimated): 29 KiB (old), 29 KiB (new), 0 bytes (No change)

Configuration xhdpi

APK file size: 57 KiB (old), 57 KiB (new), 0 bytes (No change)
APK download size (estimated): 21 KiB (old), 21 KiB (new), 0 bytes (No change)

Configuration xxhdpi

APK file size: 63 KiB (old), 63 KiB (new), 0 bytes (No change)
APK download size (estimated): 29 KiB (old), 29 KiB (new), 0 bytes (No change)

Configuration xxxhdpi

APK file size: 63 KiB (old), 63 KiB (new), 0 bytes (No change)
APK download size (estimated): 28 KiB (old), 28 KiB (new), 0 bytes (No change)

Alpha

Expand to see flavor specifics

Universal APK

APK file size: 11 MiB (old), 11 MiB (new), 4 bytes (Added)

APK download size (estimated): 10 MiB (old), 10 MiB (new), 10 bytes (Removed)

Method count: 115112 (old), 115112 (new), 0 (No change)

Features: 2 (old), 2 (new), 0 (No change)

Permissions: 6 (old), 6 (new), 0 (No change)

Resources: 5758 (old), 5758 (new), 0 (No change)

Anim: 33 (old), 33 (new), 0 (No change)
Animator: 24 (old), 24 (new), 0 (No change)
Array: 14 (old), 14 (new), 0 (No change)
Attr: 888 (old), 888 (new), 0 (No change)
Bool: 8 (old), 8 (new), 0 (No change)
Color: 806 (old), 806 (new), 0 (No change)
Dimen: 778 (old), 778 (new), 0 (No change)
Drawable: 340 (old), 340 (new), 0 (No change)
Id: 1217 (old), 1217 (new), 0 (No change)
Integer: 32 (old), 32 (new), 0 (No change)
Interpolator: 11 (old), 11 (new), 0 (No change)
Layout: 341 (old), 341 (new), 0 (No change)
Menu: 1 (old), 1 (new), 0 (No change)
Mipmap: 1 (old), 1 (new), 0 (No change)
Plurals: 10 (old), 10 (new), 0 (No change)
String: 781 (old), 781 (new), 0 (No change)
Style: 472 (old), 472 (new), 0 (No change)
Xml: 1 (old), 1 (new), 0 (No change)

Lesson assets: 111 (old), 111 (new), 0 (No change)

AAB differences

Expand to see AAB specifics

Supported configurations:

hdpi (same)
ldpi (same)
mdpi (same)
tvdpi (same)
xhdpi (same)
xxhdpi (same)
xxxhdpi (same)

Base APK

APK file size: 10 MiB (old), 10 MiB (new), 4 bytes (Added)
APK download size (estimated): 9 MiB (old), 9 MiB (new), 14 bytes (Added)

Configuration hdpi

APK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change)
APK download size (estimated): 17 KiB (old), 17 KiB (new), 0 bytes (No change)

Configuration ldpi

APK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change)
APK download size (estimated): 13 KiB (old), 13 KiB (new), 0 bytes (No change)

Configuration mdpi

APK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change)
APK download size (estimated): 13 KiB (old), 13 KiB (new), 0 bytes (No change)

Configuration tvdpi

APK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change)
APK download size (estimated): 27 KiB (old), 27 KiB (new), 0 bytes (No change)

Configuration xhdpi

APK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change)
APK download size (estimated): 20 KiB (old), 20 KiB (new), 0 bytes (No change)

Configuration xxhdpi

APK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change)
APK download size (estimated): 28 KiB (old), 28 KiB (new), 0 bytes (No change)

Configuration xxxhdpi

APK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change)
APK download size (estimated): 27 KiB (old), 27 KiB (new), 0 bytes (No change)

Beta

Expand to see flavor specifics

Universal APK

APK file size: 11 MiB (old), 11 MiB (new), 0 bytes (No change)

APK download size (estimated): 10 MiB (old), 10 MiB (new), 13 bytes (Removed)

Method count: 115108 (old), 115108 (new), 0 (No change)

Features: 2 (old), 2 (new), 0 (No change)

Permissions: 6 (old), 6 (new), 0 (No change)

Resources: 5758 (old), 5758 (new), 0 (No change)

Anim: 33 (old), 33 (new), 0 (No change)
Animator: 24 (old), 24 (new), 0 (No change)
Array: 14 (old), 14 (new), 0 (No change)
Attr: 888 (old), 888 (new), 0 (No change)
Bool: 8 (old), 8 (new), 0 (No change)
Color: 806 (old), 806 (new), 0 (No change)
Dimen: 778 (old), 778 (new), 0 (No change)
Drawable: 340 (old), 340 (new), 0 (No change)
Id: 1217 (old), 1217 (new), 0 (No change)
Integer: 32 (old), 32 (new), 0 (No change)
Interpolator: 11 (old), 11 (new), 0 (No change)
Layout: 341 (old), 341 (new), 0 (No change)
Menu: 1 (old), 1 (new), 0 (No change)
Mipmap: 1 (old), 1 (new), 0 (No change)
Plurals: 10 (old), 10 (new), 0 (No change)
String: 781 (old), 781 (new), 0 (No change)
Style: 472 (old), 472 (new), 0 (No change)
Xml: 1 (old), 1 (new), 0 (No change)

Lesson assets: 111 (old), 111 (new), 0 (No change)

AAB differences

Expand to see AAB specifics

Supported configurations:

hdpi (same)
ldpi (same)
mdpi (same)
tvdpi (same)
xhdpi (same)
xxhdpi (same)
xxxhdpi (same)

Base APK

APK file size: 10 MiB (old), 10 MiB (new), 4 bytes (Added)
APK download size (estimated): 9 MiB (old), 9 MiB (new), 7 bytes (Added)

Configuration hdpi

APK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change)
APK download size (estimated): 17 KiB (old), 17 KiB (new), 0 bytes (No change)

Configuration ldpi

APK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change)
APK download size (estimated): 13 KiB (old), 13 KiB (new), 0 bytes (No change)

Configuration mdpi

APK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change)
APK download size (estimated): 13 KiB (old), 13 KiB (new), 0 bytes (No change)

Configuration tvdpi

APK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change)
APK download size (estimated): 27 KiB (old), 27 KiB (new), 0 bytes (No change)

Configuration xhdpi

APK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change)
APK download size (estimated): 20 KiB (old), 20 KiB (new), 0 bytes (No change)

Configuration xxhdpi

APK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change)
APK download size (estimated): 28 KiB (old), 28 KiB (new), 0 bytes (No change)

Configuration xxxhdpi

APK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change)
APK download size (estimated): 27 KiB (old), 27 KiB (new), 0 bytes (No change)

Ga

Expand to see flavor specifics

Universal APK

APK file size: 11 MiB (old), 11 MiB (new), 8 bytes (Added)

APK download size (estimated): 10 MiB (old), 10 MiB (new), 4 bytes (Added)

Method count: 115108 (old), 115108 (new), 0 (No change)

Features: 2 (old), 2 (new), 0 (No change)

Permissions: 6 (old), 6 (new), 0 (No change)

Resources: 5758 (old), 5758 (new), 0 (No change)

Anim: 33 (old), 33 (new), 0 (No change)
Animator: 24 (old), 24 (new), 0 (No change)
Array: 14 (old), 14 (new), 0 (No change)
Attr: 888 (old), 888 (new), 0 (No change)
Bool: 8 (old), 8 (new), 0 (No change)
Color: 806 (old), 806 (new), 0 (No change)
Dimen: 778 (old), 778 (new), 0 (No change)
Drawable: 340 (old), 340 (new), 0 (No change)
Id: 1217 (old), 1217 (new), 0 (No change)
Integer: 32 (old), 32 (new), 0 (No change)
Interpolator: 11 (old), 11 (new), 0 (No change)
Layout: 341 (old), 341 (new), 0 (No change)
Menu: 1 (old), 1 (new), 0 (No change)
Mipmap: 1 (old), 1 (new), 0 (No change)
Plurals: 10 (old), 10 (new), 0 (No change)
String: 781 (old), 781 (new), 0 (No change)
Style: 472 (old), 472 (new), 0 (No change)
Xml: 1 (old), 1 (new), 0 (No change)

Lesson assets: 111 (old), 111 (new), 0 (No change)

AAB differences

Expand to see AAB specifics

Supported configurations:

hdpi (same)
ldpi (same)
mdpi (same)
tvdpi (same)
xhdpi (same)
xxhdpi (same)
xxxhdpi (same)

Base APK

APK file size: 10 MiB (old), 10 MiB (new), 4 bytes (Added)
APK download size (estimated): 9 MiB (old), 9 MiB (new), 14 bytes (Added)

Configuration hdpi

APK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change)
APK download size (estimated): 17 KiB (old), 17 KiB (new), 0 bytes (No change)

Configuration ldpi

APK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change)
APK download size (estimated): 13 KiB (old), 13 KiB (new), 0 bytes (No change)

Configuration mdpi

APK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change)
APK download size (estimated): 13 KiB (old), 13 KiB (new), 0 bytes (No change)

Configuration tvdpi

APK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change)
APK download size (estimated): 27 KiB (old), 27 KiB (new), 0 bytes (No change)

Configuration xhdpi

APK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change)
APK download size (estimated): 20 KiB (old), 20 KiB (new), 0 bytes (No change)

Configuration xxhdpi

APK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change)
APK download size (estimated): 28 KiB (old), 28 KiB (new), 0 bytes (No change)

Configuration xxxhdpi

APK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change)
APK download size (estimated): 27 KiB (old), 27 KiB (new), 0 bytes (No change)

adhiamboperes

Thanks @BenHenning, I took a pass and I don't have concerns that you haven't documented.

Rd4dev · 2024-08-09T01:58:59Z

Thanks, @BenHenning. I've incorporated all the changes into #5461 and added tests for them. I've also tested this PR with a few incompatible sources, and they were exempted as expected. Other than adjusting the exemption message, Todo addition and adding the additional test case (everything you mentioned), this looks good. I can take another look and approve once the refactoring and test additions are done if needed.

This removes code that was moved to a downstream PR, adds missing test coverage for the new exemption behavior, and improves the messaging for the new exemption behavior. Also, updates the shard count for RunCoverageTest since 4 seemed to be a bit too few when running the test locally since the tests can take quite some time to complete.

…e-coverage

BenHenning

Self-reviewed changes.

Thanks again @Rd4dev and @adhiamboperes for the reviews and follow-ups. The latest changes should be code complete from my perspective, so PTAL and let me know if any additional changes are needed.

scripts/src/java/org/oppia/android/scripts/coverage/RunCoverage.kt

scripts/src/java/org/oppia/android/scripts/proto/script_exemptions.proto

scripts/src/java/org/oppia/android/scripts/coverage/RunCoverage.kt

scripts/src/java/org/oppia/android/scripts/common/BazelClient.kt

scripts/src/java/org/oppia/android/scripts/coverage/RunCoverage.kt

adhiamboperes

Thanks @BenHenning! This LGTM.

scripts/src/javatests/org/oppia/android/scripts/coverage/RunCoverageTest.kt

BenHenning

Self-reviewed latest changes.

Thanks @adhiamboperes & @Rd4dev for the review.

@Rd4dev I followed up with a change per your comment. PTAL and resolve the thread if it looks good to you. Otherwise, please let me know if any follow-up changes are needed.

I'm also enabling auto-merge on this PR since it seems like everything is addressed at this point, and it's needed for RD's downstream PRs.

scripts/src/javatests/org/oppia/android/scripts/coverage/RunCoverageTest.kt

Exempt targets incompatible with code coverage.

46a0b04

PR description will contain much more context, but this also includes adding missing test declarations and some other fixes in existing code.

BenHenning commented Aug 8, 2024

View reviewed changes

BenHenning assigned adhiamboperes and Rd4dev Aug 8, 2024

BenHenning marked this pull request as ready for review August 8, 2024 01:59

BenHenning requested review from a team as code owners August 8, 2024 01:59

BenHenning requested a review from adhiamboperes August 8, 2024 01:59

BenHenning commented Aug 8, 2024

View reviewed changes

scripts/src/java/org/oppia/android/scripts/coverage/RunCoverage.kt Outdated Show resolved Hide resolved

BenHenning commented Aug 8, 2024

View reviewed changes

scripts/src/java/org/oppia/android/scripts/coverage/RunCoverage.kt Outdated Show resolved Hide resolved

Rd4dev mentioned this pull request Aug 8, 2024

[BUG]: Certain test targets fail to execute bazel coverage commands #5481

Open

adhiamboperes reviewed Aug 8, 2024

View reviewed changes

adhiamboperes assigned BenHenning and Rd4dev and unassigned Rd4dev and adhiamboperes Aug 8, 2024

Rd4dev removed their assignment Aug 9, 2024

BenHenning added 2 commits August 8, 2024 22:20

Merge branch 'develop' into exempt-test-targets-incompatible-with-cod…

172b91f

…e-coverage

BenHenning commented Aug 9, 2024

View reviewed changes

BenHenning assigned adhiamboperes and Rd4dev and unassigned BenHenning Aug 9, 2024

adhiamboperes approved these changes Aug 9, 2024

View reviewed changes

adhiamboperes assigned BenHenning and unassigned adhiamboperes Aug 9, 2024

Rd4dev reviewed Aug 9, 2024

View reviewed changes

scripts/src/javatests/org/oppia/android/scripts/coverage/RunCoverageTest.kt Outdated Show resolved Hide resolved

Address review comment.

3f8ea11

BenHenning commented Aug 10, 2024

View reviewed changes

scripts/src/javatests/org/oppia/android/scripts/coverage/RunCoverageTest.kt Outdated Show resolved Hide resolved

BenHenning enabled auto-merge (squash) August 10, 2024 00:06

BenHenning removed their assignment Aug 10, 2024

BenHenning merged commit d0d1839 into develop Aug 10, 2024
26 checks passed

BenHenning deleted the exempt-test-targets-incompatible-with-code-coverage branch August 10, 2024 01:28

Rd4dev removed their assignment Aug 10, 2024

Rd4dev mentioned this pull request Aug 10, 2024

Fix part of #5343: Enable Coverage Generation for a list of files #5461

Merged

6 tasks

BenHenning mentioned this pull request Sep 12, 2024

[Feature Request]: Introduce support for code coverage & enforcement (GSoC'24 4.1) #5343

Closed

Fix part of #5343: Exempt targets incompatible with code coverage #5480

Fix part of #5343: Exempt targets incompatible with code coverage #5480

Conversation

BenHenning commented Aug 8, 2024 • edited Loading

Explanation

Essential Checklist

For UI-specific PRs only

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning commented Aug 8, 2024

github-actions bot commented Aug 8, 2024

APK & AAB differences analysis

Dev

Universal APK

AAB differences

Base APK

Configuration hdpi

Configuration ldpi

Configuration mdpi

Configuration tvdpi

Configuration xhdpi

Configuration xxhdpi

Configuration xxxhdpi

Alpha

Universal APK

AAB differences

Base APK

Configuration hdpi

Configuration ldpi

Configuration mdpi

Configuration tvdpi

Configuration xhdpi

Configuration xxhdpi

Configuration xxxhdpi

Beta

Universal APK

AAB differences

Base APK

Configuration hdpi

Configuration ldpi

Configuration mdpi

Configuration tvdpi

Configuration xhdpi

Configuration xxhdpi

Configuration xxxhdpi

Ga

Universal APK

AAB differences

Base APK

Configuration hdpi

Configuration ldpi

Configuration mdpi

Configuration tvdpi

Configuration xhdpi

Configuration xxhdpi

Configuration xxxhdpi

adhiamboperes left a comment

Choose a reason for hiding this comment

Rd4dev commented Aug 9, 2024

BenHenning left a comment

Choose a reason for hiding this comment

adhiamboperes left a comment

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning commented Aug 8, 2024 •

edited

Loading