Closed Rd4dev closed 3 months ago
Hey @BenHenning, just wanted to clear 2 main things up at the start.
The .md report as I assume it to be most useful for CI runs, containing essential result data about the percentage of covered code, and can be added as a comment. This base template was discussed in our meeting notes.
Total coverage:
Files covered: (# changed / # run with coverage)
Coverage percentage: ##% covered / ##% expected
LOC: # covered / # instrumented
(indent left) Specific coverage:
app/src/.../app
home
HomeActivity.kt - 87% (110/115)
...
...
The current Markdown report follows this foundational format and now appears as:
Total coverage:
Probably simplifying it to Line coverage: 19 / 19 covered, Function coverage: 7 / 11 covered, Branch 2 / 2 covered
I would like to know if any additional details that should be included in the report for local generation.
Variable Path Definitions Across Different OS
Windows users typically use absolute paths like C:/Users/...
, whereas in WSL, paths are like /mnt/c/Users
.
So if we direct developers to use an absolute path, would this cause any confusion, and probably would it need to be handled in our file creation processes.
Providing and Handling Different Kinds of Paths
coverage/coverage.html
, where the file extension could be incorrect (e.g., it might be a Markdown generation) - with command
bazel run //scripts:run_coverage -- $(pwd) utility/...parser/math/MathModel.kt MARKDOWN
coverage/
, requiring us to check if a default filename should be provided.Subpaths and Endless/Long Chains
Managing Subdirectories:
utility/src/main/java/org/oppia/android/util/parser/math/MathModel.kt
, I propose creating subdirectories under the same path to maintain organized coverage results.(absolute/relative to $repoRoot)/utility/src/main/java/org/oppia/android/util/parser/math/MathModel/coverage.html
.Handling Long Input Paths:
bazel run ... /coverageData/report/path/place/here/
) our mapping would result in $repoRoot/(user_defined_path)coverageData/report/path/place/here/(our_default_path)/utility/src/main/java/org/oppia/android/util/parser/math/MathModel/coverage.html. I just don't know, if this is a simple process and its just something that I complicate thinking of weird possibilities and approaches
I've implemented a solution based on these considerations, but it deviates from our previous discussions. I'm seeking your feedback to refine and adjust as necessary.
So basically, I just made it simple to not get developers input itself and make use of the filePath argument and create a dir coverage_reports in the repoRoot, based on the HTML or MARKDOWN arg
so for a command -
bazel run //scripts:run_coverage -- $(pwd) utility/src/main/java/org/oppia/android/util/parser/math/MathModel.kt HTML
// it would be
reportOutputPath: oppia-android/coverage_reports/utility/src/main/java/org/oppia/android/util/parser/math/MathModel/coverage.html
and for
bazel run //scripts:run_coverage -- $(pwd) app/src/main/java/org/oppia/android/app/mydownloads/MyDownloadsActivity.kt MARKDOWN
// it would be
reportOutputPath: oppia-android/coverage_reports/app/src/main/java/org/oppia/android/app/mydownloads/MyDownloadsActivity/coverage.md
(Even this feels like a train)
.gitignore
.Would it be possible to clear this up, if we can just proceed this way or should taking inputs from developers/users be mandatory?
Note that this is a summarized snapshot. See the CI artifacts for detailed differences.
@BenHenning,
Implemented a base html layout for code coverage (TODO: still doesn't have computations done to have a cumulative coverage representation)
The lcov genhtml generation produces coverage report with 2 views
Line Coverage
Function Coverage
And as far as seen no branch coverage (yet to be tested with multiple targets)
So I wanted every detail to be in one single place so wanted to combine them like this base structure (derived from other coverage reports)
This has 4 columns:
Am also thinking if we can include the actual function hit with the function names like the lcov genhtml function view did, either separately or right below the available html report.
Also, the coverage (representative color) is decided based on the cumulative coverage result of the line with line, branch and functions. So like | Line | Branch | Function | Coverage color |
---|---|---|---|---|
FULL | FULL | FULL | GREEN | |
FULL | FULL | NONE | YELLOW | |
FULL | NONE | NONE | YELLOW | |
NONE | FULL | NONE | YELLOW | |
NONE | NONE | NONE | RED |
This still needs a lot of formatting to be done. And for now the coverage representations are hardcoded.
But would like to get an approval on the html layout before proceeding.
Edit: Sorry this is long, to just address your comments here is a shorter comment: https://github.com/oppia/oppia-android/pull/5443#issuecomment-2189264044
@BenHenning, to some extent I think we might be on the same page, but I just wanted to confirm with actually showing you what and how I intend to go with the overall flow of the project
Scripts:
Start:
bazel run //scripts:run_coverage -- $(pwd) utility/src/main/java/org/oppia/android/util/parser/math/MathModel.kt HTML
Now the run_coverage scripts ie. RunCoverage.kt is triggered
if yes
then it skips coverage
if no
then
- it tries to map the file path to its respective test targets and returns a list of test targets
so for 1:1 mapping
utility/src/main/java/org/oppia/android/util/parser/math/MathModel.kt ->
[//utility/src/test/java/org/oppia/android/util/parser/math:MathModelTest]
for many:1 mapping
app/src/main/java/org/oppia/android/app/home/HomeActivity.kt ->
[//app:src/sharedTest/java/org/oppia/android/app/home/HomeActivityTest,
//app:src/test/java/org/oppia/android/app/home/HomeActivityLocalTest]
we then collect it and make it as a list with the returned CoverageReport so now we would have a list of CoverageReports
the coverage reports would be like for 1:1 mapping (just for representation not acutal data)
[{
bazel_test_target://utility/src/test/java/org/oppia/android/util/parser/math:MathModelTest,
CoveredLine{line_number:2}
}]
for many:1 mapping (just for representation not acutal data)
[{
bazel_test_target://app:src/sharedTest/java/org/oppia/android/app/home/HomeActivityTest,
CoveredLine{line_number:2}
},
{
bazel_test_target://app:src/test/java/org/oppia/android/app/home/HomeActivityLocalTest,
CoveredLine{line_number:3}
}]
I think until this there needs no huge change in the way it is implemented (except for proto data structure)
Now we have the list of coverage reports and the thing left to do is actually make it something useful by creating a .md or .html file So to do this we pass this list of coverage reports to the CoverageReporter.kt
I consider .md reports are mostly helpful for ci runs and .html reports for local development purposes
so in here the process flows like with the list of coverage reports we either send these list of coverage reports to generating html or generating md reports based on the input
for 1:1 mapping The md text is generated just with the one - coverage report and[0] and saved as md file to the output path.
for many:1 mapping (Haven't been implemented but the plan is) say for the coverage reports
[{
bazel_test_target://app:src/sharedTest/java/org/oppia/android/app/home/HomeActivityTest,
CoveredLine{line_number:2}
},
{
bazel_test_target://app:src/test/java/org/oppia/android/app/home/HomeActivityLocalTest,
CoveredLine{line_number:3}
}]
Both are essentially going to have the same number of founds (as both are for same file)
coverage reports[0] had line 1: FULL, line 2: FULL, line 3: NONE, line 4: NONE
coverage reports[1] had line 1: NONE, line 2: FULL, line 3: FULL, line 4: NONE
(to be computed) so both should be computed to result in
line 1: covered
line 2: covered
line 3: covered
line 4: not covered
and the combined result will be made as a md file and saved to the output path. so thereby we achieve the consolidated output
This is something I still haven't tried but I am planning to do this to achieve the report generation of all the affected files in the ci run It would probably be derived from the unit test, compute affected test runs
So the plan is to return the actual md data like for file 1:
**Lines:** 1/1 covered
So when a CI run is started,
say file1:
**file_path:** /file1.kt
and this md will be saved in the ci run in a variable or in a log file
then again, the process continues for file 2
say file2:
**file_path:** /file2.kt
and this md will be added to the previous saved md string or log
so now the log / stored variable will look like
**file_path:** /file1.kt
**file_path:** /file2.kt
and probably by the end it will have all the md reports in one place and by the end of all ci runs on affected / changed files we will have a single md report in string variable or log, that can be uploaded as a comment
This is what I had in my mind for the entire process Hmm.. are there any conflicts?
As with .html, I assume this to be most useful with local developments and in the first Android CLAM meet we had discussions on this and decided to just have .html for just local development for now since I mentioned there might be few things to note on how to collate the entire html report and we decided to stick with just md for now as mentioned in M2 milestone PR 2.1
with HTML the flow will be the same as done with md reports
for 1:1 mapping The html file content is generated just with the one - coverage report and[0] and saved as .html file to the output path. And I am right now sticking with generating it to the path of $repoRoot/coverage_reports/..../coverage.html as for few reasons that was described in https://github.com/oppia/oppia-android/pull/5443#issuecomment-2186663348 And that might be changed later as required
For now I am trying to accumulate all line, branch and function coverage details But since you mentioned we can skip branch coverage, I am thinking of sticking with line and function coverages And for 1:1 mapping this should be straightforward of just mapping the coverage report data to appropriate representations and the result will look something like below (this is just line coverage, functions need to be added)
(this is yet to be implemented in PR 1.5) for many:1 mapping This will also be the same for md generation ie. There will be just one single file say $repoRoot/coverage_reports/HomeActivity/coverage.html (as both targets represent coverages for same file) and first a collective result of the lines covered and not need to be found, similar to md as,
coverage reports[0] had line 1: FULL, line 2: FULL, line 3: NONE, line 4: NONE
coverage reports[1] had line 1: NONE, line 2: FULL, line 3: FULL, line 4: NONE
(to be computed) so both should be computed to result in
line 1: covered
line 2: covered
line 3: covered
line 4: not covered
and this will be same for functions coverage too, and the color of the coverage representation will look like -
Line | Branch | Function | Coverage color |
---|---|---|---|
FULL | FULL | FULL | GREEN |
FULL | FULL | NONE | YELLOW |
FULL | NONE | NONE | YELLOW |
NONE | FULL | NONE | YELLOW |
NONE | NONE | NONE | RED |
and the combined result will be made as a html file and saved to the output path. so thereby we achieve the consolidated output
But I can see in your comments you mentioning about M2 HTML genertions, if that is something we absolutely need... (I still need to figure that out then) would confirm that and may be look into that for M2
So this is just the entire high level flow of the entire project majorly on M1 details and a outline on M2, from the comments I sense the concerns on 2 main things
Another concern on proto structure to have the covered file as list, that is actually something I too wanted to discuss on I just followed the pattern provided in the issue tracker
message CoverageReport {
string bazel_test_target = 1; // The Bazel test target that was run.
repeated CoveredFile covered_file = 2; // Files with some coverage in this report.
}
but felt irrelevant to how we are proceeding so I think we will need to remove that and have everything under CoverageReport itself.
I think these might address few of the concerns but would definitely need to know if the flow is the same as you think it should be and was the flow right and would like to hear and work on feedbacks.
@BenHenning, Hmm... Okay the previous comment just looks like a novel sorry, may be in short to just address the comments,
We will basically have the list of CoverageReports in RunCoverage.kt that will be sent to generate reports to CoverageReporter.kt with the list, eg:
List of CoverageReport ->
[{
bazel_test_target://app:src/sharedTest/java/org/oppia/android/app/home/HomeActivityTest,
CoveredLine{line_number:2}
},
{
bazel_test_target://app:src/test/java/org/oppia/android/app/home/HomeActivityLocalTest,
CoveredLine{line_number:3}
}]
we will need to generate a cummulative coverage data in the way like
coverage reports[0] had line 1: FULL, line 2: FULL, line 3: NONE, line 4: NONE
coverage reports[1] had line 1: NONE, line 2: FULL, line 3: FULL, line 4: NONE
(to be computed) so both should be computed to result in
line 1: covered
line 2: covered
line 3: covered
line 4: not covered
and use this to data generate md or html reports and save them so thereby we achieve the consolidated output
The ci run will run this command for each affected file and the plan is to store each run's output md string to a variable or log and at the end upload the collective generated md string as a comment.
Note that this is a summarized snapshot. See the CI artifacts for detailed differences.
I think we discussed what was left to do here in our meeting today @Rd4dev (and I conveyed that to @adhiamboperes separately). Please let me know if anything else is needed from here, otherwise I'm deferring to @adhiamboperes for reviews over the next ~1.5 days.
Thanks @BenHenning, just one thing to confirm. Can I proceed with having the script itself set the default output storage path for the md and html files (as shown in the demo M1)?
The reasons are discussed here https://github.com/oppia/oppia-android/pull/5443#issuecomment-2186663348 But in-short, just for 2 things
For now, the entire logic is done for
md report:
html report:
html report 2: to represent fail cases
And things left to do are to add tests
Thanks @BenHenning, just one thing to confirm. Can I proceed with having the script itself set the default output storage path for the md and html files (as shown in the demo M1)?
The reasons are discussed here #5443 (comment) But in-short, just for 2 things
- Handle different OS paths and ensure consistent file structure.
- Use a default directory for coverage reports to avoid overwriting.
@Rd4dev, I think having the script set the path may be the more logical path here, so the developer doesn't have to figure this out.
Please let me know if you still have other open questions.
@Rd4dev, I think having the script set the path may be the more logical path here, so the developer doesn't have to figure this out.
Please let me know if you still have other open questions.
Thanks @adhiamboperes, that clears up everything!
Also, as update: The logic and the report generation for this PR are done and tests for CoverageReporter
are added only thing left is to update the test cases for RunCoverage
.
(each test runs are taking a bit of time to complete)
Log:
INFO: From Testing //scripts/src/javatests/org/oppia/android/scripts/coverage:RunCoverageTest:
==================== Test output for //scripts/src/javatests/org/oppia/android/scripts/coverage:RunCoverageTest:
JUnit4 Test Runner
.
Time: 208.708
OK (1 test)
I will push them too once everything is tested and open this PR for review.
Note that this is a summarized snapshot. See the CI artifacts for detailed differences.
@adhiamboperes, the PR 1.5 tasks are done.
CoverageReporter
that outputs the coverage data in .md
and .html
formats as represented in the https://github.com/oppia/oppia-android/pull/5443#issuecomment-2191913964As of now the reports are returning a pair of computed coverage ratio and the actual report text.
And RunCoverage
takes the report text and saves it to the output file (in location _coveragereports/.../coverage.md or .html)
M2 These report texts and computed coverage ratios are returned with the plan to make them useful in M2
with these plans, I have addressed the existing review comments, would it be possible to PTAL!
Unassigning @adhiamboperes since the review is done.
Note that this is a summarized snapshot. See the CI artifacts for detailed differences.
Thanks for the review @adhiamboperes, addressed all the review comments and updated tests accordingly. Can you PTAL!
Unassigning @Rd4dev since a re-review was requested. @Rd4dev, please make sure you have addressed all review comments. Thanks!
@seanlip, could you PTAL? I have verified that @BenHenning's changes have been addressed.
Assigning @BenHenning for code owner reviews. Thanks!
Explanation
Fixes part of #5343
Project
[PR 1.5 of Project 4.1]
Changes Made
This PR introduces the
CoverageReporter
utility to generate the code coverage report. The main features and changes include:New Utility:
CoverageReporter
to generate code coverage reports.Command Line Arguments:
HTML
orMARKDOWN
.Report Generation:
coverage_reports
folder relative to therepoRoot
.Gitignore Update:
coverage_reports
folder is added to.gitignore
to ensure it is not added to the repository.Essential Checklist
For UI-specific PRs only
If your PR includes UI-related changes, then: