Fix #6259: Add failure tracking to static_checks.sh#6289
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@adanaamir this PR is being marked as draft because the PR description must contain 'Fixes #' or 'Fixes part of #' for each issue the PR is changing, and each one on its own line with no other text. |
|
@adanaamir this PR is being marked as draft because you are not assigned to referenced issue #6259 (or it does not exist). You may only fix issues that you are assigned to to help avoid multiple team members duplicating work on the same issue. |
|
@adanaamir You need to sign the CLA before contributing to Oppia, which is why the checks are failing. As I mentioned earlier, please go through the contributing guidelines once. They cover these basic contribution requirements and help avoid issues like this. PS: Once the CLA is signed, you can request a review by commenting "@reviewer_name, PTAL", and the bot will automatically assign the reviewer. |
|
@manas-yu , PTAL |
Coverage ReportResultsCoverage Analysis: SKIP ⏭️ This PR did not introduce any changes to Kotlin source or test files.
|
Sandesh282
left a comment
There was a problem hiding this comment.
Hi @adanaamir, looks good overall! Just one thought below. PTAL!
| # License Texts Check | ||
| echo "********************************" | ||
| echo "Running license texts checks" | ||
| echo "********************************" | ||
| bazel run //scripts:license_texts_check -- $(pwd)/app/src/main/res/values/third_party_dependencies.xml || FAILURES=$((FAILURES+1)) | ||
| echo "" |
There was a problem hiding this comment.
Quick Question: This is a net-new check being introduced in the same PR as the failure tracking infrastructure. Was this intentional or a separate change that snuck in? If intentional, it should probably be called out explicitly in the PR description since the title only mentions failure tracking.
There was a problem hiding this comment.
Thanks for pointing this out, I didn't actually write or intend to introduce the License Texts Check. It was already present on develop and got pulled into my branch when I resolved the merge conflict. If you say, I'll remove it from this PR so it only contains the failure tracking changes.
There was a problem hiding this comment.
Looks like your merge didn't pick up the latest develop: PR #6265 removed the License Texts Check after you branched off. So, yeah you might wanna clean up that in next commit.
|
@adanaamir The CI is failing due to # TODO checks. # TODO(#12345): Fix problem.And should always point to an actual filed issue. Otherwise, our tooling and team processes are unlikely to track the work and ensure it eventually gets done. You can assign me back using "@reviewer_name, PTAL!" when CI is green, or if you need further help. |
|
@Sandesh282 , CI is green, PTAL. |
|
Unassigning @adanaamir since a re-review was requested. @adanaamir, please make sure you have addressed all review comments. Thanks! |
Sandesh282
left a comment
There was a problem hiding this comment.
Thanks @adanaamir! This LGTM!
|
@adanaamir The maintainers will be busy for a week or two. Please wait in the meantime. |
|
LGTM @adanaamir, Thanks @Sandesh282 for the review as well. @adhiamboperes or @BenHenning will merge the PR once they are available, @adanaamir feel free to work on other issues in the meantime |
|
Hi @adanaamir, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to ask someone to merge your PR once the CI checks pass and you're happy with it. Thanks! |
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), 47 KiB (Removed) APK download size (estimated): 18 MiB (old), 18 MiB (new), 41 KiB (Removed) Method count: 266903 (old), 266005 (new), 898 (Removed) Features: 1 (old), 1 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 7149 (old), 7119 (new), 30 (Removed)
Lesson assets: 69 (old), 69 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 19 MiB (old), 19 MiB (new), 47 KiB (Removed)
Configuration hdpiAPK file size: 50 KiB (old), 50 KiB (new), 4 bytes (Removed) Configuration ldpiAPK file size: 49 KiB (old), 49 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 46 KiB (old), 46 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 86 KiB (old), 86 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 57 KiB (old), 57 KiB (new), 4 bytes (Removed) Configuration xxhdpiAPK file size: 63 KiB (old), 63 KiB (new), 4 bytes (Removed) Configuration xxxhdpiAPK file size: 64 KiB (old), 64 KiB (new), 0 bytes (No change) AlphaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 32 KiB (Removed) APK download size (estimated): 10 MiB (old), 10 MiB (new), 28 KiB (Removed) Method count: 119174 (old), 118719 (new), 455 (Removed) Features: 1 (old), 1 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6084 (old), 6054 (new), 30 (Removed)
Lesson assets: 70 (old), 70 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 32 KiB (Removed)
Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 4 bytes (Removed) Configuration ldpiAPK file size: 45 KiB (old), 45 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 4 bytes (Removed) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 4 bytes (Removed) BetaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 32 KiB (Removed) APK download size (estimated): 10 MiB (old), 10 MiB (new), 26 KiB (Removed) Method count: 119181 (old), 118726 (new), 455 (Removed) Features: 1 (old), 1 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6084 (old), 6054 (new), 30 (Removed)
Lesson assets: 70 (old), 70 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 32 KiB (Removed)
Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 4 bytes (Removed) Configuration ldpiAPK file size: 45 KiB (old), 45 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 4 bytes (Removed) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 4 bytes (Removed) GaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 32 KiB (Removed) APK download size (estimated): 10 MiB (old), 10 MiB (new), 26 KiB (Removed) Method count: 119181 (old), 118726 (new), 455 (Removed) Features: 1 (old), 1 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6084 (old), 6054 (new), 30 (Removed)
Lesson assets: 70 (old), 70 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 32 KiB (Removed)
Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 4 bytes (Removed) Configuration ldpiAPK file size: 45 KiB (old), 45 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 4 bytes (Removed) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 4 bytes (Removed) |
|
Hi @adanaamir, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
|
Hi @adanaamir, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
|
Hi @adanaamir, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
adhiamboperes
left a comment
There was a problem hiding this comment.
Thanks @adanaamir!
One additional improvement to consider: instead of only tracking the number of failures, you could also keep a list of the checks that failed and print it at the end. This would make it much easier for developers to quickly identify which checks need attention, especially since the script continues running after a failure.
For example, you could maintain an array such as FAILED_CHECKS, append the check name whenever a command fails, and then print a summary before exiting:
FAILED_CHECKS+=("Ktlint")Resulting in output like:
2 check(s) failed:
- Ktlint
- XML syntax validation
This keeps the existing behavior of running all checks while providing a clearer summary of what actually failed.
|
Hi, @adanaamir, the LGTM Label has been removed because the changes were requested on this PR. Thanks!. |
|
Additionally, please fix the PR template, the Essential Checklist section is missing. |
Fixes #6259
This PR supersedes #6287, which was automatically closed after a force push.
Summary
Adds failure tracking to
scripts/static_checks.shso that all checks continue running to completion while the script exits with a non-zero status code if any check fails.Testing