Skip to content

ci: report C++ line coverage from the Metal test run - #25

Merged
Gajesh2007 merged 2 commits into
ci/fork-testsfrom
ci/coverage
Sep 28, 2026
Merged

Gajesh2007 merged 2 commits into
ci/fork-testsfrom
ci/coverage

Conversation

@davidtai

@davidtai davidtai commented Sep 25, 2026 •

Copy link
Copy Markdown

Proposed changes

This PR adds C++ line coverage to the Fork Tests job (.github/workflows/fork-tests.yml). There is no coverage floor.

  • Job-level CFLAGS and CXXFLAGS add -fprofile-instr-generate -fcoverage-mapping. Both builds in build-macos (the wheel and the C++ tests) run cmake from scratch, and cmake reads these variables. The flags cannot go through the cmake arguments, because setup.py splits CMAKE_ARGS at spaces.
  • The test step sets LLVM_PROFILE_FILE to ${{ runner.temp }}/profiles/%p-%m.profraw. Each test process writes one profile for each instrumented binary. A job-level env cannot read the runner context (actionlint: context "runner" is not allowed here), so the test step sets the path. The build steps then write no profiles into the report.
  • After the tests, xcrun llvm-profdata merge merges the profiles. xcrun llvm-cov report reads ./build/tests/tests, the installed extension (mlx/core.cpython-312-darwin.so) and the libraries in mlx/lib (libmlx.dylib, libjaccl.dylib). The Python tests run the library code in libmlx.dylib, not in the extension. -ignore-filename-regex drops /tests/, /_deps/, /python/, /.venv/ and /site-packages/.
  • The job summary gets a small table with the totals for lines, functions and regions. The full report for each file is uploaded as the coverage-report artifact with actions/upload-artifact v7.0.1 (the SHA that build_and_test.yml already uses).
  • The report steps also run after a test failure, so that a red run shows the coverage of the tests that ran. The summary shows the result of the test step.

Metal shaders cannot be covered. The .metal kernels run on the GPU, and clang coverage instruments only the host C and C++ code.

Runner probe result

Run on the head commit: https://github.com/Layr-Labs/mlx/actions/runs/36183840975. The job is red because of the C++ failure that #24 fixes.

  • Coverage of the mlx sources from the C++ tests only: lines 43.21% (22915 of 53031), functions 51.12% (1932 of 3779), regions 40.05% (10424 of 26028), branches 37.61% (6014 of 15991). The Python tests did not run, because the C++ failure stops the test step.
  • The artifact coverage-report has the full report for each file: https://github.com/Layr-Labs/mlx/actions/runs/36183840975/artifacts/10885213482
  • llvm-cov warns that 185 functions have mismatched data. The C++ test build and the wheel compile these functions differently. llvm-cov uses the record that matches the profile.
Step (minutes) Without instrumentation (runs 36172106158, 36174742752) With instrumentation (runs 36181933249, 36183840975)
Build (wheel + C++) 14.61, 13.47 13.53, 12.95
C++ tests 0.63, 0.69 0.89, 0.82
Coverage report and upload – 0.17, 0.12

Read: the build is not measurably slower, the C++ tests take about 0.2 minutes more, and the report steps take about 0.15 minutes. The cost for the Python tests shows in the first run after #22 is updated from main with #24.

Test plan

CONTRIBUTING checklist

  • Formatting: pre-commit ran on the head commit. The Check Lint job passed in run https://github.com/Layr-Labs/mlx/actions/runs/36183841039.
  • Tests: this PR adds no library code. The workflow runs in the Test plan are the test.
  • Benchmarks: this PR adds no library code. The instrumentation applies only to the CI build. No benchmark is needed.

Dependencies

The base branch is ci/fork-tests (#22). This PR does not include #24. #24 merges into main, and #22 is then updated from main. Until then, the C++ test gptoss mxfp4 prefill tiles preserve expert boundaries and partial rows fails and stops the test step, so the Python tests do not run. This PR runs again after that update. Merge order: #24 into main, #22 updated from main, this PR into ci/fork-tests, then #22 into main.

  • ☐ I understand it is strictly prohibited to use AI to write PR description
  • AI usage disclosure: this change and this description were written by Claude (Opus 5.5) under David Tai's direction and review.

🤖 Generated with Claude Code

davidtai and others added 2 commits September 25, 2026 14:48
Build with clang coverage instrumentation through CFLAGS and CXXFLAGS.
Both builds in build-macos run cmake from scratch and read them. The
test step writes one profile for each process and instrumented binary.

After the tests, merge the profiles and report line coverage for the C++
test binary and the installed Python extension and libraries. Write the
totals to the job summary and upload the full report as an artifact.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The first report counted the generated sources in the two build
folders and the headers of the uv Python. Ignore every path under a
build folder and the uv Python folder, so that the totals count only
the mlx sources.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@jonathan308

Copy link
Copy Markdown

Review follow-up for head 35ad1c9: before approval, please bring in the validated #22 stack including #24, update/retarget this PR as needed, and link a green instrumented Fork Tests run on the resulting head.

The current metal-build-and-test check is red. Please verify that all test stages complete and attach the coverage report from that successful run; a report produced after a partial failed run should not be used as the baseline.

The coverage-reporting approach itself is reasonable. For the later 80% initiative, keep the denominator explicit: this measures instrumented host code, not Metal shader coverage. I am not asking for an 80% threshold in this enabling PR.

@davidtai

Copy link
Copy Markdown
Author

Reply written by Claude (agent) on David Tai's behalf.

Agreed. This PR stays stacked on ci/fork-tests and reruns after #22 is updated from main with #24 in; the coverage baseline is then taken from that complete run, not from the partial one. The denominator is instrumented host code (C++ and the Python extension); Metal shaders are outside it, and the body says so. No threshold is proposed here.

@Gajesh2007
Gajesh2007 merged commit 68e8464 into ci/fork-tests Sep 28, 2026
10 of 11 checks passed
@Gajesh2007
Gajesh2007 deleted the ci/coverage branch September 28, 2026 21:47
Gajesh2007 pushed a commit that referenced this pull request Oct 2, 2026
…#28)

* ci: report C++ line coverage from the Metal test run

Build with clang coverage instrumentation through CFLAGS and CXXFLAGS.
Both builds in build-macos run cmake from scratch and read them. The
test step writes one profile for each process and instrumented binary.

After the tests, merge the profiles and report line coverage for the C++
test binary and the installed Python extension and libraries. Write the
totals to the job summary and upload the full report as an artifact.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

* ci: drop generated files and Python headers from the coverage report

The first report counted the generated sources in the two build
folders and the headers of the uv Python. Ignore every path under a
build folder and the uv Python folder, so that the totals count only
the mlx sources.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
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.

3 participants