Skip to content

fix: accelerate coverage upload (wip)#36

Open
lnfjpt wants to merge 6 commits intomainfrom
ln-fix-codecov
Open

fix: accelerate coverage upload (wip)#36
lnfjpt wants to merge 6 commits intomainfrom
ln-fix-codecov

Conversation

@lnfjpt
Copy link
Collaborator

@lnfjpt lnfjpt commented Mar 12, 2026

Fixes

Greptile Summary

This PR removes the branch/event/scope guards (if: conditions) from three coverage-related steps — "Set ENABLE_GCOV for main branch", "Generate Coverage Report", and "Upload coverage to Codecov" — and renames the file: parameter to files: in the Codecov upload action (a valid fix for codecov-action@v5).

  • The files: rename is correct and necessary for codecov/codecov-action@v5.
  • Removing the if conditions causes ENABLE_GCOV=ON to be injected into every build (PRs, forks, workflow_dispatch), adding GCOV instrumentation overhead to all CI runs regardless of branch.
  • Coverage generation (make coverage) will now be attempted even for extension-only builds where no .gcda/.gcno data was produced, which is likely to fail.
  • The Codecov upload will fail on fork/external PRs because CODECOV_TOKEN is unavailable in fork workflows, breaking CI for external contributors.
  • The PR is marked WIP; the intent appears to be broadening when coverage runs, but the current implementation has significant side-effects that should be addressed before merging.

Confidence Score: 2/5

  • Not safe to merge — unconditional coverage steps will break extension-only builds and fork PR workflows.
  • The files: rename is a correct fix, but the removal of all three if guards introduces build failures for extension-only runs (no coverage data exists) and authentication failures for fork PRs (no CODECOV_TOKEN). It also adds GCOV overhead to every PR build. These are concrete, reproducible breakages.
  • .github/workflows/neug-test.yml — specifically the three steps that lost their if: conditions.

Important Files Changed

Filename Overview
.github/workflows/neug-test.yml Three if conditions guarding GCOV setup, coverage generation, and Codecov upload were removed, causing those steps to run unconditionally on all triggers (PRs, forks, extension-only builds); the filefiles parameter rename for codecov-action@v5 is correct.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Workflow Triggered] --> B{Event Type}
    B -->|push to main| C[Detect Scope]
    B -->|pull_request| C
    B -->|workflow_dispatch| C

    C --> D{extension_only?}
    D -->|true| E[Skip most build & test steps]
    D -->|false| F[Full Build & Test]

    E -->|BEFORE this PR| G_OLD["❌ Skip ENABLE_GCOV\n❌ Skip Coverage Generation\n❌ Skip Codecov Upload"]
    E -->|AFTER this PR| G_NEW["⚠️ Set ENABLE_GCOV=ON\n⚠️ Run make coverage\n⚠️ Attempt Codecov Upload\n(likely fails — no coverage data)"]

    F -->|BEFORE this PR| H{Is main branch push\nfrom alibaba/neug?}
    H -->|No PR/fork| I_OLD["❌ Skip ENABLE_GCOV\n❌ Skip Coverage Generation\n❌ Skip Codecov Upload"]
    H -->|Yes| J_OLD["✅ Set ENABLE_GCOV=ON\n✅ Generate Coverage\n✅ Upload to Codecov"]

    F -->|AFTER this PR| K["⚠️ Always Set ENABLE_GCOV=ON\n⚠️ Always Generate Coverage\n⚠️ Always Upload to Codecov\n(fork PRs fail — no CODECOV_TOKEN)"]
Loading

Comments Outside Diff (1)

  1. .github/workflows/neug-test.yml, line 135-141 (link)

    ENABLE_GCOV now unconditionally set for all builds

    The if guard was removed, which means ENABLE_GCOV=ON is now injected into every run — including pull requests, workflow_dispatch, extension-only builds, and fork PRs. The original condition was:

    if: steps.scope.outputs.extension_only != 'true' && (github.ref == 'refs/heads/main' && github.repository == 'alibaba/neug' && github.event_name == 'push')
    

    Enabling GCOV instrumentation adds significant compile-time and run-time overhead. All PR builds will now compile with coverage instrumentation even though that data is never meaningful during review. This will slow down every PR's build unnecessarily.

Last reviewed commit: c99e578

Greptile also left 1 inline comment on this PR.

Committed-by: nengli.ln from Dev container
Comment on lines 404 to +416
@@ -412,8 +410,7 @@ jobs:
make coverage

- name: Upload coverage to Codecov
if: steps.scope.outputs.extension_only != 'true' && (github.ref == 'refs/heads/main' && github.repository == 'alibaba/neug' && github.event_name == 'push')
uses: codecov/codecov-action@v5
with:
token: ${{ secrets.CODECOV_TOKEN }}
file: ${{ github.workspace }}/tools/python_bind/build/neug_py_bind/coverage_filtered.info
files: ${{ github.workspace }}/tools/python_bind/build/neug_py_bind/coverage_filtered.info
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coverage generation and upload now runs on all triggers, including PRs and forks

Both the "Generate Coverage Report" step (line 404) and the "Upload coverage to Codecov" step (line 412) have had their if guards removed. This creates several problems:

  1. Extension-only builds: When extension_only == 'true', the preceding build steps that produce coverage data are skipped (they still carry if: steps.scope.outputs.extension_only != 'true'). As a result, make coverage will have no .gcda / .gcno data to process and will either fail or produce a meaningless empty report.

  2. Fork PRs / untrusted PRs: ${{ secrets.CODECOV_TOKEN }} is not available in workflows triggered from forks. The upload step will fail with an authentication error on every external contributor's PR.

  3. All pull requests: Even for internal PRs, uploading partial/PR-run coverage to Codecov on every PR push pollutes the repository's coverage history and may override the canonical main-branch coverage data.

Consider restoring the if condition on both steps, or at minimum restoring the extension_only and fork/PR guards:

Suggested change
- name: Generate Coverage Report
if: steps.scope.outputs.extension_only != 'true' && github.ref == 'refs/heads/main' && github.repository == 'alibaba/neug' && github.event_name == 'push'
run: |
python3 -m pip install fastcov
sudo apt update
sudo apt install lcov -y
cd ${GITHUB_WORKSPACE}/tools/python_bind/build/neug_py_bind
make coverage
Suggested change
- name: Upload coverage to Codecov
if: steps.scope.outputs.extension_only != 'true' && github.ref == 'refs/heads/main' && github.repository == 'alibaba/neug' && github.event_name == 'push'
uses: codecov/codecov-action@v5
with:
token: ${{ secrets.CODECOV_TOKEN }}
files: ${{ github.workspace }}/tools/python_bind/build/neug_py_bind/coverage_filtered.info

@codecov-commenter
Copy link

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

lnfjpt added 5 commits March 12, 2026 14:24
Committed-by: nengli.ln from Dev container
Committed-by: nengli.ln from Dev container
Committed-by: nengli.ln from Dev container
Committed-by: nengli.ln from Dev container
Committed-by: nengli.ln from Dev container
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants