Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -323,3 +323,31 @@ oppia_android_test(
"//utility/src/main/java/org/oppia/android/util/networking:debug_module",
],
)

oppia_android_test(
name = "FirestoreDataControllerTest",
srcs = ["FirestoreDataControllerTest.kt"],
custom_package = "org.oppia.android.domain.oppialogger.analytics",
test_class = "org.oppia.android.domain.oppialogger.analytics.FirestoreDataControllerTest",
test_manifest = "//domain:test_manifest",
deps = [
"//:dagger",
"//domain/src/main/java/org/oppia/android/domain/oppialogger/analytics:cpu_module",
"//domain/src/main/java/org/oppia/android/domain/oppialogger/analytics:data_controller",
"//domain/src/main/java/org/oppia/android/domain/oppialogger/analytics:prod_module",
"//testing",
"//testing/src/main/java/org/oppia/android/testing/data:data_provider_test_monitor",
"//testing/src/main/java/org/oppia/android/testing/logging:event_log_subject",
"//testing/src/main/java/org/oppia/android/testing/robolectric:test_module",
"//testing/src/main/java/org/oppia/android/testing/threading:test_coroutine_dispatchers",
"//testing/src/main/java/org/oppia/android/testing/threading:test_module",
"//testing/src/main/java/org/oppia/android/testing/time:test_module",
"//third_party:androidx_test_ext_junit",
"//third_party:com_google_truth_truth",
"//third_party:junit_junit",
"//third_party:org_robolectric_robolectric",
"//third_party:robolectric_android-all",
"//utility/src/main/java/org/oppia/android/util/locale:prod_module",
"//utility/src/main/java/org/oppia/android/util/networking:debug_module",
],
)
1,290 changes: 1,106 additions & 184 deletions scripts/assets/test_file_exemptions.textproto

Large diffs are not rendered by default.

36 changes: 18 additions & 18 deletions scripts/src/java/org/oppia/android/scripts/common/BazelClient.kt
Original file line number Diff line number Diff line change
Expand Up @@ -133,38 +133,38 @@ class BazelClient(private val rootDirectory: File, private val commandExecutor:
/**
* Runs code coverage for the specified Bazel test target.
*
* Null return typically occurs when the coverage command fails to generate the 'coverage.dat' file
* This can happen due to: Test failures or misconfigurations that prevent the coverage data
* from being generated properly.
* An empty list being returned typically occurs when the coverage command fails to generate any
* 'coverage.dat' file. This can happen due to tests failures or a misconfiguration that prevents
* the coverage data from being properly generated.
*
* @param bazelTestTarget Bazel test target for which code coverage will be run
* @return the generated coverage data as a list of strings
* or null if the coverage data file could not be parsed
* @return the generated coverage data as a list of list of strings (since there may be more than
* one file corresponding to a single test target, e.g. in the case of a sharded test), or an
* empty list if no coverage data was found while running the test
*/
fun runCoverageForTestTarget(bazelTestTarget: String): List<String>? {
fun runCoverageForTestTarget(bazelTestTarget: String): List<List<String>> {
val instrumentation = bazelTestTarget.split(":")[0]
val computeInstrumentation = instrumentation.split("/").let { "//${it[2]}/..." }
val coverageCommandOutputLines = executeBazelCommand(
"coverage",
bazelTestTarget,
"--instrumentation_filter=$computeInstrumentation"
)
return parseCoverageDataFilePath(coverageCommandOutputLines)?.let { path ->
return parseCoverageDataFilePath(bazelTestTarget, coverageCommandOutputLines).map { path ->
File(path).readLines()
}
}

private fun parseCoverageDataFilePath(coverageCommandOutputLines: List<String>): String? {
val regex = ".*coverage\\.dat$".toRegex()
for (line in coverageCommandOutputLines) {
val match = regex.find(line)
val extractedPath = match?.value?.substringAfterLast(",")?.trim()
if (extractedPath != null) {
println("Raw Coverage Data: $extractedPath")
return extractedPath
}
}
return null
private fun parseCoverageDataFilePath(
bazelTestTarget: String,
coverageCommandOutputLines: List<String>
): List<String> {
// TODO: RD to upstream this & update tests accordingly.
BenHenning marked this conversation as resolved.
Show resolved Hide resolved
// Use the test target as the base path for the generated coverage.dat file since the test
// itself may output lines that look like the coverage.dat line (such as in BazelClientTest).
val targetBasePath = bazelTestTarget.removePrefix("//").replace(':', '/')
val coverageDatRegex = "^.+?testlogs/$targetBasePath/[^/]*?/?coverage\\.dat$".toRegex()
return coverageCommandOutputLines.filter(coverageDatRegex::matches).map(String::trim)
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,20 +34,17 @@ class CoverageRunner(
*/
fun retrieveCoverageDataForTestTarget(
bazelTestTarget: String
): CoverageReport {
val coverageResult = retrieveCoverageResult(bazelTestTarget)
?: error("Failed to retrieve coverage result for $bazelTestTarget")

return coverageDataFileLines(coverageResult, bazelTestTarget)
}

private fun retrieveCoverageResult(
bazelTestTarget: String
): List<String>? {
return bazelClient.runCoverageForTestTarget(bazelTestTarget)
): List<CoverageReport> {
val coverageResults = bazelClient.runCoverageForTestTarget(bazelTestTarget)
check(coverageResults.isNotEmpty()) {
"Failed to retrieve coverage results for $bazelTestTarget."
}
return coverageResults.map { singleCoverageDatFileLines ->
parseCoverageDataFileLines(singleCoverageDatFileLines, bazelTestTarget)
}
}

private fun coverageDataFileLines(
private fun parseCoverageDataFileLines(
coverageData: List<String>,
bazelTestTarget: String
): CoverageReport {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,20 +106,25 @@ class RunCoverage(
fun execute() {
val testFileExemptionList = loadTestFileExemptionsProto(testFileExemptionTextProto)
.testFileExemptionList
.filter { it.testFileNotRequired }
.filter { it.testFileNotRequired || it.sourceFileIsIncompatibleWithCodeCoverage }
BenHenning marked this conversation as resolved.
Show resolved Hide resolved
.map { it.exemptedFilePath }

if (filePath in testFileExemptionList) {
println("This file is exempted from having a test file; skipping coverage check.")
BenHenning marked this conversation as resolved.
Show resolved Hide resolved
} else {
val testFilePaths = findTestFiles(repoRoot, filePath)
check(testFilePaths.isNotEmpty()) {
"No appropriate test file found for $filePath"
"No appropriate test file found for $filePath."
}

val testTargets = bazelClient.retrieveBazelTargets(testFilePaths)

val coverageReports = testTargets.map { testTarget ->
// TODO: RD to add this check in an upstream PR & add a test for it.
BenHenning marked this conversation as resolved.
Show resolved Hide resolved
check(testTargets.isNotEmpty()) {
"Missing test declaration(s) for existing test file(s): $testFilePaths."
}

val coverageReports = testTargets.flatMap { testTarget ->
CoverageRunner(rootDirectory, scriptBgDispatcher, commandExecutor)
.retrieveCoverageDataForTestTarget(testTarget.removeSuffix(".kt"))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,11 @@ message TestFileExemptions {

// Overrides the minimum coverage percent required for the given file.
int32 override_min_coverage_percent_required = 3;

// A medium-term state to indicate that something about this source file's build config, or
// that of its test(s), are not compatible with the current code coverage tooling and thus
// should be ignored.
bool source_file_is_incompatible_with_code_coverage = 4;
BenHenning marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -387,9 +387,9 @@ class BazelClientTest {
val sourceContent =
"""
package com.example

class AddNums {

companion object {
fun sumNumbers(a: Int, b: Int): Any {
return if (a == 0 && b == 0) {
Expand All @@ -405,16 +405,16 @@ class BazelClientTest {
val testContent =
"""
package com.example

import org.junit.Assert.assertEquals
import org.junit.Test

class AddNumsTest {

@Test
fun testSumNumbers() {
assertEquals(AddNums.sumNumbers(0, 1), 1)
assertEquals(AddNums.sumNumbers(3, 4), 7)
assertEquals(AddNums.sumNumbers(3, 4), 7)
assertEquals(AddNums.sumNumbers(0, 0), "Both numbers are zero")
}
}
Expand All @@ -433,26 +433,28 @@ class BazelClientTest {
"//coverage/test/java/com/example:AddNumsTest"
)
val expectedResult = listOf(
"SF:coverage/main/java/com/example/AddNums.kt",
"FN:7,com/example/AddNums${'$'}Companion::sumNumbers (II)Ljava/lang/Object;",
"FN:3,com/example/AddNums::<init> ()V",
"FNDA:1,com/example/AddNums${'$'}Companion::sumNumbers (II)Ljava/lang/Object;",
"FNDA:0,com/example/AddNums::<init> ()V",
"FNF:2",
"FNH:1",
"BRDA:7,0,0,1",
"BRDA:7,0,1,1",
"BRDA:7,0,2,1",
"BRDA:7,0,3,1",
"BRF:4",
"BRH:4",
"DA:3,0",
"DA:7,1",
"DA:8,1",
"DA:10,1",
"LH:3",
"LF:4",
"end_of_record"
listOf(
"SF:coverage/main/java/com/example/AddNums.kt",
"FN:7,com/example/AddNums${'$'}Companion::sumNumbers (II)Ljava/lang/Object;",
"FN:3,com/example/AddNums::<init> ()V",
"FNDA:1,com/example/AddNums${'$'}Companion::sumNumbers (II)Ljava/lang/Object;",
"FNDA:0,com/example/AddNums::<init> ()V",
"FNF:2",
"FNH:1",
"BRDA:7,0,0,1",
"BRDA:7,0,1,1",
"BRDA:7,0,2,1",
"BRDA:7,0,3,1",
"BRF:4",
"BRH:4",
"DA:3,0",
"DA:7,1",
"DA:8,1",
"DA:10,1",
"LH:3",
"LF:4",
"end_of_record"
)
)

assertThat(result).isEqualTo(expectedResult)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,9 @@ class CoverageRunnerTest {
sourceContent =
"""
package com.example

class AddNums {

companion object {
fun sumNumbers(a: Int, b: Int): Any {
return if (a == 0 && b == 0) {
Expand All @@ -58,16 +58,16 @@ class CoverageRunnerTest {
testContent =
"""
package com.example

import org.junit.Assert.assertEquals
import org.junit.Test

class AddNumsTest {

@Test
fun testSumNumbers() {
assertEquals(AddNums.sumNumbers(0, 1), 1)
assertEquals(AddNums.sumNumbers(3, 4), 7)
assertEquals(AddNums.sumNumbers(3, 4), 7)
assertEquals(AddNums.sumNumbers(0, 0), "Both numbers are zero")
}
}
Expand Down Expand Up @@ -137,17 +137,17 @@ class CoverageRunnerTest {
subTestFile.writeText(
"""
package com.example

import org.junit.Assert.assertEquals
import org.junit.Test
import com.example.AddNums

class SubNumsTest {

@Test
fun testSubNumbers() {
assertEquals(AddNums.sumNumbers(0, 1), 1)
assertEquals(AddNums.sumNumbers(3, 4), 7)
assertEquals(AddNums.sumNumbers(3, 4), 7)
assertEquals(AddNums.sumNumbers(0, 0), "Both numbers are zero")
}
}
Expand Down Expand Up @@ -191,7 +191,7 @@ class CoverageRunnerTest {
testSubpackage = "coverage/test/java/com/example"
)

val result = coverageRunner.retrieveCoverageDataForTestTarget(
val results = coverageRunner.retrieveCoverageDataForTestTarget(
"//coverage/test/java/com/example:AddNumsTest"
)

Expand Down Expand Up @@ -230,7 +230,8 @@ class CoverageRunnerTest {
.setLinesHit(3)
.build()

assertThat(result).isEqualTo(expectedResult)
assertThat(results).hasSize(1)
assertThat(results[0]).isEqualTo(expectedResult)
}

private fun initializeCommandExecutorWithLongProcessWaitTime(): CommandExecutorImpl {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,42 @@ Tests for Robolectric-specific utilities and configurations.

load("//:oppia_android_test.bzl", "oppia_android_test")

oppia_android_test(
name = "OppiaShadowActivityManagerTest",
srcs = ["OppiaShadowActivityManagerTest.kt"],
custom_package = "org.oppia.android.testing.robolectric",
test_class = "org.oppia.android.testing.robolectric.OppiaShadowActivityManagerTest",
test_manifest = "//testing:test_manifest",
deps = [
"//:dagger",
"//testing/src/main/java/org/oppia/android/testing/robolectric:oppia_shadow_activity_manager",
"//third_party:androidx_core_core",
"//third_party:androidx_test_ext_junit",
"//third_party:com_google_truth_truth",
"//third_party:junit_junit",
"//third_party:org_robolectric_robolectric",
"//third_party:robolectric_android-all",
],
)

oppia_android_test(
name = "OppiaShadowTrafficStatsTest",
srcs = ["OppiaShadowTrafficStatsTest.kt"],
custom_package = "org.oppia.android.testing.robolectric",
test_class = "org.oppia.android.testing.robolectric.OppiaShadowTrafficStatsTest",
test_manifest = "//testing:test_manifest",
deps = [
"//:dagger",
"//testing/src/main/java/org/oppia/android/testing/robolectric:oppia_shadow_traffic_stats",
"//third_party:androidx_core_core",
"//third_party:androidx_test_ext_junit",
"//third_party:com_google_truth_truth",
"//third_party:junit_junit",
"//third_party:org_robolectric_robolectric",
"//third_party:robolectric_android-all",
],
)

oppia_android_test(
name = "ShadowBidiFormatterTest",
srcs = ["ShadowBidiFormatterTest.kt"],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,23 @@ kt_android_library(
],
)

oppia_android_test(
name = "CurrentAppScreenNameIntentDecoratorTest",
srcs = ["CurrentAppScreenNameIntentDecoratorTest.kt"],
custom_package = "org.oppia.android.util.logging",
test_class = "org.oppia.android.util.logging.CurrentAppScreenNameIntentDecoratorTest",
test_manifest = "//utility:test_manifest",
deps = [
"//:dagger",
"//third_party:androidx_test_ext_junit",
"//third_party:androidx_test_ext_truth",
"//third_party:com_google_truth_truth",
"//third_party:org_robolectric_robolectric",
"//third_party:robolectric_android-all",
"//utility/src/main/java/org/oppia/android/util/logging:current_app_screen_name_intent_decorator",
],
)

oppia_android_test(
name = "EventBundleCreatorTest",
srcs = ["EventBundleCreatorTest.kt"],
Expand Down
Loading
Loading