Closed Rd4dev closed 3 months ago
@BenHenning can you PTAL, this PR is cherry-picked from #5420 as that was chained with migration-to-newer-bazel-kotlin
branch, which recently got merged and the branch was deleted. This PR is worked with the develop branch with newer Bazel-6.5.0 version.
Note that this is a summarized snapshot. See the CI artifacts for detailed differences.
Note that this is a summarized snapshot. See the CI artifacts for detailed differences.
Note that this is a summarized snapshot. See the CI artifacts for detailed differences.
Note that this is a summarized snapshot. See the CI artifacts for detailed differences.
I have addressed the first pass review comments @BenHenning, PTAL.
Unassigning @Rd4dev since a re-review was requested. @Rd4dev, please make sure you have addressed all review comments. Thanks!
@BenHenning sorry for assigning, missed the Todo Check failing case. I'll figure that out and re-assign for review!
@BenHenning @adhiamboperes recently after the merge of the PR #5240, I am facing the Todo not corresponding to open issues - Todo Check error, as the TODO was still mentioned in the ApplicationLifecycleObserverTest.kt [line number: 429] should I update anything (remove todo) in this PR to solve this?
Note that this is a summarized snapshot. See the CI artifacts for detailed differences.
@BenHenning @adhiamboperes recently after the merge of the PR #5240, I am facing the Todo not corresponding to open issues - Todo Check error, as the TODO was still mentioned in the ApplicationLifecycleObserverTest.kt [line number: 429] should I update anything (remove todo) in this PR to solve this?
Hi @Rd4dev, this should be fixed now...the issue number was incorrect.
Thanks @adhiamboperes, will update this with the latest changes!
@adhiamboperes @BenHenning, Update on the testDebugUnitTest
I mentioned, the previous run had this issue with Non-app Module Robolectric Tests with Execution failed for task ':domain:testDebugUnitTest' for the QuestionAssessmentProgressControllerTest
, but with merging latest changes (todo check update) all the checks with this PR PASSED.
@BenHenning, with the latest changes merged all checks pass and I have addressed all the review comment can you PTAL!
Note that this is a summarized snapshot. See the CI artifacts for detailed differences.
Note that this is a summarized snapshot. See the CI artifacts for detailed differences.
Note that this is a summarized snapshot. See the CI artifacts for detailed differences.
Thanks for the review @BenHenning. I have addressed all the comments, can you PTAL!
Unassigning @Rd4dev since a re-review was requested. @Rd4dev, please make sure you have addressed all review comments. Thanks!
Thanks @Rd4dev! Had a few more thoughts--PTAL.
Also, if it saves time feel free to avoid linking the commits in your follow-up comments (it's actually not needed at all since GitHub provides pretty easy ways to see the changes between review passes). If you prefer continuing to include them, feel free but it isn't needed for reviewing. :)
Thanks for letting me know! That will make it a bit faster for me to respond. Also addressed all the recent review comments too @BenHenning, can you PTAL!
Unassigning @Rd4dev since a re-review was requested. @Rd4dev, please make sure you have addressed all review comments. Thanks!
Note that this is a summarized snapshot. See the CI artifacts for detailed differences.
@BenHenning, fixed the nits, can you PTAL!
Unassigning @Rd4dev since a re-review was requested. @Rd4dev, please make sure you have addressed all review comments. Thanks!
Note that this is a summarized snapshot. See the CI artifacts for detailed differences.
Hey @BenHenning, as per at https://github.com/oppia/oppia-android/pull/5433#discussion_r1649961400, I have removed the RunCoverageForTestTarget in this PR. With this, this PR will now only handle the execution of Bazel commands for coverage and include updates to the TestBazelWorkspace utility to test coverage executions. Can you PTAL!
Additionally, this PR is not from upstream (as this was created before getting permissions). However, PR 1.3 is properly chained and has been updated accordingly with the removal of RunCoverageForTestTarget in sync with the upstream code_coverage_bazel_command_execution branch.
Feel free to merge when you're ready to update your chained PR (let me know if you need the commands for that).
That should be really helpful if you can share them / confirm the below previously provided commands.
You had previously provided some commands addressing the same, if that's the same can I just confirm the process for the chaining with code_coverage_bazel_command_execution and code_coverage_using_filename, (as per Meeting Notes, June 6):
base-branch: code_coverage_bazel_command_execution feature-branch: code_coverage_using_filename
git checkout base-branch
git checkout develop
git pull origin develop
git checkout base-branch
git merge develop
git checkout feature-branch
git pull origin feature-branch
git merge base-branch
git push origin feature-branch
Unassigning @Rd4dev since a re-review was requested. @Rd4dev, please make sure you have addressed all review comments. Thanks!
Your commands are correct @Rd4dev but I just noticed this PR is used your fork branch, not the one on Oppia Android. You may need to do other steps to get it to sync properly, and it might be easier doing that together over chat.
Please send this back to me once you sync it with the latest develop & when you're ready to merge it; I'll merge it and we can work together to get #5433 properly synced.
Edit: I actually went ahead and synced, so please assign back when CI is passing and you're ready for this to be merged.
Note that this is a summarized snapshot. See the CI artifacts for detailed differences.
Thanks @BenHenning, all the CI Checks have passed.
Looks like another PR has been merged. :) Syncing again.
Enabling auto-merge.
Explanation
Fixes part of #5343
Project
[PR 1.1 of Project 4.1]
This PR introduces 2 utilities:
Source:
The extracted coverage.dat file will then be used to acquire the actual coverage data.
Tests:
Essential Checklist
For UI-specific PRs only
If your PR includes UI-related changes, then: