-
Notifications
You must be signed in to change notification settings - Fork 527
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix part of #5343: Enable Coverage Generation for a list of files #5461
Conversation
…args to and make reordering possible
… a long time causing TIMEOUT failures
…on the tests adding missed paranthesis
… then convert them to uppercase
…see if it still fails (intended FAILURE output case)
… see if it still fails (intended FAILURE output case)
…to see if it was reflected
…s are limitations
APK & AAB differences analysisNote that this is a summarized snapshot. See the CI artifacts for detailed differences. DevExpand to see flavor specificsUniversal APKAPK file size: 19 MiB (old), 19 MiB (new), 0 bytes (No change) APK download size (estimated): 17 MiB (old), 17 MiB (new), 23 bytes (Removed) Method count: 259155 (old), 259155 (new), 0 (No change) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6806 (old), 6806 (new), 0 (No change)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 18 MiB (old), 18 MiB (new), 0 bytes (No change) Configuration hdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 49 KiB (old), 49 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 45 KiB (old), 45 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 86 KiB (old), 86 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 57 KiB (old), 57 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 63 KiB (old), 63 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 63 KiB (old), 63 KiB (new), 0 bytes (No change) AlphaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 0 bytes (No change) APK download size (estimated): 10 MiB (old), 10 MiB (new), 108 bytes (Added) Method count: 115689 (old), 115689 (new), 0 (No change) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5774 (old), 5774 (new), 0 (No change)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 8 bytes (Added) Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) BetaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 4 bytes (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 9 bytes (Added) Method count: 115695 (old), 115695 (new), 0 (No change) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5774 (old), 5774 (new), 0 (No change)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 10 MiB (old), 10 MiB (new), 0 bytes (No change) Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) GaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 0 bytes (No change) APK download size (estimated): 10 MiB (old), 10 MiB (new), 31 bytes (Removed) Method count: 115695 (old), 115695 (new), 0 (No change) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5774 (old), 5774 (new), 0 (No change)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 10 MiB (old), 10 MiB (new), 0 bytes (No change) Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) |
@BenHenning, thanks for the review, and addressed all the follow-up review comments, can you PTAL? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Rd4dev! I just had one nit left, but other than that there's the open comment about output format for RunCoverage
. I think if you already have a very good idea on how to collect the pb files, I'm happy to approve this. If not, I think the simplification of predictably placing all the pb, md, and html files per test based on format might be preferable just to avoid having any special casing with proto files (and then you can collect the files as suggested in my other comment).
scripts/src/javatests/org/oppia/android/scripts/coverage/CoverageReporterTest.kt
Outdated
Show resolved
Hide resolved
scripts/src/java/org/oppia/android/scripts/coverage/RunCoverage.kt
Outdated
Show resolved
Hide resolved
scripts/src/java/org/oppia/android/scripts/coverage/CoverageReporter.kt
Outdated
Show resolved
Hide resolved
Needs a cleanup with handling of protos per test name paths
…o coverage report collection type
@BenHenning, updated the PR to include changes with #5480 and also added required test cases and adjustments, also removed the output path for the proto format and let the script decide the proto storage file. The thing still left is to address the need for md report storage for individual files (if mandatory will add them asap), can you PTAL? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scripts/src/java/org/oppia/android/scripts/coverage/RunCoverage.kt
Outdated
Show resolved
Hide resolved
Unassigning @BenHenning since they have already approved the PR. |
Explanation
Fixes part of #5343
Project
[PR 2.2 of Project 4.1]
Changes Made
Updated command:
Handled Edge cases:
Non-kotlin Files:
Input variations:
Input format:
New Proto Structure:
(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 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
Failing coverage
RunCoverage.kt
scripts/src/java/org/oppia/android/scripts/coverage/RunCoverage.ktMathModel.kt
utility/src/main/java/org/oppia/android/util/parser/math/MathModel.ktMultipleChoiceInputModule.kt
domain/src/main/java/org/oppia/android/domain/classify/rules/multiplechoiceinput/MultipleChoiceInputModule.ktPassing coverage
Files with passing code coverage
MathTokenizer.kt
utility/src/main/java/org/oppia/android/util/math/MathTokenizer.ktExempted coverage
Files exempted from coverage
ActivityComponent.kt
app/src/main/java/org/oppia/android/app/activity/ActivityComponent.ktTestExemptedFile.kt
app/src/main/java/org/oppia/android/app/activity/TestExemptedFile.ktSourceIncompatible.kt
app/src/main/java/org/oppia/android/app/activity/SourceIncompatible.ktTo be discussed:
shards_count
for RunCoverageTest.MIN_THRESHOLD
that need to be set.Todo
With this doing a script run of
will do coverage analysis on files:
Essential Checklist
Old Template
FOR TESTING PURPOSE
The below provided data was tested while having a min percentage of 99% and having the test_file_exemption as
Coverage Report
Succeeded Coverages
Test File Exempted Cases
[Todo] Add report with failure case and coverage check status.
Old template
Coverage Report
Failed Coverages
Min Coverage Required: 40% // the set value is 10 (MIN_THRESHOLD), Using 40 for just testing.
Succeeded Coverages
Anomaly Cases