You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Found while addressing review feedback on #315, where one test used this shape:
if(!Avx2.IsSupported)return;// reports as PASSED on hardware without AVX2
A test that returns early without asserting is reported as passed, not skipped. On a CI agent or
developer machine lacking the required ISA, the suite goes green while the coverage silently
evaporates — which is the failure mode a test suite exists to prevent.
grep puts roughly 26 further occurrences across tests/DotLLM.Tests.Unit, gated on things like Avx2.IsSupported, Avx512F.IsSupported, CUDA availability and Vulkan device presence.
Why it matters here specifically
This repo's correctness story leans on parity tests — SIMD kernels verified against a scalar
reference, backends verified against CPU. Those are exactly the tests that carry hardware gates, so
the pattern concentrates in the highest-value tests rather than the incidental ones.
It also interacts badly with heterogeneous hardware: a contributor on a machine without AVX-512 sees
green, pushes, and only discovers the gap if someone else happens to run it on a machine that has it.
The convention already exists
tests/DotLLM.Tests.Unit already references Xunit.SkippableFact and uses it in the CUDA tests:
So this is applying an established convention consistently, not introducing one.
Proposed change
Convert hardware-gated early returns to [SkippableFact]/[SkippableTheory] + Skip.IfNot.
Where a test runs BOTH a scalar and a vector path, gate only the vector half — skipping the whole
test would discard the scalar coverage, which does run everywhere. (This distinction was applied
in kernels(cpu)(matmul): F32 outer-product tiled GEMM kernel for prefill #315 and is the reason that PR did not convert two of its tests.)
Leave a short comment at any site where the early return is genuinely intentional.
Acceptance criteria
No test in tests/DotLLM.Tests.Unit returns early on a hardware capability check without
either skipping or retaining an assertion that runs everywhere.
Skipped counts appear in the run summary on hardware lacking each capability, so the gap is
visible rather than silent.
Tests exercising both scalar and vector paths still assert the scalar path when the vector
path is unavailable.
No change to which assertions run on fully-capable hardware — the pass count there is
unchanged.
Observation
Found while addressing review feedback on #315, where one test used this shape:
A test that returns early without asserting is reported as passed, not skipped. On a CI agent or
developer machine lacking the required ISA, the suite goes green while the coverage silently
evaporates — which is the failure mode a test suite exists to prevent.
grepputs roughly 26 further occurrences acrosstests/DotLLM.Tests.Unit, gated on things likeAvx2.IsSupported,Avx512F.IsSupported, CUDA availability and Vulkan device presence.Why it matters here specifically
This repo's correctness story leans on parity tests — SIMD kernels verified against a scalar
reference, backends verified against CPU. Those are exactly the tests that carry hardware gates, so
the pattern concentrates in the highest-value tests rather than the incidental ones.
It also interacts badly with heterogeneous hardware: a contributor on a machine without AVX-512 sees
green, pushes, and only discovers the gap if someone else happens to run it on a machine that has it.
The convention already exists
tests/DotLLM.Tests.Unitalready referencesXunit.SkippableFactand uses it in the CUDA tests:So this is applying an established convention consistently, not introducing one.
Proposed change
returns to[SkippableFact]/[SkippableTheory]+Skip.IfNot.test would discard the scalar coverage, which does run everywhere. (This distinction was applied
in kernels(cpu)(matmul): F32 outer-product tiled GEMM kernel for prefill #315 and is the reason that PR did not convert two of its tests.)
Acceptance criteria
tests/DotLLM.Tests.Unitreturns early on a hardware capability check withouteither skipping or retaining an assertion that runs everywhere.
visible rather than silent.
path is unavailable.
unchanged.
References
tests/DotLLM.Tests.Unit/Cuda/— existingSkippableFactusage to follow