Fix part of #5508: Limit APK/AAB Difference analysis reports in the PR Comment Thread #5532
Conversation
|
@BenHenning, would this approach work in limiting the report thread in the right way? Can you PTAL? Also, Noticing that previous stat runs sometimes fail [workflow run], if the artifacts aren’t uploaded, the next check would treat it as if no previous log was found, potentially leading to duplicated comments. |
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.
|
There was a problem hiding this comment.
Ah this is a really clever solution--great idea! Thanks for creating this @Rd4dev and for the thorough testing that you included in the PR! I have no additional suggestions, this LGTM as-is. :)
|
Given how much the build stats workflow has been 'spamming' PRs (and keeping them from being auto-closed from Oppiabot stale detection), enabling auto-merge to get this in soon. |
|
Unassigning @BenHenning since they have already approved the PR. |
|
Assigning @adhiamboperes for code owner reviews. Thanks! |
Coverage ReportResultsCoverage Analysis: SKIP ⏭️ This PR did not introduce any changes to Kotlin source or test files.
|
|
Thank you so much @BenHenning. I’d like to clarify the failure cases with the stats reports, as they seem to fail often. Is this likely a flake, or can it be fixed? I’m concerned it might lead to duplicated comments with previous failure stats. The artifact approach seemed straightforward and clean, but wondering if it’s unreliable, would checking actual comments via the GitHub API to compare changes be a viable alternative? (I haven’t tried this yet...) but just like to know the best approach. |
What failures are you observing? It seems that the stats workflow has been fairly reliable, but admittedly I haven't actually analyzed it in detail. Separately, I notice that CI workflows are failing due to us using a very old version of upload artifact in existing workflows. Could you maybe update those in this PR? Since we're going to merge this soon, it would be convenient to go ahead and fix that here so that we can unblock the rest of the team (it seems something changed on GitHub's side that's presumably going to cause all PRs to fail). |
|
Updated this PR to use v4 upload-artifact.
Though the last couple of stats.yml failures are due to the deprecated v2 artifact, the previous checks to them too seem to fail in most cases (with ProGuard issues, I guess). Actions/stats check:
Stat runs:
Failure at build - feature branch step: (Run # 154)
The same PR passing checks (Run # 155)
|
Coverage ReportResultsCoverage Analysis: SKIP ⏭️ This PR did not introduce any changes to Kotlin source or test files.
|
|
Thanks @Rd4dev! As far as I can tell, all the failures were due to the v4 issue but I'll keep an eye out for any other build flakes that might be occurring. |
…ts (#5580) <!-- READ ME FIRST: Please fill in the explanation section below and check off every point from the Essential Checklist! --> ## Explanation <!-- - Explain what your PR does. If this PR fixes an existing bug, please include - "Fixes #bugnum:" in the explanation so that GitHub can auto-close the issue - when this PR is merged. --> Fix #5508 TODO: - [Done] - ~~Update Indents and latest upstream changes~~ ### This PR includes **1. Code Coverage Comment:** - In previous implementations, redundant code coverage reports could accumulate, even when they provided no additional information, leading to cluttered PR threads. - To address this, a comparison step has been added to **check the newly generated code coverage report against the latest posted code coverage comment**. - If the current report is identical to the latest comment, the script will skip posting a new comment. This ensures that the last coverage comment in the PR thread accurately reflects the latest report, preventing unnecessary repetitions. **2. Stats Comment:** - The Stat reports were being generated even when no new changes were made to the PR, causing repetitive APK/AAB report comments and hindering the stale comment checks. - A new step is added to track the presence of new commits. Now, the **stats analysis only triggers if there has been a new commit since the latest APK/AAB report comment**. - This approach reduces redundant analysis, ensuring that builds are only processed when relevant *PR changes are made. ***Limitation:** - These changes aim to help the stale comment checks. However, the trade-off is: merge commits to the `develop` branch will still generate new build reports. Allowing these reports would negate the benefits of stale comment checks, as weekly or bi-weekly merges can cause build variations. - Consequently, the [older method](#5532) of comparing previous and new reports has been removed due to flakiness in the stat.yml. While it is technically possible to use the currently generated report for comparison with the latest comment, it would include variations from merge changes, thus failing to prevent stale comments. Including the comparison step source for reference: ```sh - name: Compare Generated APK & AAB Analysis with the Previous Report if: ${{ steps.track_commits.outputs.new_commits == 'true' }} run: | if [ -f latest_aab_comment_body.log ]; then sed -i -e '$a\' ./develop/brief_build_summary.log sed -i -e '$a\' latest_aab_comment_body.log if diff -B ./develop/brief_build_summary.log latest_aab_comment_body.log > /dev/null; then echo "No significant changes detected; skipping apk aab analysis comment." echo "skip_apk_aab_comment=true" >> $GITHUB_ENV else echo "Changes detected; proceeding with the apk aab analysis comment." diff ./develop/brief_build_summary.log latest_aab_comment_body.log || true echo "skip_apk_aab_comment=false" >> $GITHUB_ENV fi else echo "No previous APK & AAB report posted; Commenting analysed APK & AAB report." echo "skip_apk_aab_comment=false" >> $GITHUB_ENV fi ``` # ### Demonstration >Demonstrated PR: Rd4dev/Oppia-Android-Fork-from-Fork#44 Tested with souce code - [comment_code_coverage.yml](https://github.com/Rd4dev/Oppia-Android-Fork-from-Fork/blob/4492ab36dc1efbcbec9cf0b7c164d24f9a5d4511/.github/workflows/comment_coverage_report.yml#L3) and [stats.yml](https://github.com/Rd4dev/Oppia-Android-Fork-from-Fork/blob/4492ab36dc1efbcbec9cf0b7c164d24f9a5d4511/.github/workflows/stats.yml#L3) - Initial Code Coverage Comment | [Comment](Rd4dev/Oppia-Android-Fork-from-Fork#44 (comment)) | [Stack Trace](https://github.com/Rd4dev/Oppia-Android-Fork-from-Fork/actions/runs/11957616687/job/33335730714#step:5:20) - Initial APK & AAB Analysis Comment | [Comment](Rd4dev/Oppia-Android-Fork-from-Fork#44 (comment)) | [Stack Trace](https://github.com/Rd4dev/Oppia-Android-Fork-from-Fork/actions/runs/11957621776/job/33337727450#step:20:348) - Redundant Code Coverage Comment Skipped | [Stack Trace](https://github.com/Rd4dev/Oppia-Android-Fork-from-Fork/actions/runs/11958563394/job/33338783923?pr=44#step:5:20) - Varying Code Coverage Comment Posted | [Comment](Rd4dev/Oppia-Android-Fork-from-Fork#44 (comment)) | [Stack Trace](https://github.com/Rd4dev/Oppia-Android-Fork-from-Fork/actions/runs/11959332332/job/33341699858#step:5:20) - APK & AAB Analysis Posted with follow up commits | [Comment](Rd4dev/Oppia-Android-Fork-from-Fork#44 (comment)) | [Stack Trace](https://github.com/Rd4dev/Oppia-Android-Fork-from-Fork/actions/runs/11959338918/job/33340923675#step:20:348) - APK & AAB Analysis Skipped with no follow up commits | [Stack Trace](https://github.com/Rd4dev/Oppia-Android-Fork-from-Fork/actions/runs/11959461908/job/33341312780#step:3:33) ## Essential Checklist <!-- Please tick the relevant boxes by putting an "x" in them. --> - [x] The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".) - [x] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation. - [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide). - [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)). - [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop". - [x] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)). --------- Co-authored-by: Ben Henning <[email protected]>
Explanation
Fixes #5533
Fixes part of #5508
This PR includes
Updates the workflow to use v4 of the upload-artifact
Steps to locate the previous
stats.ymlworkflow run, download its build artifact, and compare it with the current build log. If changes are detected, a comment will be uploaded to help minimize comment thread overload.The implementation:
Tested with a cloned PR
(with stats.yml implementation on develop)
Tested PR: Rd4dev/Oppia-Android-Fork-from-Fork#40
Reference for proof of implementation:
Essential Checklist