Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[BUG]: Limit repeated code coverage and APK/AAB report generations in PRs #5508

Closed
Rd4dev opened this issue Aug 22, 2024 · 2 comments · Fixed by #5580
Closed

[BUG]: Limit repeated code coverage and APK/AAB report generations in PRs #5508

Rd4dev opened this issue Aug 22, 2024 · 2 comments · Fixed by #5580
Assignees
@Rd4dev
Labels
bug End user-perceivable behaviors which are not desirable. Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet.

Comments

@Rd4dev
Copy link
Collaborator

Rd4dev commented Aug 22, 2024 •
edited
Loading

Describe the bug

The current implementation for generating code coverage and APK/AAB difference reports can lead to excessive noise in PR threads. This issue primarily affects the visibility and clarity of PR discussions.

1. Code Coverage Reports:

  • Current Behavior: When a PR does not contain changes to .kt files, the code coverage report generated shows a "SKIP" status. This report is generated with every commit, regardless of whether there are changes in .kt files. Consequently, if there are no relevant changes in the PR, this can result in a flood of "SKIP" status updates, cluttering the PR thread.
  • Proposed Solution: Implement a mechanism to check for changes in .kt files between commits. If no changes are detected, suppress the "SKIP" status report to avoid excessive notifications, while ensuring that developers are not confused about missing or outdated reports for subsequent commits if no new reports are uploaded.

Skip coverage report:

image

An overloaded skip coverage report thread

image

image

image

image

image

image

image

image

image

2. APK/AAB Difference Reports:

  • Current Behavior: The APK/AAB diff report, as of filing the issue, is scheduled to run every day at 2:30 AM UTC (8:00 AM IST, 7:30 PM PT, daylight saving time). This aligns with the similar issue described above, as the reports posted to the PR thread could eventually become noisy.

APK/AAB difference report:

image

An overloaded APK/AAB report thread

image

image

image

image

Intent

The goal is to enhance the developer experience by reducing unnecessary noise while ensuring that important updates are communicated effectively. Implementing these changes will help keep PR threads cleaner and more focused on relevant discussions.

Steps To Reproduce

Coverage Report:

  • Create a new pull request or update an existing one without making changes to .kt files.
  • Add and commit changes to the PR.
  • Wait for the CI/CD pipeline to run and observe the code coverage report. Note if it shows a "SKIP" status.
  • Look at the PR thread to find the "SKIP" coverage status report is posted, even though no .kt files were changed.

APK/AAB Report:

  • This open PR will produce APK/AAB reports at 2:30 AM UTC.
  • Observe if this results in excessive notifications in the PR thread.

Expected Behavior

Both code coverage and APK/AAB reports should only post meaningful updates based on changes, ensuring clear and relevant updates in the PR thread.

What device/emulator are you using?

No response

Which Android version is your device/emulator running?

No response

Which version of the Oppia Android app are you using?

No response

Additional Context

No response
@Rd4dev Rd4dev added bug End user-perceivable behaviors which are not desirable. triage needed labels Aug 22, 2024
@Rd4dev Rd4dev mentioned this issue Aug 22, 2024
Merged
6 tasks
Rd4dev added a commit to Rd4dev/oppia-android that referenced this issue Sep 9, 2024
@Rd4dev
Fix part of oppia#5508: Limit APK/AAB Difference analysis reports in …
da16638 
…the PR Thread
@Rd4dev Rd4dev mentioned this issue Sep 9, 2024
Merged
10 tasks
BenHenning added a commit that referenced this issue Sep 12, 2024
@Rd4dev @BenHenning
Fix part of #5508: Limit APK/AAB Difference analysis reports in the P…
c82b71b 
…R Comment Thread (#5532)

<!-- 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.
  -->
Fixes part of #5508 

### This PR includes

Steps to locate the previous `stats.yml` workflow 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:**
- Download the previous build log artifact (if available).
- Run the script.
- Compare the current generated build log with the previous build log
artifact:
  - if no differences are found -> skip commenting.
  - if differences are found -> comment the current generated build log
- if no previous artifact is found -> comment the current generated
build log
    - This occurs in 2 instances:
      - 1. It's the first run of the PR.
- 2. An error occurred during the previous stat check (since the
previous build is from the second-to-last run ID).
- Upload the current build log as an artifact (for the next stat run).
- Comment/skip the stat report based on the comparison result.

#

### 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:

- [x] should comment on initial run |
[comment](Rd4dev/Oppia-Android-Fork-from-Fork#40 (comment))
| [stack
trace](https://github.com/Rd4dev/Oppia-Android-Fork-from-Fork/actions/runs/10762876260/job/29843752198#step:19:26)
- [x] shouldn't comment when no change |
[reference1](Rd4dev/Oppia-Android-Fork-from-Fork#40 (comment))
|
[reference2](Rd4dev/Oppia-Android-Fork-from-Fork#40 (comment))
- [x] should comment on change |
[comment](Rd4dev/Oppia-Android-Fork-from-Fork#40 (comment))
|
[reference](Rd4dev/Oppia-Android-Fork-from-Fork#40 (comment))
- [x] comment on previous build fail (replicated!) |
[comment](Rd4dev/Oppia-Android-Fork-from-Fork#40 (comment))
|
[reference](Rd4dev/Oppia-Android-Fork-from-Fork#40 (comment))

## 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]>
@manas-yu
Copy link
Collaborator

manas-yu commented Nov 7, 2024 •
edited
Loading

@adhiamboperes We can check for changed Kotlin files using KT_FILES_CHANGED=$(echo "$FILE_BUCKET_LIST" | grep -E '\.kt$' || true) in compute_changed_files section in code_coverage.yml and set the can_skip_files property accordingly, which will help suppress unnecessary SKIP status updates.
cc: @Rd4dev

@Rd4dev
Copy link
Collaborator Author

Rd4dev commented Nov 8, 2024

@manas-yu, thanks for the suggestion! I’m currently working on this fix, including the adjustments for the duplicate coverages and AAB/APK comments, but feel free to explore other unassigned issues if you're interested.

@Rd4dev Rd4dev mentioned this issue Nov 22, 2024
Merged
6 tasks
@adhiamboperes adhiamboperes added Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet. labels Nov 26, 2024
subhajitxyz pushed a commit to subhajitxyz/oppia-android that referenced this issue Dec 17, 2024
@Rd4dev @BenHenning @subhajitxyz
Fix oppia#5508: Skipping redundant code coverage and APK/AAB report c…
b938a4c 
…omments (oppia#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 oppia#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](oppia#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]>
subhajitxyz added a commit to subhajitxyz/oppia-android that referenced this issue Dec 17, 2024
@subhajitxyz
Revert "Fix oppia#5508: Skipping redundant code coverage and APK/AAB …
da14b09 
…report comments (oppia#5580)"

This reverts commit b938a4c.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Rd4dev Rd4dev

Labels
bug End user-perceivable behaviors which are not desirable. Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet.
Projects
[Team] Developer Workflow & Infrastructure - Android
Status: Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix #5508: Skipping redundant code coverage and APK/AAB report comments Rd4dev/oppia-android
4 participants
@seanlip @adhiamboperes @manas-yu @Rd4dev