Skip to content
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

Merged
merged 246 commits into from
Aug 10, 2024

Conversation

Rd4dev
Copy link
Collaborator

@Rd4dev Rd4dev commented Jul 10, 2024

Explanation

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)
image

Logged Report Text:

image

  • 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

File Failure Reason
File.kt No appropriate test file found for File.kt.

Failing coverage

File Coverage Lines Hit Status Min Required
RunCoverage.ktscripts/src/java/org/oppia/android/scripts/coverage/RunCoverage.kt
51.38% 148 / 288 70%
MathModel.ktutility/src/main/java/org/oppia/android/util/parser/math/MathModel.kt
77.78% 7 / 9 80% *
MultipleChoiceInputModule.ktdomain/src/main/java/org/oppia/android/domain/classify/rules/multiplechoiceinput/MultipleChoiceInputModule.kt
10.00% 1 / 10 30% *

* represents tests with custom overridden pass/fail coverage thresholds

Passing coverage

Files with passing code coverage
File Coverage Lines Hit Status Min Required
MathTokenizer.ktutility/src/main/java/org/oppia/android/util/math/MathTokenizer.kt
94.26% 197 / 209 70%

Exempted coverage

Files exempted from coverage
ActivityComponent.ktapp/src/main/java/org/oppia/android/app/activity/ActivityComponent.kt
TestExemptedFile.ktapp/src/main/java/org/oppia/android/app/activity/TestExemptedFile.kt
SourceIncompatible.ktapp/src/main/java/org/oppia/android/app/activity/SourceIncompatible.kt

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

  • 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: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • 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).
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

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 94% 197 / 209 99%
Exempted 🔻
FirebaseAuthWrapperImpl.kt 31% 5 / 16 70%
Succeeded Coverages
File Coverage Lines Hit Status Min Required
MathModel.kt 100% 19 / 19 99%
Exempted 🔻
ConsoleLogger.kt 54% 30 / 55 20%

Test File Exempted Cases

[Todo] Add report with failure case and coverage check status.

Old template

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 31.25% 5 / 16
Succeeded Coverages
Covered File Percentage Line Coverage Status
NumericExpressionEvaluator.kt 86.36% 19 / 22
MathTokenizer.kt 94.26% 197 / 209
RealExtensions.kt 89.73% 201 / 224

Anomaly Cases


Rd4dev added 30 commits June 28, 2024 21:46
…see if it still fails (intended FAILURE output case)
… see if it still fails (intended FAILURE output case)
Copy link

github-actions bot commented Aug 9, 2024

APK & AAB differences analysis

Note that this is a summarized snapshot. See the CI artifacts for detailed differences.

Dev

Expand to see flavor specifics

Universal APK

APK 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)

  • Anim: 43 (old), 43 (new), 0 (No change)
  • Animator: 26 (old), 26 (new), 0 (No change)
  • Array: 15 (old), 15 (new), 0 (No change)
  • Attr: 922 (old), 922 (new), 0 (No change)
  • Bool: 9 (old), 9 (new), 0 (No change)
  • Color: 967 (old), 967 (new), 0 (No change)
  • Dimen: 1048 (old), 1048 (new), 0 (No change)
  • Drawable: 378 (old), 378 (new), 0 (No change)
  • Id: 1271 (old), 1271 (new), 0 (No change)
  • Integer: 37 (old), 37 (new), 0 (No change)
  • Interpolator: 11 (old), 11 (new), 0 (No change)
  • Layout: 378 (old), 378 (new), 0 (No change)
  • Menu: 3 (old), 3 (new), 0 (No change)
  • Mipmap: 1 (old), 1 (new), 0 (No change)
  • Plurals: 10 (old), 10 (new), 0 (No change)
  • Raw: 2 (old), 2 (new), 0 (No change)
  • String: 848 (old), 848 (new), 0 (No change)
  • Style: 831 (old), 831 (new), 0 (No change)
  • Xml: 6 (old), 6 (new), 0 (No change)

Lesson assets: 111 (old), 111 (new), 0 (No change)

AAB differences

Expand to see AAB specifics

Supported configurations:

  • hdpi (same)
  • ldpi (same)
  • mdpi (same)
  • tvdpi (same)
  • xhdpi (same)
  • xxhdpi (same)
  • xxxhdpi (same)

Base APK

APK file size: 18 MiB (old), 18 MiB (new), 0 bytes (No change)
APK download size (estimated): 17 MiB (old), 17 MiB (new), 26 bytes (Removed)

Configuration hdpi

APK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change)
APK download size (estimated): 18 KiB (old), 18 KiB (new), 0 bytes (No change)

Configuration ldpi

APK file size: 49 KiB (old), 49 KiB (new), 0 bytes (No change)
APK download size (estimated): 14 KiB (old), 14 KiB (new), 0 bytes (No change)

Configuration mdpi

APK file size: 45 KiB (old), 45 KiB (new), 0 bytes (No change)
APK download size (estimated): 14 KiB (old), 14 KiB (new), 0 bytes (No change)

Configuration tvdpi

APK file size: 86 KiB (old), 86 KiB (new), 0 bytes (No change)
APK download size (estimated): 29 KiB (old), 29 KiB (new), 0 bytes (No change)

Configuration xhdpi

APK file size: 57 KiB (old), 57 KiB (new), 0 bytes (No change)
APK download size (estimated): 21 KiB (old), 21 KiB (new), 0 bytes (No change)

Configuration xxhdpi

APK file size: 63 KiB (old), 63 KiB (new), 0 bytes (No change)
APK download size (estimated): 29 KiB (old), 29 KiB (new), 0 bytes (No change)

Configuration xxxhdpi

APK file size: 63 KiB (old), 63 KiB (new), 0 bytes (No change)
APK download size (estimated): 28 KiB (old), 28 KiB (new), 0 bytes (No change)

Alpha

Expand to see flavor specifics

Universal APK

APK 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)

  • Anim: 33 (old), 33 (new), 0 (No change)
  • Animator: 24 (old), 24 (new), 0 (No change)
  • Array: 14 (old), 14 (new), 0 (No change)
  • Attr: 888 (old), 888 (new), 0 (No change)
  • Bool: 8 (old), 8 (new), 0 (No change)
  • Color: 820 (old), 820 (new), 0 (No change)
  • Dimen: 780 (old), 780 (new), 0 (No change)
  • Drawable: 340 (old), 340 (new), 0 (No change)
  • Id: 1217 (old), 1217 (new), 0 (No change)
  • Integer: 32 (old), 32 (new), 0 (No change)
  • Interpolator: 11 (old), 11 (new), 0 (No change)
  • Layout: 341 (old), 341 (new), 0 (No change)
  • Menu: 1 (old), 1 (new), 0 (No change)
  • Mipmap: 1 (old), 1 (new), 0 (No change)
  • Plurals: 10 (old), 10 (new), 0 (No change)
  • String: 781 (old), 781 (new), 0 (No change)
  • Style: 472 (old), 472 (new), 0 (No change)
  • Xml: 1 (old), 1 (new), 0 (No change)

Lesson assets: 111 (old), 111 (new), 0 (No change)

AAB differences

Expand to see AAB specifics

Supported configurations:

  • hdpi (same)
  • ldpi (same)
  • mdpi (same)
  • tvdpi (same)
  • xhdpi (same)
  • xxhdpi (same)
  • xxxhdpi (same)

Base APK

APK file size: 11 MiB (old), 11 MiB (new), 8 bytes (Added)
APK download size (estimated): 10 MiB (old), 10 MiB (new), 24 bytes (Added)

Configuration hdpi

APK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change)
APK download size (estimated): 17 KiB (old), 17 KiB (new), 0 bytes (No change)

Configuration ldpi

APK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change)
APK download size (estimated): 13 KiB (old), 13 KiB (new), 0 bytes (No change)

Configuration mdpi

APK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change)
APK download size (estimated): 13 KiB (old), 13 KiB (new), 0 bytes (No change)

Configuration tvdpi

APK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change)
APK download size (estimated): 27 KiB (old), 27 KiB (new), 0 bytes (No change)

Configuration xhdpi

APK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change)
APK download size (estimated): 20 KiB (old), 20 KiB (new), 0 bytes (No change)

Configuration xxhdpi

APK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change)
APK download size (estimated): 28 KiB (old), 28 KiB (new), 0 bytes (No change)

Configuration xxxhdpi

APK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change)
APK download size (estimated): 27 KiB (old), 27 KiB (new), 0 bytes (No change)

Beta

Expand to see flavor specifics

Universal APK

APK 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)

  • Anim: 33 (old), 33 (new), 0 (No change)
  • Animator: 24 (old), 24 (new), 0 (No change)
  • Array: 14 (old), 14 (new), 0 (No change)
  • Attr: 888 (old), 888 (new), 0 (No change)
  • Bool: 8 (old), 8 (new), 0 (No change)
  • Color: 820 (old), 820 (new), 0 (No change)
  • Dimen: 780 (old), 780 (new), 0 (No change)
  • Drawable: 340 (old), 340 (new), 0 (No change)
  • Id: 1217 (old), 1217 (new), 0 (No change)
  • Integer: 32 (old), 32 (new), 0 (No change)
  • Interpolator: 11 (old), 11 (new), 0 (No change)
  • Layout: 341 (old), 341 (new), 0 (No change)
  • Menu: 1 (old), 1 (new), 0 (No change)
  • Mipmap: 1 (old), 1 (new), 0 (No change)
  • Plurals: 10 (old), 10 (new), 0 (No change)
  • String: 781 (old), 781 (new), 0 (No change)
  • Style: 472 (old), 472 (new), 0 (No change)
  • Xml: 1 (old), 1 (new), 0 (No change)

Lesson assets: 111 (old), 111 (new), 0 (No change)

AAB differences

Expand to see AAB specifics

Supported configurations:

  • hdpi (same)
  • ldpi (same)
  • mdpi (same)
  • tvdpi (same)
  • xhdpi (same)
  • xxhdpi (same)
  • xxxhdpi (same)

Base APK

APK file size: 10 MiB (old), 10 MiB (new), 0 bytes (No change)
APK download size (estimated): 9 MiB (old), 9 MiB (new), 7 bytes (Added)

Configuration hdpi

APK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change)
APK download size (estimated): 17 KiB (old), 17 KiB (new), 0 bytes (No change)

Configuration ldpi

APK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change)
APK download size (estimated): 13 KiB (old), 13 KiB (new), 0 bytes (No change)

Configuration mdpi

APK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change)
APK download size (estimated): 13 KiB (old), 13 KiB (new), 0 bytes (No change)

Configuration tvdpi

APK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change)
APK download size (estimated): 27 KiB (old), 27 KiB (new), 0 bytes (No change)

Configuration xhdpi

APK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change)
APK download size (estimated): 20 KiB (old), 20 KiB (new), 0 bytes (No change)

Configuration xxhdpi

APK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change)
APK download size (estimated): 28 KiB (old), 28 KiB (new), 0 bytes (No change)

Configuration xxxhdpi

APK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change)
APK download size (estimated): 27 KiB (old), 27 KiB (new), 0 bytes (No change)

Ga

Expand to see flavor specifics

Universal APK

APK 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)

  • Anim: 33 (old), 33 (new), 0 (No change)
  • Animator: 24 (old), 24 (new), 0 (No change)
  • Array: 14 (old), 14 (new), 0 (No change)
  • Attr: 888 (old), 888 (new), 0 (No change)
  • Bool: 8 (old), 8 (new), 0 (No change)
  • Color: 820 (old), 820 (new), 0 (No change)
  • Dimen: 780 (old), 780 (new), 0 (No change)
  • Drawable: 340 (old), 340 (new), 0 (No change)
  • Id: 1217 (old), 1217 (new), 0 (No change)
  • Integer: 32 (old), 32 (new), 0 (No change)
  • Interpolator: 11 (old), 11 (new), 0 (No change)
  • Layout: 341 (old), 341 (new), 0 (No change)
  • Menu: 1 (old), 1 (new), 0 (No change)
  • Mipmap: 1 (old), 1 (new), 0 (No change)
  • Plurals: 10 (old), 10 (new), 0 (No change)
  • String: 781 (old), 781 (new), 0 (No change)
  • Style: 472 (old), 472 (new), 0 (No change)
  • Xml: 1 (old), 1 (new), 0 (No change)

Lesson assets: 111 (old), 111 (new), 0 (No change)

AAB differences

Expand to see AAB specifics

Supported configurations:

  • hdpi (same)
  • ldpi (same)
  • mdpi (same)
  • tvdpi (same)
  • xhdpi (same)
  • xxhdpi (same)
  • xxxhdpi (same)

Base APK

APK file size: 10 MiB (old), 10 MiB (new), 0 bytes (No change)
APK download size (estimated): 9 MiB (old), 9 MiB (new), 26 bytes (Added)

Configuration hdpi

APK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change)
APK download size (estimated): 17 KiB (old), 17 KiB (new), 0 bytes (No change)

Configuration ldpi

APK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change)
APK download size (estimated): 13 KiB (old), 13 KiB (new), 0 bytes (No change)

Configuration mdpi

APK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change)
APK download size (estimated): 13 KiB (old), 13 KiB (new), 0 bytes (No change)

Configuration tvdpi

APK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change)
APK download size (estimated): 27 KiB (old), 27 KiB (new), 0 bytes (No change)

Configuration xhdpi

APK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change)
APK download size (estimated): 20 KiB (old), 20 KiB (new), 0 bytes (No change)

Configuration xxhdpi

APK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change)
APK download size (estimated): 28 KiB (old), 28 KiB (new), 0 bytes (No change)

Configuration xxxhdpi

APK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change)
APK download size (estimated): 27 KiB (old), 27 KiB (new), 0 bytes (No change)

@Rd4dev
Copy link
Collaborator Author

Rd4dev commented Aug 9, 2024

@BenHenning, thanks for the review, and addressed all the follow-up review comments, can you PTAL?

@oppiabot oppiabot bot assigned BenHenning and unassigned Rd4dev Aug 9, 2024
Copy link

oppiabot bot commented Aug 9, 2024

Unassigning @Rd4dev since a re-review was requested. @Rd4dev, please make sure you have addressed all review comments. Thanks!

Copy link
Member

@BenHenning BenHenning left a 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).

@BenHenning BenHenning assigned Rd4dev and unassigned BenHenning Aug 9, 2024
@Rd4dev
Copy link
Collaborator Author

Rd4dev commented Aug 10, 2024

@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?

@oppiabot oppiabot bot assigned BenHenning and unassigned Rd4dev Aug 10, 2024
Copy link

oppiabot bot commented Aug 10, 2024

Unassigning @Rd4dev since a re-review was requested. @Rd4dev, please make sure you have addressed all review comments. Thanks!

Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Rd4dev! This LGTM. I don't think any additional work is needed on the MD pathing (see my other comment).

Please hold from merging this until I finish my review pass on #5465 (will follow up here).

Copy link

oppiabot bot commented Aug 10, 2024

Unassigning @BenHenning since they have already approved the PR.

@BenHenning BenHenning merged commit 8719642 into develop Aug 10, 2024
24 checks passed
@BenHenning BenHenning deleted the code_coverage_list_of_files branch August 10, 2024 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants