test: add GUI manager coverage tests#305
Conversation
github-actions
Bot
added
level:intermediate
|
Looking for one thing? Review this PR in Change Stack to search files, summaries, diffs, and code without losing your place. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new test header with shared AudioEngine helpers and unit tests; introduces headless dialog mocks and WindowContext headless flow with mock flags; centralizes PortAudio mocks via RAII; expands GuiManager tests with macro-based ImGui/update-checker mocks; and handles fetch rejections in service worker. ChangesTests, headless wiring, and mock infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)tests/ui/test_gui_manager.cpptests/ui/test_gui_manager.cpp:8:10: fatal error: 'amplitron_session.h' file not found ... [truncated 1097 characters] ... stall/lib/clang/18/include" 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
CMakeLists.txt (1)
526-527:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate comment to reflect GUI sources inclusion.
The comment states "Effect + core sources needed by tests (no GUI, no main)" but the CORE_SOURCES list now includes 21 GUI-related files (gui_manager, pedal_board, gui_snapshots, etc.) needed for the new GUI test coverage. The comment should be updated to accurately describe the expanded scope.
📝 Proposed comment update
- # Effect + core sources needed by tests (no GUI, no main) + # Effect + core sources needed by tests (includes GUI components, no main) set(CORE_SOURCES🤖 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 526 - 527, Update the top comment that currently reads "Effect + core sources needed by tests (no GUI, no main)" to reflect that CORE_SOURCES now includes GUI-related files; mention that GUI sources are included for expanded GUI test coverage and optionally cite examples such as gui_manager, pedal_board, gui_snapshots to make the intent clear so readers know GUI code is part of the test build.
🧹 Nitpick comments (3)
tests/test_gui_manager.cpp (3)
243-244: 💤 Low valueInconsistent cleanup pattern across tests.
Several tests (e.g., lines 243-244, 255, 294) set specific gain values (0.7f, 0.8f) at the end, presumably to leave the
shared_engine()in a known state. However, this pattern is applied inconsistently—many tests omit it entirely. If cleanup is necessary, apply it uniformly; if not, remove these statements to reduce noise.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_gui_manager.cpp` around lines 243 - 244, The test suite contains inconsistent cleanup where some tests call engine.set_input_gain(0.7f) and engine.set_output_gain(0.8f) on the shared_engine() while others do not; decide on a single approach and make it consistent: either remove these trailing set_input_gain/set_output_gain calls from all tests if they are unnecessary, or add the same cleanup at the end of every test that mutates shared_engine() (use shared_engine().set_input_gain(...) and shared_engine().set_output_gain(...) and centralize into a helper like reset_shared_engine() if desired) so tests uniformly restore engine state.
17-20: 💤 Low valueConsider test isolation implications of shared singleton.
The static
AudioEngineinstance creates implicit state sharing between tests. Whileclear_engine()is called appropriately, this pattern could cause issues if tests ever run in parallel or if cleanup is missed. For now, the sequential execution and careful cleanup make this workable, but consider using a fresh instance per test if isolation becomes a concern.🤖 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_gui_manager.cpp` around lines 17 - 20, The tests currently use a process-wide static AudioEngine via shared_engine(), which creates implicit state sharing between tests; to improve isolation, change tests to construct and use a fresh AudioEngine instance per test (or return a unique_ptr from a factory) instead of the static in shared_engine(), and update callers to call the new per-test factory (or pass the instance explicitly) rather than relying on shared_engine(); if retaining a helper, rename/replace shared_engine() to make_new_engine() or make_unique_engine() and ensure clear_engine() is no longer relied upon for cleanup.
320-349: ⚖️ Poor tradeoffVersion parsing logic duplication is acknowledged but creates maintenance debt.
The comments (lines 311-317) correctly note that
parse_versionandis_version_newerare duplicated fromgui_manager_update.cppbecause the originals are local lambdas. While this duplication is currently necessary for testing, any future changes to the source must be manually mirrored here, creating maintenance risk.Consider extracting the version parsing and comparison logic to a separate utility header (e.g.,
version_utils.h) that both the production code and tests can include directly. This would eliminate duplication and ensure test coverage automatically tracks implementation changes.🤖 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_gui_manager.cpp` around lines 320 - 349, The parse_version and is_version_newer functions duplicated in tests/test_gui_manager.cpp should be moved into a shared utility header (e.g., version_utils.h) and used from both the production file (gui_manager_update.cpp) and the test file to remove maintenance duplication; create version_utils.h with the parse_version and is_version_newer declarations/implementations, update gui_manager_update.cpp to use those functions instead of its local lambdas, and replace the copies in tests/test_gui_manager.cpp to include and call parse_version/is_version_newer from the new header.
🤖 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 `@CMakeLists.txt`:
- Around line 526-527: Update the top comment that currently reads "Effect +
core sources needed by tests (no GUI, no main)" to reflect that CORE_SOURCES now
includes GUI-related files; mention that GUI sources are included for expanded
GUI test coverage and optionally cite examples such as gui_manager, pedal_board,
gui_snapshots to make the intent clear so readers know GUI code is part of the
test build.
---
Nitpick comments:
In `@tests/test_gui_manager.cpp`:
- Around line 243-244: The test suite contains inconsistent cleanup where some
tests call engine.set_input_gain(0.7f) and engine.set_output_gain(0.8f) on the
shared_engine() while others do not; decide on a single approach and make it
consistent: either remove these trailing set_input_gain/set_output_gain calls
from all tests if they are unnecessary, or add the same cleanup at the end of
every test that mutates shared_engine() (use shared_engine().set_input_gain(...)
and shared_engine().set_output_gain(...) and centralize into a helper like
reset_shared_engine() if desired) so tests uniformly restore engine state.
- Around line 17-20: The tests currently use a process-wide static AudioEngine
via shared_engine(), which creates implicit state sharing between tests; to
improve isolation, change tests to construct and use a fresh AudioEngine
instance per test (or return a unique_ptr from a factory) instead of the static
in shared_engine(), and update callers to call the new per-test factory (or pass
the instance explicitly) rather than relying on shared_engine(); if retaining a
helper, rename/replace shared_engine() to make_new_engine() or
make_unique_engine() and ensure clear_engine() is no longer relied upon for
cleanup.
- Around line 320-349: The parse_version and is_version_newer functions
duplicated in tests/test_gui_manager.cpp should be moved into a shared utility
header (e.g., version_utils.h) and used from both the production file
(gui_manager_update.cpp) and the test file to remove maintenance duplication;
create version_utils.h with the parse_version and is_version_newer
declarations/implementations, update gui_manager_update.cpp to use those
functions instead of its local lambdas, and replace the copies in
tests/test_gui_manager.cpp to include and call parse_version/is_version_newer
from the new header.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f8d75c92-4f6a-49d2-b708-611dd3fff316
📒 Files selected for processing (2)
CMakeLists.txttests/test_gui_manager.cpp
🌍 Cross-Platform Coverage ReportPlatforms: Linux + macOS + Windows ✅ Coverage meets threshold: 78.1% >= 60% Full Merged Coverage Summary |
PR Preview ReadyPreview URL: https://amplitron.sudipmondal.co.in/pr-previews/pr-305/ Built from commit |
|
@YASHcode-IIITV lmk when complete A huge part is missing test coverage for GUI module |
sudip-mondal-2002
left a comment
sudip-mondal-2002
left a comment
There was a problem hiding this comment.
Tests must go to test folder
and coverage is not enough
|
@YASHcode-IIITV are you still working on it? |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/audio_engine.h`:
- Around line 14-15: The production header currently includes test_framework.h
and contains TEST(...) blocks (test registration) which must be removed from the
header; move the include of test_framework.h and all TEST(...) blocks (and any
test helper functions) into a new test-only translation unit (e.g.,
audio_engine_test.cpp) that `#includes` the original audio_engine.h, leaving
audio_engine.h with only production declarations/definitions and no test
symbols; update build files to compile the new test TU only in test targets and
delete the test include/TEST blocks from the header.
In `@src/gui/dialogs/file_dialog_native.cpp`:
- Around line 90-97: The code builds shell command strings (cmd) using unescaped
user-controlled variables (default_name, filter_desc, filter_ext) and passes
them to popen(), allowing shell injection; replace this by invoking
/usr/bin/osascript (or the AppleScript API) without a shell: construct an argv
array with program "/usr/bin/osascript" and separate "-e" arguments (one per
script fragment) and call posix_spawn/execvp (or use NSTask/NSAppleScript on
macOS) so arguments are passed directly rather than concatenated into a shell
command; update the code paths that build cmd in file_dialog_native.cpp (the
blocks that create the "choose file name" and related osascript commands) to
stop using popen(cmd.c_str(), "r") with interpolated strings and instead spawn
the process with safe argv passing or use native Cocoa APIs to present the
save/open dialogs.
🪄 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: 5ee1191f-114b-433a-8073-b28104da8dee
📒 Files selected for processing (9)
src/audio/audio_engine.hsrc/gui/dialogs/file_dialog.hsrc/gui/dialogs/file_dialog_native.cppsrc/gui/dialogs/file_dialog_native_folder.cppsrc/gui/dialogs/file_dialog_native_open.cppsrc/gui/window_context.cpptests/integration/test_audio_backend_portaudio.cpptests/ui/test_gui_manager.cppweb/coi-serviceworker.js
💤 Files with no reviewable changes (4)
- web/coi-serviceworker.js
- src/gui/window_context.cpp
- tests/ui/test_gui_manager.cpp
- tests/integration/test_audio_backend_portaudio.cpp
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 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/audio_engine.h`:
- Around line 14-15: The production header currently includes test_framework.h
and contains TEST(...) blocks (test registration) which must be removed from the
header; move the include of test_framework.h and all TEST(...) blocks (and any
test helper functions) into a new test-only translation unit (e.g.,
audio_engine_test.cpp) that `#includes` the original audio_engine.h, leaving
audio_engine.h with only production declarations/definitions and no test
symbols; update build files to compile the new test TU only in test targets and
delete the test include/TEST blocks from the header.
In `@src/gui/dialogs/file_dialog_native.cpp`:
- Around line 90-97: The code builds shell command strings (cmd) using unescaped
user-controlled variables (default_name, filter_desc, filter_ext) and passes
them to popen(), allowing shell injection; replace this by invoking
/usr/bin/osascript (or the AppleScript API) without a shell: construct an argv
array with program "/usr/bin/osascript" and separate "-e" arguments (one per
script fragment) and call posix_spawn/execvp (or use NSTask/NSAppleScript on
macOS) so arguments are passed directly rather than concatenated into a shell
command; update the code paths that build cmd in file_dialog_native.cpp (the
blocks that create the "choose file name" and related osascript commands) to
stop using popen(cmd.c_str(), "r") with interpolated strings and instead spawn
the process with safe argv passing or use native Cocoa APIs to present the
save/open dialogs.
🪄 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: 5ee1191f-114b-433a-8073-b28104da8dee
📒 Files selected for processing (9)
src/audio/audio_engine.hsrc/gui/dialogs/file_dialog.hsrc/gui/dialogs/file_dialog_native.cppsrc/gui/dialogs/file_dialog_native_folder.cppsrc/gui/dialogs/file_dialog_native_open.cppsrc/gui/window_context.cpptests/integration/test_audio_backend_portaudio.cpptests/ui/test_gui_manager.cppweb/coi-serviceworker.js
💤 Files with no reviewable changes (4)
- web/coi-serviceworker.js
- src/gui/window_context.cpp
- tests/ui/test_gui_manager.cpp
- tests/integration/test_audio_backend_portaudio.cpp
🛑 Comments failed to post (2)
src/audio/audio_engine.h (1)
14-15:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMove this test suite out of
src/audio/audio_engine.hinto a test-only translation unit.Line 14 and the
TEST(...)blocks (Line 36 onward) put test framework dependencies and test registration code in a production header path. This risks leaking test symbols/includes into non-test builds and creates fragile include/build contracts.Also applies to: 36-385
🤖 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/audio_engine.h` around lines 14 - 15, The production header currently includes test_framework.h and contains TEST(...) blocks (test registration) which must be removed from the header; move the include of test_framework.h and all TEST(...) blocks (and any test helper functions) into a new test-only translation unit (e.g., audio_engine_test.cpp) that `#includes` the original audio_engine.h, leaving audio_engine.h with only production declarations/definitions and no test symbols; update build files to compile the new test TU only in test targets and delete the test include/TEST blocks from the header.src/gui/dialogs/file_dialog_native.cpp (1)
90-97:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick winEliminate shell command injection in save-dialog command construction.
At Line 90 and Line 124,
popen()executes shell commands built from function parameters (default_name,filter_desc,filter_ext) without escaping. A crafted value containing quotes/shell operators can break out of the intended argument and execute arbitrary commands.Suggested fix direction
- FILE* pipe = popen(cmd.c_str(), "r"); + // Avoid shell interpolation; use fork/exec (as in other dialog paths) + // and pass arguments as discrete argv entries, or strictly escape every + // interpolated token before constructing shell command strings.Also applies to: 124-133, 147-149
🤖 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.cpp` around lines 90 - 97, The code builds shell command strings (cmd) using unescaped user-controlled variables (default_name, filter_desc, filter_ext) and passes them to popen(), allowing shell injection; replace this by invoking /usr/bin/osascript (or the AppleScript API) without a shell: construct an argv array with program "/usr/bin/osascript" and separate "-e" arguments (one per script fragment) and call posix_spawn/execvp (or use NSTask/NSAppleScript on macOS) so arguments are passed directly rather than concatenated into a shell command; update the code paths that build cmd in file_dialog_native.cpp (the blocks that create the "choose file name" and related osascript commands) to stop using popen(cmd.c_str(), "r") with interpolated strings and instead spawn the process with safe argv passing or use native Cocoa APIs to present the save/open dialogs.
Removed the early '#define fork() (-1)' which conflicted with the later '#define fork mock_fork' redefinition on Linux. The compiler is stricter than macOS about redefining function-like macros as object-like macros. Moved POSIX system headers (unistd.h, fcntl.h, sys/types.h, sys/wait.h) to the top of the file so that pid_t and other types are available for mock function signatures before any macros are defined.
2 participants
What does this PR do?
Adds and enables GUI module test coverage infrastructure for Issue #270.
Changes made
test_gui_manager.cppcoverage tests.amplitron-teststarget to include required GUI sources.AMPLITRON_VERSION).amplitron-tests.exe.Related Issue
Fixes #270
Type of Change
How Was This Tested?
.\Debug\amplitron-tests.exeResults
290 tests passed
1 unrelated preset-loading test failed:
json_can_load_existing_factory_presetsChecklist
./amplitron-tests)Screenshots / Demo
N/A (test and build infrastructure changes)
Summary by CodeRabbit
Bug Fixes
Tests