Fix #5486 & part of #5343: Introducing new wiki page for code coverag…

…e usage and limitations (#5483) ## Explanation Fixes part of #5343 Fixes #5486 ### Project [PR 2.6 of Project 4.1] ### Changes Made - This PR introduces 2 new wiki pages: - Oppia Android Code Coverage - Writing tests with Good Behavioural Coverage - Fix to the comment upload feature permission issues with PRs created from the forked branches. - Split the code coverage workflow into 2 1. Core code coverage workflow to handle - Collection of changed files - Bucket partitioning - Run coverage in matrices 2. Comment upload workflow to handle - evaluation of reports - generation of md reports - uploading comments - Coverage Status Checks (as the later required `pull_request_target`) - Fix to #5486 - The issue should have arisen as the pr got merged with the branch being deleted while the publish comment job still running, finding it hard to fetch the pr-issue number to publish a comment. - Now the Coverage Check Status was made to be dependent on the comment uploader ie. the final Coverage check job occurs only after the Evaluation and Comment jobs are done, so it will always have the pr reference) ``` check_coverage_results: name: Check Code Coverage Results needs: [ evaluate-code-coverage-reports, comment_coverage_report ] ``` # ### Reasons for splitting the code_coverage workflow The single code_coverage workflow was split into 1. **code_coverage** (to run coverages) 2. **coverage_report** (to generate and publish reports) ### Separating the comment upload job - The primary reason is the need to have ability to upload comments from PRs opened from a fork branch. - [`pull_request_target`](https://docs.github.com/en/actions/writing-workflows/choosing-when-your-workflow-runs/events-that-trigger-workflows#pull_request_target): For workflows that are triggered by the pull_request_target event, the GITHUB_TOKEN is granted read/write repository permission, even when it is triggered from a fork. - Workflows triggered by `pull_request_target` events are run in the context of the base branch. Since the base branch is considered trusted, workflows triggered by these events will always run, regardless of approval settings. [[GitHub Docs](https://docs.github.com/en/actions/managing-workflow-runs-and-deployments/managing-workflow-runs/approving-workflow-runs-from-public-forks#about-workflow-runs-from-public-forks)] ### Separating the evaluation / generation of report job - While initially it was split to help with 'skip files' - no files changed conditional check, with the introduction to 'SKIP' status check, it doesn't continue to serve the mentioned purpose. - But if we still have it as one workflow then the flow will work as such: Workflow 1: **code_coverage** - compute changed files - run coverage (needs compute changed files) - evaluate / generate md - code coverage check result Workflow 2: **coverage_report** - comment publication If no `.kt` files changes are detected - compute changed files - skips run coverage - skips evaluate / generate md - pass code coverage check result As the workflow was concluded as success, the 2nd workflow runs as, - failed comment publication (as no report is generation due to skip) But expectation is to still produce a pass check for the comment publication. (either to at least skip or upload a skip status as coverage comment report) - With moving it to a separate workflow allows us to not make the evaluation / generation jobs rely on the Run coverage job making it independently behave once the code_coverage workflows are completed successfully. - And it checks if pb files are generated and based on that it decides whether to generate PASS, FAIL or SKIP status checks. ### Separating the coverage status check result job There are 2 main reasons to moving it to new workflow. While it would still make sense to have it with the 1st workflow itself after Run coverage, the drawbacks are, - If the check coverage status result was left with the 1st workflow, then when the Run coverage job completes in the 1st workflow the coverage status check result is set to true on success even before the sibling part of upload comment is done, making it an incomplete status result. - #5486 occurred as it lost its reference to the pr number, so making the coverage check status result dependent on (needs) the comment upload job should resolve this by only allowing the PR to be closed once the comment is uploaded. # **Todo:** - **[Done]** Add a new wiki page for "Writing effective tests / Writing test with good behavioural coverage" ## Essential Checklist - [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 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 --------- Co-authored-by: Ben Henning <[email protected]>
oppia · Aug 23, 2024 · a85cc50 · a85cc50
1 parent cfc41cc
commit a85cc50
Show file tree

Hide file tree

Showing 12 changed files with 2,268 additions and 44 deletions.
diff --git a/.github/workflows/build_tests.yml b/.github/workflows/build_tests.yml
@@ -11,7 +11,7 @@ on:
       - develop
 
 concurrency:
-  group: ${{ github.workflow }}-${{ github.ref }}
+  group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }}
   cancel-in-progress: true
 
 jobs:

diff --git a/.github/workflows/code_coverage.yml b/.github/workflows/code_coverage.yml
@@ -12,7 +12,7 @@ on:
       - develop
 
 concurrency:
-  group: ${{ github.workflow }}-${{ github.ref }}
+  group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }}
   cancel-in-progress: true
 
 jobs:
@@ -255,10 +255,13 @@ jobs:
           name: coverage-report-${{ env.SHARD_NAME }} # Saving with unique names to avoid conflict
           path: coverage_reports
 
-  evaluate-code-coverage-reports:
+  evaluate_code_coverage_reports:
     name: Evaluate Code Coverage Reports
     runs-on: ubuntu-20.04
     needs: 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() }}
     env:
       CACHE_DIRECTORY: ~/.bazel_cache
     steps:
@@ -305,32 +308,10 @@ jobs:
           name: final-coverage-report
           path: coverage_reports/CoverageReport.md
 
-  publish_coverage_report:
-    name: Publish Code Coverage Report
-    needs: evaluate-code-coverage-reports
-    permissions:
-      pull-requests: write
-
-    # 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-latest
-    steps:
-      - name: Download Generated Markdown Report
-        uses: actions/download-artifact@v4
-        with:
-          name: final-coverage-report
-
-      - name: Upload Coverage Report as PR Comment
-        uses: peter-evans/create-or-update-comment@v4
-        with:
-          issue-number: ${{ github.event.pull_request.number }}
-          body-path: 'CoverageReport.md'
-
   # Reference: https://github.community/t/127354/7.
   check_coverage_results:
     name: Check Code Coverage Results
-    needs: [ compute_changed_files, code_coverage_run, evaluate-code-coverage-reports ]
+    needs: [ 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() }}
@@ -341,5 +322,5 @@ jobs:
         run: exit 1
 
       - name: Check that coverage status is passed
-        if: ${{ needs.evaluate-code-coverage-reports.result != 'success' }}
+        if: ${{ needs.compute_changed_files.outputs.can_skip_files != 'true' && needs.evaluate_code_coverage_reports.result != 'success' }}
         run: exit 1
diff --git a/.github/workflows/comment_coverage_report.yml b/.github/workflows/comment_coverage_report.yml
@@ -0,0 +1,81 @@
+# Contains jobs corresponding to publishing coverage reports generated by code_coverage.yml.
+
+name: Comment Coverage Report
+
+# Controls when the action will run. Triggers the workflow  on pull request events
+# (assigned, opened, synchronize, reopened)
+
+on:
+  pull_request_target:
+    types: [assigned, opened, synchronize, reopened]
+
+concurrency:
+  group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }}
+  cancel-in-progress: true
+
+jobs:
+  check_code_coverage_completed:
+    name: Check code coverage completed
+    runs-on: ubuntu-latest
+    steps:
+      - name: Wait for code coverage to complete
+        id: wait-for-coverage
+        uses: ArcticLampyrid/[email protected]
+        with:
+          workflow: code_coverage.yml
+          sha: auto
+          allowed-conclusions: |
+            success
+            failure
+
+  comment_coverage_report:
+    name: Comment Code Coverage Report
+    needs: check_code_coverage_completed
+    permissions:
+      pull-requests: write
+
+    # 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-latest
+    steps:
+      - name: Find CI workflow run for PR
+        id: find-workflow-run
+        uses: actions/github-script@v7
+        continue-on-error: true
+        with:
+          script: |
+            // Find the last successful workflow run for the current PR's head
+            const { owner, repo } = context.repo;
+            const runsResponse = await github.rest.actions.listWorkflowRuns({
+              owner,
+              repo,
+              workflow_id: 'code_coverage.yml',
+              event: 'pull_request',
+              head_sha: '${{ github.event.pull_request.head.sha }}',
+            });
+
+            const runs = runsResponse.data.workflow_runs;
+            runs.sort((a, b) => new Date(b.created_at).getTime() - new Date(a.created_at).getTime());
+
+            const run = runs[0];
+            if(!run) {
+              core.setFailed('Could not find a succesful workflow run for the PR');
+              return;
+            }
+
+            core.setOutput('run-id', run.id);
+
+      - name: Download Generated Markdown Report
+        uses: actions/download-artifact@v4
+        if: ${{ !cancelled() }} # IMPORTANT: Upload reports regardless of success or failure status
+        with:
+          name: final-coverage-report
+          github-token: ${{ secrets.GITHUB_TOKEN }}
+          run-id: ${{ steps.find-workflow-run.outputs.run-id }}
+
+      - name: Upload Coverage Report as PR Comment
+        uses: peter-evans/create-or-update-comment@v4
+        with:
+          issue-number: ${{ github.event.pull_request.number }}
+          body-path: 'CoverageReport.md'
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
@@ -13,7 +13,7 @@ on:
       - develop
 
 concurrency:
-  group: ${{ github.workflow }}-${{ github.ref }}
+  group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }}
   cancel-in-progress: true
 
 # This workflow has the following jobs:

diff --git a/.github/workflows/static_checks.yml b/.github/workflows/static_checks.yml
@@ -10,7 +10,7 @@ on:
       - develop
 
 concurrency:
-  group: ${{ github.workflow }}-${{ github.ref }}
+  group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }}
   cancel-in-progress: true
 
 jobs:

diff --git a/.github/workflows/unit_tests.yml b/.github/workflows/unit_tests.yml
@@ -13,7 +13,7 @@ on:
       - develop
 
 concurrency:
-  group: ${{ github.workflow }}-${{ github.ref }}
+  group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }}
   cancel-in-progress: true
 
 jobs:

diff --git a/scripts/src/java/org/oppia/android/scripts/coverage/reporter/CoverageReporter.kt b/scripts/src/java/org/oppia/android/scripts/coverage/reporter/CoverageReporter.kt
@@ -496,16 +496,37 @@ class CoverageReporter(
       }
     }
 
-    val finalReportText = "## Coverage Report\n\n" +
-      "### Results\n" +
-      "Number of files assessed: ${coverageReportContainer.coverageReportList.size}\n" +
-      "Overall Coverage: **${"%.2f".format(calculateOverallCoveragePercentage())}%**\n" +
-      "Coverage Analysis: $status\n" +
-      "##" +
-      failureMarkdownTable +
-      failureMarkdownEntries +
-      successMarkdownEntries +
-      testFileExemptedSection
+    val wikiPageLinkNote = buildString {
+      val wikiPageReferenceNote = ">To learn more, visit the [Oppia Android Code Coverage]" +
+        "(https://github.com/oppia/oppia-android/wiki/Oppia-Android-Code-Coverage) wiki page"
+      append("\n\n")
+      append("#")
+      append("\n")
+      append(wikiPageReferenceNote)
+    }
+
+    val skipCoverageReportText = buildString {
+      append("## Coverage Report\n")
+      append("### Results\n")
+      append("Coverage Analysis: **SKIP** :next_track_button:\n\n")
+      append("_This PR did not introduce any changes to Kotlin source or test files._")
+      append(wikiPageLinkNote)
+    }
+
+    val finalReportText = coverageReportContainer.coverageReportList.takeIf { it.isNotEmpty() }
+      ?.let {
+        "## Coverage Report\n\n" +
+          "### Results\n" +
+          "Number of files assessed: ${coverageReportContainer.coverageReportList.size}\n" +
+          "Overall Coverage: **${"%.2f".format(calculateOverallCoveragePercentage())}%**\n" +
+          "Coverage Analysis: $status\n" +
+          "##" +
+          failureMarkdownTable +
+          failureMarkdownEntries +
+          successMarkdownEntries +
+          testFileExemptedSection +
+          wikiPageLinkNote
+      } ?: skipCoverageReportText
 
     val finalReportOutputPath = mdReportOutputPath
       ?.let { it }

diff --git a/scripts/src/javatests/org/oppia/android/scripts/coverage/RunCoverageTest.kt b/scripts/src/javatests/org/oppia/android/scripts/coverage/RunCoverageTest.kt
@@ -90,6 +90,7 @@ class RunCoverageTest {
       append("| File | Failure Reason | Status |\n")
       append("|------|----------------|--------|\n")
       append("| ${getFilenameAsDetailsSummary(sampleFile)} | $failureMessage | :x: |")
+      append(oppiaCoverageWikiPageLinkNote)
     }
 
     assertThat(readFinalMdReport()).isEqualTo(expectedMarkdown)
@@ -229,6 +230,7 @@ class RunCoverageTest {
       append("\n\n")
       append(exemptionsReferenceNote)
       append("</details>")
+      append(oppiaCoverageWikiPageLinkNote)
     }
 
     assertThat(readFinalMdReport()).isEqualTo(expectedResult)
@@ -278,6 +280,7 @@ class RunCoverageTest {
       append("\n\n")
       append(exemptionsReferenceNote)
       append("</details>")
+      append(oppiaCoverageWikiPageLinkNote)
     }
 
     assertThat(readFinalMdReport()).isEqualTo(expectedResult)
@@ -385,6 +388,7 @@ class RunCoverageTest {
       append("| File | Failure Reason | Status |\n")
       append("|------|----------------|--------|\n")
       append("| //coverage/example:AddNumsTest | $failureMessage | :x: |")
+      append(oppiaCoverageWikiPageLinkNote)
     }
 
     assertThat(readFinalMdReport()).isEqualTo(expectedMarkdown)
@@ -469,6 +473,7 @@ class RunCoverageTest {
       append("| File | Failure Reason | Status |\n")
       append("|------|----------------|--------|\n")
       append("| //coverage/test/java/com/example:SubNumsTest | $failureMessage | :x: |")
+      append(oppiaCoverageWikiPageLinkNote)
     }
 
     assertThat(readFinalMdReport()).isEqualTo(expectedMarkdown)
@@ -655,6 +660,7 @@ class RunCoverageTest {
           ":white_check_mark: | $MIN_THRESHOLD% |\n\n"
       )
       append("</details>")
+      append(oppiaCoverageWikiPageLinkNote)
     }
 
     assertThat(readFinalMdReport()).isEqualTo(expectedResult)
@@ -843,6 +849,7 @@ class RunCoverageTest {
         "| ${getFilenameAsDetailsSummary(filePathList.get(0))} | 0.00% | 0 / 4 | " +
           ":x: | $MIN_THRESHOLD% |\n"
       )
+      append(oppiaCoverageWikiPageLinkNote)
     }
 
     assertThat(readFinalMdReport()).isEqualTo(expectedResult)
@@ -934,6 +941,7 @@ class RunCoverageTest {
           ":x: | 101% _*_ |"
       )
       append("\n\n>**_*_** represents tests with custom overridden pass/fail coverage thresholds")
+      append(oppiaCoverageWikiPageLinkNote)
     }
 
     assertThat(readFinalMdReport()).isEqualTo(expectedResult)
@@ -1021,6 +1029,7 @@ class RunCoverageTest {
       )
       append("\n\n>**_*_** represents tests with custom overridden pass/fail coverage thresholds\n")
       append("</details>")
+      append(oppiaCoverageWikiPageLinkNote)
     }
 
     assertThat(readFinalMdReport()).isEqualTo(expectedResult)
@@ -1090,6 +1099,7 @@ class RunCoverageTest {
           ":white_check_mark: | $MIN_THRESHOLD% |\n\n"
       )
       append("</details>")
+      append(oppiaCoverageWikiPageLinkNote)
     }
 
     assertThat(readFinalMdReport()).isEqualTo(expectedResult)
@@ -1162,6 +1172,7 @@ class RunCoverageTest {
       append("\n\n")
       append(exemptionsReferenceNote)
       append("</details>")
+      append(oppiaCoverageWikiPageLinkNote)
     }
 
     assertThat(readFinalMdReport()).isEqualTo(expectedResult)
@@ -1238,6 +1249,7 @@ class RunCoverageTest {
       append("\n\n")
       append(exemptionsReferenceNote)
       append("</details>")
+      append(oppiaCoverageWikiPageLinkNote)
     }
 
     assertThat(readFinalMdReport()).isEqualTo(expectedResult)
@@ -1333,6 +1345,7 @@ class RunCoverageTest {
       append("\n\n")
       append(exemptionsReferenceNote)
       append("</details>")
+      append(oppiaCoverageWikiPageLinkNote)
     }
 
     assertThat(readFinalMdReport()).isEqualTo(expectedResult)
@@ -1438,6 +1451,7 @@ class RunCoverageTest {
       append("\n\n")
       append(exemptionsReferenceNote)
       append("</details>")
+      append(oppiaCoverageWikiPageLinkNote)
     }
 
     assertThat(readFinalMdReport()).isEqualTo(expectedResult)
@@ -1642,6 +1656,7 @@ class RunCoverageTest {
           ":white_check_mark: | $MIN_THRESHOLD% |\n\n"
       )
       append("</details>")
+      append(oppiaCoverageWikiPageLinkNote)
     }
 
     assertThat(readFinalMdReport()).isEqualTo(expectedResult)
@@ -1722,6 +1737,7 @@ class RunCoverageTest {
           ":white_check_mark: | $MIN_THRESHOLD% |\n\n"
       )
       append("</details>")
+      append(oppiaCoverageWikiPageLinkNote)
     }
 
     assertThat(readFinalMdReport()).isEqualTo(expectedResult)
@@ -2282,6 +2298,7 @@ class RunCoverageTest {
           "3 / 4 | :white_check_mark: | $MIN_THRESHOLD% |\n\n"
       )
       append("</details>")
+      append(oppiaCoverageWikiPageLinkNote)
     }
 
     return markdownText
@@ -2575,13 +2592,20 @@ class RunCoverageTest {
   }
 
   private fun getFilenameAsDetailsSummary(
-    filePath: String,
-    additionalData: String? = null
+    filePath: String
   ): String {
     val fileName = filePath.substringAfterLast("/")
-    val additionalDataPart = additionalData?.let { " - $it" } ?: ""
 
-    return "<details><summary><b>$fileName</b>$additionalDataPart</summary>$filePath</details>"
+    return "<details><summary><b>$fileName</b></summary>$filePath</details>"
+  }
+
+  private val oppiaCoverageWikiPageLinkNote = buildString {
+    val wikiPageReferenceNote = ">To learn more, visit the [Oppia Android Code Coverage]" +
+      "(https://github.com/oppia/oppia-android/wiki/Oppia-Android-Code-Coverage) wiki page"
+    append("\n\n")
+    append("#")
+    append("\n")
+    append(wikiPageReferenceNote)
   }
 
   private fun loadCoverageReportProto(