[BUG]: CI is currently failing for code coverage

[BenHenning]

Describe the bug

Code coverage is failing on develop with an indicator that the test for RecentlyPlayedActivity is currently missing:

  COVERAGE FAILURE REPORT:
  -----------------------
  Coverage Report Failure:
  ------------------------
  Test Target: 
  Failure Message: No appropriate test file found for app/src/main/java/org/oppia/android/app/home/recentlyplayed/RecentlyPlayedActivity.kt.

This was first introduced in #5759 due to a new test being added: RecentlyPlayedActivityTest. What's odd is that this was successfully checked in meaning that code coverage analysis passed incrementally, and the test file exemption check didn't trigger. It's unclear why there's a behavior discrepancy, but notably the test was put in the wrong package (it should be under home/recentlyplayed not home/). I would, however, have expected it to fail for both incremental and non-incremental checks.

Steps To Reproduce

Either try running code coverage for the specific RecentlyPlayedActivity or run the whole suite (I haven't verified either repro case).

Expected Behavior

Since there's a valid test, this issue should not be triggered.

Screenshots/Videos

No response

What device/emulator are you using?

No response

Which Android version is your device/emulator running?

No response

Which version of the Oppia Android app are you using?

No response

Additional Context

We need to understand why this failed in CI before we can be confident in a fix.

[BenHenning]

FYI @adhiamboperes and @Rd4dev. This will be a continual failure for develop until we fix it, and may block some PRs.

[BenHenning]

I will also try to take a look at it. Any insight that you have would be helpful @Rd4dev but I don't expect you to dig into the issue itself (and if you don't have any thoughts, no worries).

[Rd4dev]

I will also try to take a look at it. Any insight that you have would be helpful @Rd4dev but I don't expect you to dig into the issue itself (and if you don't have any thoughts, no worries).

The error seems specific to missing PB files, as no coverage report being generated. I’ll try to run the tests and coverages to see how this might have slipped past the file exemptions.

[Rd4dev]

@BenHenning, actually, the code coverage passed incrementally (i.e., the "Run Code Coverages" succeeded), but the "Evaluate Code Coverage" failed due to how I had implemented the coverage runs with the PROTO format.

With PROTO format it ran coverages for individual files, save their results as .pb files (without status checks), and later aggregate them in the Evaluate job (which runs with the MARKDOWN format). That’s where the failure was caught, leading to the error.

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

Lines 148 to 161 in 2197e03

	if (reportFormat == ReportFormat.PROTO) {
	filePathList.forEach { filePath ->
	val coverageReport = runCoverageForFile(filePath)
	val filePathDir = filePath.substringBeforeLast(".")
	val protoOutputPath = "$repoRoot/coverage_reports/$filePathDir/coverage_report.pb"
	val protoOutputFile = File(protoOutputPath)
	protoOutputFile.parentFile?.mkdirs()
	protoOutputFile.outputStream().use { stream ->
	coverageReport.writeTo(stream)
	}
	}
	return
	}

I can’t quite recall why I initially set it up this way — maybe to ensure the final evaluation handled things cumulatively — but thinking about it now, it might be better to allow individual runs to fail earlier too by may be checking their status with MARKDOWN format after individual runs.

Edit: Oh, just remembered why it was set up that way—if PROTO format was set to throw errors, it would halt the check, preventing other coverage runs from proceeding, since even failing to meet the minimum threshold would trigger an error.

Additionaly:

1. Running with Test Files:
   • When a test file is provided, the coverage run attempts to map it to its corresponding source files and targets.
   • When running the script with RecentlyPlayedActivityTest.kt, the script tried to map it to its sources — but since no valid source files were found, the script exited. As observed in PR Fix #5308: Remove uses of ActivityTestRule. #5759 - Fix #5308: Remove uses of ActivityTestRule. #5759 (comment)

bazel run test_file

bazel run //scripts:run_coverage -- $(pwd) app/src/sharedTest/java/org/oppia/android/app/home/RecentlyPlayedActivityTest.kt
INFO: Analyzed target //scripts:run_coverage (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
Target //scripts:run_coverage up-to-date:
  bazel-bin/scripts/run_coverage.jar
  bazel-bin/scripts/run_coverage.jdeps
INFO: Elapsed time: 0.125s, Critical Path: 0.00s
INFO: 1 process: 1 internal.
INFO: Build completed successfully, 1 total action
INFO: Running command line: bazel-bin/scripts/run_coverage /home/rd/opensource/oppia-android app/src/sharedTest/java/org/oppia/android/app/home/RecentlyPlayedActivityTest.kt
Finding source file for - app/src/sharedTest/java/org/oppia/android/app/home/RecentlyPlayedActivityTest.kt
Possible source file paths - [app/src/main/java/org/oppia/android/app/home/RecentlyPlayedActivity.kt]
Does the possible file source exist - false
Running coverage analysis for the files: []
Using format: HTML
Coverage Results - []

Coverage Analysis PASSED

• May be: We could throw an error explicitly when no possible source files match, rather than silently exiting?

2. Running with Source Files:
   • When running the script with source file RecentlyPlayedActivity.kt, it attempts to map the source file to its corresponding test files and targets. If the test file is missing or incorrectly placed (e.g., wrong package), the script exits with a failed coverage report status CoverageFailure.
   • This failure is reported as an error in HTML and Markdown formats but gets bypassed in the PROTO format.

bazel run source_file

bazel run //scripts:run_coverage -- $(pwd) app/src/main/java/org/oppia/android/app/home/recentlyplayed/RecentlyPlayedActivity.kt --format=HTML
INFO: Analyzed target //scripts:run_coverage (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
Target //scripts:run_coverage up-to-date:
  bazel-bin/scripts/run_coverage.jar
  bazel-bin/scripts/run_coverage.jdeps
INFO: Elapsed time: 0.116s, Critical Path: 0.00s
INFO: 1 process: 1 internal.
INFO: Build completed successfully, 1 total action
INFO: Running command line: bazel-bin/scripts/run_coverage /home/rd/opensource/oppia-android app/src/main/java/org/oppia/android/app/home/recentlyplayed/RecentlyPlayedActivity.kt '--format=HTML'
Running coverage analysis for the files: [app/src/main/java/org/oppia/android/app/home/recentlyplayed/RecentlyPlayedActivity.kt]
Using format: HTML
Running coverage for file - app/src/main/java/org/oppia/android/app/home/recentlyplayed/RecentlyPlayedActivity.kt
Possible Test file paths are - [app/src/sharedTest/java/org/oppia/android/app/home/recentlyplayed/RecentlyPlayedActivityTest.kt, app/src/test/java/org/oppia/android/app/home/recentlyplayed/RecentlyPlayedActivityTest.kt, app/src/test/java/org/oppia/android/app/home/recentlyplayed/RecentlyPlayedActivityLocalTest.kt]
Test file path - []
Coverage Results - [failure {
  file_path: "app/src/main/java/org/oppia/android/app/home/recentlyplayed/RecentlyPlayedActivity.kt"
  failure_message: "No appropriate test file found for app/src/main/java/org/oppia/android/app/home/recentlyplayed/RecentlyPlayedActivity.kt."
}
]

-> The coverage analysis for  failed - reason: No appropriate test file found for app/src/main/java/org/oppia/android/app/home/recentlyplayed/RecentlyPlayedActivity.kt. 

  
  COVERAGE FAILURE REPORT:
  -----------------------
  Coverage Report Failure:
  ------------------------
  Test Target: 
  Failure Message: No appropriate test file found for app/src/main/java/org/oppia/android/app/home/recentlyplayed/RecentlyPlayedActivity.kt.

Exception in thread "main" java.lang.IllegalStateException: Coverage Analysis FAILED
	at org.oppia.android.scripts.coverage.RunCoverage.execute(RunCoverage.kt:183)
	at org.oppia.android.scripts.coverage.RunCoverageKt.main(RunCoverage.kt:110)

• May be: Allow this too to throw an error with PROTO format similar to / utilizing the HTML / MARKDOWN format.

3. RecentlyPlayedActivity Coverage:

   • Even after moving the file to its correct location, the script might still fail due to source file incompatibilities. These files may need to be added to the test file exemptions list for the script to handle unmodularized files.

4. File Relocation Reference:

   • Also referencing the list of files that need to be moved to proper locations - as doing this could enable coverage runs on modularized files. - [BUG]: Certain test targets fail to execute bazel coverage commands #5481 (comment)

[BenHenning]

Ah that's an excellent write-up, thanks @Rd4dev!

It makes sense that we need to move the misplaced files to properly fix them, but given that there are other scenarios where the CI check could let in a failure again, that's not great. Do you have a specific recommendation for how to address that part? It seems that you lean toward throwing an error if no source files match, which I think I agree with. What would be the most straightforward way to make that change?

[shkshreyas]

Plan to Fix the Code Coverage CI Issue

There are two main issues to address:

1. The immediate issue: RecentlyPlayedActivityTest is in the wrong package
2. The underlying issue: The PROTO format in the coverage script doesn't fail early when there are coverage issues

Solution Plan

Part 1: Move the Test File to the Correct Package

First, we need to move the test file to the correct package structure:

mkdir -p c:\Users\shksh\Desktop\OS\oppia-android\app\src\test\java\org\oppia\android\app\home\recentlyplayed
move c:\Users\shksh\Desktop\OS\oppia-android\app\src\test\java\org\oppia\android\app\home\RecentlyPlayedActivityTest.kt c:\Users\shksh\Desktop\OS\oppia-android\app\src\test\java\org\oppia\android\app\home\recentlyplayed\

Part 2: Modify the RunCoverage.kt Script to Fail Early

The more important fix is to modify the coverage script to fail early when no source files match in PROTO format. Here's the change needed:

// ... existing code ...
if (reportFormat == ReportFormat.PROTO) { 
  filePathList.forEach { filePath -> 
    val coverageReport = runCoverageForFile(filePath) 
    
    // Add check to fail early if no source files match
    if (coverageReport.status == CoverageReportStatus.COVERAGE_FAILURE) {
      println("COVERAGE FAILURE REPORT:")
      println("-----------------------")
      println("Coverage Report Failure:")
      println("------------------------")
      println("Test Target: ${coverageReport.testTarget}")
      println("Failure Message: ${coverageReport.failureMessage}")
      exitProcess(1)
    }

    val filePathDir = filePath.substringBeforeLast(".") 
    val protoOutputPath = "$repoRoot/coverage_reports/$filePathDir/coverage_report.pb" 
    val protoOutputFile = File(protoOutputPath) 
    protoOutputFile.parentFile?.mkdirs() 
    protoOutputFile.outputStream().use { stream -> 
      coverageReport.writeTo(stream) 
    } 
  } 
  return 
} 
// ... existing code ...

Additionally, we should modify the code that handles test files to explicitly fail when no source files are found:

// ... existing code ...
// In the function that maps test files to source files
// This would likely be in a function like mapTestFileToSourceFiles() or similar

// Add this after attempting to find source files
if (sourceFiles.isEmpty()) {
  return CoverageReport.newBuilder()
    .setStatus(CoverageReportStatus.COVERAGE_FAILURE)
    .setFailureMessage("No source files found for test file: $testFilePath")
    .build()
}
// ... existing code ...

These changes will ensure that:

1. When running in PROTO format, the script will fail early if a coverage failure is detected
2. When a test file has no matching source files, it will explicitly report a failure instead of silently exiting

This approach addresses both the immediate issue and prevents similar issues from occurring in the future by making the CI process more robust.

[Rd4dev]

It seems that you lean toward throwing an error if no source files match

Yeah, I believe this could fail and indicate individual file mapping unavailability. I'm considering failing when both source and test mappings are missing. The most straightforward approach would be to throw an exception right at the file mapping stage - https://github.com/Rd4dev/oppia-android/pull/25/files#diff-62c81482e7c4b720da2d4efc73a48d10369eee097e0bf3439def6648ff150c23R332

Currently trying to run all tests in this PR - Rd4dev#25, on my fork where I tried to migrate most of the mentioned files (which includes all files except test files of - HtmlParser and ListItemLeadingMarginSpan). I believe if this passes, it would indicate that these can be safely moved.