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

Open
Rd4dev opened this issue Aug 22, 2024 · 2 comments · May be fixed by #5580
Open

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

Rd4dev opened this issue Aug 22, 2024 · 2 comments · May be fixed by #5580
Assignees
@Rd4dev
Labels
bug End user-perceivable behaviors which are not desirable.

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
Contributor

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 linked a pull request Nov 22, 2024 that will close this issue
Open
6 tasks
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.
Projects
[Team] Developer Workflow & Infrastructure - Android
Status: Todo
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
3 participants
@seanlip @manas-yu @Rd4dev