Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Browse files
Browse the repository at this point in the history
) ## 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 #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: ```kotlin 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: ```bazel 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: ```bazel 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 - [x] 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: ...".) - [x] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation. - [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide). - [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)). - [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop". - [x] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)). ## For UI-specific PRs only N/A -- This is only affecting build and test infrastructure.
- Loading branch information
1 parent
83f5356
commit d0d1839