Skip to content

Commit

Permalink
Fix part of #5343: Enable Code Coverage Analysis for many to one test…
Browse files Browse the repository at this point in the history
… target files (#5459)

<!-- READ ME FIRST: Please fill in the explanation section below and
check off every point from the Essential Checklist! -->
## Explanation
<!--
- Explain what your PR does. If this PR fixes an existing bug, please
include
- "Fixes #bugnum:" in the explanation so that GitHub can auto-close the
issue
  - when this PR is merged.
  -->

Fixes part of #5343 

### Project
[PR 2.1 of Project 4.1]

### Changes Made
- Enabling code coverage analysis for many-to-one relationship test
target files by aggregating the coverage reports of each related test
target into a single report using the
`calculateAggregateCoverageReport()` function.
- Using the calculated aggregated coverage report to generate Markdown
and HTML files.
- Properly re-implemented the asynchronous flow as previous
implementation though has async calls was still handling it sequentially
as targets are provided one by one and made to wait until the coverage
report is generated and the previous one is returned back.

**Working test asynchronous flow screenshot**
With static test targets provided:
2ac7b04


![image](https://github.com/oppia/oppia-android/assets/122200035/295cfd9d-7c3a-4784-ba51-a1e8241dccbd)

**Low impact changes:**

Not related to the current PR implementation but added few fixes along
with it,
- Restyled the HTML report slightly since the previous version appeared
somewhat vague and empty 😶‍🌫️.
- Renamed sample test name from `TwoSum` to `AddNums`, felt more
appropriate and clear.

### Note
I couldn't find any actual working app module file with both shared and
local tests to fully test this. Instead, I mocked the behavior with a
sample coverage report and tested it with test cases, assuming that both
tests for the same source file would have identical **line_number**,
**lines_hit**, and **lines_found** data.

___

### Screenshot of current HTML report
![Screenshot
(1460)](https://github.com/oppia/oppia-android/assets/122200035/922a6228-1685-4463-8d64-6d162d5d1e59)

**Responsive layouts**
![zero contributions
(48)](https://github.com/oppia/oppia-android/assets/122200035/626e08eb-eeec-446b-b54c-46dd6aa5eaf8)

## Essential Checklist
<!-- Please tick the relevant boxes by putting an "x" in them. -->
- [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
<!-- Delete these section if this PR does not include UI-related
changes. -->
If your PR includes UI-related changes, then:
- Add screenshots for portrait/landscape for both a tablet & phone of
the before & after UI changes
- For the screenshots above, include both English and pseudo-localized
(RTL) screenshots (see [RTL
guide](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines))
- Add a video showing the full UX flow with a screen reader enabled (see
[accessibility
guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide))
- For PRs introducing new UI elements or color changes, both light and
dark mode screenshots must be included
- Add a screenshot demonstrating that you ran affected Espresso tests
locally & that they're passing
  • Loading branch information
Rd4dev authored Aug 7, 2024
1 parent 7568bd3 commit 8099260
Show file tree
Hide file tree
Showing 9 changed files with 624 additions and 240 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,8 @@ class BazelClient(private val rootDirectory: File, private val commandExecutor:
* or null if the coverage data file could not be parsed
*/
fun runCoverageForTestTarget(bazelTestTarget: String): List<String>? {
val computeInstrumentation = bazelTestTarget.split("/").let { "//${it[2]}/..." }
val instrumentation = bazelTestTarget.split(":")[0]
val computeInstrumentation = instrumentation.split("/").let { "//${it[2]}/..." }
val coverageCommandOutputLines = executeBazelCommand(
"coverage",
bazelTestTarget,
Expand All @@ -159,7 +160,7 @@ class BazelClient(private val rootDirectory: File, private val commandExecutor:
val match = regex.find(line)
val extractedPath = match?.value?.substringAfterLast(",")?.trim()
if (extractedPath != null) {
println("Parsed Coverage Data File: $extractedPath")
println("Raw Coverage Data: $extractedPath")
return extractedPath
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,21 +8,21 @@ import java.io.File
* Class responsible for generating rich text coverage report.
*
* @param repoRoot the root directory of the repository
* @param coverageReportList the list of coverage data proto
* @param coverageReport the coverage data proto
* @param reportFormat the format in which the report will be generated
*/
class CoverageReporter(
private val repoRoot: String,
private val coverageReportList: List<CoverageReport>,
private val coverageReport: CoverageReport,
private val reportFormat: ReportFormat,
) {
private val computedCoverageRatio = computeCoverageRatio()
private val formattedCoveragePercentage = "%.2f".format(computedCoverageRatio * 100)

private val filePath = coverageReportList.firstOrNull()?.filePath ?: "Unknown"
private val filePath = coverageReport.filePath

private val totalLinesFound = coverageReportList.getOrNull(0)?.linesFound ?: 0
private val totalLinesHit = coverageReportList.getOrNull(0)?.linesHit ?: 0
private val totalLinesFound = coverageReport.linesFound
private val totalLinesHit = coverageReport.linesHit

/**
* Generates a rich text report for the analysed coverage data based on the specified format.
Expand All @@ -32,7 +32,7 @@ class CoverageReporter(
* and the second value is the generated report text
*/
fun generateRichTextReport(): Pair<Float, String> {
println("report format: $reportFormat")
println("Report format: $reportFormat")
return when (reportFormat) {
ReportFormat.MARKDOWN -> generateMarkdownReport()
ReportFormat.HTML -> generateHtmlReport()
Expand Down Expand Up @@ -66,6 +66,7 @@ class CoverageReporter(
<style>
body {
font-family: Arial, sans-serif;
font-size: 12px;
line-height: 1.6;
padding: 20px;
}
Expand All @@ -78,24 +79,20 @@ class CoverageReporter(
padding: 8px;
margin-left: 20px;
text-align: left;
border-bottom: 1px solid #fdfdfd;
white-space: pre-wrap;
border-bottom: 1px solid #e3e3e3;
}
.line-number-col {
width: 2%;
width: 4%;
}
.line-number-row {
border-right: 1px dashed #000000
border-right: 1px solid #ababab
}
.source-code-col {
width: 98%;
width: 96%;
}
.covered-line, .not-covered-line, .uncovered-line {
white-space: pre-wrap;
word-wrap: break-word;
box-sizing: border-box;
border-radius: 4px;
padding: 2px 8px 2px 4px;
display: inline-block;
/*white-space: pre-wrap;*/
}
.covered-line {
background-color: #c8e6c9; /* Light green */
Expand All @@ -104,7 +101,7 @@ class CoverageReporter(
background-color: #ffcdd2; /* Light red */
}
.uncovered-line {
background-color: #f1f1f1; /* light gray */
background-color: #f7f7f7; /* light gray */
}
.coverage-summary {
margin-bottom: 20px;
Expand All @@ -118,7 +115,6 @@ class CoverageReporter(
text-align: center;
}
.summary-box {
background-color: #f0f0f0;
border: 1px solid #ccc;
border-radius: 8px;
padding: 10px;
Expand Down Expand Up @@ -152,12 +148,12 @@ class CoverageReporter(
background-color: #ffcdd2; /* Light red */
}
@media screen and (max-width: 768px) {
body {
padding: 10px;
}
table {
width: auto;
}
body {
padding: 10px;
}
table {
width: auto;
}
}
</style>
</head>
Expand Down Expand Up @@ -189,12 +185,11 @@ class CoverageReporter(
""".trimIndent()

val fileContent = File(repoRoot, filePath).readLines()
val coverageMap = coverageReportList
.firstOrNull()?.coveredLineList?.associateBy { it.lineNumber }
val coverageMap = coverageReport.coveredLineList.associateBy { it.lineNumber }

fileContent.forEachIndexed { index, line ->
val lineNumber = index + 1
val lineClass = when (coverageMap?.get(lineNumber)?.coverage) {
val lineClass = when (coverageMap.get(lineNumber)?.coverage) {
Coverage.FULL -> "covered-line"
Coverage.NONE -> "not-covered-line"
else -> "uncovered-line"
Expand All @@ -218,9 +213,8 @@ class CoverageReporter(
}

private fun computeCoverageRatio(): Float {
val report = coverageReportList.getOrNull(0)
return if (report != null && report.linesFound != 0) {
report.linesHit.toFloat() / report.linesFound.toFloat()
return if (coverageReport.linesFound != 0) {
coverageReport.linesHit.toFloat() / coverageReport.linesFound.toFloat()
} else {
0f
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
package org.oppia.android.scripts.coverage

import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Deferred
import kotlinx.coroutines.async
import org.oppia.android.scripts.common.BazelClient
import org.oppia.android.scripts.common.CommandExecutor
import org.oppia.android.scripts.common.ScriptBackgroundCoroutineDispatcher
import org.oppia.android.scripts.proto.BazelTestTarget
import org.oppia.android.scripts.proto.Coverage
import org.oppia.android.scripts.proto.CoverageReport
import org.oppia.android.scripts.proto.CoveredLine
Expand Down Expand Up @@ -34,15 +32,13 @@ class CoverageRunner(
* @param bazelTestTarget Bazel test target to analyze coverage
* @return a deferred value that contains the coverage data
*/
fun runWithCoverageAsync(
fun retrieveCoverageDataForTestTarget(
bazelTestTarget: String
): Deferred<CoverageReport> {
return CoroutineScope(scriptBgDispatcher).async {
val coverageResult = retrieveCoverageResult(bazelTestTarget)
?: error("Failed to retrieve coverage result for $bazelTestTarget")
): CoverageReport {
val coverageResult = retrieveCoverageResult(bazelTestTarget)
?: error("Failed to retrieve coverage result for $bazelTestTarget")

coverageDataFileLines(coverageResult, bazelTestTarget)
}
return coverageDataFileLines(coverageResult, bazelTestTarget)
}

private fun retrieveCoverageResult(
Expand Down Expand Up @@ -93,8 +89,12 @@ class CoverageRunner(
val file = File(repoRoot, filePath)
val fileSha1Hash = calculateSha1(file.absolutePath)

val bazelTestTargetName = BazelTestTarget.newBuilder()
.setTestTargetName(bazelTestTarget)
.build()

return CoverageReport.newBuilder()
.setBazelTestTarget(bazelTestTarget)
.addBazelTestTargets(bazelTestTargetName)
.setFilePath(filePath)
.setFileSha1Hash(fileSha1Hash)
.addAllCoveredLine(coveredLines)
Expand Down
81 changes: 56 additions & 25 deletions scripts/src/java/org/oppia/android/scripts/coverage/RunCoverage.kt
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
package org.oppia.android.scripts.coverage

import kotlinx.coroutines.runBlocking
import org.oppia.android.scripts.common.BazelClient
import org.oppia.android.scripts.common.CommandExecutor
import org.oppia.android.scripts.common.CommandExecutorImpl
import org.oppia.android.scripts.common.ScriptBackgroundCoroutineDispatcher
import org.oppia.android.scripts.proto.Coverage
import org.oppia.android.scripts.proto.CoverageReport
import org.oppia.android.scripts.proto.CoveredLine
import org.oppia.android.scripts.proto.TestFileExemptions
import java.io.File
import java.util.concurrent.TimeUnit
Expand All @@ -27,7 +28,7 @@ import java.util.concurrent.TimeUnit
* utility/src/main/java/org/oppia/android/util/parser/math/MathModel.kt format=HTML
* Example with custom process timeout:
* bazel run //scripts:run_coverage -- $(pwd)
* utility/src/main/java/org/oppia/android/util/parser/math/MathModel.kt processTimeout=10
* utility/src/main/java/org/oppia/android/util/parser/math/MathModel.kt processTimeout=15
*
*/
fun main(vararg args: String) {
Expand All @@ -46,8 +47,8 @@ fun main(vararg args: String) {

val reportOutputPath = getReportOutputPath(repoRoot, filePath, reportFormat)

if (!File(repoRoot, filePath).exists()) {
error("File doesn't exist: $filePath.")
check(File(repoRoot, filePath).exists()) {
"File doesn't exist: $filePath."
}

ScriptBackgroundCoroutineDispatcher().use { scriptBgDispatcher ->
Expand Down Expand Up @@ -110,22 +111,21 @@ class RunCoverage(

if (filePath in testFileExemptionList) {
println("This file is exempted from having a test file; skipping coverage check.")
return
}

val testFilePaths = findTestFile(repoRoot, filePath)
if (testFilePaths.isEmpty()) {
error("No appropriate test file found for $filePath")
}
} else {
val testFilePaths = findTestFiles(repoRoot, filePath)
check(testFilePaths.isNotEmpty()) {
"No appropriate test file found for $filePath"
}

val testTargets = bazelClient.retrieveBazelTargets(testFilePaths)
val testTargets = bazelClient.retrieveBazelTargets(testFilePaths)

val coverageReports = testTargets.mapNotNull { testTarget ->
runCoverageForTarget(testTarget)
}
val coverageReports = testTargets.map { testTarget ->
CoverageRunner(rootDirectory, scriptBgDispatcher, commandExecutor)
.retrieveCoverageDataForTestTarget(testTarget.removeSuffix(".kt"))
}

coverageReports.takeIf { it.isNotEmpty() }?.run {
val reporter = CoverageReporter(repoRoot, this, reportFormat)
val aggregatedCoverageReport = calculateAggregateCoverageReport(coverageReports)
val reporter = CoverageReporter(repoRoot, aggregatedCoverageReport, reportFormat)
val (computedCoverageRatio, reportText) = reporter.generateRichTextReport()

File(reportOutputPath).apply {
Expand All @@ -137,19 +137,50 @@ class RunCoverage(
println("\nComputed Coverage Ratio is: $computedCoverageRatio")
println("\nGenerated report at: $reportOutputPath\n")
}
} ?: println("No coverage reports generated.")
}

private fun runCoverageForTarget(testTarget: String): CoverageReport {
return runBlocking {
CoverageRunner(rootDirectory, scriptBgDispatcher, commandExecutor)
.runWithCoverageAsync(testTarget.removeSuffix(".kt"))
.await()
println("COVERAGE ANALYSIS COMPLETED.")
}
}
}

private fun findTestFile(repoRoot: String, filePath: String): List<String> {
private fun calculateAggregateCoverageReport(
coverageReports: List<CoverageReport>
): CoverageReport {
fun aggregateCoverage(coverages: List<Coverage>): Coverage {
return coverages.find { it == Coverage.FULL } ?: Coverage.NONE
}

val groupedCoverageReports = coverageReports.groupBy {
Pair(it.filePath, it.fileSha1Hash)
}

val (key, reports) = groupedCoverageReports.entries.single()
val (filePath, fileSha1Hash) = key

val allBazelTestTargets = reports.flatMap { it.bazelTestTargetsList }
val allCoveredLines = reports.flatMap { it.coveredLineList }
val groupedCoveredLines = allCoveredLines.groupBy { it.lineNumber }
val aggregatedCoveredLines = groupedCoveredLines.map { (lineNumber, coveredLines) ->
CoveredLine.newBuilder()
.setLineNumber(lineNumber)
.setCoverage(aggregateCoverage(coveredLines.map { it.coverage }))
.build()
}

val totalLinesFound = aggregatedCoveredLines.size
val totalLinesHit = aggregatedCoveredLines.count { it.coverage == Coverage.FULL }

return CoverageReport.newBuilder()
.addAllBazelTestTargets(allBazelTestTargets)
.setFilePath(filePath)
.setFileSha1Hash(fileSha1Hash)
.addAllCoveredLine(aggregatedCoveredLines)
.setLinesFound(totalLinesFound)
.setLinesHit(totalLinesHit)
.build()
}

private fun findTestFiles(repoRoot: String, filePath: String): List<String> {
val possibleTestFilePaths = when {
filePath.startsWith("scripts/") -> {
listOf(filePath.replace("/java/", "/javatests/").replace(".kt", "Test.kt"))
Expand Down
12 changes: 9 additions & 3 deletions scripts/src/java/org/oppia/android/scripts/proto/coverage.proto
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ option java_multiple_files = true;
// Bazel coverage execution.
message CoverageReport {
// The test target for which the coverage report is generated.
string bazel_test_target = 1;
repeated BazelTestTarget bazel_test_targets = 1;
// The relative path of the covered file.
string file_path = 2;
// SHA-1 hash of the file content at the time of report (to guard against changes).
Expand All @@ -22,6 +22,12 @@ message CoverageReport {
int32 lines_hit = 6;
}

// Represents a single bazel test target corresponding to a test file.
message BazelTestTarget {
// The name of the bazel test target.
string test_target_name = 1;
}

// Information about a single line that was covered during the tests.
message CoveredLine {
// The line number of the covered line.
Expand All @@ -33,8 +39,8 @@ message CoveredLine {
enum Coverage {
// Coverage status is unspecified.
UNSPECIFIED = 0;
// The line, branch, or function is fully covered, ie. executed atleast once.
// The line is fully covered in a test, that is, it was executed at least once.
FULL = 1;
// The line, branch, or function is not covered at all.
// The line is not covered at all, that is, it was never executed during a test.
NONE = 2;
}
Loading

0 comments on commit 8099260

Please sign in to comment.