test: achieve 100% test coverage for core audio engine#209
Merged
sudip-mondal-2002 merged 8 commits intoMay 25, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a new desktop test file ChangesAudioEngine Test Coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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
Code Coverage Report 📊Line Coverage: 79.1% ✅ Coverage meets threshold: 79.1% >= 60% Full Coverage Summary |
Contributor
PR Preview RemovedThe GitHub Pages preview for this PR has been removed because the PR was closed. |
sudip-mondal-2002
added
High Impact
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.
some files has a little less coverage, otherwise LGTM
Please keep .wav files inside tests/assets folder
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
tests/test_audio_engine.cpp (4)
261-277: ⚡ Quick winRecorder test should validate write side effects and avoid shared filename collisions.
State toggles alone don’t prove samples were written. Also, a fixed filename can collide under parallel runs. Use a unique temp path, assert output file exists/non-empty after stop, then clean it up.
💡 Suggested direction
+ `#include` <filesystem> ... - engine.recorder().start("test_dummy_record.wav", 48000, 1); + const auto record_path = + std::filesystem::temp_directory_path() / "amplitron_test_dummy_record.wav"; + engine.recorder().start(record_path.string(), 48000, 1); ... engine.recorder().stop(); ASSERT_EQ(engine.recorder().is_recording(), false); + ASSERT_EQ(std::filesystem::exists(record_path), true); + ASSERT_TRUE(std::filesystem::file_size(record_path) > 0u); + std::filesystem::remove(record_path);🤖 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_audio_engine.cpp` around lines 261 - 277, The test AudioEngineProcess_RecorderBranch only checks recorder state transitions; update it to use a unique temporary filename (e.g., generated via std::filesystem::temp_directory_path + unique suffix) when calling AudioEngine::recorder().start so parallel runs don’t collide, call engine.process_audio(...) to trigger recorder_.write_samples(), then after engine.recorder().stop assert the output file exists and its size is > 0 to prove samples were written (and finally remove the temp file as cleanup); keep references to AudioEngine, engine.process_audio, and engine.recorder().start/stop/is_recording to locate and change the test.
140-143: ⚡ Quick winAdd post-conditions for invalid/self
move_effectcalls.These calls currently only execute branches; they don’t verify state integrity after invalid/self moves. Add assertions that order/count remain unchanged.
♻️ Proposed assertion additions
engine.move_effect(-1, 0); + ASSERT_EQ(engine.effects().size(), 2u); + ASSERT_EQ(engine.effects()[0], od); + ASSERT_EQ(engine.effects()[1], dist); engine.move_effect(0, 5); + ASSERT_EQ(engine.effects().size(), 2u); + ASSERT_EQ(engine.effects()[0], od); + ASSERT_EQ(engine.effects()[1], dist); engine.move_effect(0, 0); // Self-move + ASSERT_EQ(engine.effects().size(), 2u); + ASSERT_EQ(engine.effects()[0], od); + ASSERT_EQ(engine.effects()[1], dist);🤖 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_audio_engine.cpp` around lines 140 - 143, Before calling the three move_effect invocations, snapshot the engine's ordering state (e.g., call engine.order_count() and capture each order entry via engine.get_order(i) or the equivalent accessor), then after each invalid/self call assert that the order count remains equal to the snapshot and that every order entry equals the corresponding saved snapshot entry; apply these assertions for engine.move_effect(-1, 0), engine.move_effect(0, 5) and engine.move_effect(0, 0) to ensure no changes.
162-170: ⚡ Quick winMetronome branch is exercised, but click generation is not validated.
The loop has no assertion proving a click was emitted. Add an output assertion (e.g., detect any non-zero sample) and make iteration count deterministic from BPM/sample-rate assumptions.
🤖 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_audio_engine.cpp` around lines 162 - 170, The test currently exercises engine.initialize(), engine.set_buffer_size(), and engine.process_audio() but never asserts that the metronome click was produced; modify the test to compute a deterministic number of process_audio calls from the metronome BPM and the engine/sample-rate (convert BPM to samples per click using sampleRate * 60 / BPM, then divide by buffer size) so the loop runs exactly until a click should occur, and add an assertion after each process_audio call that checks the out buffer (the vector passed to engine.process_audio) for any non-zero sample to validate that a click was emitted; reference the existing engine.process_audio, engine.set_buffer_size, and the out vector when adding the detection/assertion.
215-218: Tuner tap branch should already be reached by this test (no extra initialization needed).
set_tuner_tap()setstopology_dirty_ = trueand prepares the tap (set_sample_rate(sample_rate_),reset()), so the firstprocess_audio()call should assignaudio_shadow_tuner_fromtuner_tap_.- The processing call is guarded by
audio_shadow_tuner_ && audio_shadow_tuner_->is_enabled().Effect::enabled_defaults totrue, and the test’s tap (Distortion) doesn’t disable it, so the guard should pass.process_audio()also resizesprocess_buffer_/process_buffer_right_toframe_count, soset_buffer_size()/initialize()isn’t required for this path.Optional: to make the coverage deterministic, use a test Effect that records whether its
process()was called and assert it.🤖 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_audio_engine.cpp` around lines 215 - 218, The test is relying on process_audio() to exercise the tuner tap path but doesn't assert that the tap's process() ran; ensure set_tuner_tap() is called (so topology_dirty_ is true and tuner_tap_ gets prepared) and then call process_audio() so audio_shadow_tuner_ is assigned from tuner_tap_ and the guard (audio_shadow_tuner_ && audio_shadow_tuner_->is_enabled()) passes (Effect::enabled_ defaults true and Distortion doesn't disable it); to make the test deterministic, replace or wrap the Distortion tap with a simple test Effect that records a flag when its process() is invoked and add an assertion that that flag is set after calling process_audio(); no additional set_buffer_size()/initialize() calls are required because process_audio() resizes process_buffer_/process_buffer_right_ to frame_count.
🤖 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_audio_engine.cpp`:
- Around line 261-277: The test AudioEngineProcess_RecorderBranch only checks
recorder state transitions; update it to use a unique temporary filename (e.g.,
generated via std::filesystem::temp_directory_path + unique suffix) when calling
AudioEngine::recorder().start so parallel runs don’t collide, call
engine.process_audio(...) to trigger recorder_.write_samples(), then after
engine.recorder().stop assert the output file exists and its size is > 0 to
prove samples were written (and finally remove the temp file as cleanup); keep
references to AudioEngine, engine.process_audio, and
engine.recorder().start/stop/is_recording to locate and change the test.
- Around line 140-143: Before calling the three move_effect invocations,
snapshot the engine's ordering state (e.g., call engine.order_count() and
capture each order entry via engine.get_order(i) or the equivalent accessor),
then after each invalid/self call assert that the order count remains equal to
the snapshot and that every order entry equals the corresponding saved snapshot
entry; apply these assertions for engine.move_effect(-1, 0),
engine.move_effect(0, 5) and engine.move_effect(0, 0) to ensure no changes.
- Around line 162-170: The test currently exercises engine.initialize(),
engine.set_buffer_size(), and engine.process_audio() but never asserts that the
metronome click was produced; modify the test to compute a deterministic number
of process_audio calls from the metronome BPM and the engine/sample-rate
(convert BPM to samples per click using sampleRate * 60 / BPM, then divide by
buffer size) so the loop runs exactly until a click should occur, and add an
assertion after each process_audio call that checks the out buffer (the vector
passed to engine.process_audio) for any non-zero sample to validate that a click
was emitted; reference the existing engine.process_audio,
engine.set_buffer_size, and the out vector when adding the detection/assertion.
- Around line 215-218: The test is relying on process_audio() to exercise the
tuner tap path but doesn't assert that the tap's process() ran; ensure
set_tuner_tap() is called (so topology_dirty_ is true and tuner_tap_ gets
prepared) and then call process_audio() so audio_shadow_tuner_ is assigned from
tuner_tap_ and the guard (audio_shadow_tuner_ &&
audio_shadow_tuner_->is_enabled()) passes (Effect::enabled_ defaults true and
Distortion doesn't disable it); to make the test deterministic, replace or wrap
the Distortion tap with a simple test Effect that records a flag when its
process() is invoked and add an assertion that that flag is set after calling
process_audio(); no additional set_buffer_size()/initialize() calls are required
because process_audio() resizes process_buffer_/process_buffer_right_ to
frame_count.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a95ed8ed-2bab-49ff-8805-3f1a3c5f8edc
⛔ Files ignored due to path filters (1)
test_dummy_record.wavis excluded by!**/*.wav
📒 Files selected for processing (1)
tests/test_audio_engine.cpp
Collaborator
Author
|
@sudip-mondal-2002 LGTM from my side |
MohitBareja16
commented
May 25, 2026
MohitBareja16
left a comment
MohitBareja16
left a comment
Collaborator
Author
There was a problem hiding this comment.
I checked this file and it is covering more than 90% test coverage in all the files
Owner
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_audio_engine.cpp`:
- Around line 185-203: The test computes timing assuming a 48kHz sample rate
(sampleRate variable) before configuring or querying the engine, which can make
click detection flaky if the engine default differs; fix by querying or setting
the engine's actual sample rate (e.g., call engine.sample_rate() or set
engine.set_sample_rate(...) before computing samplesPerClick/processCalls) and
then recompute samplesPerClick and processCalls based on that real rate so the
loop that calls engine.process_audio(...) uses correct timing for click
detection.
🪄 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: 346e6d44-d65c-4fcc-9c5f-a27ee4287dc7
📒 Files selected for processing (4)
CMakeLists.txtsrc/audio/audio_engine_process.cppsrc/audio/spsc_queue.htests/test_audio_engine.cpp
💤 Files with no reviewable changes (2)
- src/audio/audio_engine_process.cpp
- src/audio/spsc_queue.h
Comment on lines
+185
to
+203
| // Compute deterministic number of process_audio calls | ||
| double sampleRate = 48000.0; | ||
| double bpm = 40.0; // The bounded BPM from above | ||
| int samplesPerClick = static_cast<int>(sampleRate * 60.0 / bpm); | ||
| int bufferSize = 64; | ||
| int processCalls = (samplesPerClick / bufferSize) + 1; | ||
|
|
||
| bool clickDetected = false; | ||
| for(int i = 0; i < processCalls; ++i) { | ||
| std::fill(out.begin(), out.end(), 0.0f); | ||
| engine.process_audio(in.data(), out.data(), 64); | ||
| for (float s : out) { | ||
| if (std::abs(s) > 1e-6f) { | ||
| clickDetected = true; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| ASSERT_TRUE(clickDetected); |
Contributor
There was a problem hiding this comment.
Avoid default-dependent sample-rate math in click-detection timing.
Line 186 assumes 48kHz, but the test never sets engine sample rate before computing processCalls. If default sample rate differs, this can under-run click window and create flaky failures.
🔧 Proposed fix
- double sampleRate = 48000.0;
+ double sampleRate = 48000.0;
+ engine.set_sample_rate(static_cast<int>(sampleRate));
double bpm = 40.0; // The bounded BPM from above🤖 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_audio_engine.cpp` around lines 185 - 203, The test computes timing
assuming a 48kHz sample rate (sampleRate variable) before configuring or
querying the engine, which can make click detection flaky if the engine default
differs; fix by querying or setting the engine's actual sample rate (e.g., call
engine.sample_rate() or set engine.set_sample_rate(...) before computing
samplesPerClick/processCalls) and then recompute samplesPerClick and
processCalls based on that real rate so the loop that calls
engine.process_audio(...) uses correct timing for click detection.
Collaborator
Author
Merge kardo bhaiya |
Owner
|
@MohitBareja16 otherwise lgtm |
sudip-mondal-2002
added
gssoc:approved
sudip-mondal-2002
approved these changes
May 25, 2026
12 tasks
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
2 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?
Achieves 100% unit test coverage for the core DSP hot path (
audio_engine_process.cpp), the engine API (audio_engine_api.cpp), and the chain manipulation logic (audio_engine_chain.cpp). Also resolves serialization bugs related to legacy preset migration and graph reconstruction.Specifically, this adds a new test suite (
test_audio_engine.cpp) covering:SPSCQueuebehavior ensuring the GUI threads can safely push parameters, bypasses, and gain changes without stalling the audio thread.insert_effect,remove_effect,move_effect,restore_effects_state, and thetuner_tapsubsystem.IR Cabinetlegacy instantiation bug.Related Issue
Fixes #182
Type of Change
How Was This Tested?
cmake --build build && ./build/amplitron-testsgcovrtools after test execution to mathematically verify all lines in the DSPprocess_audiocallback were successfully executed during the test suite.Checklist
./amplitron-tests)std::mutex::lock()on the hot path)Screenshots / Demo
N/A - Backend coverage and logic fixes, no GUI alterations.
Summary by CodeRabbit