Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Browse files
Browse the repository at this point in the history
) <!-- 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 ### Project [PR 2.2 of Project 4.1] ### Changes Made - **File Path List:** The implementation now enables doing coverage analysis for a list of provided files (which used to be just a single file). Coverage analysis runs sequentially for each file. **Updated command:** ``` bazel run //scripts:run_coverage -- $(pwd) <path/to/file1.kt> <path/to/file2.kt> <path/to/file3.kt> --format=MARKDOWN --processTimeout=15 ``` **Handled Edge cases:** - **Non-kotlin Files:** - Since when retrieving changed files list other non-kotlin files (resource files - .xml, .txt, .md, .pb, .json, etc.) could be included, this script will handle the analysis by only filtering in the required kotlin files. - The kotlin files which are test files are mapped to their appropriate source files and then provided for coverage analysis. - So the provided list of files: [utility/.../MathTokenizer.kt, app/.../layout.xml, scripts/.../TestFileCheckTest.kt] will only do coverage analysis for the files "utility/.../MathTokenizer.kt", "scripts/.../TestFileCheck.kt". - **Input variations:** - Developers would be able to provide list of files in all below variations - [file1.kt, file2.kt, file3.kt] - ["file1.kt", "file2.kt", "file3.kt"] - file1.kt file2.kt file3.kt - file1.kt, file2.kt, file3.kt - **Input format:** - Included the option to use the short form 'md' for the markdown format (just to enhance user experience). ``` bazel run //scripts:run_coverage -- $(pwd) <path/to/file1.kt> <path/to/file2.kt> --format=md ``` - **Coverage Results Display:** Based on the discussions, it was decided to display only the failure cases (i.e., below the minimum threshold). However, I included success cases in a hidden dropdown. This allows developers to see the coverage checks and percentages of other files, enabling them to improve their test scenarios if needed. (yet to discussed if that's ok) - **Error Handling:** Error statements in the coverage flow have been replaced with the introduction to Failure and Exception files having their own types to identify them. **New Proto Structure:** ``` message CoverageReport { oneof report_status { // Coverage report details when coverage data retrieval succeeds. CoverageDetails details = 1; // Coverage report failure when coverage data retrieval fails. CoverageFailure failure = 2; // Coverage exemption when the source file is exempted from having a test file. CoverageExemption exemption = 3; } } ``` - **Handling Anomaly Cases:** There are scenarios where files could be exempted, coverage cannot be retrieved, or coverage does not exist. Previously, an error in one file could halt the entire process. Instead of throwing an error, the flow continues while displaying the encountered cases. - **PASS / FAIL:** While still the coverage analysis continues with failure / anomaly cases with any case other than a PASS would be considered FAIL and checked at the end to throw an error at the end of the entire execution completion, helping with both local dev and ci checks. - **Format:** Defaulting the report generation format to **HTML** considering priority format for local development. While with both .md and .html reports a text report will be logged to the console, so a quick glance is provided and that will make .md obsolete for local development. (Updated) The Logged Report is now updated to only log reports that Fail (either a hasFailure case or details that fail below threshold or if exempted under overridden threshold) Sample log while having min threshold as 99% and exempted percentage for MathModel.kt as 101% (for testing purposes)  **Logged Report Text:**  - **Final Markdown Report:** The final Markdown report is generated in the CoverageReporter.kt file, with the entire list of coverage data proto collected from each file's coverage analysis. The report template is inspired from [Oppia Web's Codecov report](oppia/oppia#9618 (comment)) and includes a dropdown for better readability (provided below). Though the original template was to have it as a list of dropdowns (the new template is yet to be confirmed). - The updated template also has the exempted percentage included to make it clear and not cause any if any exempted cases are being included in the analysis. ### MD Report Template ## Coverage Report ### Results Number of files assessed: 5 Overall Coverage: **94.26%** Coverage Analysis: **FAIL** ❌ ## ### Failure Cases | File | Failure Reason | |------|----------------| | File.kt | No appropriate test file found for File.kt. | ### Failing coverage | File | Coverage | Lines Hit | Status | Min Required | |------|:--------:|----------:|:------:|:------------:| | <details><summary>RunCoverage.kt</summary>scripts/src/java/org/oppia/android/scripts/coverage/RunCoverage.kt</details> | 51.38% | 148 / 288 | ❌ | 70% | | <details><summary>MathModel.kt</summary>utility/src/main/java/org/oppia/android/util/parser/math/MathModel.kt</details> | 77.78% | 7 / 9 | ❌ | 80% _*_ | | <details><summary>MultipleChoiceInputModule.kt</summary>domain/src/main/java/org/oppia/android/domain/classify/rules/multiplechoiceinput/MultipleChoiceInputModule.kt</details> | 10.00% | 1 / 10 | ❌ | 30% _*_ | >**_*_** represents tests with custom overridden pass/fail coverage thresholds ### Passing coverage <details> <summary>Files with passing code coverage</summary><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 | ✅ | 70% | </details> ### Exempted coverage <details><summary>Files exempted from coverage</summary> <br> <details><summary>ActivityComponent.kt</summary>app/src/main/java/org/oppia/android/app/activity/ActivityComponent.kt</details> <details><summary>TestExemptedFile.kt</summary>app/src/main/java/org/oppia/android/app/activity/TestExemptedFile.kt</details> <details><summary>SourceIncompatible.kt</summary>app/src/main/java/org/oppia/android/app/activity/SourceIncompatible.kt</details> </details> --- ### To be discussed: - The `shards_count` for RunCoverageTest. - The `MIN_THRESHOLD` that need to be set. - The final report having Success cases table included. - On how to handle the anomaly cases (should they be considered FAIL case for coverage analysis) - To discuss cases when the coverage data are unavailable for source targets (files) (ie SF: doesn't point to source files) ### Todo - **[Done]** Use dynamic filename list taken from the arg (required for both ci and developer workflow) - **[Done]** Figure out a way on how to handle the delivery of the final md report and check status generated - The final md report will be used as an uploadable comment (considering saving it to $(pwd)/finalreport.md (to access the same file in ci) - The check status will be used to set the ci run fail/pass case - **[Done]** Update tests - **[Done]** add min threshold for exempted files in the table to let developers know them. - **[Done]** Add links for anomaly case file paths too. - **[Done]** Remove Asynchronous flow and have it Sequential - **[Done]** The test files too need to be taken into account while considering coverage analysis (mapping test files to their source files and provide it to the script run, also these files need to be figured out even in the compute changed files utility as that will help with building the targets) With this doing a script run of ``` bazel run //scripts:run_coverage -- $(pwd) utility/src/main/java/org/oppia/android/util/parser/math/MathModel.kt scripts/src/javatests/org/oppia/android/scripts/coverage/CoverageRunnerTest.kt data/src/test/java/org/oppia/android/data/backends/gae/JsonPrefixNetworkInterceptorTest.kt app/src/sharedTest/java/org/oppia/android/app/story/StoryFragmentTest.kt app/src/test/java/org/oppia/android/app/application/alpha/AlphaBuildFlavorModuleTest.kt test.xml app/src/test/java/org/oppia/android/app/home/HomeActivityLocalTest.kt ``` will do coverage analysis on files: ``` Running coverage analysis for the files: [ utility/src/main/java/org/oppia/android/util/parser/math/MathModel.kt, scripts/src/java/org/oppia/android/scripts/coverage/CoverageRunner.kt, data/src/main/java/org/oppia/android/data/backends/gae/JsonPrefixNetworkInterceptor.kt, app/src/main/java/org/oppia/android/app/story/StoryFragment.kt, app/src/main/java/org/oppia/android/app/application/alpha/AlphaBuildFlavorModule.kt, app/src/main/java/org/oppia/android/app/home/HomeActivity.kt ] ``` - **[Done]** The Error statements need to have a clear indication of why it failed. - **[Done]** Every fail case is a Coverage Status "FAIL' only a pass is 'PASS' - **[Done]** Re-work on the final md generation [use a centralized proto collection way, move the existing md generation script and have it with the CoverageReporter just for md, have the exisiting html generation for individual files] - **[Done]** Add tests with addition to including test file's source files. ## 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". - [ ] 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)). <details><summary>Old Template</summary> **FOR TESTING PURPOSE** The below provided data was tested while having a min percentage of 99% and having the test_file_exemption as ``` test_file_exemption { exempted_file_path: "utility/src/main/java/org/oppia/android/util/logging/ConsoleLogger.kt" override_min_coverage_percent_required: 20 } test_file_exemption { exempted_file_path: "domain/src/main/java/org/oppia/android/domain/auth/FirebaseAuthWrapperImpl.kt" override_min_coverage_percent_required: 70 } ``` ## Coverage Report - Number of files assessed: 5 - Coverage Analysis: **FAIL** ❌ | File | Coverage | Lines Hit | Status | Min Required | |-----|:----------:|---------:|:-------:|:--------------:| | [MathTokenizer.kt](https://github.com/oppia/oppia-android/tree/develop/utility/src/main/java/org/oppia/android/util/math/MathTokenizer.kt) | 94% | 197 / 209 | ❌ | 99% | |Exempted 🔻| | [FirebaseAuthWrapperImpl.kt](https://github.com/oppia/oppia-android/tree/develop/domain/src/main/java/org/oppia/android/domain/auth/FirebaseAuthWrapperImpl.kt) | 31% | 5 / 16 | ❌ | 70% | <details> <summary>Succeeded Coverages</summary><br> | File | Coverage | Lines Hit | Status | Min Required | |-----|:----------:|---------:|:-------:|:--------------:| | [MathModel.kt](https://github.com/oppia/oppia-android/tree/develop/utility/src/main/java/org/oppia/android/util/parser/math/MathModel.kt) | 100% | 19 / 19 | ✅ | 99% | |Exempted 🔻| | [ConsoleLogger.kt](https://github.com/oppia/oppia-android/tree/develop/utility/src/main/java/org/oppia/android/util/logging/ConsoleLogger.kt) | 54% | 30 / 55 | ✅ | 20% | </details> ### Test File Exempted Cases - [ActivityComponent.kt](https://github.com/oppia/oppia-android/tree/develop/app/src/main/java/org/oppia/android/app/activity/ActivityComponent.kt) [Todo] Add report with failure case and coverage check status. <details> <summary>Old template</summary> --- ## Coverage Report - Total covered files: 5 - Coverage Status: **FAIL** ### Failed Coverages Min Coverage Required: 40% _// the set value is 10 (MIN_THRESHOLD), Using 40 for just testing._ | Covered File | Percentage | Line Coverage | Status | |--------------|------------|---------------|--------| |[FirebaseAuthWrapperImpl.kt](https://github.com/oppia/oppia-android/tree/develop/domain/src/main/java/org/oppia/android/domain/auth/FirebaseAuthWrapperImpl.kt)|31.25%|5 / 16|:x:| <details> <summary>Succeeded Coverages</summary><br> | Covered File | Percentage | Line Coverage | Status | |--------------|------------|---------------|--------| |[NumericExpressionEvaluator.kt](https://github.com/oppia/oppia-android/tree/develop/utility/src/main/java/org/oppia/android/util/math/NumericExpressionEvaluator.kt)|86.36%|19 / 22|:white_check_mark:| |[MathTokenizer.kt](https://github.com/oppia/oppia-android/tree/develop/utility/src/main/java/org/oppia/android/util/math/MathTokenizer.kt)|94.26%|197 / 209|:white_check_mark:| |[RealExtensions.kt](https://github.com/oppia/oppia-android/tree/develop/utility/src/main/java/org/oppia/android/util/math/RealExtensions.kt)|89.73%|201 / 224|:white_check_mark:| </details> ### Anomaly Cases - The file: [ActivityComponent.kt](https://github.com/oppia/oppia-android/tree/develop/app/src/main/java/org/oppia/android/app/activity/ActivityComponent.kt) is exempted from having a test file; skipping coverage check. --- </details> </details>
- Loading branch information
