Fix part of #5343: Introduce new CI workflow for Code Coverage #5465
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… causing any mismatched misconfiguration in ci (test pass locally)
…ge cases and many to one targets
…ent specific to the test case
…ia/oppia-android into code_coverage_many_to_one_targets
…verageReporter, merge updates
…emoving it to see if that is actually causing the issues (may be with idle times)
… most importantly helps with max idle run time issues
…ia/oppia-android into code_coverage_many_to_one_targets
…m/oppia/oppia-android into code_coverage_list_of_files
…o code_coverage_fix_edge_cases
…ia/oppia-android into code_coverage_many_to_one_targets
…test cases for sets of success, failure and anomaly cases
… to note with 12 the run completed in just 200 seconds
|
@BenHenning, thanks for the review, addressed the remaining comments, with just 2 requests for clarification,
Can you PTAL? |
… status check as there is no call to the coverage reporter yet and that will only be introduced with the upcoming pr to upload the artifacts, and use them to derive a coverage status
6 tasks
BenHenning
reviewed
Aug 10, 2024
There was a problem hiding this comment.
Thanks @Rd4dev! Basically LGTM, but just a couple of follow-ups. PTAL.
Also, I added some clarity to your question on why the new tests you added in the most recent changes are really important (though it's in a resolved conversation thread).
Please also update the branch changes now that #5461 has been merged.
scripts/src/java/org/oppia/android/scripts/coverage/RunCoverage.kt
Outdated
Show resolved
Hide resolved
scripts/src/javatests/org/oppia/android/scripts/ci/ComputeChangedFilesTest.kt
Outdated
Show resolved
Hide resolved
|
@BenHenning, addressed the review comments, but not sure if I was clear enough on explaining on why proto is not used (for now), can you PTAL and see if that was well communicated? |
BenHenning
reviewed
Aug 10, 2024
scripts/src/java/org/oppia/android/scripts/coverage/RunCoverage.kt
Outdated
Show resolved
Hide resolved
BenHenning
approved these changes
Aug 10, 2024
|
Going ahead and merging this since everything's resolved. |
6 tasks
BenHenning
pushed a commit
that referenced
this pull request
Aug 13, 2024
…t as comments (#5469) ## Explanation Fixes part of #5343 ### Project [PR 2.4 of Project 4.1] ### Changes Made With the protos collected and stored with their respective file paths through #5465, this PR will focus on collecting the stored proto files and passing it to the script to generate a coverage report along with a status check to decide the success failure case of the CI Coverage Check run. **Collection of protos** - The stored protos are now being uploaded as an artifact. - The name of the artifact is set to a dynamic value corresponding to its shard_name. - The shard_name is extracted from the first portion of the `CHANGED_FILES_BUCKET_BASE64_ENCODED_SHARD` value. - eg: The matrix job - [app-shard0;H4sIAAAAAAAAAONiTiwoEHIFEvrFRcn6uYmZefpZi....] will save the artifact with name **coverage-report-app-shard0** - The dynamic or unique name is important to prevent the following error: ``` Error: Failed to CreateArtifact: Received non-retryable error: Failed request: (409) Conflict: an artifact with this name already exists on the workflow run ``` - The artifacts that match the pattern of coverage-report-* are downloaded. ``` Found 4 artifact(s) Filtering artifacts by pattern 'coverage-report-*' Preparing to download the following artifacts: - coverage-report-domain-shard0 (ID: 1799350592, Size: 489) - coverage-report-scripts-shard2 (ID: 1799348996, Size: 2075) - coverage-report-app-shard3 (ID: 1799348891, Size: 770) - coverage-report-generic-shard1 (ID: 1799348719, Size: 551) ``` - All the stored coverage_report.pb files are found and their paths are stored as list. - The list of coverage_report.pb files are then passed to the CoverageReporter.kt script to handle the coverage report protos. - The script combines them as one single CoverageReportContainer to generate a Markdown report with it. - Finally, a .md report is stored and based on the status of the coverage check, the job either succeeds or fails. - The stored md report is again uploaded and downloaded as an artifact to pass it for publication of the report. - The CoverageReport.md file is then uploaded as a comment to the corresponding PR using - https://github.com/peter-evans/create-or-update-comment action >The comment will be published regardless of whether the coverage passes or fails. The only time comments will not be published is if the unit tests themselves fail, making the coverage checks non-functional (also less cluttered). # ### CI Run Data This check was run with 4 different shards on all possible cases capable of being produced. Check : https://github.com/oppia/oppia-android/actions/runs/10340867060?pr=5469 Comment Published with the above run: #5469 (comment) The above cases are run with custom temporary changes to the test exemption file (testing purposes). ``` test_file_exemption { exempted_file_path: "scripts/src/java/org/oppia/android/scripts/testfile/TestFileCheck.kt" override_min_coverage_percent_required: 101 } test_file_exemption { exempted_file_path: "utility/src/main/java/org/oppia/android/util/logging/ConsoleLogger.kt" override_min_coverage_percent_required: 30 } ``` ## 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)).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation
Fixes part of #5343
Project
[PR 2.3 of Project 4.1]
Changes Made
Todo:
Essential Checklist