Fix part of #5343: Update Incorrect Link for the Oppia Android Code Coverage Wiki Page #5511
Conversation
…ode Coverage Wiki Page
|
@BenHenning, I incorrectly referenced the wrong link in PR #5483; updated to the correct link here, can you PTAL? Oh, and also should we include other coverage gaps to the "Limitations sections"? |
Coverage ReportResultsCoverage Analysis: SKIP ⏭️ This PR did not introduce any changes to Kotlin source or test files.
|
|
Thanks @Rd4dev. I think adding other limitations sounds like a good idea. |
Coverage ReportResultsCoverage Analysis: SKIP ⏭️ This PR did not introduce any changes to Kotlin source or test files.
|
|
Seems like I also had included assigned to the triggers, comment posted on assignment: #5511 (comment) Coverage Gaps
The lines are fun parseAlgebraicEquation(
rawExpression: String,
allowedVariables: List<String>,
errorCheckingMode: ErrorCheckingMode = ErrorCheckingMode.ALL_ERRORS
): MathParsingResult<MathEquation> {
println("Hitting...")
return createAlgebraicParser(
rawExpression, isPartOfEquation = true, allowedVariables, errorCheckingMode
).parseGenericEquationGrammar().map { it.stripParseInfo() }
}The test is: @Test
fun testParseAlgebraicEquation_basicEquation_doesNotFail() {
expectSuccessWhenParsingAlgebraicEquation("y = 2x + 3")
}
private fun parseAlgebraicEquation(expression: String): MathParsingResult<MathEquation> {
return MathExpressionParser.parseAlgebraicEquation(
expression, allowedVariables = listOf("x", "y", "z"), ALL_ERRORS
)
}The report says:
This was a similarly observed with methods: parseAlgebraicExpression() and parseNumericExpression().
abstract fun hasPendingTasks(): Boolean
abstract fun getNextFutureTaskCompletionTimeMillis(timeMillis: Long): Long?
abstract fun runCurrent(
timeout: Long = DEFAULT_TIMEOUT_SECONDS,
timeoutUnit: TimeUnit = DEFAULT_TIMEOUT_UNIT
)and these are implemented in TestCorotineDispatcherRobolectricImpl.kt, TestCoroutineDispatchersRobolectricImpl.kt, TestCoroutineDispatchersEspressoImpl.kt, etc. But as per jaCoCo documentation:
So, should the actual abstract methods in the TestCoroutineDispatcher.kt be uninstrumented as we only acquire coverage only from their respective test cases? While most abstract methods are uninstrumented (highlighted as grey), the runCurrent shows as not covered (highlighted in red). Is this a coverage gap? The html report:
The finally block was reported as uncovered in is ControllerMessage.FinishSurveySession -> {
try {
controllerState.completeSurveyImpl(message.callbackFlow)
} finally {
println("Finally...")
// Ensure the actor ends since the session requires no further message processing.
break
}
}The report with break statement:
But I think this will also align with the same exception gap, and the presence of break statement causes the partial coverage and commenting the break statement covers the finally block: } finally {
println("Finally...")
// Ensure the actor ends since the session requires no further message processing.
// break
}The report without break statement:
If confirmed I will add it to the exception / termination statement #5506 issues.
While this one is not observed in most cases, but with The code: @OptIn(ObsoleteCoroutinesApi::class)
private fun createCommandQueueActor(): SendChannel<CommandMessage> {
return coroutineScope.actor(capacity = Channel.UNLIMITED) {
for (message in channel) {
...
}
- }
}The generated report:
Similarly observed in CommandExecutorImpl.kt The generated report:
also with TodoOpenCheck.kt The generated report:
One related issue that can be found is: jacoco/jacoco#1233 While I'm unsure if this is related to the test implementation or something that demands a configuration change or an actual coverage gap? If it is a coverage gap, then it will require further investigation on what specific cases they occur.
It is observed with
and I doubt if they are related to :
same with TodoCollector.kt
I think I ran coverages on most if not all of the Passing cases (not on exempted overridden files as most are reported as 0 or very low and figuring out gaps would be not straightforward) and based on the other reports it can be seen that most of the uncovered lines are based on unhit else or other branch cases, overrides and I tried adding tests to see if they can be hit for few of them and they do seem to get covered. So, I think once all filed and (the above-mentioned gaps) to be filed issues are addressed, we can achieve move towards 100% code coverage. While I’m not sure if we’ll uncover more gaps once the source file incompatibility issues are resolved, that’s something we’ll only know once it’s done. Can you take a look at the above cases and direct if those can be filed as coverage gap or if any are uncovered with missing tests. Once everything is filed, I will add them to the wiki limitations in one go. Other issues (Not coverage gaps)HTML reports with source code containing html tags / other tags mess up the reports. For eg: With CoverageReporter.kt, LiTagHandler.kt, TodoCollector.kt reports, since it already have tags as strings within them, when the report is generated it mix matches the report structure. One possible way is to move these html strings as templates to a separate place and refer them from there so the source code report generation would still preserve the format. eg: chopped - TodoCollector.kt
formatted CoverageReport.kt
LiTagHandler.kt
|
Coverage ReportResultsCoverage Analysis: SKIP ⏭️ This PR did not introduce any changes to Kotlin source or test files.
|
Coverage ReportResultsCoverage Analysis: SKIP ⏭️ This PR did not introduce any changes to Kotlin source or test files.
|
|
Thanks @Rd4dev. The code changes LGTM, but I'll need to process your last message a bit before I have any follow-up thoughts/suggestions. |
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), 4 bytes (Added) Method count: 259140 (old), 259140 (new), 0 (No change) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6808 (old), 6808 (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), 4 bytes (Added) 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), 4 bytes (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 2 bytes (Removed) Method count: 115714 (old), 115714 (new), 0 (No change) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5776 (old), 5776 (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), 4 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 (Removed) APK download size (estimated): 10 MiB (old), 10 MiB (new), 7 bytes (Added) Method count: 115720 (old), 115720 (new), 0 (No change) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5776 (old), 5776 (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), 4 bytes (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 1 bytes (Added) Method count: 115720 (old), 115720 (new), 0 (No change) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5776 (old), 5776 (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), 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) |
|
(1) is probably a real gap. It looks like the tests never verify the default parameter since they always provide one. (2) is touching on several different things:
(3) I'm not exactly sure if this is #5506 as-is, but I think it's reasonable to extend that issue to include this case. It definitely seems like a JaCoCo issue to me. (4) Worth filing an issue for this case with the specific examples you've found. I suspect these are either coming from:
Both will require investigation, and we need to rule out the first case before looking into the second one (which would require a bytecode analysis on the production code being instrumented). (5) I suspect this is a case of the constructors not being called (since Kotlin generates public default constructors for classes) as we don't need to. If that's the case, we should just be turning the classes into public objects at the top-level if they don't need to be instances. This seems to be a coverage gap on our side, not a tooling issue. At a high level:
|
Coverage ReportResultsCoverage Analysis: SKIP ⏭️ This PR did not introduce any changes to Kotlin source or test files.
|
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), 1672 bytes (Removed) APK download size (estimated): 17 MiB (old), 17 MiB (new), 1564 bytes (Removed) Method count: 259183 (old), 259140 (new), 43 (Removed) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6808 (old), 6808 (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), 1672 bytes (Removed) 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), 1324 bytes (Removed) APK download size (estimated): 10 MiB (old), 10 MiB (new), 5016 bytes (Removed) Method count: 115727 (old), 115714 (new), 13 (Removed) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5776 (old), 5776 (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), 1320 bytes (Removed) 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), 1804 bytes (Removed) APK download size (estimated): 10 MiB (old), 10 MiB (new), 3650 bytes (Removed) Method count: 115733 (old), 115720 (new), 13 (Removed) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5776 (old), 5776 (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), 1800 bytes (Removed) 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), 1684 bytes (Removed) APK download size (estimated): 10 MiB (old), 10 MiB (new), 3963 bytes (Removed) Method count: 115733 (old), 115720 (new), 13 (Removed) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5776 (old), 5776 (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), 1684 bytes (Removed) 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, can you PTAL? and would it require any other verification on the Coverage Checks? Ok, I found another case - https://github.com/oppia/oppia-android/pull/5522/checks, seems like evaluate runs irrespective of unit test failure and throwing "SKIP" status - #5522 (comment), will also adress that over here. Log:
Thanks for the detailed information on each case.
val unescapedOutputReportText = outputReportText
.replace(""", "\"")
.replace("&", "&")
.replace("<", "<")
.replace(">", ">")not sure if there is anything to unescape (and escaping expected report caused issues with other tags). Edit: Escaped CoverageReporter.html
|
Coverage ReportResultsCoverage Analysis: SKIP ⏭️ This PR did not introduce any changes to Kotlin source or test files.
|
Coverage ReportResultsNumber of files assessed: 2 Passing coverageFiles with passing code coverage
|
Coverage ReportResultsNumber of files assessed: 2 Passing coverageFiles with passing code coverage
|
scripts/src/javatests/org/oppia/android/scripts/coverage/RunCoverageTest.kt
Show resolved
Hide resolved
Coverage ReportResultsNumber of files assessed: 2 Passing coverageFiles with passing code coverage
|
|
@Rd4dev can you also please make sure the PR description is updated to include all of the new changes (since there have been a few iterations on this PR?). |
|
Hmm also I think the coverage report comment is reposting after PR event changes like assignees--are its triggers correct? |
Yeah, that shouldn't be triggering and spotted it earlier #5511 (comment) and included a fix for that too here - PR Changes Ah, I’ve updated the PR description with the details—hopefully, they’ll stand out better now and won’t get lost in the mix. @BenHenning, can you please confirm on this #5511 (comment) or other approaches that could be followed. Edit: |
Coverage ReportResultsNumber of files assessed: 2 Passing coverageFiles with passing code coverage
|
There was a problem hiding this comment.
Ah, I forgot about the fact that the old version of the comment workflow is running since it's pull_request_target--thanks for pointing out that it should now be fixed @Rd4dev.
Since there's nothing left to resolve here, approving and merging. Thanks!
Explanation
Fixes Part of #5343
This PR includes
pull_request_targettriggered, fixed it by removing the 'assigned' trigger.Essential Checklist
For UI-specific PRs only
If your PR includes UI-related changes, then: