refactor(tests): split inline test mods into sibling tests.rs files + 2 PR #20 review fixes#21
Merged
refactor(tests): split inline test mods into sibling tests.rs files + 2 PR #20 review fixes#21
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This was referenced Apr 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Mechanical cleanup PR. Moves the inline
#[cfg(all(test, feature = "std"))] mod tests { ... }blocks out of seven large source files (one per file) into siblingtests.rsfiles, and folds in two unmerged Copilot review fixes from PR #20. Zero behavior change — pure relocation + two doc/codegen tweaks.Driven by a long-running file-size concern:
src/sinker/mixed.rshad grown to 13,758 lines (of which ~5,900 were inline tests), and each per-arch SIMD backend was in the 5,000–5,700-line range. IDE navigation, code review, andgit blameall suffered. Splitting tests into siblings shrinks the production-facing surface by 36% on average across the seven files without changing what runs in CI.Mechanics
Rust 2024 edition supports the
foo.rs+foo/submodule layout natively. For each source file<dir>/<stem>.rs:#[cfg(all(test, feature = "std"))] mod tests { ... }block is replaced with the one-line declaration#[cfg(all(test, feature = "std"))] mod tests;.<dir>/<stem>/tests.rs.use super::*;continues to resolve the same way (the new file is still inside the parent module's namespace).#[cfg_attr(miri, ignore = ...)]and other per-test attributes carry through unchanged.The wasm backend keeps its
target_feature = "simd128"cfg gate (#[cfg(all(test, feature = "std", target_feature = "simd128"))] mod tests;).What's in this PR
Splits (7 files → 7 sibling test files)
#[test]markers)src/sinker/mixed.rssrc/sinker/mixed/tests.rs(5,897 / 188)src/row/scalar.rssrc/row/scalar/tests.rs(489 / 33)src/row/arch/neon.rssrc/row/arch/neon/tests.rs(2,042 / 67)src/row/arch/x86_sse41.rssrc/row/arch/x86_sse41/tests.rs(1,818 / 62)src/row/arch/x86_avx2.rssrc/row/arch/x86_avx2/tests.rs(1,782 / 62)src/row/arch/x86_avx512.rssrc/row/arch/x86_avx512/tests.rs(1,798 / 62)src/row/arch/wasm_simd128.rssrc/row/arch/wasm_simd128/tests.rs(1,569 / 58)Total: 15,402 lines of test code relocated; production-facing surface dropped by ~36% on average across the seven files. Test count unchanged (475 lib tests still discovered + run on aarch64).
Copilot review fixes from PR #20 (folded in)
Two of the four findings on Copilot review #4176623587 were applied to PR #20 locally but never made it into the merge commit. Folded in here since they're trivial and adjacent:
src/sinker/mixed.rs:2591–2601.MixedSinker<Nv42>::with_rgbadoc now lists both failure modes (RgbaBufferTooShortandGeometryOverflow), matching the Nv24 doc above it.src/row/scalar.rs:512–528.expand_rgb_to_rgba_rowrewritten to iterate viachunks_exact(3)zipped withchunks_exact_mut(4)instead of[x * 3 + k]indexing — tighter codegen on the Strategy A RGB→RGBA fan-out hot path.What's deferred (still in the cleanup memory)
The other two PR #20 review findings stay deferred to future cleanup PRs because addressing them properly requires a sweep beyond this PR's scope:
PixelSinkcontract #2 — visibility tightening onpub(crate) fn nv12_or_nv21_to_rgb_or_rgba_row_impl<...>andpub(crate) fn nv24_or_nv42_to_rgb_or_rgba_row_impl<...>insrc/row/scalar.rs, plus the corresponding SIMDpub(crate) unsafe fn *_to_rgb_or_rgba_row_implfunctions across all 5 backends. Each is only called from its own file's wrappers — they should all befn(module-private). Single uniform sweep, no behavior change. Defer because mixing visibility tightening into this PR would obscure the test-mod relocation diff.rgba_plane_slicehelper to remove the duplicatedone_plane_end.checked_mul(4)? + one_plane_start * 4 + slicepattern across 8MixedSinker<F>::processimpls (Yuv420p / Yuv422p / Yuv444p / Nv12 / Nv16 / Nv21 / Nv24 / Nv42), in both the standalone-RGBA branch and the Strategy A expand branch. Parallelrgb_plane_slice(3-channel + scratch fallback) helper should ship at the same time. Worth doing before tranches 5–7 (high-bit-depth RGBA) start landing — those will multiply the duplication.The cleanup memory tracks both of these for follow-up work.
What's NOT in scope here (intentionally)
high_bit_plane_*,interleave_uv_*, etc., duplicated across the 5 backend test files). Already noted indocs/color-conversion-functions.md§ Cleanup follow-ups; would belong in a sibling refactor PR.Verification
cargo test --lib— 475 passed; 0 failed (same as pre-refactor baseline)cargo test --doc— 1 passed (thecompile_faildoctest onMixedSinker::<Yuv420p>::with_rgbastill rejectsMixedSinker::<Yuv440p>::with_rgba)cargo check --lib --target wasm32-unknown-unknown— cleancargo check --lib --target x86_64-unknown-linux-gnu— cleancargo clippy --lib --tests— only one pre-existing warning insrc/raw/bayer.rs(unrelatedunnecessary_cast)Test plan
test,test-sde-avx512,cross,coverage,clippy,build,miri-*jobs.colconv_disable_*rustflags (the relocation should be transparent to the matrix).src/sinker/mixed/tests.rs,src/row/arch/neon/tests.rs, etc. resolve under bothcargo test --liband IDE jump-to-test.🤖 Generated with Claude Code