Test coverage#366
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
Contributor
📝 WalkthroughWalkthroughAdds many unit/integration/UI tests, native dialog and audio mocks, exposes SDL callback for testing, and updates test-runner, CMake, and CI to emit and collect JUnit XML test results. ChangesComprehensive Test & CI Update
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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 |
Contributor
PR Preview RemovedThe GitHub Pages preview for this PR has been removed because the PR was closed. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![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: 16
🧹 Nitpick comments (9)
src/audio/backend/sdl_backend.cpp (1)
14-14: ⚡ Quick winDocument test-only callback exposure.
Removing
staticenables unit testing but exposes an implementation detail without a corresponding header declaration. Consider adding a comment documenting that this function is intentionally exposed for unit testing purposes, or alternatively, declare it in a header (e.g.,sdl_backend.hwith a test-utilities comment, or a separate test header).📝 Suggested documentation comment
+// Note: External linkage (non-static) for unit test access. +// Not intended as a public API; tests may forward-declare. void sdl_audio_callback(void* userdata, Uint8* stream, int len) {🤖 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/backend/sdl_backend.cpp` at line 14, The function sdl_audio_callback was made non-static to allow unit testing, but that exposes an implementation symbol without documentation or a header declaration; add a clear comment above sdl_audio_callback stating it is intentionally non-static for unit tests (or alternatively declare it in a header such as sdl_backend.h or a dedicated test header with a "for testing" note) so callers/readers know this exposure is deliberate and tracked.tests/unit/test_audio_backend_sdl.cpp (1)
61-68: ⚡ Quick winAdd test coverage for null engine path.
The callback includes a null engine check (
if (!engine) return;in sdl_backend.cpp:17), but this defensive path is not tested. Consider adding a test case that invokes the callback with a backend that hasengine_ = nullptrto validate this early-return 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/unit/test_audio_backend_sdl.cpp` around lines 61 - 68, Add a unit test that exercises the null-engine early return in sdl_audio_callback by creating or reusing the Amplitron::Backend instance used in the test, explicitly setting its engine_ member to nullptr (or constructing it with no engine), then calling Amplitron::sdl_audio_callback(&backend, reinterpret_cast<Uint8*>(output_buffer.data()), bytes_len) and asserting the call returns cleanly and the output_buffer remains unchanged (or that no data was written). Target the existing test file (tests/unit/test_audio_backend_sdl.cpp) and reference the sdl_audio_callback function and the backend.engine_ field to locate where to add this null-engine case.tests/unit/test_effects_modulation.cpp (4)
305-306: ⚡ Quick winConsider asserting pass-through behavior when disabled.
The test calls
process_stereoafter disabling the effect but doesn't verify that the buffers remain unchanged. While this exercises the early-return path (confirmed by context snippet), adding assertions would strengthen the test.✅ Suggested enhancement
+ float left_copy[BUFFER_SIZE], right_copy[BUFFER_SIZE]; + std::memcpy(left_copy, left, sizeof(left)); + std::memcpy(right_copy, right, sizeof(right)); + ch.set_enabled(false); ch.process_stereo(left, right, BUFFER_SIZE); + + for (int i = 0; i < BUFFER_SIZE; ++i) { + ASSERT_EQ(left[i], left_copy[i]); + ASSERT_EQ(right[i], right_copy[i]); + }🤖 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_effects_modulation.cpp` around lines 305 - 306, The test disables the effect with ch.set_enabled(false) then calls ch.process_stereo(left, right, BUFFER_SIZE) but doesn't assert that the buffers were left unchanged; update the test to copy the original left and right buffers (e.g., saved_left, saved_right) before calling ch.process_stereo and then assert equality between saved_left and left and between saved_right and right after the call to verify pass-through behavior when disabled (reference the ch.set_enabled and ch.process_stereo calls and the left/right buffers).
340-341: ⚡ Quick winConsider asserting pass-through behavior when disabled.
Similar to the
chorus_process_stereotest, this test callsprocess_stereoafter disabling but doesn't verify buffer contents remain unchanged. Adding assertions would provide better coverage of the disabled state.✅ Suggested enhancement
+ float left_copy[BUFFER_SIZE], right_copy[BUFFER_SIZE]; + std::memcpy(left_copy, left, sizeof(left)); + std::memcpy(right_copy, right, sizeof(right)); + fl.set_enabled(false); fl.process_stereo(left, right, BUFFER_SIZE); + + for (int i = 0; i < BUFFER_SIZE; ++i) { + ASSERT_EQ(left[i], left_copy[i]); + ASSERT_EQ(right[i], right_copy[i]); + }🤖 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_effects_modulation.cpp` around lines 340 - 341, The test disables the Flanger via fl.set_enabled(false) and calls fl.process_stereo(left, right, BUFFER_SIZE) but doesn't assert that the buffers are unchanged; add assertions that left and right match the original input buffers after processing to verify pass-through behavior when disabled (similar to the chorus_process_stereo test), using the same input arrays/variables you used before calling process_stereo to compare against.
361-362: ⚡ Quick winConsider asserting pass-through behavior when disabled.
Consistent with the other stereo processing tests, this test would benefit from asserting that buffers remain unchanged when the effect is disabled.
✅ Suggested enhancement
+ float left_copy[BUFFER_SIZE], right_copy[BUFFER_SIZE]; + std::memcpy(left_copy, left, sizeof(left)); + std::memcpy(right_copy, right, sizeof(right)); + ph.set_enabled(false); ph.process_stereo(left, right, BUFFER_SIZE); + + for (int i = 0; i < BUFFER_SIZE; ++i) { + ASSERT_EQ(left[i], left_copy[i]); + ASSERT_EQ(right[i], right_copy[i]); + }🤖 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_effects_modulation.cpp` around lines 361 - 362, Add an assertion that process_stereo is a no-op when disabled: before calling ph.set_enabled(false) / ph.process_stereo(left, right, BUFFER_SIZE) make copies of the original left and right buffers, then after ph.process_stereo assert the processed left and right equal the originals (e.g., with EXPECT_EQ/ASSERT_EQ or array comparison) to verify pass-through behavior for the object under test (ph).
309-321: ⚡ Quick winConsider verifying that invalid inputs don't modify state.
The test exercises edge cases (negative BPM, NAN, repeated values) but doesn't assert expected behavior. While crash-testing is valuable, verifying that
params()[0].value(rate parameter) remains unchanged after invalid calls would strengthen the test.✅ Suggested enhancement
Chorus ch; ch.set_sample_rate(SR); ch.reset(); + + float initial_rate = ch.params()[0].value; // Invalid BPM values ch.set_transport_state(-10.0f); + ASSERT_EQ(ch.params()[0].value, initial_rate); + ch.set_transport_state(NAN); + ASSERT_EQ(ch.params()[0].value, initial_rate); // Repeated BPM values ch.set_transport_state(120.0f); + float rate_after_first = ch.params()[0].value; ch.set_transport_state(120.0f); + ASSERT_EQ(ch.params()[0].value, rate_after_first);🤖 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_effects_modulation.cpp` around lines 309 - 321, The test currently calls Chorus::set_transport_state with invalid and repeated BPMs but lacks assertions; capture the initial rate via auto initial = ch.params()[0].value (or read the rate param before the invalid calls), then after each invalid call (ch.set_transport_state(-10.0f); ch.set_transport_state(NAN);) assert that ch.params()[0].value == initial (or nearly equal if float) to ensure state wasn't modified, and after calling the same valid BPM twice (ch.set_transport_state(120.0f); ch.set_transport_state(120.0f);) assert that params()[0].value remains stable (no unintended change) possibly using an epsilon for float comparisons.tests/integration/test_audio_backend_portaudio.cpp (1)
563-565: 💤 Low valueConsider adding assertions for the auto-detection behavior.
After forcing
Pa_GetHostApiInfoto returnnullptrand callinginitialize(), the test doesn't verify any specific behavior. Consider asserting the expected device indices or checking that the engine doesn't crash when host API information is unavailable.🤖 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_backend_portaudio.cpp` around lines 563 - 565, After forcing g_mock_pa_get_host_api_info to return nullptr and calling engine.shutdown() then engine.initialize(), add assertions to verify auto-detection behavior: assert that initialize() completed without throwing (or engine.isInitialized() is true), and assert expected device index state (e.g., engine's input/output device indices are the sentinel value or unchanged from pre-shutdown). Use the existing engine API (e.g., engine.initialize(), engine.isInitialized(), engine.getInputDeviceIndex()/getOutputDeviceIndex() or the equivalent accessors) to check no crash occurred and device indices reflect the fallback/unavailable-host-API behavior.tests/integration/test_audio_backend_jack.cpp (1)
134-138: 💤 Low valueConsider using a more realistic subset of status flags.
Setting all status flags simultaneously, including contradictory pairs like
JackServerStarted(success) andJackServerFailed(failure), doesn't reflect real-world JACK behavior. Consider testing with a representative subset of mutually compatible error flags instead.🤖 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_backend_jack.cpp` around lines 134 - 138, The test currently sets g_mock_jack_status to every JACK flag at once, which mixes contradictory states (e.g., JackServerStarted with JackServerFailed) and is unrealistic; change the assignment of g_mock_jack_status to a representative, mutually compatible subset of flags (for example, choose either a success-related flag set like JackServerStarted alone or a failure-related set such as JackFailure | JackNameNotUnique | JackLoadFailure | JackInitFailure | JackShmFailure | JackVersionError | JackBackendError) that reflects the scenario you want to test, and remove conflicting flags like JackServerStarted when JackServerFailed (or vice versa) so the mock represents a plausible JACK status.tests/ui/test_crash_recovery_mocked.cpp (1)
31-41: 💤 Low valueConsider documenting the macro-based mocking constraints.
The macro redirection approach (lines 32-34) depends on include order: SDL.h must be included before the macros, and
crash_recovery_ui.hmust not re-include SDL.h in a way that bypasses the macros. While this works correctly with the current header structure, a brief comment explaining this dependency would help future maintainers avoid breaking the test when refactoring headers.📝 Suggested documentation comment
// Redirect SDL calls in the header to our mocks +// NOTE: Requires SDL.h included first (line 1) so macros replace calls in crash_recovery_ui.h `#define` SDL_WasInit mock_SDL_WasInit🤖 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/ui/test_crash_recovery_mocked.cpp` around lines 31 - 41, Add a short comment above the SDL macro redirects (the `#define` lines for SDL_WasInit, SDL_InitSubSystem, SDL_ShowMessageBox) explaining the include-order constraint: SDL.h must be included before these macros are defined and before including gui/crash_recovery_ui.h so the macro redirection applies, and crash_recovery_ui.h must not re-include SDL.h in a way that bypasses the macros; also note that the test undefines AMPLITRON_HEADLESS to exercise non-headless code paths. This documents the mocking constraint and helps future maintainers avoid breaking the test when refactoring includes.
🤖 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/integration/test_audio_backend_jack.cpp`:
- Around line 134-138: The multi-line bitwise OR assignment to
g_mock_jack_status (cast to jack_status_t) is misformatted; reflow the
expression so clang-format accepts it—put each enumerator on its own indented
line (or wrap the entire OR chain on one line) and ensure the closing
parenthesis/semicolon align with the opening cast, e.g., keep the
static_cast<jack_status_t>( on the first line, list each Jack... constant on its
own line joined by '|' and then close the parenthesis and semicolon—apply this
change to the assignment to g_mock_jack_status.
In `@tests/integration/test_audio_backend_portaudio.cpp`:
- Around line 546-566: The test TEST(PortAudioBackend_ExtraCoverage) contains
clang-format violations; reformat the test to comply with project style (fix
indentation, spacing, and line breaks) — specifically ensure the lambda
assignments to g_mock_pa_get_device_info, g_mock_pa_open_stream and
g_mock_pa_get_host_api_info and the call lines for engine.initialize(),
engine.start(), and engine.shutdown() follow the repository's clang-format rules
(proper spacing around operators, aligned parameter lists, and line wrapping for
the g_mock_pa_open_stream lambda signature); run clang-format and commit the
reformatted TEST(PortAudioBackend_ExtraCoverage) block.
In `@tests/integration/test_audio_graph.cpp`:
- Around line 1093-1178: The test contains clang-format style violations in
TEST(AudioGraph_ExtraBranches) around calls to
restore_node/restore_link/restore_input_pin/restore_output_pin/add_output_pin/remove_output_pin/remove_input_pin
and the executor code; fix by running the project's formatter or applying the
project's clang-format rules to this test (e.g., run clang-format -i on the test
file) so spacing, indentation, and line breaks conform to the repo style and
re-run CI.
- Line 1093: The test name TEST(AudioGraph_ExtraBranches) uses CamelCase; rename
it to snake_case (e.g., TEST(audio_graph_extra_branches)) to match the file's
convention. Update the TEST macro identifier from AudioGraph_ExtraBranches to
audio_graph_extra_branches (and adjust any local references or expectations that
rely on the test name) so the test follows the same naming style as
audio_graph_sequential_sorting and audio_graph_mixer_gains.
- Around line 1117-1134: Add assertions after the restore_input_pin and
restore_output_pin calls to verify the pins were actually added: check the
relevant node's input_pin_ids/output_pin_ids sizes and contents. For the node
with id 10 (created earlier in the test), assert expected input_pin_ids length
and that it contains IDs 103 and 104 where appropriate and that negative index
did not add a pin; for the splitter_node (id 20) assert output_pin_ids length
increased and contains 303 but not 304 for the out-of-bounds call. Use the
DSPNode from the test and the graph accessor methods already used in the file to
fetch the node state for these assertions.
- Around line 1110-1115: The test must assert that the cycle link is rejected:
after calling graph.restore_link(link1) (GraphLink id 50) verify the link was
NOT added by checking the graph's link storage (e.g., ensure graph.find_link(50)
returns null/false or that graph.links does not contain id 50); this relies on
restore_link invoking rebuild_topology() and rolling back on validation failure,
so add an expectation immediately after graph.restore_link(link1) that the link
with id 50 is absent.
In `@tests/ui/test_crash_recovery_mocked.cpp`:
- Line 41: There are clang-format spacing violations in the
tests/ui/test_crash_recovery_mocked.cpp around inline comments (within namespace
mocked); run clang-format -i tests/ui/test_crash_recovery_mocked.cpp or manually
fix the inline comment spacing (ensure a single space before '//' and proper
spacing after '//' and remove trailing spaces) for the occurrences near the end
of the mocked namespace and the other affected lines (around lines mentioned in
the review) so the file conforms to the project's clang-format rules.
In `@tests/unit/test_audio_backend_sdl.cpp`:
- Around line 61-85: Add concrete assertions after each
Amplitron::sdl_audio_callback invocation to verify behavior: assert expected
contents of output_buffer (e.g., non-zero for full data, zeroed tail for
partial/cleared cases), and verify the engine's process_audio was called with
the expected input frames by spying/mocking the engine instance used by
SdlBackend (check arguments passed to process_audio). Also assert capture
device/queue state where relevant (e.g., SDL_GetQueuedAudioSize(cap_dev) or that
backend.capture_device_ is 0 after backend.stop()) to validate the
capture-buffer transitions across the four scenarios. Ensure assertions are
placed immediately after each callback call so each scenario is independently
validated.
- Around line 1-89: Run clang-format over the test file (or apply the project's
pre-commit formatting) to fix style violations reported by CI; specifically
reformat the TEST function SdlBackend_LifecycleAndCallback and adjacent lines
where spacing/indentation and blank lines are off (affecting lines around the
SDL_QueueAudio calls, sdl_audio_callback invocations, and the
backend.stop()/shutdown() section) so the file matches the repo's clang-format
config, then re-run CI. Ensure the produced changes only adjust
whitespace/formatting in the TEST(SdlBackend_LifecycleAndCallback) block and
related includes, leaving logic in Amplitron::sdl_audio_callback and SdlBackend
usage unchanged.
In `@tests/unit/test_audio_command_dispatcher.cpp`:
- Line 1: Run clang-format on tests/unit/test_audio_command_dispatcher.cpp to
resolve the formatting violations reported at the include line and at line 81;
reformat the file so the `#include` "audio/engine/audio_command_dispatcher.h" line
and the code around the location referenced (near the test(s) in this file)
comply with the project's clang-format rules, then stage the updated file and
re-run CI to verify the formatting errors are resolved.
In `@tests/unit/test_audio_metrics_service.cpp`:
- Line 1: Run clang-format on the test file to fix the formatting violations
reported at lines 1 and 19: apply the project's style (e.g., run clang-format -i
tests/unit/test_audio_metrics_service.cpp or your repo's configured formatter
command) and re-commit the updated file so the include line (`#include`
"audio/engine/audio_metrics_service.h") and the surrounding code are formatted
according to the CI config.
In `@tests/unit/test_effects_modulation.cpp`:
- Line 363: There is a clang-format violation in
tests/unit/test_effects_modulation.cpp (around a stray closing brace '}'); run
clang-format on that file (e.g., clang-format -i
tests/unit/test_effects_modulation.cpp) to reformat the file and resolve the
violation so the test file's braces and spacing conform to project style.
In `@tests/unit/test_mock_backend.cpp`:
- Line 164: The line setting mock->start_fail_count is misformatted; run
clang-format on tests/unit/test_mock_backend.cpp (or the whole repo) to fix
spacing/formatting so the line reads with proper spacing/punctuation; ensure the
change affects the expression mock->start_fail_count = 2; and re-run CI
formatting check.
- Line 157: Run clang-format on the test file to fix spacing/formatting around
the assignment to mock->start_fail_count; ensure the line
"mock->start_fail_count = 2; // fails first start, and second start during
revert" is formatted according to project style (proper indentation and
spacing). Re-run the formatter (or apply the project's clang-format
configuration) so the test_mock_backend.cpp file conforms to the code style
enforced by the CI.
- Line 154: The failing check is a formatting issue in
tests/unit/test_mock_backend.cpp where the comment line "// 4. Reversion
failures (when was_running is true, device set succeeds, but start fails, then
revert fails)" is not clang-formatted; run clang-format (or the project's
formatting script) on tests/unit/test_mock_backend.cpp or reformat that comment
line to match the project's style, then re-run the formatter/pre-commit to
ensure the pipeline passes.
- Line 173: Run clang-format on the modified test source named
test_mock_backend.cpp to fix the reported formatting issue (the stray/misaligned
closing brace '}'); reformat the file using the repository's .clang-format
settings, stage the resulting changes, and commit so the pipeline will pass.
---
Nitpick comments:
In `@src/audio/backend/sdl_backend.cpp`:
- Line 14: The function sdl_audio_callback was made non-static to allow unit
testing, but that exposes an implementation symbol without documentation or a
header declaration; add a clear comment above sdl_audio_callback stating it is
intentionally non-static for unit tests (or alternatively declare it in a header
such as sdl_backend.h or a dedicated test header with a "for testing" note) so
callers/readers know this exposure is deliberate and tracked.
In `@tests/integration/test_audio_backend_jack.cpp`:
- Around line 134-138: The test currently sets g_mock_jack_status to every JACK
flag at once, which mixes contradictory states (e.g., JackServerStarted with
JackServerFailed) and is unrealistic; change the assignment of
g_mock_jack_status to a representative, mutually compatible subset of flags (for
example, choose either a success-related flag set like JackServerStarted alone
or a failure-related set such as JackFailure | JackNameNotUnique |
JackLoadFailure | JackInitFailure | JackShmFailure | JackVersionError |
JackBackendError) that reflects the scenario you want to test, and remove
conflicting flags like JackServerStarted when JackServerFailed (or vice versa)
so the mock represents a plausible JACK status.
In `@tests/integration/test_audio_backend_portaudio.cpp`:
- Around line 563-565: After forcing g_mock_pa_get_host_api_info to return
nullptr and calling engine.shutdown() then engine.initialize(), add assertions
to verify auto-detection behavior: assert that initialize() completed without
throwing (or engine.isInitialized() is true), and assert expected device index
state (e.g., engine's input/output device indices are the sentinel value or
unchanged from pre-shutdown). Use the existing engine API (e.g.,
engine.initialize(), engine.isInitialized(),
engine.getInputDeviceIndex()/getOutputDeviceIndex() or the equivalent accessors)
to check no crash occurred and device indices reflect the
fallback/unavailable-host-API behavior.
In `@tests/ui/test_crash_recovery_mocked.cpp`:
- Around line 31-41: Add a short comment above the SDL macro redirects (the
`#define` lines for SDL_WasInit, SDL_InitSubSystem, SDL_ShowMessageBox) explaining
the include-order constraint: SDL.h must be included before these macros are
defined and before including gui/crash_recovery_ui.h so the macro redirection
applies, and crash_recovery_ui.h must not re-include SDL.h in a way that
bypasses the macros; also note that the test undefines AMPLITRON_HEADLESS to
exercise non-headless code paths. This documents the mocking constraint and
helps future maintainers avoid breaking the test when refactoring includes.
In `@tests/unit/test_audio_backend_sdl.cpp`:
- Around line 61-68: Add a unit test that exercises the null-engine early return
in sdl_audio_callback by creating or reusing the Amplitron::Backend instance
used in the test, explicitly setting its engine_ member to nullptr (or
constructing it with no engine), then calling
Amplitron::sdl_audio_callback(&backend,
reinterpret_cast<Uint8*>(output_buffer.data()), bytes_len) and asserting the
call returns cleanly and the output_buffer remains unchanged (or that no data
was written). Target the existing test file
(tests/unit/test_audio_backend_sdl.cpp) and reference the sdl_audio_callback
function and the backend.engine_ field to locate where to add this null-engine
case.
In `@tests/unit/test_effects_modulation.cpp`:
- Around line 305-306: The test disables the effect with ch.set_enabled(false)
then calls ch.process_stereo(left, right, BUFFER_SIZE) but doesn't assert that
the buffers were left unchanged; update the test to copy the original left and
right buffers (e.g., saved_left, saved_right) before calling ch.process_stereo
and then assert equality between saved_left and left and between saved_right and
right after the call to verify pass-through behavior when disabled (reference
the ch.set_enabled and ch.process_stereo calls and the left/right buffers).
- Around line 340-341: The test disables the Flanger via fl.set_enabled(false)
and calls fl.process_stereo(left, right, BUFFER_SIZE) but doesn't assert that
the buffers are unchanged; add assertions that left and right match the original
input buffers after processing to verify pass-through behavior when disabled
(similar to the chorus_process_stereo test), using the same input
arrays/variables you used before calling process_stereo to compare against.
- Around line 361-362: Add an assertion that process_stereo is a no-op when
disabled: before calling ph.set_enabled(false) / ph.process_stereo(left, right,
BUFFER_SIZE) make copies of the original left and right buffers, then after
ph.process_stereo assert the processed left and right equal the originals (e.g.,
with EXPECT_EQ/ASSERT_EQ or array comparison) to verify pass-through behavior
for the object under test (ph).
- Around line 309-321: The test currently calls Chorus::set_transport_state with
invalid and repeated BPMs but lacks assertions; capture the initial rate via
auto initial = ch.params()[0].value (or read the rate param before the invalid
calls), then after each invalid call (ch.set_transport_state(-10.0f);
ch.set_transport_state(NAN);) assert that ch.params()[0].value == initial (or
nearly equal if float) to ensure state wasn't modified, and after calling the
same valid BPM twice (ch.set_transport_state(120.0f);
ch.set_transport_state(120.0f);) assert that params()[0].value remains stable
(no unintended change) possibly using an epsilon for float comparisons.
🪄 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: 0c519327-27a7-43a2-8a6b-42d2a858d7d3
📒 Files selected for processing (14)
CMakeLists.txtsrc/audio/backend/sdl_backend.cpptests/fixtures/jack_mock.cpptests/fixtures/jack_mock.htests/integration/test_audio_backend_jack.cpptests/integration/test_audio_backend_portaudio.cpptests/integration/test_audio_graph.cpptests/ui/test_crash_recovery_mocked.cpptests/unit/test_audio_backend_registry.cpptests/unit/test_audio_backend_sdl.cpptests/unit/test_audio_command_dispatcher.cpptests/unit/test_audio_metrics_service.cpptests/unit/test_effects_modulation.cpptests/unit/test_mock_backend.cpp
Comment on lines
+134
to
+138
| g_mock_jack_status = static_cast<jack_status_t>( | ||
| JackFailure | JackInvalidOption | JackNameNotUnique | JackServerStarted | | ||
| JackServerFailed | JackServerError | JackNoSuchClient | JackLoadFailure | | ||
| JackInitFailure | JackShmFailure | JackVersionError | JackBackendError | | ||
| JackClientZombie); |
Contributor
There was a problem hiding this comment.
Fix clang-format violations.
The multi-line bitwise OR expression needs formatting adjustment to pass clang-format checks.
🧰 Tools
🪛 GitHub Actions: Clang-Format Check / 0_Check Code Formatting.txt
[error] 135-135: clang-format check failed for file. error: code should be clang-formatted [-Wclang-format-violations]
[error] 136-136: clang-format check failed for file. error: code should be clang-formatted [-Wclang-format-violations]
[error] 136-136: clang-format check failed for file. error: code should be clang-formatted [-Wclang-format-violations]
[error] 137-137: clang-format check failed for file. error: code should be clang-formatted [-Wclang-format-violations]
[error] 137-137: clang-format check failed for file. error: code should be clang-formatted [-Wclang-format-violations]
🪛 GitHub Actions: Clang-Format Check / Check Code Formatting
[error] 135-135: clang-format reported code should be clang-formatted [-Wclang-format-violations].
[error] 136-136: clang-format reported code should be clang-formatted [-Wclang-format-violations].
[error] 136-136: clang-format reported code should be clang-formatted [-Wclang-format-violations] (duplicate report at same line).
[error] 137-137: clang-format reported code should be clang-formatted [-Wclang-format-violations].
[error] 137-137: clang-format reported code should be clang-formatted [-Wclang-format-violations] (duplicate report at same line).
🤖 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_backend_jack.cpp` around lines 134 - 138, The
multi-line bitwise OR assignment to g_mock_jack_status (cast to jack_status_t)
is misformatted; reflow the expression so clang-format accepts it—put each
enumerator on its own indented line (or wrap the entire OR chain on one line)
and ensure the closing parenthesis/semicolon align with the opening cast, e.g.,
keep the static_cast<jack_status_t>( on the first line, list each Jack...
constant on its own line joined by '|' and then close the parenthesis and
semicolon—apply this change to the assignment to g_mock_jack_status.
|
|
||
| ph.set_enabled(false); | ||
| ph.process_stereo(left, right, BUFFER_SIZE); | ||
| } |
Contributor
There was a problem hiding this comment.
Fix clang-format violation.
The pipeline reports a formatting violation at line 363. Run clang-format to fix this issue.
#!/bin/bash
# Format the file to resolve the violation
clang-format -i tests/unit/test_effects_modulation.cpp🧰 Tools
🪛 GitHub Actions: Clang-Format Check / 0_Check Code Formatting.txt
[error] 363-363: clang-format check failed for file. error: code should be clang-formatted [-Wclang-format-violations]
🪛 GitHub Actions: Clang-Format Check / Check Code Formatting
[error] 363-363: clang-format reported code should be clang-formatted [-Wclang-format-violations].
🤖 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_effects_modulation.cpp` at line 363, There is a clang-format
violation in tests/unit/test_effects_modulation.cpp (around a stray closing
brace '}'); run clang-format on that file (e.g., clang-format -i
tests/unit/test_effects_modulation.cpp) to reformat the file and resolve the
violation so the test file's braces and spacing conform to project style.
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_main.cpp (1)
1-471:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix clang-format violations flagged by CI pipeline.
The GitHub Actions pipeline reports clang-format violations across the file, including multi-line array initializers (lines 190-198, 249-253, 308-316, etc.) and lambda expressions.
Run
clang-format -i tests/integration/test_main.cppto auto-fix.🤖 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_main.cpp` around lines 1 - 471, Reformat the file to satisfy clang-format: run clang-format -i on tests/integration/test_main.cpp or reflow the multi-line array initializers and lambda expressions to match project style; specifically adjust the char* argv initializers in TESTs main_cli_help_exits_with_zero, main_cli_version_exits_with_zero, main_headless_full_execution, main_headless_preset_not_found, main_headless_strict_requirement, main_headless_routing_warnings_and_failures, main_audio_failures and the lambda assignments inside setup_main_pa_mocks/clear_main_pa_mocks so they meet the formatter (align braces, commas, and indentation). Ensure no other logic is changed and re-run CI formatting check.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 `@tests/integration/test_main.cpp`:
- Around line 248-268: The test main_headless_preset_not_found relies on real
PortAudio; call setup_main_pa_mocks() at the start of the test (before invoking
app_main) to ensure PortAudio is mocked and engine.initialize() cannot fail due
to missing system audio; locate the test by its name and add the
setup_main_pa_mocks() invocation (and any matching teardown if available) so the
test exercises the preset-not-found path deterministically.
---
Outside diff comments:
In `@tests/integration/test_main.cpp`:
- Around line 1-471: Reformat the file to satisfy clang-format: run clang-format
-i on tests/integration/test_main.cpp or reflow the multi-line array
initializers and lambda expressions to match project style; specifically adjust
the char* argv initializers in TESTs main_cli_help_exits_with_zero,
main_cli_version_exits_with_zero, main_headless_full_execution,
main_headless_preset_not_found, main_headless_strict_requirement,
main_headless_routing_warnings_and_failures, main_audio_failures and the lambda
assignments inside setup_main_pa_mocks/clear_main_pa_mocks so they meet the
formatter (align braces, commas, and indentation). Ensure no other logic is
changed and re-run CI formatting check.
🪄 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: 9466522a-1e43-4616-bb68-11bb699e9c5c
📒 Files selected for processing (1)
tests/integration/test_main.cpp
Comment on lines
+248
to
+268
| TEST(main_headless_preset_not_found) { | ||
| char* argv[] = { | ||
| (char*)"amplitron", | ||
| (char*)"--preset", | ||
| (char*)"nonexistent_preset_file.json", | ||
| (char*)"--headless" | ||
| }; | ||
|
|
||
| std::streambuf* old_out = std::cout.rdbuf(); | ||
| std::streambuf* old_err = std::cerr.rdbuf(); | ||
| std::stringstream ss_out, ss_err; | ||
| std::cout.rdbuf(ss_out.rdbuf()); | ||
| std::cerr.rdbuf(ss_err.rdbuf()); | ||
|
|
||
| int exit_code = app_main(4, argv); | ||
|
|
||
| std::cout.rdbuf(old_out); | ||
| std::cerr.rdbuf(old_err); | ||
|
|
||
| ASSERT_EQ(exit_code, 1); | ||
| } |
Contributor
There was a problem hiding this comment.
Missing PortAudio mock setup may cause flaky/misleading test.
This test doesn't call setup_main_pa_mocks(), so it relies on real PortAudio behavior. Per the mock implementation in portaudio_mock.cpp, when mock pointers are nullptr, the real Pa_* functions are invoked.
The issue: engine.initialize() runs before preset loading (see src/main.cpp context). Without mocks:
- On systems with working audio: test reaches the intended "preset not found" path
- On headless CI:
engine.initialize()may fail first, returningexit_code == 1for the wrong reason
The test passes in both cases but may not exercise the claimed failure path.
Suggested fix
TEST(main_headless_preset_not_found) {
+ setup_main_pa_mocks();
+
char* argv[] = {
(char*)"amplitron",
(char*)"--preset",
@@ -265,6 +267,8 @@ TEST(main_headless_preset_not_found) {
std::cerr.rdbuf(old_err);
ASSERT_EQ(exit_code, 1);
+ ASSERT_TRUE(ss_err.str().find("Could not load preset") != std::string::npos);
+ clear_main_pa_mocks();
}🤖 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_main.cpp` around lines 248 - 268, The test
main_headless_preset_not_found relies on real PortAudio; call
setup_main_pa_mocks() at the start of the test (before invoking app_main) to
ensure PortAudio is mocked and engine.initialize() cannot fail due to missing
system audio; locate the test by its name and add the setup_main_pa_mocks()
invocation (and any matching teardown if available) so the test exercises the
preset-not-found path deterministically.
…ralized artifacts and unified Codecov upload
… modulation effects.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/ui/test_file_dialog.cpp (1)
62-78: 💤 Low valueClarify Unicode escape sequence in assertion.
Line 76 uses
\u0027(Unicode single quote) in the middle of a string that already contains literal single quotes. The assertion string is:"Prompt \"with\" '\\''quotes'\u0027 \\"The
\u0027appears to be checking for a literal single quote character but is inconsistent with the surrounding literal quotes. This might work but makes the expected string harder to read.Consider using a consistent quoting style or adding a comment explaining the expected shell escaping pattern.
🤖 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/ui/test_file_dialog.cpp` around lines 62 - 78, The assertion in TEST_F FileDialogNativeTest::ShowOpenDialogNativeSanitization uses an inconsistent Unicode escape (\u0027) inside the expected shell-escaped string which is hard to read; update the expected string in the non-Apple branch (the ASSERT_NE on cmds[0]) to use a consistent, clear representation for the single-quote (e.g., use the explicit shell-escape sequence '\'' or a clearly commented literal) so the test matches the actual shell-escaped output from Amplitron::TestMocks::get_executed_commands(); ensure the surrounding escaping for backslash and quotes remains correct to reflect the shell sanitization produced by show_open_dialog.src/gui/dialogs/file_dialog_native_open.cpp (1)
36-47: ⚡ Quick winForward declaration couples production code to test fixtures.
The forward declaration of
TestMocks::set_mock_resultcreates a fragile dependency. If the signature intests/fixtures/file_dialog_mock.hchanges, the mismatch won't be caught until link time.Consider either:
- Including
fixtures/file_dialog_mock.hwhenAMPLITRON_TEST_NATIVE_DIALOGSis defined, or- Moving this mock-forwarding logic entirely into the test fixture setup.
🤖 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/gui/dialogs/file_dialog_native_open.cpp` around lines 36 - 47, The production function set_mock_open_dialog_path currently forward-declares TestMocks::set_mock_result under AMPLITRON_TEST_NATIVE_DIALOGS, which couples prod code to test fixtures; fix by removing the forward-declaration and either (A) include the test fixture header (e.g., fixtures/file_dialog_mock.h) inside the `#ifdef` AMPLITRON_TEST_NATIVE_DIALOGS block so the signature is checked at compile time, or (B) move the TestMocks::set_mock_result call out of set_mock_open_dialog_path entirely and let the test fixture invoke TestMocks::set_mock_result from its setup; update references to s_mock_open_path and ensure the conditional compilation only wraps the include or test-only invocation, not production code paths.
🤖 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_framework.h`:
- Around line 146-148: The write_junit_xml function silently returns when the
output file cannot be opened; modify write_junit_xml to detect the failure to
open the std::ofstream and report it (either by logging an error via your test
framework's logger or by returning/throwing an error code/exception) so callers
know the JUnit XML was not written. Update the function's control flow around
the xml.is_open() check to emit a clear error message including the filepath and
errno/strerror details (or set an error return value) and ensure callers can
surface this failure to the user.
---
Nitpick comments:
In `@src/gui/dialogs/file_dialog_native_open.cpp`:
- Around line 36-47: The production function set_mock_open_dialog_path currently
forward-declares TestMocks::set_mock_result under AMPLITRON_TEST_NATIVE_DIALOGS,
which couples prod code to test fixtures; fix by removing the
forward-declaration and either (A) include the test fixture header (e.g.,
fixtures/file_dialog_mock.h) inside the `#ifdef` AMPLITRON_TEST_NATIVE_DIALOGS
block so the signature is checked at compile time, or (B) move the
TestMocks::set_mock_result call out of set_mock_open_dialog_path entirely and
let the test fixture invoke TestMocks::set_mock_result from its setup; update
references to s_mock_open_path and ensure the conditional compilation only wraps
the include or test-only invocation, not production code paths.
In `@tests/ui/test_file_dialog.cpp`:
- Around line 62-78: The assertion in TEST_F
FileDialogNativeTest::ShowOpenDialogNativeSanitization uses an inconsistent
Unicode escape (\u0027) inside the expected shell-escaped string which is hard
to read; update the expected string in the non-Apple branch (the ASSERT_NE on
cmds[0]) to use a consistent, clear representation for the single-quote (e.g.,
use the explicit shell-escape sequence '\'' or a clearly commented literal) so
the test matches the actual shell-escaped output from
Amplitron::TestMocks::get_executed_commands(); ensure the surrounding escaping
for backslash and quotes remains correct to reflect the shell sanitization
produced by show_open_dialog.
🪄 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: fb65e641-ed23-46e7-bca6-3cacd350b739
📒 Files selected for processing (24)
.github/workflows/ci.yml.github/workflows/deploy-preview.ymlCMakeLists.txtcodecov.ymlsrc/audio/backend/sdl_backend.cppsrc/gui/dialogs/file_dialog_native.cppsrc/gui/dialogs/file_dialog_native_folder.cppsrc/gui/dialogs/file_dialog_native_open.cpptests/fixtures/file_dialog_mock.cpptests/fixtures/file_dialog_mock.htests/integration/test_audio_backend_jack.cpptests/integration/test_audio_backend_portaudio.cpptests/integration/test_audio_graph.cpptests/integration/test_main.cpptests/test_framework.htests/test_main.cpptests/ui/test_crash_recovery_mocked.cpptests/ui/test_file_dialog.cpptests/unit/test_audio_backend_sdl.cpptests/unit/test_audio_command_dispatcher.cpptests/unit/test_audio_metrics_service.cpptests/unit/test_effects_modulation.cpptests/unit/test_mock_backend.cpptests/web/playwright.config.ts
✅ Files skipped from review due to trivial changes (1)
- codecov.yml
🚧 Files skipped from review as they are similar to previous changes (10)
- src/audio/backend/sdl_backend.cpp
- tests/integration/test_audio_backend_portaudio.cpp
- tests/ui/test_crash_recovery_mocked.cpp
- tests/integration/test_audio_backend_jack.cpp
- tests/integration/test_audio_graph.cpp
- tests/unit/test_effects_modulation.cpp
- tests/unit/test_audio_metrics_service.cpp
- tests/unit/test_mock_backend.cpp
- tests/unit/test_audio_backend_sdl.cpp
- tests/integration/test_main.cpp
Comment on lines
+146
to
+148
| void write_junit_xml(const std::string& filepath, int passed, int failed) { | ||
| std::ofstream xml(filepath); | ||
| if (!xml.is_open()) return; |
Contributor
There was a problem hiding this comment.
Silent failure when JUnit XML file cannot be written.
If the output file cannot be opened (e.g., directory does not exist, insufficient permissions), the method returns silently without logging or signaling an error. Users expecting JUnit XML output may not realize the file was never created.
🛠️ Proposed fix to log the failure
void write_junit_xml(const std::string& filepath, int passed, int failed) {
std::ofstream xml(filepath);
- if (!xml.is_open()) return;
+ if (!xml.is_open()) {
+ std::cerr << "Warning: Failed to write JUnit XML to " << filepath << std::endl;
+ return;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| void write_junit_xml(const std::string& filepath, int passed, int failed) { | |
| std::ofstream xml(filepath); | |
| if (!xml.is_open()) return; | |
| void write_junit_xml(const std::string& filepath, int passed, int failed) { | |
| std::ofstream xml(filepath); | |
| if (!xml.is_open()) { | |
| std::cerr << "Warning: Failed to write JUnit XML to " << filepath << std::endl; | |
| return; | |
| } |
🤖 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_framework.h` around lines 146 - 148, The write_junit_xml function
silently returns when the output file cannot be opened; modify write_junit_xml
to detect the failure to open the std::ofstream and report it (either by logging
an error via your test framework's logger or by returning/throwing an error
code/exception) so callers know the JUnit XML was not written. Update the
function's control flow around the xml.is_open() check to emit a clear error
message including the filepath and errno/strerror details (or set an error
return value) and ensure callers can surface this failure to the user.
…data structures to support headless builds
… preset management
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/ui/test_crash_recovery_mocked.cpp (1)
13-23: ⚡ Quick winUse lowercase snake_case for mock helper function names.
The changed mock function names are mixed-case (
mock_SDL_*), which violates the repository naming rule for functions/methods.
As per coding guidelines, “Functions and Methods should use lowercase snake_case”.♻️ Proposed fix
-inline Uint32 mock_SDL_WasInit(Uint32 /*flags*/) { +inline Uint32 mock_sdl_was_init(Uint32 /*flags*/) { @@ -inline int mock_SDL_InitSubSystem(Uint32 /*flags*/) { +inline int mock_sdl_init_sub_system(Uint32 /*flags*/) { @@ -inline int mock_SDL_ShowMessageBox(const SDL_MessageBoxData* /*messageboxdata*/, int* buttonid) { +inline int mock_sdl_show_message_box(const SDL_MessageBoxData* /*messageboxdata*/, + int* buttonid) { @@ -#define SDL_WasInit mock_SDL_WasInit -#define SDL_InitSubSystem mock_SDL_InitSubSystem -#define SDL_ShowMessageBox mock_SDL_ShowMessageBox +#define SDL_WasInit mock_sdl_was_init +#define SDL_InitSubSystem mock_sdl_init_sub_system +#define SDL_ShowMessageBox mock_sdl_show_message_boxAlso applies to: 36-38
🤖 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/ui/test_crash_recovery_mocked.cpp` around lines 13 - 23, Rename the mixed-case mock helpers to lowercase snake_case and update all references: change mock_SDL_WasInit -> mock_sdl_was_init, mock_SDL_InitSubSystem -> mock_sdl_init_subsystem, and mock_SDL_ShowMessageBox -> mock_sdl_show_message_box; update their definitions in tests/ui/test_crash_recovery_mocked.cpp and any places that call them (including the additional occurrences noted at lines 36-38) so the symbol names match the repository naming convention and compilation succeeds.Source: Coding guidelines
🤖 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/ui/test_crash_recovery_mocked.cpp`:
- Around line 13-23: Rename the mixed-case mock helpers to lowercase snake_case
and update all references: change mock_SDL_WasInit -> mock_sdl_was_init,
mock_SDL_InitSubSystem -> mock_sdl_init_subsystem, and mock_SDL_ShowMessageBox
-> mock_sdl_show_message_box; update their definitions in
tests/ui/test_crash_recovery_mocked.cpp and any places that call them (including
the additional occurrences noted at lines 36-38) so the symbol names match the
repository naming convention and compilation succeeds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3824c152-00c9-4c6c-a04d-f201f5f72ab8
📒 Files selected for processing (35)
CMakeLists.txtsrc/audio/backend/portaudio_backend.cppsrc/audio/effects/pitch/pitch_shifter.hsrc/audio/engine/audio_graph.cppsrc/gui/components/screen.cppsrc/gui/dialogs/file_dialog_native.cppsrc/gui/dialogs/file_dialog_native_folder.cppsrc/gui/dialogs/file_dialog_native_open.cppsrc/gui/pedalboard/pedal_board.hsrc/gui/pedalboard/pedal_widget.hsrc/gui/pedalboard/pedal_widget_knobs.cppsrc/gui/window_context.cppsrc/gui/window_context.hsrc/midi/midi_manager.hsrc/presets/preset_manager_io.cpptests/fixtures/file_dialog_mock.cpptests/fixtures/file_dialog_mock.htests/integration/test_audio_backend_portaudio.cpptests/integration/test_audio_engine.cpptests/integration/test_audio_graph.cpptests/integration/test_command_graph.cpptests/integration/test_main.cpptests/ui/test_crash_recovery_mocked.cpptests/ui/test_file_dialog.cpptests/ui/test_gui_manager.cpptests/ui/test_gui_tuner.cpptests/ui/test_knob.cpptests/ui/test_pedal_board.cpptests/ui/test_pedal_board_chain.cpptests/ui/test_pedal_board_menu.cpptests/ui/test_screen.cpptests/unit/test_audio_backend_sdl.cpptests/unit/test_audio_command_dispatcher.cpptests/unit/test_level_analyzer.cpptests/unit/test_mock_backend.cpp
💤 Files with no reviewable changes (6)
- src/audio/backend/portaudio_backend.cpp
- tests/ui/test_pedal_board.cpp
- src/audio/effects/pitch/pitch_shifter.h
- tests/ui/test_pedal_board_chain.cpp
- tests/ui/test_pedal_board_menu.cpp
- tests/ui/test_screen.cpp
✅ Files skipped from review due to trivial changes (10)
- src/gui/pedalboard/pedal_board.h
- src/gui/window_context.h
- src/gui/components/screen.cpp
- src/gui/pedalboard/pedal_widget.h
- src/midi/midi_manager.h
- src/gui/pedalboard/pedal_widget_knobs.cpp
- tests/integration/test_audio_engine.cpp
- src/gui/window_context.cpp
- tests/ui/test_gui_tuner.cpp
- tests/ui/test_knob.cpp
🚧 Files skipped from review as they are similar to previous changes (9)
- CMakeLists.txt
- tests/unit/test_audio_command_dispatcher.cpp
- tests/integration/test_audio_backend_portaudio.cpp
- tests/fixtures/file_dialog_mock.h
- tests/unit/test_mock_backend.cpp
- tests/unit/test_audio_backend_sdl.cpp
- tests/ui/test_file_dialog.cpp
- tests/fixtures/file_dialog_mock.cpp
- tests/integration/test_main.cpp
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
1 participant
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