Closed BenHenning closed 3 weeks ago
BenHenning
commented
7 months ago NB: This issue is locked to ensure that it only contains updates for requirements. If you have questions regarding this project, please use its corresponding discussion category: https://github.com/oppia/oppia-android/discussions/categories/gsoc-q-a-4-1-code-coverage-support.
BenHenning
commented
7 months ago NB: The main issue comment has been updated to include the technical details directly rather than a link to a Gist (so that we can easily change this if needed). None of the technical requirements have actually changed.
Rd4dev
commented
3 months ago Introduce RunCoverage and CoverageRunner utilities to run code coverage for a specific bazel test target.
| Starting Date | Creation Date | Merge Date |
|---|---|---|
| 27-05-2024 | 01-06-2024 | 03-06-2024 |
| Task | Due Date |
|---|---|
| ✅ Setup the workspace with bazel 6.5.0. | 27-05-2024 |
| ✅ Introduce RunCoverage script. | 27-05-2024 |
| ✅ Introduce CoverageRunner utility to call Bazel command. | 28-05-2024 |
| ✅ Add implementation in BazelClient to execute Bazel coverage command. | 29-05-2024 |
| ✅ Add tests for execution of Bazel coverage command through RunCoverage script. | 01-06-2024 |
Updating the test exemption script.
| Starting Date | Creation Date | Merge Date |
|---|---|---|
| 01-06-2024 | 03-06-2024 | 05-06-2024 |
| Revised Dates | Starting Date | Creation Date | Merge Date |
|---|---|---|---|
| 17-06-2024 | 18-06-2024 | 20-06-2024 |
| Task | Original Due Date | Revised Due Date |
|---|---|---|
| ✅ Update the test exemption check script proto. | 01-06-2024 | 17-06-2024 |
| ✅ Update the exemption_type data. | 02-06-2024 | 17-06-2024 |
| ✅ Add / Update tests for TestFileCheckTest scripts as required. | 03-06-2024 | 18-06-2024 |
Update / Implement script to run code coverage for a specific file replacing test target argument.
| Starting Date | Creation Date | Merge Date |
|---|---|---|
| 03-06-2024 | 08-06-2024 | 10-06-2024 |
| Revised Dates | Starting Date | Creation Date | Merge Date |
|---|---|---|---|
| 19-06-2024 | 20-06-2024 | 22-06-2024 |
| Task | Original Due Date | Revised Due Date |
|---|---|---|
| ✅ Update the script to take in a file as argument instead of target. | 03-06-2024 | 19-06-2024 |
| ✅ Check with the exempted test file list to log the coverage ratio to 0%. | 03-06-2024 | 19-06-2024 |
| ✅ Add functionality to map the file name to related test targets. | 04-06-2024 | 19-06-2024 |
| ✅ Include execution for related Test and LocalTest Targets. | 06-06-2024 | 20-06-2024 |
| ✅ Add tests for execution of Bazel coverage command with filename. | 08-06-2024 | 20-06-2024 |
Interpreting the results and building proto for data processing.
| Starting Date | Creation Date | Merge Date |
|---|---|---|
| 08-06-2024 | 13-06-2024 | 15-06-2024 |
| Revised Dates | Starting Date | Creation Date | Merge Date |
|---|---|---|---|
| 21-06-2024 | 23-06-2024 | 25-06-2024 |
| Task | Original Due Date | Revised Due Date |
|---|---|---|
| ✅ Extract the coverage data path from the results. | 08-06-2024 | 21-06-2024 |
| ✅ Introduce coverage.proto to store the produced coverage data. | 08-06-2024 | 21-06-2024 |
| ✅ Parse the coverage data and store them to the proto. | 10-06-2024 | 21-06-2024 |
| ✅ Isolate the coverage data to only contain the target coverage data. | 11-06-2024 | 22-06-2024 |
| ✅ Include coverage data to proto for any related tests - 'LocalTest'. | 12-06-2024 | 22-06-2024 |
| ✅ Add tests to validate acquired coverage data stored in the proto. | 13-06-2024 | 23-06-2024 |
Introduce CoverageReporter utility and generate the code coverage report.
| Starting Date | Creation Date | Merge Date |
|---|---|---|
| 13-06-2024 | 20-06-2024 | 22-06-2024 |
| Revised Dates | Starting Date | Creation Date | Merge Date |
|---|---|---|---|
| 24-06-2024 | 28-06-2024 | 30-06-2024 |
| Task | Original Due Date | Revised Due Date |
|---|---|---|
| ✅ Introduce CoverageReporter utility to generate the code coverage report. | 13-06-2024 | 24-06-2024 |
| ✅ Compute the coverage Ratio. | 13-06-2024 | 24-06-2024 |
| ✅ Generate Markdown report with proto. | 15-06-2024 | 25-06-2024 |
| ✅ Generate HTML report with proto. | 18-06-2024 | 27-06-2024 |
| ✅ Add tests to validate the Markdown and HTML report generation and coverage ratios. | 20-06-2024 | 28-06-2024 |
To work on reviews and code changes.
| Starting Date | Creation Date | Merge Date |
|---|---|---|
| 22-06-2024 | 26-06-2024 | 28-06-2024 |
Code Reviews and Evaluations
| Starting Date | Creation Date | Merge Date |
|---|---|---|
| 28-06-2024 | 08-07-2024 | 12-07-2024 |
Implement Aggregate Coverage Analysis for many:1 relationship files
| Starting Date | Creation Date | Merge Date |
|---|---|---|
| 12-07-2024 | 15-07-2024 | 19-07-2024 |
| Task | Due Date |
|---|---|
| ✅ Aggregate by calculating multiple resultant coverage reports into one single report | 12-07-2024 |
| ✅ Generate Markdown reports with the aggregated coverage report. | 13-07-2024 |
| ✅ Generate Markdown reports with the aggregated coverage report. | 14-07-2024 |
| ✅ Update test cases to validate many to one relationship coverage analysis | 15-07-2024 |
Enable Coverage Generation for a list of files
| Starting Date | Creation Date | Merge Date |
|---|---|---|
| 16-07-2024 | 23-07-2024 | 27-07-2024 |
| Task | Due Date |
|---|---|
| ✅ Let the command-line args take in a list of string (files). | 16-07-2024 |
| ✅ Update the implementation to handle mapping of multiple files to appropriate targets. | 17-07-2024 |
| ✅ Update the implementation to generate proto with multiple file inputs. | 19-07-2024 |
| ✅ Update the implementation to generate markdown with multiple file inputs. | 21-07-2024 |
| ✅ Update the implementation to generate html with multiple file inputs. | 22-07-2024 |
| ✅ Update the test cases to handle multiple file inputs and outputs. | 23-07-2024 |
Introduce new CI workflow for code coverage
| Starting Date | Creation Date | Merge Date |
|---|---|---|
| 24-07-2024 | 30-07-2024 | 03-08-2024 |
| Task | Due Date |
|---|---|
| ✅ Introduce a new CI workflow that utilizes the coverage script. | 24-07-2024 |
| ✅ Configure it to run it with and without cache. | 25-07-2024 |
| ✅ Configure it to run as a series of buckets. | 27-07-2024 |
| ✅ Configure it to fail if it's below the threshold. | 28-07-2024 |
| ✅ Configure it to generate markdown reports. | 29-07-2024 |
| ✅ (Updated) Run Coverage Check only after Unit Test Pass. | 30-07-2024 |
| ✅ Analyze the new runs. | 30-07-2024 |
Upload generated markdown report as comment
| Starting Date | Creation Date | Merge Date |
|---|---|---|
| 31-07-2024 | 02-08-2024 | 06-08-2024 |
| Task | Due Date |
|---|---|
| ✅ Modify the workflow to upload markdown report as a comment. | 01-08-2024 |
| ✅ (Updated) Combine generated reports from each run into one final report. | 01-08-2024 |
| ✅ Review and refine markdown upload implementation. | 02-08-2024 |
Fix/replace cancellation workflow
| Starting Date | Creation Date | Merge Date |
|---|---|---|
| 03-08-2024 | 07-08-2024 | 10-08-2024 |
| Task | Due Date |
|---|---|
| ✅ Identify issues with the current cancellation workflow. | 03-08-2024 |
| ✅ Execute and analyze cancellation with styfle - PR #2890. | 05-08-2024 |
| ✅ Develop replacement or fix for the cancellation workflow. | 06-08-2024 |
| ✅ Validate the new cancellation workflow. | 07-08-2024 |
Create wiki page explaining code coverage usage, limitations, file issues for coverage gaps, and test writing tips
| Starting Date | Creation Date | Merge Date |
|---|---|---|
| 08-08-2024 | 10-08-2024 | 12-08-2024 |
| Task | Due Date |
|---|---|
| ✅ Introduce new wiki page for "how to use the code coverage tool". | 08-08-2024 |
| ✅ Introduce new wiki page for "how to write tests with good behavioral coverage". | 09-08-2024 |
| ✅ Introduce new wiki page for "Limitations of the code coverage tool". | 10-08-2024 |
| ✅ File issues for missing / incorrect code coverages. | 10-08-2024 |
Code Reviews and Evaluations
| Starting Date | Creation Date | Merge Date |
|---|---|---|
| 12-08-2024 | 26-08-2024 | 03-09-2024 |
Spare intervals to fix any further issues.
| Starting Date | Creation Date | Merge Date |
|---|---|---|
| 03-09-2024 | N/A | End of GSoC period |
DubeySandeep
commented
3 months ago @Rd4dev The dates for milestone 1(creation date, some due date), can you please fix those? Also, I don't see any update on the milestone/PRs task, can you please actively update this table? (whenever you complete a task or get a PR merged)
Rd4dev
commented
3 months ago @DubeySandeep Corrected the dates and updated the table to reflect the current progress, will ensure it remains up to date in the future.
BenHenning
commented
3 months ago Continuing the discussion from #4886 regarding why some app and testing module tests seem to fail to run in coverage. Copying in my comment from that thread:
Thanks @Rd4dev. It's interesting to see the differences with Gradle.
I spent some time today trying a bunch of different things to try and even get a newer version of JaCoCo running. It would be a lot to summarize, so here's the very high-level:
- We can build a deploy version of our Robolectric tests, but actually directly running them via JVM CLI is quite hard because there's a lot of environment setup stuff Bazel does behind the scenes.
- It is possible to set up a custom Java toolchain to replace the built-in one that comes with Bazel (so that we can swap out the JaCoCo binary it points to).
- Swapping out JaCoCo is hard. Bazel builds its own wrapper around the JaCoCo library rather than using its CLI, so we'd have to build our own version of that which points to a newer JaCoCo library.
The above isn't too much of an issue, except Bazel is in a weird mid-migration state where it's moving its remote_java_tools dependencies to a separate repo, but it's in between state is essentially:
- Bazel itself instruments remote_java_tools when building (since rules_java still just points to the native Java rules as they haven't yet been fully migrated to Starlark).
- Bazel does the above by pointing to remote_java_tools prebuilt deployments which are generated using a script that points back to Bazel in order to create the deployable Jars for its package.
That's as far as I've made it to updating JaCoCo even for a light test. Essentially next steps would be:
- Build a custom remote_java_tools deployment that contains a newer version of JaCoCo.
- Upload this somewhere (like a GitHub fork).
- Update my local checkout of Bazel to point to these new packages & rebuild.
- Use the locally built Bazel binary to try and run coverage.
It would probably be much easier to try and create a similar failure case outside of Bazel entirely (i.e. using direct
javacand JaCoCo's CLIs), but without a better sense of what the root issue is that will presumably be hard to come up with. Hopefully I can make headway on the above to at least try to use a newer version of JaCoCo to see if that makes any difference on the failures.
Will add any other findings I have here.
BenHenning
commented
3 months ago I found that, interestingly, two things:
This is leading me to believe that there may be a problem with the top-level test targets that are defined, by default, for tests with no dedicated BUILD files. I need to dig a bit more to confirm this (& maybe come up with some explanation as to why), but if this is right then the pathway to fixing it is at least fairly straightforward.
BenHenning
commented
3 months ago Okay I'm almost certain this is the problem area, though I'm still unsure why. The following command passes for me (running all non-top-level tests):
bazel coverage //{app,domain,testing,utility,data,scripts}/src/...However, this fails:
bazel coverage --keep_going //domain/...This fails with 84 targets that don't build. I took a specific example: MultipleChoiceInputEqualsRuleClassifierProviderTest. This test fails to build with coverage as-is, but moving it to its own target causes it to pass.
I'm now going to investigate the differences between the local declaration and top-level declaration for this test to see if it's something that's actually fixable.
Edit: Ah, this is interesting. I'm trying a different classifier:
bazel coverage //domain:src/test/java/org/oppia/android/domain/classify/rules/imageClickInput/ImageClickInputIsInRegionRuleClassifierProviderTestI get the following build error:
INFO: From KotlinCompile @//domain/src/main/java/org/oppia/android/domain/classify/rules/dragAndDropSortInput:drag_and_drop_sort_input_providers_kt { kt: 4, java: 0, srcjars: 0 } for k8:
Jun 20, 2024 11:10:10 PM worker request 0
SEVERE: Uncaught exception
java.io.IOException: Error while instrumenting /tmp/pwd7199928558572205927/_kotlinc/domain_src_main_java_org_oppia_android_domain_classify_rules_dragAndDropSortInput-drag_and_drop_sort_input_providers_kt_jvm/classes/org/oppia/android/domain/classify/rules/dragAndDropSortInput/DragDropSortInputHasElementXAtPositionYClassifierProvider$createRuleClassifier$$inlined$createDoubleInputClassifier$2.class with JaCoCo 0.8.9.202303300400/461ebf3.
at org.jacoco.core.instr.Instrumenter.instrumentError(Instrumenter.java:162)
at org.jacoco.core.instr.Instrumenter.instrument(Instrumenter.java:111)
at org.jacoco.core.instr.Instrumenter.instrument(Instrumenter.java:136)
at org.jacoco.core.instr.Instrumenter.instrument(Instrumenter.java:156)
at io.bazel.kotlin.builder.tasks.jvm.JacocoInstrumentationKt$instrumentRecursively$visitor$1.visitFile(JacocoInstrumentation.kt:66)
at io.bazel.kotlin.builder.tasks.jvm.JacocoInstrumentationKt$instrumentRecursively$visitor$1.visitFile(JacocoInstrumentation.kt:52)
at java.base/java.nio.file.Files.walkFileTree(Files.java:2725)
at java.base/java.nio.file.Files.walkFileTree(Files.java:2797)
at io.bazel.kotlin.builder.tasks.jvm.JacocoInstrumentationKt.instrumentRecursively(JacocoInstrumentation.kt:73)
at io.bazel.kotlin.builder.tasks.jvm.JacocoInstrumentationKt.createCoverageInstrumentedJar(JacocoInstrumentation.kt:21)
at io.bazel.kotlin.builder.tasks.jvm.KotlinJvmTaskExecutor$execute$1$1$6.invoke(KotlinJvmTaskExecutor.kt:112)
at io.bazel.kotlin.builder.tasks.jvm.KotlinJvmTaskExecutor$execute$1$1$6.invoke(KotlinJvmTaskExecutor.kt:112)
at io.bazel.kotlin.builder.toolchain.CompilationTaskContext.execute(CompilationTaskContext.kt:153)
at io.bazel.kotlin.builder.toolchain.CompilationTaskContext.execute(CompilationTaskContext.kt:145)
at io.bazel.kotlin.builder.tasks.jvm.KotlinJvmTaskExecutor$execute$1.invoke(KotlinJvmTaskExecutor.kt:112)
at io.bazel.kotlin.builder.tasks.jvm.KotlinJvmTaskExecutor$execute$1.invoke(KotlinJvmTaskExecutor.kt:54)
at io.bazel.kotlin.builder.toolchain.CompilationTaskContext.execute(CompilationTaskContext.kt:153)
at io.bazel.kotlin.builder.toolchain.CompilationTaskContext.execute(CompilationTaskContext.kt:145)
at io.bazel.kotlin.builder.tasks.jvm.KotlinJvmTaskExecutor.execute(KotlinJvmTaskExecutor.kt:54)
at io.bazel.kotlin.builder.tasks.KotlinBuilder.executeJvmTask(KotlinBuilder.kt:209)
at io.bazel.kotlin.builder.tasks.KotlinBuilder.build(KotlinBuilder.kt:102)
at io.bazel.kotlin.builder.tasks.CompileKotlin.invoke(CompileKotlin.kt:27)
at io.bazel.worker.PersistentWorker$workTo$1.invoke(PersistentWorker.kt:97)
at io.bazel.worker.PersistentWorker$workTo$1.invoke(PersistentWorker.kt:97)
at io.bazel.worker.WorkerContext$TaskContext.resultOf(WorkerContext.kt:128)
at io.bazel.worker.WorkerContext.doTask(WorkerContext.kt:156)
at io.bazel.worker.PersistentWorker$start$1$1$producer$1$1.invoke$lambda$0(PersistentWorker.kt:70)
at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
at java.base/java.lang.Thread.run(Thread.java:829)
Caused by: java.lang.IllegalStateException: Cannot process instrumented class org/oppia/android/domain/classify/rules/dragAndDropSortInput/DragDropSortInputHasElementXAtPositionYClassifierProvider$createRuleClassifier$$inlined$createDoubleInputClassifier$2. Please supply original non-instrumented classes.
at org.jacoco.core.internal.instr.InstrSupport.assertNotInstrumented(InstrSupport.java:238)
at org.jacoco.core.internal.instr.ClassInstrumenter.visitMethod(ClassInstrumenter.java:65)
at org.jacoco.core.internal.flow.ClassProbesAdapter.visitMethod(ClassProbesAdapter.java:66)
at com.google.testing.coverage.jarjar.org.objectweb.asm.ClassReader.readMethod(ClassReader.java:1353)
at com.google.testing.coverage.jarjar.org.objectweb.asm.ClassReader.accept(ClassReader.java:744)
at com.google.testing.coverage.jarjar.org.objectweb.asm.ClassReader.accept(ClassReader.java:424)
at org.jacoco.core.instr.Instrumenter.instrument(Instrumenter.java:91)
at org.jacoco.core.instr.Instrumenter.instrument(Instrumenter.java:109)
... 31 moreI didn't notice this before, but I find it especially noteworthy that the failure is tied to DragDropSortInputHasElementXAtPositionYClassifierProvider which is completely unrelated to the test that I'm trying to build. I find this interesting because it's an entirely different target, so I'm not sure why it's trying to build the other target (even if they're both umodularized top-level defined domain tests).
Running the coverage request multiple times tends to fail with a different test each time. I'm also noticing a bunch of compile targets are kicking off when running coverage when they really shouldn't.
Edit 2: Ah, I misread the above. These are production libraries that are failing in the context of the test build, not other tests. That sounds more like some sort of package conflict is happening since so many tests are packed in the same top-level package for unmodularized tests. Still digging.
BenHenning
commented
3 months ago So I've tried a few things to try and fix this: replacing the '/' part of the top-level test names with '_' (in case there are directory pathing assumptions being made somewhere in the coverage pipeline), forcing a modularized test into a subdirectory (for the same check), and bring closer parity between top-level unmodularized (failing) tests with modularized (passing tests). Despite these, I still can't find an obvious reason why prod targets are failing in the context of non-modularized tests.
It very much seems like there's some sort of instrumentation duplication happening for production targets shared across tests. It's not obvious to me why this is happening, nor is it that modularized multi-test packages build & run fine (but not the top-level unmodularized tests). It could be a number of tests thing, but that implies a (scary) hidden limitation of the coverage pipeline. Alternatively, there could just be some sort of highly specific pathing assumption happening somewhere that I can't easily isolate.
The best path forward is probably to finish the modularization work. The good news is that those PRs are very close to being ready to go into review (since they were shortly after #4886 in the Bazel PR chain). The bad news is that these are very large and complex PRs that will probably require significant time to bring back into review.
I'm going to see if we can at least disable the broken tests for coverage for now, but I'm not sure if that's actually possible.
Edit: Per https://github.com/bazelbuild/bazel/issues/21520 it seems we are not able to easily selectively disable certain tests for code coverage. :( I think that means we must work to bring in modularization in order to address this. I will dig to see just how difficult that effort will be.
Rd4dev
commented
3 months ago I'm going to see if we can at least disable the broken tests for coverage for now, but I'm not sure if that's actually possible.
Hey @BenHenning,
If disabling the broken tests directly through Bazel isn't an option, could we consider using our existing test_files_exemption to add a field for exempting tests from coverage? Since we already check for exemptions at the beginning and skip execution if a file is exempted, we could apply the same logic to coverage checks. This wouldn't fix the underlying issue but if skipping those coverages is something we can do then, I am wondering if this could serve as a temporary workaround to skip the problematic coverage executions.
BenHenning
commented
3 months ago I'm going to see if we can at least disable the broken tests for coverage for now, but I'm not sure if that's actually possible.
Hey @BenHenning,
If disabling the broken tests directly through Bazel isn't an option, could we consider using our existing
test_files_exemptionto add a field for exempting tests from coverage? Since we already check for exemptions at the beginning and skip execution if a file is exempted, we could apply the same logic to coverage checks. This wouldn't fix the underlying issue but if skipping those coverages is something we can do then, I am wondering if this could serve as a temporary workaround to skip the problematic coverage executions.
I think that's a good backup option @Rd4dev. I want to see if it's possible to bring forward the changes that would ultimately unblock the work (since we want them, anyway), but if that ends up being infeasible I think your approach is the best option.
BenHenning
commented
3 months ago So I spent a lot of time this week trying to get up to a point of verifying this result, but I couldn't quite get one of the modularization PRs building again (the prerequisites #4920 and #4978 took a long time to get working). I'll need to revisit this when I next have availability (in July).
DubeySandeep
commented
3 months ago @Rd4dev The merge date for PR 1.3 is behind the expected revised deadline, can you please help us understand what's going on and when can we expect the PR to be merged?
Rd4dev
commented
3 months ago @Rd4dev The merge date for PR 1.3 is behind the expected revised deadline, can you please help us understand what's going on and when can we expect the PR to be merged?
@DubeySandeep The PR 1.3 was in review iterations, and just got merged: https://github.com/oppia/oppia-android/pull/5433 Updated the same with issue tracker
Other PRs PR 1.4 is in review PR 1.5 will be out of draft by EOD
Rd4dev
commented
2 months ago So I spent a lot of time this week trying to get up to a point of verifying this result, but I couldn't quite get one of the modularization PRs building again (the prerequisites #4920 and #4978 took a long time to get working). I'll need to revisit this when I next have availability (in July).
Hey @BenHenning,
I tried to see if I could make any conversion to package-specific targets, but I'm not sure if I did it correctly or if the coverage results are accurate. I wanted to update you on how things went (in case any of this could be useful for future reference).
As mentioned earlier, I first tried to run the coverage for the domain module's MultipleChoiceInputEqualsRuleClassifierProviderTest with the top-level package label version, but it failed to retrieve coverage.
//domain:src/test/java/org/oppia/android/domain/classify/rules/multiplechoiceinput/MultipleChoiceInputEqualsRuleClassifierProviderTestThis fails with 84 targets that don't build. I took a specific example: MultipleChoiceInputEqualsRuleClassifierProviderTest. This test fails to build with coverage as-is, but moving it to its own target causes it to pass.
So, I tried to refactor it to package-relative label, and that yielded a desirable output with coverage data containing coverages for the source file MultipleChoiceInputEqualsRuleClassifierProvider.kt (since the source files already had their own targets, it was straightforward).
//domain/src/test/java/org/oppia/android/domain/classify/rules/multiplechoiceinput:MultipleChoiceInputEqualsRuleClassifierProviderTestThen, I tried replicating the same behavior for SplashActivity, which was more complex :| so I skipped that and instead tried doing FractionParsingUiError.kt in the app module. While the top-level package label coverage run failed, converting it to a package-relative label it eventually passed the coverage run,
$ bazel coverage //app/src/test/java/org/oppia/android/app/parser:FractionParsingUiErrorTest --instrumentation_filter=//app/...BUT the coverage data didn't have the desired output. Specifically, the coverage result should provide the coverage data related to the source file, i.e., for FractionParsingUiErrorTest.kt, it should point to -> SF: FractionParsingUiError.kt, but it was not there. Instead, it had coverage data related to FractionParsingUiErrorTest.
I noticed the same behavior while testing with another file, FakeOppiaClock.kt, which I kept for discussion later. The same behavior was seen with the package-relative labelled StringToRatioParserTest target.
I am not sure if I did the refactoring of package-specific labeling right (I just tried replicating similar target builds). I have kept the code for reference in my fork (though it's messy). I will also try with other packages to see if I can find anything different that can be helpful.
Rd4dev
commented
2 months ago I found that, interestingly, two things:
- Coverage is failing for some groups of tests outside of app module.
- All app module tests that have their own explicit test targets work with coverage.
This is leading me to believe that there may be a problem with the top-level test targets that are defined, by default, for tests with no dedicated BUILD files. I need to dig a bit more to confirm this (& maybe come up with some explanation as to why), but if this is right then the pathway to fixing it is at least fairly straightforward.
Also adding to this (nothing useful but just to confirm the cases and few additional things) there are few test targets in the app module (with top level package labelling) that do pass the coverage but as observed above they too don't contain the relevant coverage data.
Few Example targets:
And as mentioned looking at the coverage reports all files that are included in the coverage result do have their own explicit targets specified however I'm uncertain if the opposite holds true. i.e. Not all files that have their explicit targets specified were included in the coverage report.
What I mean by that is, the coverage data includes the files -
SF:app/src/main/java/org/oppia/android/app/activity/route/Route.kt,
SF:app/src/main/java/org/oppia/android/app/spotlight/SpotlightTarget.kt,
SF:app/src/main/java/org/oppia/android/app/application/ApplicationModule.ktand all these files have their targets set explicitly.
But also, recently I tried running the test target:
A similar observation when I tried to convert the top-level labelling to package specific labelling
Rd4dev
commented
2 months ago @BenHenning Uhmm.. apologies Ben I couldn't remember to bring this up in the meeting. So, you mentioned that the coverage execution fails in cases where the test files don't have their own explicit targets (not the source files). I tried acquiring coverage reports for the test target - SplashActivityTest which has its own explicit target set (for the test file - SplashActivityTest but not for its source).
bazel coverage //app/src/sharedTest/java/org/oppia/android/app/splash:SplashActivityTest --instrumentation_filter=//app/...The coverage execution does pass when having the test modularized, but as mentioned the generated coverage reports doesn't seem to include coverage data for the related file.
And there were doubts if that could be due to having the genrule - I think that's the case. Just wanted to provide an example of confirming the scenerio.
The files in the app/src/main/java/org/oppia/android/app/spotlight package has the following files
app/src/main/java/org/oppia/android/app/spotlight/BUILD.bazel
"""
Constructs for setting up spotlights.
"""
load("@io_bazel_rules_kotlin//kotlin:android.bzl", "kt_android_library")
genrule(
name = "update_SpotlightFragment",
srcs = ["SpotlightFragment.kt"],
outs = ["SpotlightFragment_updated.kt"],
cmd = """
cat $(SRCS) |
sed 's/import org.oppia.android.R/import org.oppia.android.app.R/g' |
sed 's/import org.oppia.android.databinding./import org.oppia.android.app.databinding.databinding./g' > $(OUTS)
""",
visibility = ["//app:app_visibility"],
)
kt_android_library(
name = "spotlight",
srcs = [
"SpotlightManager.kt",
"SpotlightNavigationListener.kt",
"SpotlightShape.kt",
"SpotlightTarget.kt",
],
visibility = ["//app:app_visibility"],
deps = ["//model/src/main/proto:spotlight_java_proto_lite"],
)
It only has tests for SpotlightFragment -> SpotlightFragmentTest (but it doesn't have explicit test target declared) and running the command
bazel coverage //app:src/sharedTest/java/org/oppia/android/app/spotlight/SpotlightFragmentTest --instrumentation_filter=//app/..does produce pass result but most importantly it includes / provides coverage data for the files (under kt_android_library)
and ignores the report for the file (under genrule)
Also, with this case would it mean (at least considering the app modules), the coverage reports being included / passing would be related to having package specific source targets? (since the spotlight test modules didn't have an explicit one)
Rd4dev
commented
1 month ago @BenHenning another finding that might be helpful
Just tried running coverage on the non-modularized test: BundleExtensionsTest
bazel coverage //utility:src/test/java/org/oppia/android/util/extensions/BundleExtensionsTestNote: The source file is modularized while the test is not
Running the bazel coverage command did pass the coverage analysis:
And also it has its SourceFile included in the coverage report:
...
SF:utility/src/main/java/org/oppia/android/util/extensions/BundleExtensions.kt
FN:37,org/oppia/android/util/extensions/BundleExtensionsKt::getProto (Landroid/os/Bundle;Ljava/lang/String;Lcom/google/protobuf/MessageLite;)Lcom/google/protobuf/MessageLite;
FN:66,org/oppia/android/util/extensions/BundleExtensionsKt::getProtoExtra (Landroid/content/Intent;Ljava/lang/String;Lcom/google/protobuf/MessageLite;)Lcom/google/protobuf/MessageLite;
FN:84,org/oppia/android/util/extensions/BundleExtensionsKt::getSerializableAboveApi32 (Landroid/os/Bundle;Ljava/lang/String;Ljava/lang/Class;)Ljava/io/Serializable;
FN:73,org/oppia/android/util/extensions/BundleExtensionsKt::getStringFromBundle (Landroid/os/Bundle;Ljava/lang/String;)Ljava/lang/String;
FN:18,org/oppia/android/util/extensions/BundleExtensionsKt::putProto (Landroid/os/Bundle;Ljava/lang/String;Lcom/google/protobuf/MessageLite;)V
FN:57,org/oppia/android/util/extensions/BundleExtensionsKt::putProtoExtra (Landroid/content/Intent;Ljava/lang/String;Lcom/google/protobuf/MessageLite;)V
FNDA:1,org/oppia/android/util/extensions/BundleExtensionsKt::getProto (Landroid/os/Bundle;Ljava/lang/String;Lcom/google/protobuf/MessageLite;)Lcom/google/protobuf/MessageLite;
FNDA:1,org/oppia/android/util/extensions/BundleExtensionsKt::getProtoExtra (Landroid/content/Intent;Ljava/lang/String;Lcom/google/protobuf/MessageLite;)Lcom/google/protobuf/MessageLite;
FNDA:0,org/oppia/android/util/extensions/BundleExtensionsKt::getSerializableAboveApi32 (Landroid/os/Bundle;Ljava/lang/String;Ljava/lang/Class;)Ljava/io/Serializable;
FNDA:0,org/oppia/android/util/extensions/BundleExtensionsKt::getStringFromBundle (Landroid/os/Bundle;Ljava/lang/String;)Ljava/lang/String;
FNDA:1,org/oppia/android/util/extensions/BundleExtensionsKt::putProto (Landroid/os/Bundle;Ljava/lang/String;Lcom/google/protobuf/MessageLite;)V
FNDA:1,org/oppia/android/util/extensions/BundleExtensionsKt::putProtoExtra (Landroid/content/Intent;Ljava/lang/String;Lcom/google/protobuf/MessageLite;)V
FNF:6
FNH:4
BRDA:37,0,0,1
BRDA:37,0,1,1
BRDA:37,0,2,1
BRDA:37,0,3,1
BRDA:57,0,0,1
BRDA:57,0,1,1
BRDA:66,0,0,1
BRDA:66,0,1,1
BRDA:66,0,2,1
BRDA:66,0,3,0
BRF:10
BRH:9
DA:18,1
DA:19,1
DA:37,1
DA:40,1
DA:42,1
DA:43,1
DA:44,1
DA:46,1
DA:57,1
DA:58,1
DA:66,1
DA:73,0
DA:84,0
LH:11
LF:13
end_of_record
Todo: (will try later) -> Tried running
bazel coverage //domain:src/test/java/org/oppia/android/domain/util/StringExtensionsTestwhose source is specified in the same build file as of BundleExtensions
The coverage run failed.
but test file seems to be in an improper place (might be that could be a troublemaker), will later try to move it to its respective package and again try analysing it.
BenHenning
commented
3 weeks ago I think we can close this as the project is essentially finished now. :) Thanks again @Rd4dev for all the work on this!
BenHenning
commented
2 weeks ago So I spent a lot of time this week trying to get up to a point of verifying this result, but I couldn't quite get one of the modularization PRs building again (the prerequisites #4920 and #4978 took a long time to get working). I'll need to revisit this when I next have availability (in July).
So I meant to follow up to this earlier. We decided to forego trying to finish modularization work before adding in code coverage because the lift was too much, and we can still get a lot of value out of code coverage in the meantime. Instead, #5481 was filed to track getting these tests explicitly fixed (and they were exempted in #5480).
Modularization work still needs to happen before we can comfortably move off of Gradle (since the large Bazel library targets take comparably much longer to build--Bazel shines at incremental builds, but only if the incremental targets are reasonably small). The next PR that will be merged to move this forward is #4979 and it has some prerequisite work that needs to be finished first. I started updating these old branches to the latest tip-of-tree, but unsurprisingly there are a lot of new dependency fixes that needed to be added (and the script introduced in #4978 to support this has also had a lot of recent fixes). It will probably be a few months yet before the modularization work can make it into a proper review state, but after that most test exemptions for code coverage should be able to be lifted.
Is your feature request related to a problem? Please describe.
The team cannot currently monitor and enforce minimum code coverage (as a proxy for functional behavioral coverage) in automated tests.
Describe the solution you'd like
Suggested milestones
Milestone 1: Introduce a new script to compute a per-unit code coverage percentage for a single file.
Milestone 2: Integrate code coverage checking.
Technical hints / guidance
#### Top-level components needed for code-coverage support - ``coverage.proto``: Describes the new scripting data structures needed for code coverage. - ``CoverageRunner.kt``: A new scripting utility for running code coverage for a selected target. - ``CoverageReporter.kt``: A new scripting utility for generating an HTML report of code coverage. - ``RunCoverage.kt``: A new script for running code coverage locally. - ``scripts/src/java/org/oppia/android/scripts/testfile/TestFileCheck.kt``: Existing utility that needs to be updated to include support for code coverage enforcement. - ``scripts/src/java/org/oppia/android/scripts/proto/script_exemptions.proto``: Existing exemptions definition that needs to be updated per the ``TestFileCheck`` changes. - ``.github/workflows/unit_tests.yml``: Existing CI workflow that needs to be updated in order to support code coverage. - ``.github/workflows/workflow_canceller.yml``: Existing CI workflow that needs to be fixed. - (Other files for wiki changes). #### Some key technical notes - This spec overview does not include details for the Yaml or wiki changes. - The test file exemptions textproto file will need to be rebuilt. The simplest way to do this would be to update TestFileCheck to include a text proto regenerate argument (as used in other scripts). - workflow_canceller.yml changes may required manually introducing support for workflow cancellation, or finding an available open source utility that works correctly for dynamic matrix jobs. - unit_tests.yml will probably be extended to introduce a new job that blocks on the test run (in the same way as check_tests) except it (note that this may require a new script not spec'ed out in this overview): - 1. Intreprets the same matrix data as the main test run but omits the failing tests using the outputs from the test runs themselves. - 2. Starts a new test matrix that runs the ``RunCoverage.kt`` script for each affected file corresponding to each affected test. For example, if StateFragmentTest.kt is one of the affected tests, then RunCoverage would be run for StateFragment.kt (which would then correspond to StateFragmentTest per its behavior). - Note that the min enforced coverage for now should be something very modest, like 10%. This number will be increased in future changes outside the scope of this project (as the number may not be believable until we understand the limitations of what can be covered by JaCoCo). - ``BazelClient.kt`` may need to be updated to include a function for running coverage on a specific target & output the standard output lines from that (which can then be parsed to get the report to generate the CoverageReport proto object). - All new components require new corresponding test files. All updated components will need their tests updated based on the changes to those components. #### Suggested files to add/change coverage.proto: ```proto 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. } message CoveredFile { string file_path = 1; // Relative to the project root. string file_sha1_hash = 2; // SHA-1 hash of the file content at the time of report (to guard against changes). repeated CoveredLine covered_line = 3; // Lines of code covered in the report. } message CoveredLine { int32 line_number = 1; // 0-starting line number of the covered line. Coverage coverage = 2; // Detected coverage. enum Coverage { FULL, // This line was fully covered by the test. PARTIAL, // This line was partially covered by the test, indicating some branches not being covered. NONE // This line was not executed during the test. } } ``` CoverageReporter.kt: ```kotlin class CoverageReporter { fun generateRichTextReport(report: CoverageReport, format: ReportFormat): String fun computeCoverageRatio(file: String, report: CoverageReport): Float // Returns in [0, 1]. enum class ReportFormat { MARKDOWN, HTML } } ``` CoverageRunner.kt: ```kotlin class CoverageRunner { // Uses bazel coverage to run code coverage on the specified test target & interprets the results to generate and return a CoverageReport. fun runWithCoverageAsync(bazelTestTarget: String): DeferredDescribe alternatives you've considered
No response
Additional context
This is the high-level tracking issue corresponding to https://github.com/oppia/oppia/wiki/Google-Summer-of-Code-2024#41-code-coverage-support-and-enforcement.
Note that this project includes work that relate to the following issues: #1497, #1726, #1727, and #1728.