-
Notifications
You must be signed in to change notification settings - Fork 527
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix part of #5343: Code Coverage script edge cases #5453
Fix part of #5343: Code Coverage script edge cases #5453
Conversation
…args to and make reordering possible
Hi @adhiamboperes, addressed the argument name / ordering edge case as suggested. Can you PTAL! |
@adhiamboperes added tests for case sensitivity and reordered arguments, would it require any other tests? |
… a long time causing TIMEOUT failures
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Rd4dev. This LGTM.
@adhiamboperes the tests fail due to TIMEOUT, I tried changing them in the test |
Unassigning @adhiamboperes since they have already approved the PR. |
Assigning @BenHenning for code owner reviews. Thanks! |
@seanlip, we discovered some edge cases from RD's last PR, and made a follow up PR instead of reverting the last one. cc @DubeySandeep |
@Rd4dev PTAL for CI failures |
…on the tests adding missed paranthesis
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: 16 MiB (old), 16 MiB (new), 8 bytes (Removed) APK download size (estimated): 14 MiB (old), 14 MiB (new), 15 bytes (Added) Method count: 227007 (old), 227007 (new), 0 (No change) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6550 (old), 6550 (new), 0 (No change)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 16 MiB (old), 16 MiB (new), 8 bytes (Removed) Configuration hdpiAPK file size: 59 KiB (old), 59 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 56 KiB (old), 56 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 53 KiB (old), 53 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 102 KiB (old), 102 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 67 KiB (old), 67 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 76 KiB (old), 76 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 79 KiB (old), 79 KiB (new), 0 bytes (No change) AlphaExpand to see flavor specificsUniversal APKAPK file size: 10 MiB (old), 10 MiB (new), 4 bytes (Removed) APK download size (estimated): 9184 KiB (old), 9184 KiB (new), 5 bytes (Removed) Method count: 101341 (old), 101341 (new), 0 (No change) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5504 (old), 5504 (new), 0 (No change)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 9 MiB (old), 9 MiB (new), 8 bytes (Removed) Configuration hdpiAPK file size: 52 KiB (old), 52 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 52 KiB (old), 52 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 46 KiB (old), 46 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 90 KiB (old), 90 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 60 KiB (old), 60 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 69 KiB (old), 69 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 71 KiB (old), 71 KiB (new), 0 bytes (No change) BetaExpand to see flavor specificsUniversal APKAPK file size: 10 MiB (old), 10 MiB (new), 0 bytes (No change) APK download size (estimated): 9169 KiB (old), 9169 KiB (new), 21 bytes (Added) Method count: 101341 (old), 101341 (new), 0 (No change) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5504 (old), 5504 (new), 0 (No change)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 9 MiB (old), 9 MiB (new), 0 bytes (No change) Configuration hdpiAPK file size: 52 KiB (old), 52 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 52 KiB (old), 52 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 46 KiB (old), 46 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 90 KiB (old), 90 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 60 KiB (old), 60 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 69 KiB (old), 69 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 71 KiB (old), 71 KiB (new), 0 bytes (No change) GaExpand to see flavor specificsUniversal APKAPK file size: 10 MiB (old), 10 MiB (new), 0 bytes (No change) APK download size (estimated): 9169 KiB (old), 9169 KiB (new), 6 bytes (Removed) Method count: 101341 (old), 101341 (new), 0 (No change) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5504 (old), 5504 (new), 0 (No change)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 9 MiB (old), 9 MiB (new), 0 bytes (No change) Configuration hdpiAPK file size: 52 KiB (old), 52 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 52 KiB (old), 52 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 46 KiB (old), 46 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 90 KiB (old), 90 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 60 KiB (old), 60 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 69 KiB (old), 69 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 71 KiB (old), 71 KiB (new), 0 bytes (No change) |
Hey @adhiamboperes, I just tried reverting all the changes to see if those are causing any issues and the tests though timed out at Attempt 1 seems to pass from Attempt 2: stack trace from Attempt 2 So, there is something from the newly introduced changes (which is only the arg parsing) that was messing up the tests. Edit: There is nothing failing with the logic (all checks passed reverting the test cases), its just the addition of tests making it wait too long causing TIMEOUT! Passing locally:Edit: Edit: This is the stack trace of : e82f2fd where I tried to set I am not sure where the actual max limitation is set (if that's something that is restricting the processTimeout to go beyond 300s which is actually needed to run coverages). Edit: I probably doubt if this could be due to varied processTimeout references at various places. Edit: Tried increasing the processTimeout in Edit: I doubt the timeout is not actually related to RunCoverage script settings solely but was failing because of the kotlin coroutine timeouts Edit: Tried testing with a more time intensive test case locally - assertEquals(computeFibonacci(45), 1134903170L) and that failed with kotlin coroutine timed out error and processTimeout set via RunCoverage was ignored. Its ultimately the runBlocking timing out (earlier the Edit: Tried adding a delay to the MavenDependeciesCheckList, in ci it throws the same TIMED OUT error on Coroutines but here the timeout occured after 800+ seconds which was 300s for RunCoverage [stack trace] FixEdit: Ok, I think I understood where the actual problem is, so it was basically due to the test exceeding bazel's --test_timeout value, it was set to bazel runner doc bazel test's moderate value causing a timeout at 300s. And increasing the timeout value using |
… then convert them to uppercase
@BenHenning, @adhiamboperes, I think I finally figured it out (shouldn't have taken this long). The test runtimes consistently exceed 300 seconds, so the previous implementation introduced It turned out to be a straightforward fix 75e28e2 to add
Uhm.. I'd like to know if this is the right approach and hope this resolves everything related to Edit: Also, I wonder if this should be subdivided to help parallelize test execution using |
scripts/src/java/org/oppia/android/scripts/testing/TestBazelWorkspace.kt
Show resolved
Hide resolved
@Rd4dev, good job on identifying the problem! Re:
That could be discussed later, I will not block merging this PR on that. Can you add it to your list of tasks for CI validations later in M2? |
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), 4 bytes (Removed) APK download size (estimated): 17 MiB (old), 17 MiB (new), 5 bytes (Added) Method count: 258445 (old), 258445 (new), 0 (No change) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6775 (old), 6775 (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 (Removed) Configuration hdpiAPK file size: 49 KiB (old), 49 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 48 KiB (old), 48 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 45 KiB (old), 45 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 85 KiB (old), 85 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 56 KiB (old), 56 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 62 KiB (old), 62 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), 8 bytes (Removed) APK download size (estimated): 10 MiB (old), 10 MiB (new), 6 bytes (Removed) Method count: 114844 (old), 114844 (new), 0 (No change) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5745 (old), 5745 (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 (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: 72 KiB (old), 72 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 49 KiB (old), 49 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) BetaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 4 bytes (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 7 bytes (Added) Method count: 114850 (old), 114850 (new), 0 (No change) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5745 (old), 5745 (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), 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: 72 KiB (old), 72 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 49 KiB (old), 49 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) GaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 0 bytes (No change) APK download size (estimated): 10 MiB (old), 10 MiB (new), 5 bytes (Removed) Method count: 114850 (old), 114850 (new), 0 (No change) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5745 (old), 5745 (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: 72 KiB (old), 72 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 49 KiB (old), 49 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) |
…ent specific to the test case
Thanks! Sure, I'll include it as a sub-task for the M2 list. |
…emoving it to see if that is actually causing the issues (may be with idle times)
… most importantly helps with max idle run time issues
Hey @adhiamboperes, The tests failed with the following error:
They passed locally, so I tried adding shards_count and set it to 4, which is the minimum for other large tests. This significantly reduced the run time (from ~350 seconds to ~240 seconds) as they now run in parallel. It also seems to help with timeouts and the max_idle_secs issue, as introducing it solved the problem. I verified it by removing and re-adding it; the issue reappears when it's removed [failure stack trace] and is resolved when re-added [success stack trace]. I guess the issue should be with tests taking too long to complete and hitting idle times and causing this issue. And this was something mentioned to be considered for M2 but given the priority to merge this PR ASAP and the small fix it represents; I'm including it here. The exact number of shards can be updated in M2 if needed. Also, I believe this will be very beneficial for upcoming PRs, as they are likely to introduce more test cases with multiple test suites. Can you PTAL. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Rd4dev! Latest changes LGTM.
Unassigning @adhiamboperes since they have already approved the PR. |
Assigning @BenHenning for code owner reviews. Thanks! |
Explanation
Fixes part of #5343
This is an extended part of Milestone 1 to address any edge cases.
Project
[Project 4.1]
Changes Made
Updated script run command:
Essential Checklist
For UI-specific PRs only
If your PR includes UI-related changes, then: