coverage CI: mark SlangcCoverageManifestOutput as expected failure on macOS#11630
Conversation
… macOS The two MetalLib sub-checks inside SlangcCoverageManifestOutput (_testCoverageAutoSidecarForMetalLibDisassembly and _testCoverageExplicitSidecarForMetalLibDisassembly) always fail on the macOS coverage nightly because coverage metadata is not propagated through the Metal downstream compilation chain — _maybeWriteCoverageManifest finds no ICoverageTracingMetadata on the final artifact and writes no manifest file. On non-Apple CI the sub-checks skip via _isMetalLibDownstreamUnavailable, so the test appears to pass everywhere except the coverage nightly. Marking the whole unit test as expected-failure keeps the nightly green while the underlying compiler issue is tracked in shader-slang#11629.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds ChangesCoverage CI Expected-Failure Entry
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 🟡 Has issues — 0 bugs, 2 gaps
The PR adds one entry to tests/expected-failure-coverage.txt to mark slang-unit-test-tool/SlangcCoverageManifestOutput.internal as an expected failure on macOS coverage CI, deferring the underlying metallib-asm metadata-propagation fix to #11629. The technical claims about _maybeWriteCoverageManifest / ICoverageTracingMetadata and the entry-name format match source. Two issues: (1) expected-failure granularity is per-unit-test, but only 2 of the test's 27 SLANG_CHECK sub-checks are Metal-related, so the entry silently masks future regressions in 25 unrelated sub-checks on the macOS-coverage lane; (2) the inline comment misattributes the non-Apple skip to _isMetalLibDownstreamUnavailable, when it is actually the #if SLANG_APPLE_FAMILY preprocessor guard that compiles those sub-checks out.
Changes Overview
Coverage CI expected-failure list (tests/expected-failure-coverage.txt)
- What changed: Adds
slang-unit-test-tool/SlangcCoverageManifestOutput.internalplus a 7-line explanatory comment. The comment describes that_maybeWriteCoverageManifest(source/slang/slang-end-to-end-request.cpp:511) finds noICoverageTracingMetadataon the Metal final artifact because metadata is dropped going throughxcrun metallib-asm, so the test's two MetalLib sub-checks fail on macOS coverage CI; on non-Apple CI the sub-checks are skipped. No source code, IR pass, emitter, or prelude is touched.
Findings (2 total)
| Severity | Location | Finding |
|---|---|---|
| 🟡 Gap | tests/expected-failure-coverage.txt:36 |
Per-test granularity masks future regressions in the 25 non-Metal sub-checks of SlangcCoverageManifestOutput on macOS coverage CI |
| 🟡 Gap | tests/expected-failure-coverage.txt:35 |
Comment says non-Apple CI skips via _isMetalLibDownstreamUnavailable, but the actual non-Apple skip is the #if SLANG_APPLE_FAMILY preprocessor guard at unit-test-stdin-compile.cpp:800–803, 876–879 |
| # inside this unit test therefore always fail on macOS coverage CI where the | ||
| # Metal toolchain is present. Tracked in: https://github.com/shader-slang/slang/issues/11629 | ||
| # On non-Apple CI the sub-checks are skipped via _isMetalLibDownstreamUnavailable. | ||
| slang-unit-test-tool/SlangcCoverageManifestOutput.internal |
There was a problem hiding this comment.
🟡 Gap: Expected-failure granularity masks future regressions in 25 unrelated sub-checks
expected-failure-coverage.txt operates at unit-test granularity, but SLANG_UNIT_TEST(SlangcCoverageManifestOutput) at tools/slang-unit-test/unit-test-stdin-compile.cpp:1610 aggregates 27 SLANG_CHECK calls. Only sub-checks 3 and 6 — _testCoverageAutoSidecarForMetalLibDisassembly (line 778) and _testCoverageExplicitSidecarForMetalLibDisassembly (line 852) — are Metal-related. The other 25 cover auto-sidecar, explicit-sidecar, stdout artifact, overwrite/symlink/case-alias collisions, multi-entry, whole-program, unsupported-target rejection, write-failure reporting, and compiler-option-hash invariance. With this entry, a future regression in any of those 25 that only manifests under LLVM coverage instrumentation on macOS will be reclassified as "expected" and ship undetected on that lane (Linux/Windows non-coverage and macOS non-coverage CI would still catch it, so the blind spot is genuinely macOS-coverage-only — but the file's own header documents that LLVM-coverage-only failure modes do exist).
Example: If a refactor of the sidecar-overwrite logic introduces a regression that only repros under LLVM coverage on macOS, the unit test fails, the runner reclassifies it as expected, the regression ships, and is only caught when #11629 is fixed and this entry is removed.
Suggested fix: Guard the two MetalLib sub-checks at the test level rather than skipping the whole test. For example, skip lines 1614 and 1617–1618 (the two SLANG_CHECK(_testCoverageExplicit/AutoSidecarForMetalLibDisassembly(...)) invocations) when running under coverage instrumentation on macOS — via a runtime probe in those helpers, an env-var, or a coverage-build define. Then drop this expected-failure-coverage.txt entry. That preserves regression coverage for the 25 non-Metal sub-checks until #11629 is fixed.
| # final artifact and writes no manifest file. The two MetalLib sub-checks | ||
| # inside this unit test therefore always fail on macOS coverage CI where the | ||
| # Metal toolchain is present. Tracked in: https://github.com/shader-slang/slang/issues/11629 | ||
| # On non-Apple CI the sub-checks are skipped via _isMetalLibDownstreamUnavailable. |
There was a problem hiding this comment.
🟡 Gap: Comment misidentifies the non-Apple skip mechanism
The line says: # On non-Apple CI the sub-checks are skipped via _isMetalLibDownstreamUnavailable. In source, both Metal sub-checks at tools/slang-unit-test/unit-test-stdin-compile.cpp:778 (_testCoverageAutoSidecarForMetalLibDisassembly) and :852 (_testCoverageExplicitSidecarForMetalLibDisassembly) are wrapped in #if SLANG_APPLE_FAMILY with an #else return SLANG_OK; branch (around lines 800–803 and 876–879). On non-Apple builds the entire body is compiled out — _isMetalLibDownstreamUnavailable is never reached. That helper only handles the runtime case where the Metal toolchain is missing on Apple.
Suggestion: Reword to accurately credit the preprocessor guard:
# On non-Apple builds the sub-checks are compiled out via `#if SLANG_APPLE_FAMILY`;
# on Apple platforms without a Metal toolchain they return early via
# `_isMetalLibDownstreamUnavailable` (E52002 "pass-through compiler not found").
|
@nv-slang-bot , please create a new issue to track the follow up. We need a proper fix to make it working for both mac-os and linux; or at least bring back the coverage on linux. |
|
@jkwak-work Tracked as #11632, assigned to @jvepsalainen-nv. The follow-up issue captures the request — a proper cross-platform fix, or at minimum restoring coverage — plus the verified specifics:
🤖 Generated by an automated Slang coworker — may be inaccurate. A human maintainer should verify. |
… macOS (#11630) ## Motivation `slang-unit-test-tool/SlangcCoverageManifestOutput.internal` has been failing on the macOS coverage nightly runner (https://github.com/shader-slang/slang/actions/runs/27591487109/job/81572949146) since the runner gained a Metal toolchain. PR #11610 loosened the manifest content check but did not fix the root cause: coverage metadata (`ICoverageTracingMetadata`) is not propagated through the Metal downstream compilation chain, so `_maybeWriteCoverageManifest` finds no metadata on the final artifact and writes no file at all. The manifest-content check never runs. On non-Apple CI the two MetalLib sub-checks skip cleanly via `_isMetalLibDownstreamUnavailable`, so the test appeared to pass everywhere before the nightly. ## Proposed solution Add the unit test to `tests/expected-failure-coverage.txt` so the nightly classifies the known failure as expected rather than breaking the run. The underlying compiler issue (metadata propagation through downstream compilation) is tracked in #11629. ## Change summary | File | Change | |---|---| | `tests/expected-failure-coverage.txt` | Add `slang-unit-test-tool/SlangcCoverageManifestOutput.internal` with a comment linking to #11629 | ## Process report No code change. The expected-failure entry uses the same `-expected-failure-list` mechanism already in place for other coverage-only skips. The entry is scoped to this file (consumed only by the coverage CI workflow) and does not affect regular CI.
3 participants
Motivation
slang-unit-test-tool/SlangcCoverageManifestOutput.internalhas been failing on the macOS coverage nightly runner (https://github.com/shader-slang/slang/actions/runs/27591487109/job/81572949146) since the runner gained a Metal toolchain. PR #11610 loosened the manifest content check but did not fix the root cause: coverage metadata (ICoverageTracingMetadata) is not propagated through the Metal downstream compilation chain, so_maybeWriteCoverageManifestfinds no metadata on the final artifact and writes no file at all. The manifest-content check never runs.On non-Apple CI the two MetalLib sub-checks skip cleanly via
_isMetalLibDownstreamUnavailable, so the test appeared to pass everywhere before the nightly.Proposed solution
Add the unit test to
tests/expected-failure-coverage.txtso the nightly classifies the known failure as expected rather than breaking the run. The underlying compiler issue (metadata propagation through downstream compilation) is tracked in #11629.Change summary
tests/expected-failure-coverage.txtslang-unit-test-tool/SlangcCoverageManifestOutput.internalwith a comment linking to #11629Process report
No code change. The expected-failure entry uses the same
-expected-failure-listmechanism already in place for other coverage-only skips. The entry is scoped to this file (consumed only by the coverage CI workflow) and does not affect regular CI.