feat(ci): Implement test sharding (5x testing speed goal part-2)#3281
feat(ci): Implement test sharding (5x testing speed goal part-2)#3281may-tas wants to merge 5 commits intoPalisadoesFoundation:developfrom
Conversation
Our Pull Request Approval ProcessThis PR will be reviewed according to our: Your PR may be automatically closed if:
Thanks for contributing! |
WalkthroughConverts Flutter tests to a 12-shard matrix (TOTAL_SHARDS=12) with per-shard coverage artifacts, and adds a Merge-Coverage job that downloads and validates per-shard coverage, merges lcov.info into ./coverage/lcov.info, emits stats, conditionally uploads to Codecov and runs VeryGoodCoverage, and makes Android-Build depend on the merged coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant ShardJob as "Shard Jobs (0..11)"
participant Artifacts as "GitHub Artifacts"
participant MergeJob as "Merge-Coverage Job"
participant LcovTool as "lcov (merge)"
participant Codecov as "Codecov"
participant Android as "Android-Build"
ShardJob->>Artifacts: upload `coverage-shard-<shard>` (lcov.info)
MergeJob->>Artifacts: download `coverage-shard-*` artifacts
MergeJob->>LcovTool: merge shard lcov.info files -> ./coverage/lcov.info
MergeJob->>MergeJob: validate merged file, compute stats, warn if low file count
MergeJob->>Codecov: upload ./coverage/lcov.info (conditional)
MergeJob->>MergeJob: run VeryGoodCoverage against merged lcov (conditional)
MergeJob->>Android: signal success (Android-Build depends on Merge-Coverage)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
@coderabbitai add |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 70
You'll need to add the
Alternatively, if you have the necessary permissions, you can add it via the GitHub CLI or API. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
.github/workflows/push.yml (2)
249-255: Inconsistent job dependencies:iOS-Builddepends onFlutter-Testing, notMerge-Coverage.
Android-Build-and-Release(line 214) depends onMerge-Coverage, butiOS-Builddepends directly onFlutter-Testing. This means iOS build can start before coverage is merged and won't fail if coverage merging fails. Consider aligning both builds to depend on the same job for consistency.
99-124: Fix: Flutter's--shard-indexis 0-based, but the matrix uses 1-based values.Flutter test sharding expects
--shard-indexvalues from0tototalShards - 1. WithTOTAL_SHARDS=4, valid indices are 0–3, but the matrix defines[1, 2, 3, 4]. When the workflow runs withmatrix.shard=4, it passes--shard-index=4, which is out of bounds and will fail.Proposed fix
strategy: fail-fast: false matrix: - shard: [1, 2, 3, 4] + shard: [0, 1, 2, 3] steps:Optionally update the display name to show user-friendly shard numbers:
- - name: Run tests (shard ${{ matrix.shard }}/${{ env.TOTAL_SHARDS }}) + - name: Run tests (shard ${{ matrix.shard + 1 }}/${{ env.TOTAL_SHARDS }}).github/workflows/pull-request.yml (2)
319-342: Critical: Same 0-based vs 1-based shard index issue as in push.yml.Flutter's
--shard-indexexpects 0-based indices (0-3), but matrix uses[1, 2, 3, 4]. This will cause shard 4 to fail.🐛 Proposed fix
strategy: fail-fast: false matrix: - shard: [1, 2, 3, 4] + shard: [0, 1, 2, 3]
453-458: Same dependency inconsistency:iOS-Builddepends onFlutter-TestingwhileAndroid-Builddepends onMerge-Coverage.For consistency, consider aligning both build jobs to depend on the same prerequisite job.
🤖 Fix all issues with AI agents
In @.github/workflows/pull-request.yml:
- Around line 356-360: The Merge-Coverage job is gated by if: success() which
skips the job when any dependent shard fails; update the Merge-Coverage job's
conditional to if: ${{ !cancelled() }} so it runs unless the workflow was
explicitly cancelled (allowing partial coverage from remaining shards), keeping
the job name Merge-Coverage and its needs: [Flutter-Testing] intact.
- Around line 390-408: The current merge step concatenates lcov files into
./coverage/lcov.info using cat which yields duplicate entries; replace that
logic by iterating over the list in lcov-files.txt and using lcov
--add-tracefile <file> --output-file <tmp> (or accumulate into
./coverage/lcov.info) to properly merge tracefiles, ensuring lcov is installed
beforehand and that you generate the final merged file at ./coverage/lcov.info
and then recalc SF_COUNT from that merged file; update references to the
temporary and final filenames (lcov-files.txt, ./coverage/lcov.info) and remove
the cat $(cat lcov-files.txt) usage.
In @.github/workflows/push.yml:
- Around line 137-141: The Merge-Coverage job currently uses if: success() which
prevents it from running when any Flutter-Testing shard fails; update the
Merge-Coverage job condition to if: always() (or if: ${{ !cancelled() }} if you
want to skip only explicit cancels) and add an internal guard that detects full
cancellation vs. partial failures (e.g., inspect needs.Flutter-Testing.result or
use a step that checks ${{ needs.Flutter-Testing.result }} / ${{ !cancelled()
}}) so the job runs to merge/report whatever coverage is available even when
some shards fail; modify the if line and add a small preliminary step in the
Merge-Coverage job to bail out only on cancelled runs while allowing partial
coverage uploads otherwise.
- Around line 171-189: The current step concatenates LCOV shards with cat
(creating duplicate SF: entries), so replace the naive cat merge with proper
lcov merging: ensure lcov is installed (e.g., apt-get install lcov or use a
setup action), read lcov-files.txt and run lcov -a file1 -a file2 ... -o
./coverage/lcov.info (or loop: start with the first file then lcov -a each
subsequent file into the same output) instead of cat $(cat lcov-files.txt); keep
the existing checks (empty file check, SF_COUNT via grep "^SF:" on
./coverage/lcov.info and warning) but create the merged file via lcov -a to
avoid duplicate SF entries.
|
@coderabbitai add |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 70
You'll need to add the Via GitHub UI:
Via GitHub CLI (if you have write access): gh pr edit 3281 --add-label "ignore-sensitive-files-pr" --repo PalisadoesFoundation/talawaUnfortunately, my integration doesn't have write permissions for label management in this repository, so I cannot perform this action on your behalf. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/pull-request.yml (1)
268-289: Fix step output reference and condition logic (changed-filesvschanged_files, and missingany_deletedoutput).The step ID is
changed-files(hyphenated), sosteps.changed_filesuses incorrect dot notation and resolves to nothing. Additionally, the condition referencesany_deletedoutput which is never defined in the step—onlyany_changedis set (line 282-284). The step condition cannot trigger as written.🐛 Proposed fix
- env: - CHANGED_FILES: ${{ steps.changed_files.outputs.all_changed_files }} - if: steps.changed_files.outputs.any_changed == 'true' || steps.changed_files.outputs.any_deleted == 'true' + CHANGED_FILES: ${{ steps['changed-files'].outputs.all_changed_files }} + if: steps['changed-files'].outputs.any_changed == 'true'
🤖 Fix all issues with AI agents
In @.github/workflows/push.yml:
- Around line 137-141: The Merge-Coverage job should only run when the
Flutter-Testing job actually succeeded; update the job's if condition to combine
the cancelled check with the dependent job result check (e.g., change the
current if: ${{ !cancelled() }} to if: ${{ !cancelled() &&
needs.Flutter-Testing.result == 'success' }}) so Merge-Coverage skips when
Flutter-Testing is skipped or does not succeed.
- Around line 125-129: The "Remove generated files from coverage" step currently
uses sed which only deletes SF lines and leaves orphaned LCOV records; replace
that sed invocation with the remove_from_coverage tool: add the
remove_from_coverage package (or ensure it's available in CI), then invoke the
package to remove files matching the '\.g\.dart' pattern from coverage/lcov.info
so entire LCOV records (SF/FN/FNDA/DA/end_of_record) are removed before the
"Upload coverage artifact" step; update the workflow step named "Remove
generated files from coverage" to run the remove_from_coverage command against
coverage/lcov.info.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3281 +/- ##
===========================================
- Coverage 94.14% 86.61% -7.53%
===========================================
Files 221 221
Lines 12727 12726 -1
===========================================
- Hits 11982 11023 -959
- Misses 745 1703 +958 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
While testing the sharding implementation, I noticed some test failures that aren't related to the sharding changes themselves. Shard-0 runs perfectly, which confirms the infrastructure works - but some other shards are failing. The failures:
These look like they existed before but weren't showing up consistently. The parallel execution just makes them more visible. I can fix these now if you want - Would make sense to deliver a fully working solution in one go. Or if you prefer, I can keep this PR focused on just the sharding infrastructure and handle the test fixes separately. Either way works for me. What do you think? Also, I will be experimenting with more shards, around 12 to test the speed improvement in this PR. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/pull-request.yml (1)
268-285: Fix step id/reference mismatch for outputs to work correctly.The step
id: changed-filesat line 269 uses a hyphen, but GitHub Actions does not normalize step id references. The references at lines 287–288 incorrectly usesteps.changed_files(underscore), which will resolve to null and cause the conditional at line 288 and the environment variable at line 287 to fail. Lines 300, 304, and 306 correctly usesteps.changed-files(hyphen).Rename the step id to
changed_files(underscore) and update the remaining hyphenated references at lines 300, 304, and 306.Proposed fix
- name: Get changed files - id: changed-files + id: changed_files- CHANGED_FILES_COUNT: ${{ steps.changed-files.outputs.all_changed_files_count }} + CHANGED_FILES_COUNT: ${{ steps.changed_files.outputs.all_changed_files_count }}- if: steps.changed-files.outputs.all_changed_files_count > 100 + if: steps.changed_files.outputs.all_changed_files_count > 100- CHANGED_FILES_COUNT: ${{ steps.changed-files.outputs.all_changed_files_count }} + CHANGED_FILES_COUNT: ${{ steps.changed_files.outputs.all_changed_files_count }}
🤖 Fix all issues with AI agents
In @.github/workflows/pull-request.yml:
- Around line 343-354: Pin the remove_from_coverage package to a fixed pub
version in the "Remove generated files from coverage" workflow step: change the
activation command for remove_from_coverage to include an explicit version (e.g.
remove_from_coverage@<version>) so CI uses a reproducible release, and keep the
subsequent dart pub global run remove_from_coverage:remove_from_coverage
invocation unchanged; ensure the chosen version is used consistently for
activation.
In @.github/workflows/push.yml:
- Around line 125-129: The GitHub Actions step "Remove generated files from
coverage" currently activates the `remove_from_coverage` package without a
version which risks non-reproducible CI; update the activation command used in
that step so `dart pub global activate remove_from_coverage` is changed to pin a
specific version (e.g., `dart pub global activate remove_from_coverage 2.0.0`)
and keep the subsequent `dart pub global run
remove_from_coverage:remove_from_coverage -f coverage/lcov.info -r '\.g\.dart$'`
invocation unchanged so the workflow consistently uses the intended
`remove_from_coverage` release.
| - name: Remove generated files from coverage | ||
| if: always() | ||
| run: | | ||
| dart pub global activate remove_from_coverage | ||
| dart pub global run remove_from_coverage:remove_from_coverage -f coverage/lcov.info -r '\.g\.dart$' | ||
| - name: Upload coverage artifact | ||
| if: always() | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: coverage-shard-${{ matrix.shard }} | ||
| path: ./coverage/ | ||
| retention-days: 1 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Pin remove_from_coverage to a fixed version for reproducible CI.
Same concern as push.yml: pinning avoids surprise changes from pub.dev.
🤖 Prompt for AI Agents
In @.github/workflows/pull-request.yml around lines 343 - 354, Pin the
remove_from_coverage package to a fixed pub version in the "Remove generated
files from coverage" workflow step: change the activation command for
remove_from_coverage to include an explicit version (e.g.
remove_from_coverage@<version>) so CI uses a reproducible release, and keep the
subsequent dart pub global run remove_from_coverage:remove_from_coverage
invocation unchanged; ensure the chosen version is used consistently for
activation.
| - name: Remove generated files from coverage | ||
| if: always() | ||
| run: | | ||
| sed -i '/\.g\.dart/d' coverage/lcov.info | ||
| dart pub global activate remove_from_coverage | ||
| dart pub global run remove_from_coverage:remove_from_coverage -f coverage/lcov.info -r '\.g\.dart$' |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🌐 Web query:
remove_from_coverage pub.dev Dart package versions
💡 Result:
remove_from_coverage on pub.dev currently has 5 published (stable) versions: 2.0.0, 1.0.7, 1.0.3, 1.0.1, 1.0.0. The latest is 2.0.0 (min Dart SDK 2.12). (pub.dev)
Usage (from the package docs), e.g. to remove generated *.g.dart files from coverage/lcov.info:
remove_from_coverage -f coverage/lcov.info -r '\.g\.dart$'(or pub global run remove_from_coverage:remove_from_coverage ...). (pub.dev)
Citations:
- 1: https://pub.dev/packages/remove_from_coverage/versions
- 2: https://pub.dev/documentation/remove_from_coverage/latest/
🌐 Web query:
Flutter 3.32.x remove_from_coverage compatibility
💡 Result:
remove_from_coverage is compatible with Flutter 3.32.x.
- Latest
remove_from_coverage(2.0.0) requires Dart SDK >= 2.12 (null-safety) and has no upper bound that would block Dart 3.x, so it will resolve under Flutter 3.32.x’s Dart SDK. [1] - Flutter 3.32.0 rolls to Dart 3.8.0 (and 3.32.x bugfix releases use Dart 3.8.x). [2]
Practical note: the package docs still show older pub global run ... examples; on modern Flutter/Dart you can typically use dart run remove_from_coverage ... (as a dev_dependency) or dart pub global activate remove_from_coverage then run it. [3]
Sources:
[1] https://pub.dev/packages/remove_from_coverage/versions
[2] https://docs.flutter.dev/release/release-notes/release-notes-3.32.0
[3] https://pub.dev/documentation/remove_from_coverage/latest/
🌐 Web query:
Dart pub global activate version pinning best practices
💡 Result:
Best practices for version pinning with dart pub global activate
-
Prefer an explicit version (or tight range) instead of “latest”.
dart pub global activate <package> <version-constraint>lets you pin exactly (or constrain) what gets installed (for example,markdown 0.6.0, or a range like<3.0.0). This is the simplest way to make installs reproducible across machines and CI runs. [1] -
In CI, pin exactly (and install every run).
Don’t rely on whatever happens to already be in the runner’s global cache. Use a pinned activation step (exact version), then run the tool. -
For Git-based global tools, pin to a commit SHA (or tag) with
--git-ref.
Pub uses the repo’s default branch unless you specify otherwise;--git-reflets you lock to a specific branch/tag/commit (best is a commit SHA for full reproducibility). [1] -
Document the tool version in your repo (even if installed globally).
Put the exactdart pub global activate … <version>(or--git-ref …) command inREADME,Makefile, or CI config so everyone installs the same version. -
Audit and update intentionally.
You can see what’s globally activated viadart pub global list, and upgrades happen by re-activating (so updates are an explicit action you control). [1]
Note: Dart docs also mention
dart installas a newer alternative todart pub global activate. [1]
Sources
[1] Dart docs: dart pub global (activation with version constraints; --git-ref; listing; updating) https://dart.dev/tools/pub/cmd/pub-global
Pin remove_from_coverage to a specific version (e.g., 2.0.0) for reproducible CI.
Currently, dart pub global activate remove_from_coverage fetches the latest version on every run. Unpinned activations can introduce unexpected behavior changes. Update to dart pub global activate remove_from_coverage 2.0.0 (the latest stable version) and review for updates intentionally.
Current code
- name: Remove generated files from coverage
if: always()
run: |
dart pub global activate remove_from_coverage
dart pub global run remove_from_coverage:remove_from_coverage -f coverage/lcov.info -r '\.g\.dart$'🤖 Prompt for AI Agents
In @.github/workflows/push.yml around lines 125 - 129, The GitHub Actions step
"Remove generated files from coverage" currently activates the
`remove_from_coverage` package without a version which risks non-reproducible
CI; update the activation command used in that step so `dart pub global activate
remove_from_coverage` is changed to pin a specific version (e.g., `dart pub
global activate remove_from_coverage 2.0.0`) and keep the subsequent `dart pub
global run remove_from_coverage:remove_from_coverage -f coverage/lcov.info -r
'\.g\.dart$'` invocation unchanged so the workflow consistently uses the
intended `remove_from_coverage` release.
|
@may-tas You can use the talawa api and admin as reference for implementing this sharding implementation, they have a 12 shard configuration and also the implementation of how the coverage has to be calculated and merged in a step right after all the shards have passed. In case you have any more doubts feel free to reach out to me. |
|
@Ansingh0305 yes no issues, actually im going to make it 12 atleast. |
|
@may-tas i got the same issue when i was implementing the sharding implementation, and this usually occurs because of lack of test mock isolation, which is basically that some tests are dependant on other tests and hence fail. |
|
@Ansingh0305 yeah, I will be solving all those tests in this PR itself. |
|
In case you get stuck somewhere, feel free to reach out. |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/pull-request.yml (1)
501-501: Inconsistent dependency: iOS-Build depends onFlutter-Testingwhile Android-Build depends onMerge-Coverage.
iOS-Buildstill referencesFlutter-Testingdirectly, whileAndroid-Buildwas updated to depend onMerge-Coverage. This creates an inconsistency:
- iOS build can start in parallel with coverage merge
- Android build must wait for coverage merge to complete
If this is intentional (iOS can start earlier), consider adding a comment explaining the design choice. Otherwise, align both build jobs to use the same dependency.
🤖 Fix all issues with AI agents
In @.github/workflows/pull-request.yml:
- Around line 319-322: The matrix hardcodes shard indices while TOTAL_SHARDS is
defined separately, risking drift; update the workflow so the matrix is derived
from TOTAL_SHARDS (e.g., generate the shard list via fromJson/sequence using
env.TOTAL_SHARDS) or add a validation step that compares matrix.shard against
env.TOTAL_SHARDS and fails if matrix.shard >= TOTAL_SHARDS; refer to
TOTAL_SHARDS and matrix.shard to locate where to change the strategy matrix and
where to add the validation check.
- Line 360: The Merge-Coverage job is gated by the `if: success()` condition
which prevents running when any shard fails; update the job's conditional to
`if: ${{ !cancelled() }}` so Merge-Coverage runs for partial results (replace
`if: success()` with `if: ${{ !cancelled() }}` in the Merge-Coverage job
definition), and make the same change for the equivalent job in the other
workflow so both `pull-request.yml` and `push.yml` use `if: ${{ !cancelled() }}`
instead of `if: success()`.
- Around line 377-384: In the Merge-Coverage job remove the unnecessary Java and
Flutter setup steps (the actions/setup-java@v4 block and the
subosito/flutter-action@v2 block) since the job only downloads artifacts, merges
lcov files, and uploads to Codecov; delete those "uses:" entries and their
"with:" subkeys (and verify no other steps in the Merge-Coverage job reference
java-version, distribution, or env.FLUTTER_VERSION) so the job runs without
installing Java or Flutter and reduces execution time.
- Around line 344-354: The "Upload coverage artifact" step currently uses if:
always() which can upload incomplete or missing artifacts; update the step that
uses actions/upload-artifact@v4 (named coverage-shard-${{ matrix.shard }}) to
only run when appropriate by replacing if: always() with a safer condition such
as if: success() || failure() (to exclude cancelled runs) or add a pre-check
that ensures the ./coverage/ directory or coverage/lcov.info exists before
uploading; modify the Upload coverage artifact step's if clause or add a
conditional check so the artifact upload only happens when valid coverage files
are present.
- Line 461: The workflow currently sets fail_ci_if_error: true for the
codecov/codecov-action@v5 step which causes fork PRs (without
secrets.CODECOV_TOKEN) to fail CI; update the step to either set
fail_ci_if_error: false, or add a guard to skip the upload when the token is
missing (use an if: condition checking secrets.CODECOV_TOKEN != '' on the upload
step), or enable OIDC by setting use_oidc: true so uploads can run without
exposing a secret; modify the codecov action configuration accordingly (look for
fail_ci_if_error, use_oidc and the codecov/codecov-action@v5 invocation) to
implement one of these fixes.
In @.github/workflows/push.yml:
- Around line 222-226: The workflow computes MERGE_BASE in the "Calculate merge
base for Codecov" step but never uses it; either remove that step or propagate
the value into the Codecov upload step so it actually affects upload behavior.
Locate the step that sets MERGE_BASE (the git merge-base call) and either delete
it, or modify the Codecov upload step to consume the value by referencing the
environment variable produced (e.g., reference MERGE_BASE via the step's inputs
or env using the workflow expression for environment variables) so the computed
merge base is passed to Codecov.
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| shard: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Matrix shard count should reference TOTAL_SHARDS to avoid drift.
The matrix explicitly lists [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11] while TOTAL_SHARDS is defined separately as 12. If someone updates TOTAL_SHARDS without updating the matrix (or vice versa), tests will be incorrectly distributed.
Consider using fromJson with a generated sequence or adding a validation step to ensure matrix size equals TOTAL_SHARDS.
♻️ Option: Add validation step to catch drift
- name: Validate shard configuration
run: |
if [ "${{ matrix.shard }}" -ge "${{ env.TOTAL_SHARDS }}" ]; then
echo "ERROR: Shard index ${{ matrix.shard }} >= TOTAL_SHARDS (${{ env.TOTAL_SHARDS }})"
exit 1
fiAlternatively, document clearly that both must be updated together.
🤖 Prompt for AI Agents
In @.github/workflows/pull-request.yml around lines 319 - 322, The matrix
hardcodes shard indices while TOTAL_SHARDS is defined separately, risking drift;
update the workflow so the matrix is derived from TOTAL_SHARDS (e.g., generate
the shard list via fromJson/sequence using env.TOTAL_SHARDS) or add a validation
step that compares matrix.shard against env.TOTAL_SHARDS and fails if
matrix.shard >= TOTAL_SHARDS; refer to TOTAL_SHARDS and matrix.shard to locate
where to change the strategy matrix and where to add the validation check.
| if: always() | ||
| run: | | ||
| dart pub global activate remove_from_coverage | ||
| dart pub global run remove_from_coverage:remove_from_coverage -f coverage/lcov.info -r '\.g\.dart$' | ||
| - name: Upload coverage artifact | ||
| if: always() | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: coverage-shard-${{ matrix.shard }} | ||
| path: ./coverage/ | ||
| retention-days: 1 |
There was a problem hiding this comment.
if: always() on coverage steps may upload incomplete artifacts.
When a shard's tests fail, the coverage file may be incomplete or missing entirely. Using if: always() uploads potentially corrupt coverage data that could skew merged results.
Consider using if: success() || failure() (excludes cancelled) or checking for file existence before upload.
🛡️ Proposed fix: Conditional upload with file check
- name: Upload coverage artifact
- if: always()
+ if: (success() || failure()) && hashFiles('coverage/lcov.info') != ''
uses: actions/upload-artifact@v4
with:
name: coverage-shard-${{ matrix.shard }}
path: ./coverage/
retention-days: 1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if: always() | |
| run: | | |
| dart pub global activate remove_from_coverage | |
| dart pub global run remove_from_coverage:remove_from_coverage -f coverage/lcov.info -r '\.g\.dart$' | |
| - name: Upload coverage artifact | |
| if: always() | |
| uses: actions/upload-artifact@v4 | |
| with: | |
| name: coverage-shard-${{ matrix.shard }} | |
| path: ./coverage/ | |
| retention-days: 1 | |
| if: always() | |
| run: | | |
| dart pub global activate remove_from_coverage | |
| dart pub global run remove_from_coverage:remove_from_coverage -f coverage/lcov.info -r '\.g\.dart$' | |
| - name: Upload coverage artifact | |
| if: (success() || failure()) && hashFiles('coverage/lcov.info') != '' | |
| uses: actions/upload-artifact@v4 | |
| with: | |
| name: coverage-shard-${{ matrix.shard }} | |
| path: ./coverage/ | |
| retention-days: 1 |
🤖 Prompt for AI Agents
In @.github/workflows/pull-request.yml around lines 344 - 354, The "Upload
coverage artifact" step currently uses if: always() which can upload incomplete
or missing artifacts; update the step that uses actions/upload-artifact@v4
(named coverage-shard-${{ matrix.shard }}) to only run when appropriate by
replacing if: always() with a safer condition such as if: success() || failure()
(to exclude cancelled runs) or add a pre-check that ensures the ./coverage/
directory or coverage/lcov.info exists before uploading; modify the Upload
coverage artifact step's if clause or add a conditional check so the artifact
upload only happens when valid coverage files are present.
| name: Merge Coverage Reports | ||
| runs-on: ubuntu-latest | ||
| needs: [Flutter-Testing] | ||
| if: success() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the current state of the pull-request.yml file around line 360
head -370 .github/workflows/pull-request.yml | tail -20Repository: PalisadoesFoundation/talawa
Length of output: 608
🏁 Script executed:
# Also check what condition is used in push.yml for comparison
grep -n "Merge.*[Cc]overage\|if:.*success\|if:.*cancelled" .github/workflows/push.ymlRepository: PalisadoesFoundation/talawa
Length of output: 308
🏁 Script executed:
# Get more context around line 360 in pull-request.yml
sed -n '350,370p' .github/workflows/pull-request.ymlRepository: PalisadoesFoundation/talawa
Length of output: 649
🏁 Script executed:
# Check if there's a Merge-Coverage job definition to understand what should follow
grep -n "Merge-Coverage\|merge.*coverage" .github/workflows/pull-request.yml -iRepository: PalisadoesFoundation/talawa
Length of output: 355
Merge-Coverage condition still uses if: success(), which skips coverage when any shard fails.
The if: success() condition at line 360 prevents the Merge-Coverage job from running if any shard fails, losing partial coverage from passing shards. This behavior is present in both pull-request.yml and push.yml. If the goal is to report partial coverage even when some shards fail, consider using if: ${{ !cancelled() }} instead.
🤖 Prompt for AI Agents
In @.github/workflows/pull-request.yml at line 360, The Merge-Coverage job is
gated by the `if: success()` condition which prevents running when any shard
fails; update the job's conditional to `if: ${{ !cancelled() }}` so
Merge-Coverage runs for partial results (replace `if: success()` with `if: ${{
!cancelled() }}` in the Merge-Coverage job definition), and make the same change
for the equivalent job in the other workflow so both `pull-request.yml` and
`push.yml` use `if: ${{ !cancelled() }}` instead of `if: success()`.
| - uses: actions/setup-java@v4 | ||
| with: | ||
| distribution: "zulu" | ||
| java-version: "17.0" | ||
| - uses: subosito/flutter-action@v2 | ||
| with: | ||
| flutter-version: ${{ env.FLUTTER_VERSION }} | ||
| channel: "stable" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Java and Flutter setup appear unnecessary in Merge-Coverage job.
The Merge-Coverage job only downloads artifacts, merges lcov files, and uploads to Codecov. Java and Flutter are not used in any of these steps.
Removing these setup steps would reduce job execution time by ~30-60 seconds.
♻️ Proposed removal
- - uses: actions/setup-java@v4
- with:
- distribution: "zulu"
- java-version: "17.0"
- - uses: subosito/flutter-action@v2
- with:
- flutter-version: ${{ env.FLUTTER_VERSION }}
- channel: "stable"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - uses: actions/setup-java@v4 | |
| with: | |
| distribution: "zulu" | |
| java-version: "17.0" | |
| - uses: subosito/flutter-action@v2 | |
| with: | |
| flutter-version: ${{ env.FLUTTER_VERSION }} | |
| channel: "stable" |
🤖 Prompt for AI Agents
In @.github/workflows/pull-request.yml around lines 377 - 384, In the
Merge-Coverage job remove the unnecessary Java and Flutter setup steps (the
actions/setup-java@v4 block and the subosito/flutter-action@v2 block) since the
job only downloads artifacts, merges lcov files, and uploads to Codecov; delete
those "uses:" entries and their "with:" subkeys (and verify no other steps in
the Merge-Coverage job reference java-version, distribution, or
env.FLUTTER_VERSION) so the job runs without installing Java or Flutter and
reduces execution time.
| name: "${{env.CODECOV_UNIQUE_NAME}}" | ||
| token: ${{ secrets.CODECOV_TOKEN }} | ||
| fail_ci_if_error: false | ||
| fail_ci_if_error: true |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/pull-request.yml | sed -n '450,475p'Repository: PalisadoesFoundation/talawa
Length of output: 1297
🌐 Web query:
Codecov action fail CI error fork pull request CODECOV_TOKEN
💡 Result:
On fork-based pull requests, GitHub Actions does not provide repository secrets to the workflow run, so secrets.CODECOV_TOKEN is empty. If your Codecov setup requires a token, the Codecov upload step will error/fail on fork PRs. [5][2]
Fix options
- Public repo (recommended): use tokenless uploads
- In Codecov, set Token authentication for public repositories = Not required (org setting; admin-only), then don’t pass
CODECOV_TOKENfor uploads. Codecov supports tokenless uploads for public repos in certain cases (and newer uploaders/actions are recommended). [1] - Also ensure you’re using a modern action (e.g.,
codecov/codecov-action@v4), which is designed to work better with fork PRs. [1]
- Private repo: you must use a token
- Codecov requires a token for private repos, so fork PR uploads will fail unless you redesign the workflow to run in a trusted context. [1]
- If you choose to expose the token to fork PRs (risky)
- Some projects run coverage upload on
pull_request_target(so secrets are available), but you must treat this as security-sensitive (untrusted code + secrets). - Codecov notes that the token mainly allows uploading coverage for that repo (not code access), but an attacker could upload misleading coverage if they get it. [6]
Minimal workflow patterns
A. Upload only when token exists (skips forks safely):
- uses: codecov/codecov-action@v4
if: ${{ secrets.CODECOV_TOKEN != '' }}
with:
token: ${{ secrets.CODECOV_TOKEN }}(Adding the token is the standard approach when you can rely on secrets.) [4][1]
B. Tokenless (public repo + token auth disabled):
- uses: codecov/codecov-action@v4(Tokenless behavior depends on your Codecov org/repo settings.) [1]
If you paste the exact Codecov error lines from the job log, I can tell which of the above applies (token required vs. GitHub API/build lookup vs. config). [2]
🌐 Web query:
Codecov GitHub Actions fork PR missing token behavior
💡 Result:
In GitHub Actions, pull requests coming from forks do not get access to the upstream repo’s secrets, so secrets.CODECOV_TOKEN will be empty/missing in that context (this is a GitHub security behavior, and Codecov calls it out in their Action docs). As a result, a Codecov upload that requires a token will fail on fork PRs. [2]
What happens next depends on repo visibility / Codecov settings:
-
Public repo, fork PRs (recommended path): tokenless upload can work.
Codecov supports tokenless uploads for public repos when the upload is treated as coming from an “unprotected” branch (a branch name that contains a:prefix likeforkname:branch). Starting withcodecov/codecov-action> v4.0, the Action automatically rewrites fork PR branch names to include:, so fork PR uploads “should work smoothly … out of the box” without exposing a token. [1] -
Private repo: token is always required.
Codecov’s token docs state: “For private repositories, all uploads require a token.” So fork PR uploads will fail unless you change your workflow strategy (because the secret token won’t be available to the forked PR run). [1] -
Public repo, but you’ve required tokens for protected branches:
A token is required for uploads on protected branches unless the org has disabled token auth for public repos. [1]
If you’re trying to make fork PRs work without exposing CODECOV_TOKEN, typical fixes are:
- Ensure you’re using
codecov/codecov-action@v4+(or newer), and rely on the fork-PR tokenless behavior for public repos. [1] - If you’re on
codecov/codecov-action@v5, you can also use OIDC (use_oidc: true) instead of upload tokens (requirespermissions: id-token: write). [2] - For public repos, admins can optionally configure org settings so token auth is “not required” for public uploads. [1]
🏁 Script executed:
git remote -vRepository: PalisadoesFoundation/talawa
Length of output: 321
🏁 Script executed:
rg -i "fork\|pull_request_target\|external.contributor" .github/workflows/pull-request.ymlRepository: PalisadoesFoundation/talawa
Length of output: 53
🌐 Web query:
PalisadoesFoundation/talawa repository public or private GitHub
💡 Result:
The PalisadoesFoundation/talawa GitHub repository is public. [1]
Consider handling fork PR coverage uploads differently to prevent blocking external contributions.
With fail_ci_if_error: true, the Codecov upload will fail for fork PRs (which lack access to secrets.CODECOV_TOKEN), blocking the PR. For this public repository using codecov/codecov-action@v5, consider:
- Setting
fail_ci_if_error: falseto allow the workflow to continue, or - Skipping the upload when the token is unavailable (e.g.,
if: secrets.CODECOV_TOKEN != ''), or - Using OIDC authentication (
use_oidc: true) which doesn't require secrets exposure.
🤖 Prompt for AI Agents
In @.github/workflows/pull-request.yml at line 461, The workflow currently sets
fail_ci_if_error: true for the codecov/codecov-action@v5 step which causes fork
PRs (without secrets.CODECOV_TOKEN) to fail CI; update the step to either set
fail_ci_if_error: false, or add a guard to skip the upload when the token is
missing (use an if: condition checking secrets.CODECOV_TOKEN != '' on the upload
step), or enable OIDC by setting use_oidc: true so uploads can run without
exposing a secret; modify the codecov action configuration accordingly (look for
fail_ci_if_error, use_oidc and the codecov/codecov-action@v5 invocation) to
implement one of these fixes.
| - name: Calculate merge base for Codecov | ||
| run: | | ||
| MERGE_BASE=$(git merge-base origin/develop HEAD || echo "HEAD") | ||
| echo "MERGE_BASE=$MERGE_BASE" >> $GITHUB_ENV | ||
| echo "Using merge base: $MERGE_BASE" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
rg -n "MERGE_BASE" .github/workflows/push.ymlRepository: PalisadoesFoundation/talawa
Length of output: 258
🏁 Script executed:
sed -n '220,250p' .github/workflows/push.ymlRepository: PalisadoesFoundation/talawa
Length of output: 1280
The MERGE_BASE environment variable is calculated but never used in the workflow.
The merge base is computed and stored in $GITHUB_ENV, but it's not referenced in the subsequent Codecov upload step. Either remove this step if unnecessary, or pass the merge base to Codecov via its action parameters if needed for accurate diff coverage.
🤖 Prompt for AI Agents
In @.github/workflows/push.yml around lines 222 - 226, The workflow computes
MERGE_BASE in the "Calculate merge base for Codecov" step but never uses it;
either remove that step or propagate the value into the Codecov upload step so
it actually affects upload behavior. Locate the step that sets MERGE_BASE (the
git merge-base call) and either delete it, or modify the Codecov upload step to
consume the value by referencing the environment variable produced (e.g.,
reference MERGE_BASE via the step's inputs or env using the workflow expression
for environment variables) so the computed merge base is passed to Codecov.
|
@may-tas Please fix the failing tests and address the coderabbit comments. |
|
This pull request did not get any activity in the past 10 days and will be closed in 360 days if no update occurs. Please verify it has no conflicts with the develop branch and rebase if needed. Mention it now if you need help or give permission to other people to finish your work. |
3 participants
What kind of change does this PR introduce?
Issue Number:
Did you add tests for your changes?
Not required
Tests are written for all changes made in this PR.
Test coverage meets or exceeds the current coverage (~90/95%).
Summary
This PR implements test sharding across 4 parallel jobs in GitHub Actions workflows to achieve significant CI speed improvements
Changes implemented:
pull-request.ymlandpush.ymlworkflows with identical sharding strategyDoes this PR introduce a breaking change?
Checklist for Repository Standards
coderaabbitaireview suggestions?Have you read the contributing guide?
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.