Test coverage#369
Merged
Merged
Conversation
…erage - Expose sdl_audio_callback as non-static to enable direct testing - Add SdlBackend lifecycle and callback tests (91% line coverage) - Add AudioBackendRegistry/Factory unit tests (95.7%/92.3% coverage) - Extend jack_mock with g_mock_jack_status for better branch coverage - Extend PortAudio and JACK integration tests for additional branches
…ralized artifacts and unified Codecov upload
… modulation effects.
…data structures to support headless builds
… preset management
…rate new preset component tests
…warnings, and clean up test formatting
…ueue integration verification
…al registration name collisions
… size, and compression enabled
…into test-coverage
…ing with expanded unit and integration tests
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:
📝 WalkthroughWalkthroughThis PR isolates the MIDI manager in per-target builds, excludes GUI font/icon code from headless builds, introduces test-only RtMidi and SDL mocks, exposes test accessors, implements effect JSON (de)serialization out-of-line, and adds many unit and integration tests across audio, MIDI, presets, graph, metronome, cabinet IR, and backend paths. ChangesTest Infrastructure and Component Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…mplement platform-specific path handling
![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: 15
🧹 Nitpick comments (7)
tests/integration/test_json_serialization.cpp (1)
765-767: ⚡ Quick winRoundtrip test should also assert
target_typeandmode.The test sets
m.target_typeandm.mode(Line 765–Line 767) but doesn’t verify them after deserialize (Line 797–Line 801). Add assertions so enum-field regression is caught.Suggested patch
ASSERT_EQ(restored.midi_mappings.size(), 1u); ASSERT_EQ(restored.midi_mappings[0].cc_number, 42); ASSERT_EQ(restored.midi_mappings[0].midi_channel, 3); + ASSERT_EQ(restored.midi_mappings[0].target_type, MidiTargetType::EffectParam); + ASSERT_EQ(restored.midi_mappings[0].mode, MidiMappingMode::Continuous); ASSERT_EQ(restored.midi_mappings[0].effect_name, "Overdrive"); ASSERT_EQ(restored.midi_mappings[0].param_name, "Drive");Also applies to: 797-801
🤖 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/integration/test_json_serialization.cpp` around lines 765 - 767, The test sets m.target_type and m.mode but never checks them after deserialization; update the assertions after the deserialize step to assert that the deserialized object's target_type equals MidiTargetType::EffectParam and its mode equals MidiMappingMode::Continuous (just like the existing check for effect_name), using the same field identifiers (target_type, mode) and the original variable m as the expected values.tests/unit/test_cabinet_sim_ir.cpp (1)
326-326: ⚡ Quick winStrengthen the wet/dry mixing assertion.
The assertion
ASSERT_NE(buf[0], 1.0f)only verifies that the buffer changed, not that it changed correctly. Consider asserting a specific expected range or pattern based on the mix value (0.5f) and the IR coefficients to validate proper wet/dry blending.🤖 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/unit/test_cabinet_sim_ir.cpp` at line 326, The current assertion ASSERT_NE(buf[0], 1.0f) only checks that the output changed; instead compute the expected mixed sample using the test's input sample and the IR coefficients with the configured mix (0.5f) — i.e., expected = dry_sample*(1.0f - 0.5f) + wet_sample*0.5f (where wet_sample is the convolution result using the IR coefficients used in the test) — and replace ASSERT_NE with ASSERT_NEAR(buf[0], expected, epsilon) or an explicit range check (e.g., expected ± 1e-5); refer to buf, the mix value 0.5f, and the IR coefficient array in this test to locate and compute the expected value.tests/integration/test_metronome.cpp (1)
1-6: ⚡ Quick winConsider using friend declarations or test accessor classes instead of macro redefinition.
Redefining
privateandprotectedaspublicvia preprocessor macros breaks encapsulation and couples tests tightly to implementation details. If the internal member names or structure changes, these tests will break.Better alternatives:
- Add a friend test fixture declaration in the header
- Create a
TestAccessorclass with friend access (similar to the pattern used intest_midi_manager.cppline 1502)- Test only the public API behavior and infer internal correctness from observable outputs
🤖 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/integration/test_metronome.cpp` around lines 1 - 6, Replace the preprocessor macro trick in tests/integration/test_metronome.cpp that redefines private/protected with a proper test accessor or friend declaration: remove the "`#define` private public" / "`#define` protected public" blocks and instead add a friend test fixture or TestAccessor class for Metronome (referencing the Metronome class in audio/engine/metronome.h) or add a friend declaration in Metronome's header so the test can access required internals, or update the tests to exercise only the public API of Metronome/AudioEngine; ensure any references to internal members in the test are moved into the TestAccessor or covered via public methods.tests/integration/test_preset_manager.cpp (4)
1631-1641: ⚡ Quick winUse RAII guard or test fixture for cleanup to prevent resource leaks on test failure.
If the test assertion at line 1637 fails, or if an exception is thrown, the
Resourcesdirectory will not be cleaned up. Useregister_temp_dir("Resources")or wrap cleanup in a RAII guard.🛡️ Suggested fix using RAII
TEST(PresetManagerDirs, AppleBundlePresetsExists) { + struct Cleanup { + ~Cleanup() { std::filesystem::remove_all("Resources"); } + } cleanup; + std::filesystem::create_directories("Resources/presets"); // Call get_bundled_presets_dir() std::string bundled = Amplitron::get_bundled_presets_dir(); // It should find and return the Resources path on Apple! ASSERT_TRUE(bundled.find("Resources/presets") != std::string::npos); - - // Cleanup - std::filesystem::remove_all("Resources"); }🤖 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/integration/test_preset_manager.cpp` around lines 1631 - 1641, The test TEST(PresetManagerDirs, AppleBundlePresetsExists) creates a "Resources" directory but performs manual cleanup that will be skipped on assertion failure; change the test to use an RAII guard or test fixture to ensure teardown (e.g., call register_temp_dir("Resources") or scope a RAII temp-dir helper) so Resources is always removed even if get_bundled_presets_dir() assertion fails; update the test body around the create_directories/ASSERT_TRUE/cleanup sequence to rely on the RAII guard instead of std::filesystem::remove_all.
1592-1592: ⚡ Quick winVerify that setting an unwritable path doesn't leave global state corrupted.
Line 1592 calls
PresetManager::set_presets_dir("/unwritable_root_path/presets_dir")but doesn't verify the call fails gracefully or reset the state afterward. Ifset_presets_dirhas side effects (logging, cached state), subsequent tests may be affected.🧹 Suggested fix to add cleanup
// 2. Exception on set_presets_dir (permission-denied path) PresetManager::set_presets_dir("/unwritable_root_path/presets_dir"); + // Reset to avoid side effects in subsequent tests + PresetManager::set_presets_dir("");🤖 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/integration/test_preset_manager.cpp` at line 1592, When calling PresetManager::set_presets_dir("/unwritable_root_path/presets_dir") ensure the test captures and asserts the failure (or return/exception) and then restores the previous global state so other tests aren't impacted: read and save the current presets dir before the call (using PresetManager::get_presets_dir or equivalent), call set_presets_dir and assert it failed or threw the expected error, and finally reset the presets dir back to the saved value (or use teardown/RAII) and verify the reset succeeded; reference PresetManager::set_presets_dir and PresetManager::get_presets_dir in your changes.
17-22: ⚡ Quick winConsider using friend declarations or test accessor classes instead of macro redefinition.
Redefining
privateandprotectedaspublicbreaks encapsulation and couples tests to implementation details. Consider using friend declarations, test accessor classes, or testing only the public API.🤖 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/integration/test_preset_manager.cpp` around lines 17 - 22, Tests currently redefine private/protected to public around includes, breaking encapsulation; remove those macro redefinitions and instead expose needed internals via a targeted test accessor or friend declaration. Replace the macro block that wraps "preset_manager.h" and "presets/preset_manager_impl.h" with either: 1) add a test-only friend declaration in the class (e.g., add "friend class PresetManagerTestAccessor;" to PresetManagerImpl or PresetManager), or 2) create a small test accessor class (PresetManagerTestAccessor) in a test-only header that calls internal helpers/getters you add to PresetManagerImpl (e.g., getInternalState(), accessCache()) and include that header in the test, or 3) refactor the test to exercise only the public API of PresetManager; ensure the unique types PresetManagerImpl and PresetManager are the targets of the friend/accessor changes so tests can access required internals without macro hacks.
1645-1658: ⚡ Quick winDirect static member access couples tests to internal implementation.
Lines 1645 and 1655 directly assign to
PresetManager::custom_presets_dir_, bypassing the public API (set_presets_dir). This was enabled by the#define private publicmacro at line 17-22. If the internal member name or type changes, this test will break.🤖 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/integration/test_preset_manager.cpp` around lines 1645 - 1658, The test directly mutates the private static PresetManager::custom_presets_dir_ (and relies on a private->public macro), which couples the test to implementation; change the test to use the public API PresetManager::set_presets_dir(...) to set and clear the custom presets directory and continue to read values via PresetManager::get_presets_dir(), and remove/avoid use of the private-to-public macro—also ensure the ScopedEnvVar usage remains to simulate environment fallbacks and reset the presets via set_presets_dir("") when testing the fallback path.
🤖 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 `@src/midi/midi_manager.cpp`:
- Line 45: The cancelCallback() method currently has two statements on one line
causing a clang-format violation; split them onto separate lines inside the
function so each assignment is its own statement (i.e., in cancelCallback() set
g_mock_rtmidi_callback = nullptr; on its own line and g_mock_rtmidi_user_data =
nullptr; on the next line) to satisfy formatting rules.
In `@tests/integration/test_audio_engine.cpp`:
- Around line 501-521: Reformat the ResetTrackingMockTunerEffect class to
satisfy clang-format: run clang-format on the block defining class
ResetTrackingMockTunerEffect (including its member variables reset_called,
sample_rate_received and all overridden methods process, reset, set_sample_rate,
name, params) and apply the resulting spacing/indentation changes so lines
around the class (previously flagged at the declarations and method definitions)
conform to the project style.
- Around line 523-535: The TunerTapSampleRateAndReset test has clang-format
style issues; run clang-format (or apply project formatting rules) on the test
block for TEST_F(AudioEngineTest, TunerTapSampleRateAndReset) so that spacing
and line breaks around the ResetTrackingMockTunerEffect creation,
engine.set_tuner_tap(tap), engine.set_sample_rate(44100) /
ASSERT_EQ(tap->sample_rate_received, 44100), ASSERT_TRUE(tap->reset_called), and
engine.clear_tuner_tap() conform to the repo style; specifically reformat the
lines referencing ResetTrackingMockTunerEffect, engine.set_tuner_tap,
engine.set_sample_rate, ASSERT_EQ and ASSERT_TRUE to eliminate the reported
clang-format violations.
- Around line 537-548: The test
"ProcessAudioResizesInternalBuffersWhenFrameCountExceedsCapacity" has
clang-format violations; run clang-format on
tests/integration/test_audio_engine.cpp or reformat the function (the TEST_F
block and its lines that create large_frame_count, vectors in/out, the
engine.process_audio call, and the ASSERT_GE assertions) so spacing and
indentation match project style; ensure the vector initializers, function call,
and ASSERT_GE lines are formatted correctly and then re-run the tests/lint to
confirm the clang-format errors on those lines are resolved.
In `@tests/integration/test_command_graph.cpp`:
- Around line 227-344: The new test blocks in
tests/integration/test_command_graph.cpp and
tests/integration/test_json_serialization.cpp are not clang-formatted (causing
-Wclang-format-violations); run clang-format on those files (or reformat the
added test code) to fix whitespace/formatting around the test code that uses
symbols like AddGraphNodeCommand, RemoveGraphNodeCommand, AddGraphLinkCommand,
RemoveGraphLinkCommand, MoveGraphNodeCommand and the associated assertions so
the CI clang-format check passes; commit the formatted files.
- Around line 251-255: The test caches raw DSPNode* pointers (splitter, mixer,
node1, node2, node3) returned by AudioGraph::find_node(), but AudioGraph stores
nodes in a std::vector<DSPNode> (nodes_) and remove_node()/restore_node() can
move/erase elements making those pointers dangling; instead store each node's
unique ID (or index/identifier exposed by DSPNode) after initial lookup and call
AudioGraph::find_node(id) to re-fetch the current const DSPNode* immediately
before any assertion or use after remove/undo/redo operations (update all uses
of splitter, mixer, node1, node2, node3 in tests to re-query via find_node()).
In `@tests/integration/test_metronome.cpp`:
- Around line 59-128: The test file TEST(Metronome_DirectUnitTests) has
clang-format style violations; fix them by running the project formatter on this
file. Run clang-format -i on tests/integration/test_metronome.cpp (or invoke
your repo's formatting tool) to reformat the TEST(Metronome_DirectUnitTests)
block and its lines referencing met.set_bpm, met.set_volume,
met.set_sample_rate, met.next_sample, and the
met.metronome_sample_counter_/met.metronome_samples_per_beat_ assertions so the
file conforms to the repository's style rules.
In `@tests/integration/test_midi_manager.cpp`:
- Around line 1598-1646: The test file has clang-format style violations in the
midi_manager_error_handling_edge_cases test; run the formatter to fix
whitespace/formatting issues by executing clang-format -i on
tests/integration/test_midi_manager.cpp (or apply your project's clang-format
settings) so the TEST(midi_manager_error_handling_edge_cases) block and related
braces/indentation conform to the repository style; verify the test compiles and
that symbols MidiManager, initialize, get_available_ports, open_port, and
close_port remain unchanged by formatting.
- Around line 1538-1596: Fix formatting violations in the test by running your
formatter on the file that contains TEST(midi_manager_mock_init_and_port_ops);
specifically reformat the extern declarations
(g_mock_rtmidi_should_fail_enumerate, g_mock_rtmidi_should_fail_open,
g_mock_rtmidi_callback, g_mock_rtmidi_user_data, g_mock_rtmidi_port_open) and
the TEST body so the code passes clang-format checks, then commit the
reformatted file; the test exercises MidiManager::initialize, open_port, and
close_port so ensure those blocks are properly indented and spaced.
In `@tests/integration/test_preset_manager.cpp`:
- Around line 1566-1678: This file has clang-format style violations; run the
auto-formatter (clang-format -i tests/integration/test_preset_manager.cpp) to
fix spacing and alignment issues around the tests (e.g., TEST_F(PresetTest,
AdvancedPresetManagerEdgeCases), TEST(PresetManagerDirs,
AppleBundlePresetsExists), TEST(PresetManagerDirs,
GetPresetsDirUnwritableFallback), TEST(PresetManagerIO,
SaveFactoryPresetsUnreadableSource) and usages of
PresetManager::set_presets_dir(), PresetManager::get_presets_dir(),
PresetManager::save_config(), PresetManager::load_config()) so the file conforms
to the project's formatting rules.
- Around line 1569-1571: The test creates an invalid Windows path via test_dir
("presets/escape_\"test\"_\\dir\\") which makes
std::filesystem::create_directories throw on Windows; update the test in
tests/integration/test_preset_manager.cpp (variable test_dir and its use with
create_directories and register_temp_dir) to use a platform-safe directory name
(e.g., remove the double-quote and backslash characters or substitute them with
underscores) or guard the assignment with a platform check so Windows uses a
valid name; ensure the updated test_dir value is used everywhere
create_directories and register_temp_dir are called.
In `@tests/unit/test_audio_backend_sdl.cpp`:
- Around line 265-336: The TEST function
SdlBackend_FailurePathsAndDeviceSelection has clang-format violations; run
clang-format on tests/unit/test_audio_backend_sdl.cpp (or format just the
TEST(SdlBackend_FailurePathsAndDeviceSelection) block) to fix
spacing/indentation and line breaks around the test body and initializer lists
(references: TEST(SdlBackend_FailurePathsAndDeviceSelection),
g_mock_sdl_init_subsystem_result, g_mock_sdl_device_names,
g_mock_sdl_last_opened_device, g_mock_sdl_open_audio_device_fail_out/cap);
ensure the braces, indentation, and list initializers conform to the repo's
clang-format rules and re-run the test pipeline.
In `@tests/unit/test_cabinet_sim_ir.cpp`:
- Around line 313-369: The test contains clang-format style violations; run
clang-format on the file and reformat the TEST function
CabinetSim_IR_AdvancedSwapAndMismatch so spacing and indentation around
statements like ASSERT_TRUE(write_wav_mono_pcm16(...)), method calls on the
CabinetSim instance (set_sample_rate, load_ir, set_enabled, set_mix, process,
clear_ir, has_ir), and vector initializers are consistent with the project
style; after auto-formatting, re-run the unit tests to ensure no behavioral
changes.
In `@tests/unit/test_effect_base.cpp`:
- Around line 165-191: The test file contains clang-format style violations in
the newly added tests (effects_metadata_and_type_id, TestFallbackEffect class,
and effect_base_type_id_fallback test) causing pipeline failures; run
clang-format over the file or reformat the affected blocks (the
TEST(effects_metadata_and_type_id) loop, the TestFallbackEffect class
definition, and the TEST(effect_base_type_id_fallback) body) to match project
style (spacing, braces, and indentation) so that TestFallbackEffect, its methods
(process, reset, name, params), and the assertions (fx.type_id() comparison) are
properly formatted; re-run clang-format and commit the fixed formatting.
In `@tests/unit/test_mock_backend.cpp`:
- Around line 197-259: Run clang-format on the test file and fix formatting in
the SimpleMockEffect class and the
TEST(AudioEngine_SetBufferSizeAndSampleRateReversion) block so the code conforms
to project style (fix spacing, brace placement, and line breaks around methods
and test steps); specifically reformat the SimpleMockEffect methods (name,
params overloads) and the test sections that manipulate engine
(set_buffer_size/set_sample_rate/start/stop/add_effect/set_tuner_tap/clear_backend_for_test/shutdown)
to eliminate the clang-format violations reported around those lines. Ensure you
run clang-format with the repo's config so no other style issues remain.
---
Nitpick comments:
In `@tests/integration/test_json_serialization.cpp`:
- Around line 765-767: The test sets m.target_type and m.mode but never checks
them after deserialization; update the assertions after the deserialize step to
assert that the deserialized object's target_type equals
MidiTargetType::EffectParam and its mode equals MidiMappingMode::Continuous
(just like the existing check for effect_name), using the same field identifiers
(target_type, mode) and the original variable m as the expected values.
In `@tests/integration/test_metronome.cpp`:
- Around line 1-6: Replace the preprocessor macro trick in
tests/integration/test_metronome.cpp that redefines private/protected with a
proper test accessor or friend declaration: remove the "`#define` private public"
/ "`#define` protected public" blocks and instead add a friend test fixture or
TestAccessor class for Metronome (referencing the Metronome class in
audio/engine/metronome.h) or add a friend declaration in Metronome's header so
the test can access required internals, or update the tests to exercise only the
public API of Metronome/AudioEngine; ensure any references to internal members
in the test are moved into the TestAccessor or covered via public methods.
In `@tests/integration/test_preset_manager.cpp`:
- Around line 1631-1641: The test TEST(PresetManagerDirs,
AppleBundlePresetsExists) creates a "Resources" directory but performs manual
cleanup that will be skipped on assertion failure; change the test to use an
RAII guard or test fixture to ensure teardown (e.g., call
register_temp_dir("Resources") or scope a RAII temp-dir helper) so Resources is
always removed even if get_bundled_presets_dir() assertion fails; update the
test body around the create_directories/ASSERT_TRUE/cleanup sequence to rely on
the RAII guard instead of std::filesystem::remove_all.
- Line 1592: When calling
PresetManager::set_presets_dir("/unwritable_root_path/presets_dir") ensure the
test captures and asserts the failure (or return/exception) and then restores
the previous global state so other tests aren't impacted: read and save the
current presets dir before the call (using PresetManager::get_presets_dir or
equivalent), call set_presets_dir and assert it failed or threw the expected
error, and finally reset the presets dir back to the saved value (or use
teardown/RAII) and verify the reset succeeded; reference
PresetManager::set_presets_dir and PresetManager::get_presets_dir in your
changes.
- Around line 17-22: Tests currently redefine private/protected to public around
includes, breaking encapsulation; remove those macro redefinitions and instead
expose needed internals via a targeted test accessor or friend declaration.
Replace the macro block that wraps "preset_manager.h" and
"presets/preset_manager_impl.h" with either: 1) add a test-only friend
declaration in the class (e.g., add "friend class PresetManagerTestAccessor;" to
PresetManagerImpl or PresetManager), or 2) create a small test accessor class
(PresetManagerTestAccessor) in a test-only header that calls internal
helpers/getters you add to PresetManagerImpl (e.g., getInternalState(),
accessCache()) and include that header in the test, or 3) refactor the test to
exercise only the public API of PresetManager; ensure the unique types
PresetManagerImpl and PresetManager are the targets of the friend/accessor
changes so tests can access required internals without macro hacks.
- Around line 1645-1658: The test directly mutates the private static
PresetManager::custom_presets_dir_ (and relies on a private->public macro),
which couples the test to implementation; change the test to use the public API
PresetManager::set_presets_dir(...) to set and clear the custom presets
directory and continue to read values via PresetManager::get_presets_dir(), and
remove/avoid use of the private-to-public macro—also ensure the ScopedEnvVar
usage remains to simulate environment fallbacks and reset the presets via
set_presets_dir("") when testing the fallback path.
In `@tests/unit/test_cabinet_sim_ir.cpp`:
- Line 326: The current assertion ASSERT_NE(buf[0], 1.0f) only checks that the
output changed; instead compute the expected mixed sample using the test's input
sample and the IR coefficients with the configured mix (0.5f) — i.e., expected =
dry_sample*(1.0f - 0.5f) + wet_sample*0.5f (where wet_sample is the convolution
result using the IR coefficients used in the test) — and replace ASSERT_NE with
ASSERT_NEAR(buf[0], expected, epsilon) or an explicit range check (e.g.,
expected ± 1e-5); refer to buf, the mix value 0.5f, and the IR coefficient array
in this test to locate and compute the expected value.
🪄 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: 207069fb-7ab0-4184-a3e2-60e5f5ec5eec
📒 Files selected for processing (16)
CMakeLists.txtsrc/audio/backend/sdl_backend.cppsrc/audio/engine/audio_engine.cppsrc/gui/window_context.cppsrc/gui/window_context.hsrc/midi/midi_manager.cpptests/integration/test_audio_engine.cpptests/integration/test_command_graph.cpptests/integration/test_json_serialization.cpptests/integration/test_metronome.cpptests/integration/test_midi_manager.cpptests/integration/test_preset_manager.cpptests/unit/test_audio_backend_sdl.cpptests/unit/test_cabinet_sim_ir.cpptests/unit/test_effect_base.cpptests/unit/test_mock_backend.cpp
- Force FetchContent for nlohmann/json instead of finding miniconda version
- Decouple spectrum_analyzer.h from audio_engine.h to break complex->sstream include chain
- Replace dangerous `#define private public` hack with clean test accessors in AudioEngine and AudioGraphExecutor
- Add explicit nlohmann/json.hpp includes in test files
- Fix unused return value warnings for system("rm -rf...") by switching to std::filesystem
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/test_audio_engine.cpp (1)
1-1:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClang-format is still failing for this file in CI.
The pipeline reports a formatting failure at the top of this file. Please run
clang-formatontests/integration/test_audio_engine.cppto unblock checks.🤖 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/integration/test_audio_engine.cpp` at line 1, Clang-format failure: run clang-format on the file to fix formatting issues at the top of tests/integration/test_audio_engine.cpp (the affected token is the include line `#include <chrono>` shown in the diff); apply your project's clang-format configuration (or run `clang-format -i tests/integration/test_audio_engine.cpp`) and commit the reformatted file so CI formatting checks pass.Source: Pipeline failures
🧹 Nitpick comments (2)
CMakeLists.txt (1)
63-72: Consider bumping the pinned nlohmann/json from v3.11.3 to v3.12.0 (latest stable, released 2025-04-11).Current pin is ~4+ years behind the latest release; v3.11.3 doesn’t show as having unaddressed security issues, but upgrading to v3.12.0 is recommended to pick up security/bugfixes and improvements from the intervening releases.
🤖 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 `@CMakeLists.txt` around lines 63 - 72, Update the pinned nlohmann/json version in the FetchContent block by changing the GIT_TAG value used in FetchContent_Declare for nlohmann_json from v3.11.3 to v3.12.0 (i.e., update the GIT_TAG line in the nlohmann_json FetchContent_Declare); keep the existing JSON_BuildTests and JSON_Install cache settings and leave FetchContent_MakeAvailable(nlohmann_json) as-is.tests/integration/test_audio_engine.cpp (1)
479-479: ⚡ Quick winAssert shadow executor is initialized before dereference.
This test dereferences
engine.test_audio_shadow_executor()directly; add an explicit null assertion first so failures are diagnosable instead of crashing.Proposed patch
- for (const auto& step : engine.test_audio_shadow_executor()->test_execution_plan()) { + auto shadow_executor = engine.test_audio_shadow_executor(); + ASSERT_NE(shadow_executor, nullptr); + for (const auto& step : shadow_executor->test_execution_plan()) {🤖 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/integration/test_audio_engine.cpp` at line 479, The test dereferences engine.test_audio_shadow_executor() without checking for null; add an explicit assertion (e.g., ASSERT_NE(nullptr, engine.test_audio_shadow_executor()) or ASSERT_TRUE(engine.test_audio_shadow_executor()) depending on test framework) immediately before the for loop that iterates over engine.test_audio_shadow_executor()->test_execution_plan() so failures produce a readable test assertion instead of a crash.
🤖 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 `@CMakeLists.txt`:
- Line 61: The global suppression "set(CMAKE_WARN_DEPRECATED OFF CACHE BOOL ""
FORCE)" should be removed and the deprecation warning suppression must be scoped
only to the nlohmann/json FetchContent step: delete that global CACHE FORCE
line, and instead set CMAKE_WARN_DEPRECATED OFF only inside the FetchContent
block that declares/uses nlohmann/json (e.g., immediately before calling
FetchContent_MakeAvailable or in a cmake scope that surrounds the nlohmann/json
fetch) and restore the previous value after the FetchContent call; keep all
other targets (including GTest) unaffected by ensuring the change is not cached
globally.
---
Outside diff comments:
In `@tests/integration/test_audio_engine.cpp`:
- Line 1: Clang-format failure: run clang-format on the file to fix formatting
issues at the top of tests/integration/test_audio_engine.cpp (the affected token
is the include line `#include <chrono>` shown in the diff); apply your project's
clang-format configuration (or run `clang-format -i
tests/integration/test_audio_engine.cpp`) and commit the reformatted file so CI
formatting checks pass.
---
Nitpick comments:
In `@CMakeLists.txt`:
- Around line 63-72: Update the pinned nlohmann/json version in the FetchContent
block by changing the GIT_TAG value used in FetchContent_Declare for
nlohmann_json from v3.11.3 to v3.12.0 (i.e., update the GIT_TAG line in the
nlohmann_json FetchContent_Declare); keep the existing JSON_BuildTests and
JSON_Install cache settings and leave FetchContent_MakeAvailable(nlohmann_json)
as-is.
In `@tests/integration/test_audio_engine.cpp`:
- Line 479: The test dereferences engine.test_audio_shadow_executor() without
checking for null; add an explicit assertion (e.g., ASSERT_NE(nullptr,
engine.test_audio_shadow_executor()) or
ASSERT_TRUE(engine.test_audio_shadow_executor()) depending on test framework)
immediately before the for loop that iterates over
engine.test_audio_shadow_executor()->test_execution_plan() so failures produce a
readable test assertion instead of a crash.
🪄 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: 08d48dd1-a240-43cb-aacd-da9db461b0cf
📒 Files selected for processing (8)
CMakeLists.txtsrc/audio/engine/audio_engine.hsrc/audio/engine/audio_graph_executor.htests/integration/test_audio_engine.cpptests/integration/test_command_graph.cpptests/integration/test_preset_manager.cpptests/unit/test_effect_base.cpptests/unit/test_mock_backend.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/unit/test_effect_base.cpp
- tests/integration/test_command_graph.cpp
- tests/integration/test_preset_manager.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/audio/effects/core/effect.cpp (1)
3-39:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRun clang-format on touched C++ files; CI is currently blocked by style check.
src/audio/effects/core/effect.cpp#L3-L39: apply project clang-format to the new JSON serialization block and includes.tests/test_fixtures.h#L15-L18: apply project clang-format after the include/macro reordering block.🤖 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 `@src/audio/effects/core/effect.cpp` around lines 3 - 39, Run the project's clang-format on the modified ranges to satisfy style checks: in src/audio/effects/core/effect.cpp (lines 3-39) reformat the includes and the JSON serialization block around Effect::clone, Effect::get_params and Effect::set_params so spacing, brace placement, and indentation match project style; in tests/test_fixtures.h (lines 15-18) reformat the include/macro reordering block so it conforms to the same clang-format rules. Ensure no code changes beyond whitespace/formatting.Source: Pipeline failures
🤖 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 `@src/audio/effects/core/effect.cpp`:
- Around line 29-37: Effect::set_params currently writes mix_ and parameter
values directly (mix_.store(...) and p.value = ...), bypassing bounds checking;
change it to validate/clamp before storing: clamp mix_ to its valid range (e.g.
std::clamp(value, 0.0f, 1.0f)) instead of storing raw JSON, and for each param
from params() avoid assigning to p.value directly — either call the parameter's
validated setter (e.g., p.set_value(...) or p.from_json(...), if such exists) or
clamp the incoming value to p.min/p.max (or other existing bounds fields) and
then store the clamped value; update Effect::set_params to use these guarded
updates so deserialized preset JSON cannot insert out-of-range values.
---
Outside diff comments:
In `@src/audio/effects/core/effect.cpp`:
- Around line 3-39: Run the project's clang-format on the modified ranges to
satisfy style checks: in src/audio/effects/core/effect.cpp (lines 3-39) reformat
the includes and the JSON serialization block around Effect::clone,
Effect::get_params and Effect::set_params so spacing, brace placement, and
indentation match project style; in tests/test_fixtures.h (lines 15-18) reformat
the include/macro reordering block so it conforms to the same clang-format
rules. Ensure no code changes beyond whitespace/formatting.
🪄 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: 3882c1dd-50c7-4eeb-87ac-5757aa01c565
📒 Files selected for processing (9)
src/audio/effects/core/effect.cppsrc/audio/effects/core/effect.hsrc/audio/effects/core/i_serializable.hsrc/audio/engine/i_audio_engine.hsrc/audio/engine/metronome.hsrc/presets/preset_json.cppsrc/presets/preset_json.htests/integration/test_metronome.cpptests/test_fixtures.h
✅ Files skipped from review due to trivial changes (2)
- src/audio/effects/core/i_serializable.h
- src/presets/preset_json.cpp
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/test_preset_manager.cpp (1)
1598-1598:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd assertion to verify permission-denied path was rejected.
After calling
set_presets_dirwith an unwritable path, the test should verify thatcustom_presets_dir_was not set, confirming the implementation correctly rejects invalid directories.✅ Proposed assertion
// 2. Exception on set_presets_dir (permission-denied path) PresetManager::set_presets_dir("/unwritable_root_path/presets_dir"); + ASSERT_EQ(PresetManager::custom_presets_dir_, std::string(""));🤖 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/integration/test_preset_manager.cpp` at line 1598, After calling PresetManager::set_presets_dir("/unwritable_root_path/presets_dir") add an assertion that the custom presets directory was not accepted: verify PresetManager::custom_presets_dir_ (or the corresponding getter, e.g. get_custom_presets_dir()) is still empty/unchanged (e.g. EXPECT_TRUE(preset_manager.custom_presets_dir_.empty()) or EXPECT_EQ(preset_manager.get_custom_presets_dir(), "")) to confirm the permission-denied path was rejected.
🧹 Nitpick comments (2)
tests/integration/test_preset_manager.cpp (2)
1665-1685: 💤 Low valueConsider saving and restoring
custom_presets_dir_for better test isolation.The test directly modifies the static
custom_presets_dir_member. Although it clears to""at line 1682, capturing and restoring the original value would improve test hygiene and prevent state leakage if the test is extended or refactored.🤖 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/integration/test_preset_manager.cpp` around lines 1665 - 1685, The test TEST(PresetManagerDirs, GetPresetsDirUnwritableFallback) mutates the static PresetManager::custom_presets_dir_ without restoring its original value if the test changes; to fix, capture the original value at the start (auto orig = PresetManager::custom_presets_dir_;), perform the test modifications, and ensure you restore it at the end (PresetManager::custom_presets_dir_ = orig;), or use an RAII/local guard to restore on scope exit so the static is always returned to its prior value even on early returns or failures.
1661-1661: ⚡ Quick winUse error_code to suppress cleanup exceptions.
The cleanup unconditionally removes the Resources parent directory and could throw if it fails. Use
std::error_codeto suppress exceptions, as done in other tests (e.g., lines 288, 1143).♻️ Proposed fix
// Cleanup - std::filesystem::remove_all(std::filesystem::path(target_dir).parent_path()); + std::error_code ec; + std::filesystem::remove_all(std::filesystem::path(target_dir).parent_path(), ec);🤖 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/integration/test_preset_manager.cpp` at line 1661, The cleanup call currently does std::filesystem::remove_all(std::filesystem::path(target_dir).parent_path()) which can throw; change it to use the noexcept overload with std::error_code — e.g. declare std::error_code ec; call std::filesystem::remove_all(std::filesystem::path(target_dir).parent_path(), ec); and ignore ec (or optionally log it) so cleanup won't throw; update the same statement in tests/integration/test_preset_manager.cpp accordingly.
🤖 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.
Outside diff comments:
In `@tests/integration/test_preset_manager.cpp`:
- Line 1598: After calling
PresetManager::set_presets_dir("/unwritable_root_path/presets_dir") add an
assertion that the custom presets directory was not accepted: verify
PresetManager::custom_presets_dir_ (or the corresponding getter, e.g.
get_custom_presets_dir()) is still empty/unchanged (e.g.
EXPECT_TRUE(preset_manager.custom_presets_dir_.empty()) or
EXPECT_EQ(preset_manager.get_custom_presets_dir(), "")) to confirm the
permission-denied path was rejected.
---
Nitpick comments:
In `@tests/integration/test_preset_manager.cpp`:
- Around line 1665-1685: The test TEST(PresetManagerDirs,
GetPresetsDirUnwritableFallback) mutates the static
PresetManager::custom_presets_dir_ without restoring its original value if the
test changes; to fix, capture the original value at the start (auto orig =
PresetManager::custom_presets_dir_;), perform the test modifications, and ensure
you restore it at the end (PresetManager::custom_presets_dir_ = orig;), or use
an RAII/local guard to restore on scope exit so the static is always returned to
its prior value even on early returns or failures.
- Line 1661: The cleanup call currently does
std::filesystem::remove_all(std::filesystem::path(target_dir).parent_path())
which can throw; change it to use the noexcept overload with std::error_code —
e.g. declare std::error_code ec; call
std::filesystem::remove_all(std::filesystem::path(target_dir).parent_path(),
ec); and ignore ec (or optionally log it) so cleanup won't throw; update the
same statement in tests/integration/test_preset_manager.cpp accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 45ce9d16-fb32-4f16-bdfc-13099b903693
📒 Files selected for processing (8)
CMakeLists.txtsrc/audio/effects/core/effect.cppsrc/presets/preset_json.cpptests/integration/test_audio_engine.cpptests/integration/test_command_graph.cpptests/integration/test_preset_manager.cpptests/test_fixtures.htests/unit/test_effect_base.cpp
✅ Files skipped from review due to trivial changes (1)
- src/presets/preset_json.cpp
🚧 Files skipped from review as they are similar to previous changes (6)
- src/audio/effects/core/effect.cpp
- tests/integration/test_command_graph.cpp
- tests/integration/test_audio_engine.cpp
- tests/unit/test_effect_base.cpp
- tests/test_fixtures.h
- CMakeLists.txt
Contributor
PR Preview RemovedThe GitHub Pages preview for this PR has been removed because the PR was closed. |
sudip-mondal-2002
added
gssoc:approved
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.
Summary by CodeRabbit
Chores
New Features
Tests
Bug Fixes