test: expand cabinet IR and DSP stability coverage#207
Merged
sudip-mondal-2002 merged 20 commits intoMay 25, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds unit tests for WAV loading/resampling, ConvolutionEngine state and block processing, CabinetSim IR loading/metadata/error paths and effects stability, and registers two new test sources in the test target. ChangesTest Coverage for Audio Processing Pipeline
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Contributor
Author
|
@sudip-mondal-2002 i have completed the changes and it is ready for the merger |
Contributor
Code Coverage Report 📊Line Coverage: 72.2% ✅ Coverage meets threshold: 72.2% >= 60% Full Coverage Summary |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/test_cabinet_sim_ir.cpp (1)
123-171: ⚡ Quick winMake processing-path intent explicit in stability/silence tests.
CabinetSim_IR_LongRunStabilityandCabinetSim_IR_SilenceRemainsSilentshould explicitly enable the effect beforeprocess(...)to avoid relying on constructor defaults.Small clarity fix
TEST(CabinetSim_IR_LongRunStability) { @@ ASSERT_TRUE(cab.load_ir(path)); + cab.set_enabled(true); @@ } TEST(CabinetSim_IR_SilenceRemainsSilent) { @@ ASSERT_TRUE(cab.load_ir(path)); + cab.set_enabled(true); @@ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_cabinet_sim_ir.cpp` around lines 123 - 171, The tests rely on the default processing state; explicitly enable the cabinet processing after loading the IR and before calling process(...) in both CabinetSim_IR_LongRunStability and CabinetSim_IR_SilenceRemainsSilent by calling the CabinetSim enable API (e.g., cab.set_enabled(true) or cab.enable(true)) after cab.load_ir(path) so the intent is clear and the tests do not depend on constructor defaults.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_cabinet_sim_ir.cpp`:
- Around line 98-121: The tests for failed IR loads do not verify that a failed
CabinetSim::load_ir(...) clears any previously-loaded IR state; update the tests
to first load a valid IR (or otherwise set a positive state via
CabinetSim::load_ir and confirm cab.has_ir() is true), then perform the failing
loads in TEST(CabinetSim_IR_MissingFileReturnsFalse) and
TEST(CabinetSim_IR_MalformedFileReturnsFalse), assert the load_ir(...) returns
false, and finally assert cab.has_ir() is false to ensure stale state is
cleared; use CabinetSim::set_sample_rate as before and clean up temporary files
as currently done.
In `@tests/test_convolution_engine.cpp`:
- Around line 51-75: The test currently only checks finiteness but should verify
that block-wise processing with ConvolutionEngine::process yields the same
result as a reference (non-streamed) convolution; fix it by computing a
reference output by performing a direct time-domain convolution of the input
buffer with the impulse response used to construct ConvolutionKernel (ir) and
then assert element-wise equality (or near-equality with a small tolerance)
between the streamed buffer (after repeated calls to ConvolutionEngine::process)
and the reference buffer; reference the existing symbols ConvolutionEngine,
ConvolutionKernel, ConvolutionEngine::process, and the test
ConvolutionEngine_OverlapAddConsistency when locating where to add the reference
computation and ASSERT_NEAR comparisons.
- Around line 29-33: The test currently only checks std::isfinite after
conv.process, which can hide leaked residual state; call conv.reset() before
conv.process(silent.data(), 256) (or ensure the reset happens immediately prior)
and replace the isfinite loop with assertions that each sample in silent is
approximately zero (e.g., use ASSERT_NEAR(s, 0.0f, <small_tolerance>) or
equivalent) to verify reset cleared internal state; reference conv.reset(),
conv.process(...), and the silent buffer in the change.
---
Nitpick comments:
In `@tests/test_cabinet_sim_ir.cpp`:
- Around line 123-171: The tests rely on the default processing state;
explicitly enable the cabinet processing after loading the IR and before calling
process(...) in both CabinetSim_IR_LongRunStability and
CabinetSim_IR_SilenceRemainsSilent by calling the CabinetSim enable API (e.g.,
cab.set_enabled(true) or cab.enable(true)) after cab.load_ir(path) so the intent
is clear and the tests do not depend on constructor defaults.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5403e7f9-bf10-4008-b94f-cdfd5fa143d5
📒 Files selected for processing (5)
CMakeLists.txttests/test_cabinet_sim_ir.cpptests/test_convolution_engine.cpptests/test_effects.cpptests/test_wav_loader.cpp
Contributor
PR Preview RemovedThe GitHub Pages preview for this PR has been removed because the PR was closed. |
Collaborator
convulation_engine and wav_loader files don't have enough coverage. Make it 90+. Also cabinet_sim is good but try to make it 90+
|
Contributor
Author
|
@MohitBareja16 plz review it . |
Owner
|
wav_loader is still red, if possible get it 90+, if not possible let us know why |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_wav_loader.cpp (1)
77-89: ⚡ Quick winUse unique temp file paths and guaranteed cleanup for test isolation.
Line 77 and Line 127 use fixed filenames in the working directory, which can make tests flaky under parallel execution and can leak artifacts if an assertion fails before Line 88/Line 140.
Suggested change
+#include <filesystem> ... TEST(WavLoader_LoadMalformedFileReturnsEmpty) { - const std::string path = "bad_wav.wav"; + const auto path = (std::filesystem::temp_directory_path() / + ("bad_wav_" + std::to_string(std::rand()) + ".wav")).string(); + struct Cleanup { std::string p; ~Cleanup(){ std::remove(p.c_str()); } } cleanup{path}; ... - std::remove(path.c_str()); } ... TEST(WavLoader_MaxLengthLimit) { - const std::string path = "limit_test.wav"; + const auto path = (std::filesystem::temp_directory_path() / + ("limit_test_" + std::to_string(std::rand()) + ".wav")).string(); + struct Cleanup { std::string p; ~Cleanup(){ std::remove(p.c_str()); } } cleanup{path}; ... - std::remove(path.c_str()); }Also applies to: 127-141
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_wav_loader.cpp` around lines 77 - 89, The test uses a fixed filename "bad_wav.wav" which can collide in parallel runs and may not be removed if an assertion fails; change the test to create a unique temporary file path (e.g. using std::filesystem::temp_directory_path with a generated unique filename or std::tmpnam) and write the invalid data there, then call load_wav_file(path) and assert on WavData; ensure guaranteed cleanup by removing the temp file in a scope guard/RAII destructor or a try/finally-style block so std::filesystem::remove is executed even if ASSERT_TRUE fails; apply the same pattern for the other test around Line 127 that currently uses a fixed filename.tests/test_cabinet_sim_ir.cpp (1)
271-290: ⚡ Quick winConsolidate duplicated
clear_ir()coverage into one test.
CabinetSim_ClearIRRemovesStateduplicates the core behavior already covered byCabinetSim_ClearIR_ResetsState(load IR →clear_ir()→has_ir()==false). Consider removing this test or merging any unique assertions into the existing one to reduce maintenance noise.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_cabinet_sim_ir.cpp` around lines 271 - 290, Duplicate test coverage: remove or merge the redundant TEST named CabinetSim_ClearIRRemovesState which repeats the load_ir → clear_ir → has_ir()==false behavior already covered by CabinetSim_ClearIR_ResetsState; either delete the entire CabinetSim_ClearIRRemovesState test or move any unique assertions (e.g., file creation via write_wav_mono_pcm16 or the path removal) into CabinetSim_ClearIR_ResetsState and keep calls to CabinetSim::load_ir, CabinetSim::clear_ir and CabinetSim::has_ir as the single authoritative check for reset behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_wav_loader.cpp`:
- Around line 137-139: Change the truncation assertion to require an exact size
of 512 instead of allowing <=512: update the test that checks wav.samples.size()
so it asserts equality with 512 (use ASSERT_EQ or equivalent), ensuring you
compare types consistently (cast or use size_t(512)) so the test fails if
samples are truncated too much; locate the assertion referencing
wav.samples.size() in tests/test_wav_loader.cpp and replace the <= 512 check
with an equality check to 512.
---
Nitpick comments:
In `@tests/test_cabinet_sim_ir.cpp`:
- Around line 271-290: Duplicate test coverage: remove or merge the redundant
TEST named CabinetSim_ClearIRRemovesState which repeats the load_ir → clear_ir →
has_ir()==false behavior already covered by CabinetSim_ClearIR_ResetsState;
either delete the entire CabinetSim_ClearIRRemovesState test or move any unique
assertions (e.g., file creation via write_wav_mono_pcm16 or the path removal)
into CabinetSim_ClearIR_ResetsState and keep calls to CabinetSim::load_ir,
CabinetSim::clear_ir and CabinetSim::has_ir as the single authoritative check
for reset behavior.
In `@tests/test_wav_loader.cpp`:
- Around line 77-89: The test uses a fixed filename "bad_wav.wav" which can
collide in parallel runs and may not be removed if an assertion fails; change
the test to create a unique temporary file path (e.g. using
std::filesystem::temp_directory_path with a generated unique filename or
std::tmpnam) and write the invalid data there, then call load_wav_file(path) and
assert on WavData; ensure guaranteed cleanup by removing the temp file in a
scope guard/RAII destructor or a try/finally-style block so
std::filesystem::remove is executed even if ASSERT_TRUE fails; apply the same
pattern for the other test around Line 127 that currently uses a fixed filename.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9b0c1350-9125-4076-a61d-d3e9e8daf4f6
⛔ Files ignored due to path filters (1)
resample_test.wavis excluded by!**/*.wav
📒 Files selected for processing (3)
tests/test_cabinet_sim_ir.cpptests/test_convolution_engine.cpptests/test_wav_loader.cpp
Contributor
Author
you can see that the test time is of yesterday at 12 pm can you check the current report with the current time test .
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/test_wav_loader.cpp (1)
262-263:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStrengthen the truncation assertion to exact expected size.
Line 263 asserts
<= 512, which can hide regressions that truncate too aggressively. The loader should cap at exactlymax_length_sampleswhen exceeded.Proposed fix
- ASSERT_TRUE( - static_cast<int>(wav.samples.size()) <= 512); + ASSERT_EQ(static_cast<int>(wav.samples.size()), 512);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_wav_loader.cpp` around lines 262 - 263, Test currently uses a weak assertion (<= 512) for wav.samples.size(), which can hide over-truncation; change the assertion to require exact truncation to max_length_samples by replacing the <= check with an equality check (e.g., ASSERT_EQ(static_cast<int>(wav.samples.size()), max_length_samples) or ASSERT_EQ(..., 512)), referencing wav.samples.size() and the max_length_samples test variable to guarantee the loader caps to the expected length.
🧹 Nitpick comments (1)
tests/test_wav_loader.cpp (1)
317-345: ⚡ Quick winConsider verifying that stereo samples are actually mixed down to mono.
The test checks
wav.channels == 2(original format metadata) and that samples are finite, but doesn't verify the core mix-down contract. According to the loader implementation, stereo frames should be averaged to mono. Forleft=1.0fandright=-1.0f, the expected mono result is0.0f.Suggested verification
Add assertions to confirm mix-down behavior:
ASSERT_EQ(wav.sample_rate, 48000); + ASSERT_EQ(static_cast<int>(wav.samples.size()), 512); + for (float s : wav.samples) { ASSERT_TRUE(std::isfinite(s)); + ASSERT_NEAR(s, 0.0f, 1e-5f); }This verifies that:
- Sample count matches frame count (mono, not interleaved stereo)
- Mix-down correctly averages left and right channels
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_wav_loader.cpp` around lines 317 - 345, The test WavLoader_StereoMixdown currently only checks metadata and finiteness but not that stereo was mixed to mono; update the test to verify mix-down by asserting that wav.channels reflects mono output (1) or that the number of samples equals the number of frames and that each sample equals the average of left and right (for left=1.0f and right=-1.0f expect 0.0f). Locate the test function WavLoader_StereoMixdown and add assertions after load_wav_file(path) that check either wav.channels == 1 (or samples.size() == left.size()) and that all entries in wav.samples are approximately 0.0f (use the existing isfinite loop pattern to iterate and assert exact/near-zero values).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@tests/test_wav_loader.cpp`:
- Around line 262-263: Test currently uses a weak assertion (<= 512) for
wav.samples.size(), which can hide over-truncation; change the assertion to
require exact truncation to max_length_samples by replacing the <= check with an
equality check (e.g., ASSERT_EQ(static_cast<int>(wav.samples.size()),
max_length_samples) or ASSERT_EQ(..., 512)), referencing wav.samples.size() and
the max_length_samples test variable to guarantee the loader caps to the
expected length.
---
Nitpick comments:
In `@tests/test_wav_loader.cpp`:
- Around line 317-345: The test WavLoader_StereoMixdown currently only checks
metadata and finiteness but not that stereo was mixed to mono; update the test
to verify mix-down by asserting that wav.channels reflects mono output (1) or
that the number of samples equals the number of frames and that each sample
equals the average of left and right (for left=1.0f and right=-1.0f expect
0.0f). Locate the test function WavLoader_StereoMixdown and add assertions after
load_wav_file(path) that check either wav.channels == 1 (or samples.size() ==
left.size()) and that all entries in wav.samples are approximately 0.0f (use the
existing isfinite loop pattern to iterate and assert exact/near-zero values).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9776237f-eced-45cf-9362-8ce82a04e155
⛔ Files ignored due to path filters (1)
stereo_test.wavis excluded by!**/*.wav
📒 Files selected for processing (1)
tests/test_wav_loader.cpp
sudip-mondal-2002
requested changes
May 24, 2026
sudip-mondal-2002
left a comment
sudip-mondal-2002
left a comment
Owner
There was a problem hiding this comment.
please keep the assets inside tests/assets folder
otherwise LGTM
sudip-mondal-2002
added
Audio Processing
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/test_wav_loader.cpp (1)
1-173: ⚡ Quick winConsider adding multi-channel (stereo) test coverage.
Issue
#184specifically mentions testing "stereo vs. mono" handling, but all tests in this file use mono (single-channel) WAVs. To fully satisfy the objectives, consider adding test cases that:
- Load a stereo WAV and verify it is correctly mixed down to mono.
- Validate that the mono mix-down averages channels correctly (per wav_loader.cpp:86-95).
- Test truncation and resampling behavior with stereo sources.
Example test structure:
TEST(WavLoader_Stereo_MixedToMono) { // Create stereo WAV with distinct left/right values // Load and verify samples = (left + right) / 2 }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_wav_loader.cpp` around lines 1 - 173, Add stereo test coverage by writing tests that create 2-channel PCM16 WAVs (use write_pcm16_wav with num_channels=2 and interleaved int16_t samples), then call load_wav_file and assert the returned WavData is mono and samples equal the per-frame average of left and right channels (matching the mix-down logic in wav_loader.cpp around the mix-down loop at lines ~86-95); include variants: (1) TEST(WavLoader_Stereo_MixedToMono) with distinct constant left/right values to verify averaging, (2) TEST(WavLoader_Stereo_Truncation_AfterResample) that writes a stereo file at a different sample_rate and asserts resampling + truncation behaves like the mono tests (check sample_rate, size limit, and sample values), and (3) TEST(WavLoader_Stereo_Truncation_PrefixContentPreserved) with a stereo sinusoid to ensure prefix content and truncation semantics match mono behavior.tests/test_effects.cpp (2)
328-339: ⚡ Quick winLoad an IR to test convolution path silence propagation.
The test doesn't call
load_ir_from_file(), soconv_engine_.has_kernel()returns false andCabinetSim::process()skips convolution entirely (see context snippet cabinet_sim.cpp:123-145). This means the partitioned overlap-add DSP path—the primary source of potential numerical instability—is not exercised.Given the PR objectives emphasize "cabinet IR and DSP stability coverage," consider loading a test IR fixture to verify that the convolution engine correctly propagates silence (i.e., that
overlap_buffers remain zero and no denormals/noise appear).💡 Suggested enhancement to test convolution path
TEST(cabinet_sim_silence_remains_silent) { CabinetSim cab; cab.set_sample_rate(48000); + // Load a test IR to exercise convolution path + cab.load_ir_from_file("tests/fixtures/impulse_512.wav"); cab.reset(); float buf[512] = {0.0f}; cab.process(buf, 512); ASSERT_TRUE(buffer_is_finite(buf, 512)); - ASSERT_LT(rms(buf, 512), 0.000001f); + ASSERT_LT(rms(buf, 512), 1e-6f); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_effects.cpp` around lines 328 - 339, The test never exercises the convolution path because conv_engine_.has_kernel() is false; fix by loading a small test IR before processing: call CabinetSim::load_ir_from_file(...) (or the appropriate loader used in tests) after cab.reset() and cab.set_sample_rate(48000) so the partitioned overlap-add path runs, then call cab.process(buf, 512) and keep the same assertions; ensure the IR fixture is silent or extremely short (and matches 48k sample rate) so the test still verifies that overlap_ remains zero/finite and rms(buf,512) is below the threshold.
340-354: ⚡ Quick winLoad an IR to test convolution engine long-run stability.
The test doesn't load an IR, so the convolution path (partitioned overlap-add FFT) is bypassed. The core stability concern—whether
overlap_buffers,fdl_circular indexing, and FFT normalization remain numerically stable across 1000 blocks—is not verified.Additionally, the RMS threshold
> 1e-4is very permissive (a 440 Hz sine with peak amplitude 1.0 has RMS ≈ 0.707). This might miss bugs where convolution heavily attenuates the signal or introduces slow drift.💡 Suggested enhancement
TEST(cabinet_sim_long_run_stability) { CabinetSim cab; cab.set_sample_rate(48000); + cab.load_ir_from_file("tests/fixtures/impulse_512.wav"); cab.reset(); float buf[512]; for (int i = 0; i < 1000; ++i) { fill_sine(buf, 512, 440.0f, 48000); cab.process(buf, 512); ASSERT_TRUE(buffer_is_finite(buf, 512)); - ASSERT_GT(rms(buf, 512), 0.0001f); + // For a unit-impulse IR, output RMS should be close to input RMS + ASSERT_GT(rms(buf, 512), 0.1f); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_effects.cpp` around lines 340 - 354, The test currently never exercises the convolution path; update the test to load an IR into the CabinetSim so the partitioned overlap-add FFT path runs (use the project’s IR API on CabinetSim — e.g., the IR-loading method such as load_ir / setImpulseResponse / similar on the cab instance or create a short impulse buffer and call the CabinetSim IR setter) before the loop, then run the same 1000 iterations calling cab.process(buf, 512); also tighten the sanity check on level by replacing the permissive ASSERT_GT(rms(...), 1e-4f) with a stronger check (for example assert rms is close to the expected sine RMS ~0.7071 using ASSERT_NEAR(rms(buf,512), 0.7071f, 0.01f) or at least ASSERT_GT(rms(...), 0.5f)) and keep the buffer_is_finite assertions to catch numerical instability.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_wav_loader.cpp`:
- Around line 133-146: In the TEST WavLoader_Truncation_PrefixContentPreserved
replace the non-portable M_PI usage in the sine expression with the project
constant (e.g., Amplitron::TWO_PI or 2*Amplitron::PI) so the expression becomes
0.7f * std::sin(Amplitron::TWO_PI * i / 64.f) * 32767.f; also ensure the test
file includes the header that declares Amplitron::PI/TWO_PI (add the appropriate
common header include if it's not already present).
---
Nitpick comments:
In `@tests/test_effects.cpp`:
- Around line 328-339: The test never exercises the convolution path because
conv_engine_.has_kernel() is false; fix by loading a small test IR before
processing: call CabinetSim::load_ir_from_file(...) (or the appropriate loader
used in tests) after cab.reset() and cab.set_sample_rate(48000) so the
partitioned overlap-add path runs, then call cab.process(buf, 512) and keep the
same assertions; ensure the IR fixture is silent or extremely short (and matches
48k sample rate) so the test still verifies that overlap_ remains zero/finite
and rms(buf,512) is below the threshold.
- Around line 340-354: The test currently never exercises the convolution path;
update the test to load an IR into the CabinetSim so the partitioned overlap-add
FFT path runs (use the project’s IR API on CabinetSim — e.g., the IR-loading
method such as load_ir / setImpulseResponse / similar on the cab instance or
create a short impulse buffer and call the CabinetSim IR setter) before the
loop, then run the same 1000 iterations calling cab.process(buf, 512); also
tighten the sanity check on level by replacing the permissive
ASSERT_GT(rms(...), 1e-4f) with a stronger check (for example assert rms is
close to the expected sine RMS ~0.7071 using ASSERT_NEAR(rms(buf,512), 0.7071f,
0.01f) or at least ASSERT_GT(rms(...), 0.5f)) and keep the buffer_is_finite
assertions to catch numerical instability.
In `@tests/test_wav_loader.cpp`:
- Around line 1-173: Add stereo test coverage by writing tests that create
2-channel PCM16 WAVs (use write_pcm16_wav with num_channels=2 and interleaved
int16_t samples), then call load_wav_file and assert the returned WavData is
mono and samples equal the per-frame average of left and right channels
(matching the mix-down logic in wav_loader.cpp around the mix-down loop at lines
~86-95); include variants: (1) TEST(WavLoader_Stereo_MixedToMono) with distinct
constant left/right values to verify averaging, (2)
TEST(WavLoader_Stereo_Truncation_AfterResample) that writes a stereo file at a
different sample_rate and asserts resampling + truncation behaves like the mono
tests (check sample_rate, size limit, and sample values), and (3)
TEST(WavLoader_Stereo_Truncation_PrefixContentPreserved) with a stereo sinusoid
to ensure prefix content and truncation semantics match mono behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c38a6af0-a34a-4361-9873-898fc880a28c
📒 Files selected for processing (2)
tests/test_effects.cpptests/test_wav_loader.cpp
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_wav_loader.cpp (1)
28-28: 💤 Low valueOptional: Extract repeated directory creation to a shared helper or test fixture setup.
The
std::filesystem::create_directories("tests/assets")call is repeated in three helper functions. While idempotent and safe, extracting this to a test fixtureSetUp()or a single initialization function would reduce duplication.Also applies to: 51-51, 90-90
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_wav_loader.cpp` at line 28, Extract the repeated std::filesystem::create_directories("tests/assets") calls into a shared setup helper used by the three helpers/tests; add a function (e.g., EnsureTestAssetsDirExists or a test fixture SetUp method) and replace the direct calls in the helper functions referenced around the creates (the locations showing std::filesystem::create_directories in tests/test_wav_loader.cpp) to call that single initializer to remove duplication and centralize directory creation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_wav_loader.cpp`:
- Line 28: Extract the repeated
std::filesystem::create_directories("tests/assets") calls into a shared setup
helper used by the three helpers/tests; add a function (e.g.,
EnsureTestAssetsDirExists or a test fixture SetUp method) and replace the direct
calls in the helper functions referenced around the creates (the locations
showing std::filesystem::create_directories in tests/test_wav_loader.cpp) to
call that single initializer to remove duplication and centralize directory
creation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c8d7b677-cda8-49ca-bdbc-ba177792b693
📒 Files selected for processing (2)
tests/assets/.gitkeeptests/test_wav_loader.cpp
YASHcode-IIITV
force-pushed
the
test/cabinet-ir-coverage
branch
from
78112db to
8ebc0fd
Compare
sudip-mondal-2002
previously approved these changes
May 24, 2026
sudip-mondal-2002
approved these changes
May 25, 2026
sudip-mondal-2002
added
gssoc:approved
This was referenced May 27, 2026
Merged
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
3 participants
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.
What does this PR do?
Expands cabinet IR and DSP stability test coverage for the cabinet simulation pipeline.
Adds regression and edge-case tests for:
Related Issue
Fixes #184
Type of Change
How Was This Tested?
Platform(s) tested on:
Test command run:
build\amplitron-tests.exeManual test steps (if applicable):
Results:
Checklist
./amplitron-tests)std::mutex::lock()on the hot path)Screenshots / Demo
N/A — test coverage changes only.
Summary by CodeRabbit