Fix part of #5343: Introduce new CI workflow for Code Coverage

.github/workflows/code_coverage.yml

@@ -12,19 +12,19 @@ on:
       - develop
 
 jobs:
-#  check_unit_tests_completed:
-#    name: Check unit test completed
-#    runs-on: ubuntu-latest
-#    steps:
-#      - name: Wait for unit tests to checks
-#        uses: ArcticLampyrid/[email protected]
-#        with:
-#          workflow: unit_tests.yml
-#          sha: auto
+  check_unit_tests_completed:
+    name: Check unit test completed
+    runs-on: ubuntu-latest
+    steps:
+      - name: Wait for unit tests to checks
+        uses: ArcticLampyrid/[email protected]
+        with:
+          workflow: unit_tests.yml
+          sha: auto
 
   compute_changed_files:
     name: Compute changed files
-#    needs: check_unit_tests_completed
+    needs: check_unit_tests_completed
     runs-on: ubuntu-20.04
     outputs:
       matrix: ${{ steps.compute-file-matrix.outputs.matrix }}
@@ -215,55 +215,6 @@ jobs:
           echo "build --disk_cache=$EXPANDED_BAZEL_CACHE_PATH" >> $HOME/.bazelrc
         shell: bash
 
-      # See explanation in bazel_build_app for how this is installed.
-      - name: Install git-secret (non-fork only)
-        if: ${{ env.ENABLE_CACHING == 'true' && ((github.ref == 'refs/heads/develop' && github.event_name == 'push') || (github.event.pull_request.head.repo.full_name == 'oppia/oppia-android')) }}
-        shell: bash
-        run: |
-          cd $HOME
-          mkdir -p $HOME/gitsecret
-          git clone https://github.com/sobolevn/git-secret.git git-secret
-          cd git-secret && make build
-          PREFIX="$HOME/gitsecret" make install
-          echo "$HOME/gitsecret" >> $GITHUB_PATH
-          echo "$HOME/gitsecret/bin" >> $GITHUB_PATH
-
-      - name: Decrypt secrets (non-fork only)
-        if: ${{ env.ENABLE_CACHING == 'true' && ((github.ref == 'refs/heads/develop' && github.event_name == 'push') || (github.event.pull_request.head.repo.full_name == 'oppia/oppia-android')) }}
-        env:
-          GIT_SECRET_GPG_PRIVATE_KEY: ${{ secrets.GIT_SECRET_GPG_PRIVATE_KEY }}
-        run: |
-          cd $HOME
-          # NOTE TO DEVELOPERS: Make sure to never print this key directly to stdout!
-          echo $GIT_SECRET_GPG_PRIVATE_KEY | base64 --decode > ./git_secret_private_key.gpg
-          gpg --import ./git_secret_private_key.gpg
-          cd $GITHUB_WORKSPACE
-          git secret reveal
-
-      # See https://www.cyberciti.biz/faq/unix-for-loop-1-to-10/ for for-loop reference.
-      - name: Build Oppia Tests (with caching, non-fork only)
-        if: ${{ env.ENABLE_CACHING == 'true' && ((github.ref == 'refs/heads/develop' && github.event_name == 'push') || (github.event.pull_request.head.repo.full_name == 'oppia/oppia-android')) }}
-        env:
-          BAZEL_REMOTE_CACHE_URL: ${{ secrets.BAZEL_REMOTE_CACHE_URL }}
-          BAZEL_TEST_TARGETS: ${{ env.BAZEL_TEST_TARGETS }}
-        run: |
-          # Attempt to build 5 times in case there are flaky builds.
-          # TODO(#3759): Remove this once there are no longer app test build failures.
-          i=0
-          # Disable exit-on-first-failure.
-          set +e
-          while [ $i -ne 5 ]; do
-            i=$(( $i+1 ))
-            echo "Attempt $i/5 to build test targets"
-            bazel build --keep_going --remote_http_cache=$BAZEL_REMOTE_CACHE_URL --google_credentials=./config/oppia-dev-workflow-remote-cache-credentials.json -- $BAZEL_TEST_TARGETS
-          done
-          # Capture the error code of the final command run (which should be a success if there isn't a real build failure).
-          last_error_code=$?
-          # Reenable exit-on-first-failure.
-          set -e
-          # Exit only if the most recent exit was a failure (by using a subshell).
-          (exit $last_error_code)
-
       - name: Build Oppia Tests (without caching, or on a fork)
         if: ${{ env.ENABLE_CACHING == 'false' || ((github.ref != 'refs/heads/develop' || github.event_name != 'push') && (github.event.pull_request.head.repo.full_name != 'oppia/oppia-android')) }}
         env:
@@ -286,14 +237,6 @@ jobs:
           # Exit only if the most recent exit was a failure (by using a subshell).
           (exit $last_error_code)
 
-      - name: Run Oppia Coverage (with caching, non-fork only)
-        if: ${{ env.ENABLE_CACHING == 'true' && ((github.ref == 'refs/heads/develop' && github.event_name == 'push') || (github.event.pull_request.head.repo.full_name == 'oppia/oppia-android')) }}
-        env:
-          BAZEL_REMOTE_CACHE_URL: ${{ secrets.BAZEL_REMOTE_CACHE_URL }}
-          CHANGED_FILES: ${{ env.CHANGED_FILES }}
-        run: |
-          bazel run //scripts:run_coverage -- $(pwd) $CHANGED_FILES --format=PROTO --processTimeout=15
-
       - name: Run Oppia Coverage (without caching, or on a fork)
         if: ${{ env.ENABLE_CACHING == 'false' || ((github.ref != 'refs/heads/develop' || github.event_name != 'push') && (github.event.pull_request.head.repo.full_name != 'oppia/oppia-android')) }}
         env:

scripts/src/java/org/oppia/android/scripts/ci/ComputeChangedFiles.kt

@@ -115,10 +115,12 @@ class ComputeChangedFiles(
     val currentBranch = gitClient.currentBranch.lowercase(Locale.US)
     val changedFiles = if (computeAllFilesSetting || currentBranch == "develop") {
       computeAllFiles(rootDirectory, pathToRoot)
-    } else computeChangedFilesForNonDevelopBranch(gitClient, rootDirectory)
+    } else computeChangedFilesForNonDevelopBranch(gitClient, rootDirectory, pathToRoot)
 
-    val ktFiles = changedFiles.filter { it.endsWith(".kt") }
-    val filteredFiles = filterFiles(ktFiles)
+    val filteredFiles = filterFiles(changedFiles)
+    println()
+    println("Changed Files:")
+    println(filteredFiles.joinToString(separator = "\n") { "- $it" })
 
     val changedFileBuckets = bucketFiles(filteredFiles)
     val encodedFileBucketEntries = changedFileBuckets
@@ -148,16 +150,31 @@ class ComputeChangedFiles(
 
   private fun computeChangedFilesForNonDevelopBranch(
     gitClient: GitClient,
-    rootDirectory: File
+    rootDirectory: File,
+    pathToRoot: String
   ): List<String> {
-    return gitClient.changedFiles
+    val changedKtFiles = gitClient.committedFiles
       .map { File(rootDirectory, it) }
       .filter { it.exists() }
       .map { it.toRelativeString(rootDirectory) }
+      .filter { it.endsWith(".kt") }
+
+    return changedKtFiles
+      .map { changedKtFile ->
+        when {
+          changedKtFile.endsWith("Test.kt") -> {
+            mapTestFileToSourceFile(rootDirectory, pathToRoot, changedKtFile)
+          }
+          changedKtFile.endsWith(".kt") -> changedKtFile
+          else -> null
+        }
+      }
+      .filterNotNull()
+      .distinct()
   }
 
   private fun filterFiles(files: List<String>): List<String> {
-    // Filtering out files that need to be ignored.
+    // Filter out instrumentation files since code coverage only runs on unit tests.
     return files.filter { file ->
       !file
         .startsWith(
@@ -167,6 +184,44 @@ class ComputeChangedFiles(
     }
   }
 
+  private fun mapTestFileToSourceFile(
+    rootDirectory: File,
+    repoRoot: String,
+    filePath: String
+  ): String? {
+    val repoRootFile = File(repoRoot).absoluteFile
+
+    val possibleSourceFilePaths = when {
+      filePath.startsWith("scripts/") -> {
+        listOf(filePath.replace("/javatests/", "/java/").replace("Test.kt", ".kt"))
+      }
+      filePath.startsWith("app/") -> {
+        when {
+          filePath.contains("/sharedTest/") -> {
+            listOf(filePath.replace("/sharedTest/", "/main/").replace("Test.kt", ".kt"))
+          }
+          filePath.contains("/test/") -> {
+            listOf(
+              filePath.replace("/test/", "/main/").replace("Test.kt", ".kt"),
+              filePath.replace("/test/", "/main/").replace("LocalTest.kt", ".kt")
+            )
+          }
+          else -> emptyList()
+        }
+      }
+      else -> {
+        listOf(filePath.replace("/test/", "/main/").replace("Test.kt", ".kt"))
+      }
+    }
+
+    return possibleSourceFilePaths
+      .mapNotNull { path ->
+        val file = File(repoRootFile, path)
+        file.takeIf { it.exists() }?.toRelativeString(rootDirectory)
+      }
+      .firstOrNull()
+  }
+
   private fun bucketFiles(filteredFiles: List<String>): List<ChangedFilesBucket> {
     val groupedBuckets = filteredFiles.groupBy { FileBucket.retrieveCorrespondingFileBucket(it) }
       .entries.groupBy(

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

@@ -27,6 +27,9 @@ class GitClient(
    */
   val changedFiles: Set<String> by lazy { retrieveChangedFilesWithPotentialDuplicates().toSet() }
 
+  /** The list of files that have been committed in the local branch. */
+  val committedFiles: List<String> by lazy { retrieveChangedCommittedFiles() }
+
   private fun retrieveCurrentCommit(): String {
     return executeGitCommandWithOneLineOutput("rev-parse HEAD")
   }

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

@@ -253,7 +253,7 @@ class CoverageReporter(
       report.failure?.let { failure ->
         val failurePath = failure.filePath
           ?.takeIf { it.isNotEmpty() }
-          ?.let { getFilenameAsLink(it) }
+          ?.let { getFilenameAsDetailsSummary(it) }
           ?: failure.bazelTestTarget
         "| $failurePath | ${failure.failureMessage} |"
       }
@@ -315,8 +315,8 @@ class CoverageReporter(
       .filter { it.hasExemption() }
       .map { exemption ->
         val filePath = exemption.exemption.filePath
-        "${getFilenameAsLink(filePath)}"
-      }.joinToString(separator = "\n") { "- $it" }
+        "${getFilenameAsDetailsSummary(filePath)}"
+      }.joinToString(separator = "\n") { "$it" }
 
     val tableHeader = buildString {
       append("| File | Coverage | Lines Hit | Status | Min Required |\n")
@@ -394,8 +394,10 @@ class CoverageReporter(
     val testFileExemptedSection = buildString {
       if (testFileExemptedCasesList.isNotEmpty()) {
         append("\n\n")
-        append("### Files Exempted from Coverage\n")
+        append("### Exempted coverage\n")
+        append("<details><summary>Files exempted from coverage</summary> <br>")
         append(testFileExemptedCasesList)
+        append("</details>")
       }
     }
 
@@ -444,19 +446,10 @@ class CoverageReporter(
   }
 
   private fun calculateOverallCoveragePercentage(): Float {
-    var totalLinesFound = 0
-    var totalLinesHit = 0
-
-    coverageReportContainer.coverageReportList.forEach { report ->
-      report.details?.let {
-        totalLinesFound += it.linesFound
-        totalLinesHit += it.linesHit
-      }
-    }
-
-    return totalLinesFound.takeIf { it > 0 }
-      ?.let { totalLinesHit.toFloat() / it * 100 }
-      ?: 0.0f
+    val reports = coverageReportContainer.coverageReportList
+    val totalLinesFound = reports.sumOf { it.details?.linesFound ?: 0 }.toFloat()
+    val totalLinesHit = reports.sumOf { it.details?.linesHit ?: 0 }.toFloat()
+    return if (totalLinesFound > 0) (totalLinesHit * 100.0f) / totalLinesFound else 0.0f
   }
 
   private fun logCoverageReport() {
@@ -494,12 +487,13 @@ class CoverageReporter(
           }
 
           if (coveragePercentage < minRequiredCoverage) {
+            val exemptionText = exemption?.let { "(exemption)" } ?: ""
             failureReports.appendLine(
               """
               |Covered File: $filePath
               |Coverage percentage: $formattedCoveragePercentage% covered
               |Line coverage: $totalLinesHit / $totalLinesFound lines covered
-              |Minimum Required: $minRequiredCoverage% "${exemption?.let { "(exemption)" } ?: ""}"
+              |Minimum Required: $minRequiredCoverage% $exemptionText
               |------------------------
               """.trimMargin().prependIndent("  ")
             )
@@ -539,7 +533,7 @@ class CoverageReporter(
         )
         val formattedCoveragePercentage = "%.2f".format(coveragePercentage)
 
-        "| ${getFilenameAsLink(filePath)} | $formattedCoveragePercentage% | " +
+        "| ${getFilenameAsDetailsSummary(filePath)} | $formattedCoveragePercentage% | " +
           "$totalLinesHit / $totalLinesFound | $statusSymbol | $exemptionPercentage |"
       }
       .joinToString(separator = "\n")
@@ -583,9 +577,6 @@ private fun getReportOutputPath(
   return "$repoRoot/coverage_reports/$fileWithoutExtension/$defaultFilename"
 }
 
-private fun getFilenameAsLink(filePath: String): String {
-  val oppiaDevelopGitHubLink = "https://github.com/oppia/oppia-android/tree/develop"
-  val filename = filePath.substringAfterLast("/").trim()
-  val filenameAsLink = "[$filename]($oppiaDevelopGitHubLink/$filePath)"
-  return filenameAsLink
+private fun getFilenameAsDetailsSummary(filePath: String): String {
+  return "<details><summary>${filePath.substringAfterLast("/")}</summary>$filePath</details>"
 }

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

@@ -40,16 +40,14 @@ class CoverageRunner(
     val coverageResults = bazelClient.runCoverageForTestTarget(bazelTestTarget)
 
     return coverageResults
-      .takeIf { it.isNotEmpty() }
-      ?.map { singleCoverageDatFileLines ->
+      .map { singleCoverageDatFileLines ->
         parseCoverageDataFileLines(singleCoverageDatFileLines, bazelTestTarget)
-      }
-      ?: listOf(
-        generateFailedCoverageReport(
-          bazelTestTarget,
-          "Coverage retrieval failed for the test target: $bazelTestTarget"
-        )
+      }.takeIf { it.isNotEmpty() } ?: listOf(
+      generateFailedCoverageReport(
+        bazelTestTarget,
+        "Coverage retrieval failed for the test target: $bazelTestTarget"
       )
+    )
   }
 
   private fun parseCoverageDataFileLines(