Add test to ensure MuEditor is removed when flag is off (#359)#362
Add test to ensure MuEditor is removed when flag is off (#359)#362guptapiyush16 wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a CTest check to ensure MuEditor sources are not included in the Main target when ENABLE_EDITOR=OFF, helping prevent editor code from leaking into non-admin builds.
Changes:
- Added a Python-based CTest that scans the configured
Maintarget sources for anyMuEditorpaths whenENABLE_EDITORis disabled. - Updated
tests/CMakeLists.txtto generate amain_sources.txtfile viafile(GENERATE)and register the new test. - Adjusted
src/.gitignoreentries around.tmp/.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/editor/test_leak.py | New Python script that fails if any MuEditor file appears in the generated Main source list. |
| tests/CMakeLists.txt | Generates a sources manifest for Main and adds a CTest entry to run the leak check when ENABLE_EDITOR is off. |
| src/.gitignore | Modifies .tmp ignore patterns (currently introduces a duplicate .tmp/ entry). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the project's build integrity by implementing a regression test to ensure that optional components are properly excluded from the final binary. By leveraging CMake's generation capabilities and a lightweight verification script, the build process now automatically validates that the MuEditor source files do not inadvertently leak into the Main target when the feature is toggled off. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a leak detection test to ensure that MuEditor source files are not included in the build when the editor is disabled. This is implemented via a new Python script and CMake configuration. Feedback focuses on improving the robustness of the leak detection logic to avoid false positives, removing a duplicate entry in the .gitignore file, cleaning up an unused import, and specifying the file encoding when reading source lists.
|
@guptapiyush16 notify me once all conversations are resolved 👍🏼 |
guptapiyush16
force-pushed
the
add-editor-leak-test
branch
from
a0e0e95 to
4b4d074
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| with open(sources_file, 'r', encoding='utf-8') as f: | ||
| sources = f.read().splitlines() | ||
|
|
||
| # Normalise to forward slashes so the path-segment check is | ||
| # platform-independent and won't fire just because the repository | ||
| # happens to be cloned inside a directory called "MuEditor". | ||
| leaked_files = [s for s in sources if '/MuEditor/' in s.replace('\\', '/')] | ||
|
|
||
| if leaked_files: | ||
| print("FAIL: MuEditor leaked into the build! The following editor files were found in the Main target sources:") | ||
| for f in leaked_files: | ||
| print(f" - {f}") |
| # Normalise to forward slashes so the path-segment check is | ||
| # platform-independent and won't fire just because the repository | ||
| # happens to be cloned inside a directory called "MuEditor". | ||
| leaked_files = [s for s in sources if '/MuEditor/' in s.replace('\\', '/')] |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a leak test to ensure that editor-specific source files are not included in the 'Main' target when 'ENABLE_EDITOR' is disabled. The implementation includes a new CMake configuration and a Python script to verify source paths. Review feedback suggests increasing the visibility of the test skip message in CMake by using a warning instead of a status message and refining the path matching logic in the Python script to be more robust against false positives.
| add_test(NAME test_editor_leak | ||
| COMMAND Python3::Interpreter ${CMAKE_CURRENT_SOURCE_DIR}/test_leak.py "${CMAKE_CURRENT_BINARY_DIR}/main_sources.txt") | ||
| else() | ||
| message(STATUS "Skipping test_editor_leak: Python3 interpreter not found") |
There was a problem hiding this comment.
If the Python 3 interpreter is not found, the leak test is silently skipped with a STATUS message. Since this test is intended to ensure build integrity (preventing editor code from leaking into production builds), it should ideally be a WARNING or even a FATAL_ERROR (if BUILD_TESTING is enabled) to ensure it isn't accidentally missed in CI environments.
message(WARNING "Skipping test_editor_leak: Python3 interpreter not found")
| # Normalise to forward slashes so the path-segment check is | ||
| # platform-independent and won't fire just because the repository | ||
| # happens to be cloned inside a directory called "MuEditor". | ||
| leaked_files = [s for s in sources if '/MuEditor/' in s.replace('\\', '/')] |
There was a problem hiding this comment.
The current path matching logic is susceptible to false positives if the repository is cloned into a directory named MuEditor (e.g., /home/user/MuEditor/src/source/main.cpp), which contradicts the intent stated in the preceding comment. Since the MuEditor directory is located inside src, using a more specific pattern like /src/MuEditor/ would be more robust. Additionally, the leading slash in '/MuEditor/' would cause the test to miss relative paths (e.g., MuEditor/file.cpp), though the current GLOB_RECURSE in CMake produces absolute paths.
| leaked_files = [s for s in sources if '/MuEditor/' in s.replace('\\', '/')] | |
| leaked_files = [s for s in sources if '/src/MuEditor/' in s.replace('\\', '/')] |
There was a problem hiding this comment.
Code Review
This pull request introduces a leak test to ensure that editor-related source files are not included in the 'Main' target when the editor is disabled. The implementation includes a new CMake configuration and a Python script to verify source file paths. The review feedback suggests improving the Python script's robustness by using relative paths to the project root, which prevents false positives if the repository itself is stored in a directory containing 'MuEditor' in its name.
| @@ -0,0 +1,28 @@ | |||
| import sys | |||
There was a problem hiding this comment.
Import the os module to enable robust path handling. This is necessary for calculating relative paths to the project root, which helps avoid false positives in the leak check.
| import sys | |
| import sys | |
| import os |
| # Normalise to forward slashes so the path-segment check is | ||
| # platform-independent and won't fire just because the repository | ||
| # happens to be cloned inside a directory called "MuEditor". | ||
| leaked_files = [s for s in sources if '/MuEditor/' in s.replace('\\', '/')] |
There was a problem hiding this comment.
The current check for '/MuEditor/' will trigger a false positive if the repository is cloned into a directory that contains MuEditor in its path (e.g., /home/user/MuEditor/project/...). Although the comment suggests this is handled, the implementation actually checks the entire absolute path.
To fix this, calculate the path of each source file relative to the project root and check if MuEditor is a directory segment within that relative path.
| # Normalise to forward slashes so the path-segment check is | |
| # platform-independent and won't fire just because the repository | |
| # happens to be cloned inside a directory called "MuEditor". | |
| leaked_files = [s for s in sources if '/MuEditor/' in s.replace('\\', '/')] | |
| # Use relative paths to the project root to avoid false positives if the | |
| # repository itself is cloned into a directory named "MuEditor". | |
| project_root = os.path.abspath(os.path.join(os.path.dirname(__file__), '..', '..')) | |
| leaked_files = [s for s in sources if 'MuEditor' in os.path.relpath(s, project_root).replace('\\', '/').split('/')] |
| # Normalise to forward slashes so the path-segment check is | ||
| # platform-independent and won't fire just because the repository | ||
| # happens to be cloned inside a directory called "MuEditor". | ||
| leaked_files = [s for s in sources if '/MuEditor/' in s.replace('\\', '/')] |
There was a problem hiding this comment.
Test name overstates coverage.
This catches file-path leaks (sources whose path contains /MuEditor/). It does NOT catch the other (and arguably more common) way editor code reaches the binary: #ifdef _EDITOR blocks inside src/source/ files. A quick grep finds 59 files under src/source/ that gate code on _EDITOR. If someone accidentally removes the if(ENABLE_EDITOR) guard around the _EDITOR compile definition (src/CMakeLists.txt:217), every one of those #ifdef blocks compiles in and this test still reports PASS.
Two options:
- Narrow the name to match what it does: rename the CTest entry to something like
test_editor_dir_not_in_main_sourcesand add a one-line docstring saying it only covers path-level leaks. - Broaden the test to match its name: add a second assertion that
_EDITORis not in$<TARGET_PROPERTY:Main,COMPILE_DEFINITIONS>whenENABLE_EDITOR=OFF.
Either is fine - but the current name + scope mismatch will mislead future readers.
| # Add a test that runs a Python script to verify MuEditor isn't in those sources | ||
| find_package(Python3 COMPONENTS Interpreter QUIET) | ||
| if(Python3_Interpreter_FOUND) | ||
| add_test(NAME test_editor_leak | ||
| COMMAND Python3::Interpreter ${CMAKE_CURRENT_SOURCE_DIR}/test_leak.py "${CMAKE_CURRENT_BINARY_DIR}/main_sources.txt") | ||
| else() | ||
| message(STATUS "Skipping test_editor_leak: Python3 interpreter not found") |
There was a problem hiding this comment.
Silent skip defeats the regression guard.
A guard test that disappears on hosts without Python3 is the worst of both worlds: green CI on those hosts, surprise red on hosts that have it. The whole point of this test is to be unconditional.
I'd suggest dropping the Python dependency entirely and doing it in pure CMake - the logic is one regex check that cmake -P handles natively:
if(NOT ENABLE_EDITOR)
file(GENERATE
OUTPUT "${CMAKE_CURRENT_BINARY_DIR}/main_sources.txt"
CONTENT "$<JOIN:$<TARGET_PROPERTY:Main,SOURCES>,\n>")
add_test(NAME test_editor_dir_not_in_main_sources
COMMAND ${CMAKE_COMMAND}
-DSOURCES_FILE=${CMAKE_CURRENT_BINARY_DIR}/main_sources.txt
-P ${CMAKE_CURRENT_SOURCE_DIR}/check_editor_leak.cmake)
endif()with tests/editor/check_editor_leak.cmake:
file(STRINGS "${SOURCES_FILE}" sources)
set(leaked "")
foreach(s IN LISTS sources)
string(REPLACE "\\" "/" s_norm "${s}")
if(s_norm MATCHES "/MuEditor/")
list(APPEND leaked "${s}")
endif()
endforeach()
if(leaked)
message(FATAL_ERROR "MuEditor leaked into Main sources: ${leaked}")
endif()Self-contained, no language hop, no environment gap. If you prefer to keep Python, switch QUIET → REQUIRED so a missing interpreter fails loudly instead of silently.
| /*.dat | ||
| *.sdf | ||
| .tmp/ | ||
| .tmp\ |
There was a problem hiding this comment.
Unrelated change.
This .tmp\ deletion isn't related to the editor-leak test. Per docs/CODING_RULES.md §10 (keep diffs focused on one concern), please split this into its own commit or its own PR. The change itself is fine - .tmp\ was never valid gitignore syntax, the .tmp/ line above already handles the directory - but it doesn't belong here.
| # Editor-specific tests only. | ||
| # Shared test harness setup (doctest include, wine setup, mu_test_main, |
There was a problem hiding this comment.
Minor: stray leading space at the start of these two comment lines ( # Editor-specific tests only.). Should be flush left like the rest of the file.
| if(Python3_Interpreter_FOUND) | ||
| add_test(NAME test_editor_leak | ||
| COMMAND Python3::Interpreter ${CMAKE_CURRENT_SOURCE_DIR}/test_leak.py "${CMAKE_CURRENT_BINARY_DIR}/main_sources.txt") | ||
| else() |
There was a problem hiding this comment.
Minor: indentation here is 9 spaces vs 4 elsewhere in the file. Consistent 4-space indent inside the if(Python3_Interpreter_FOUND) block would match the surrounding style.
Summary
This PR adds an automated test to ensure that the
MuEditorcomponent is completely excluded from the build when theENABLE_EDITORflag is set toOFF.It utilizes CMake's
file(GENERATE)command to statically dump the final list of source files configured for theMaintarget during a build. A lightweight Python script is then hooked intoctestto parse those sources and fail the build if any paths containingMuEditoraccidentally leak through.Related issues
Closes #359
Checklist
docs/CODING_RULES.md.docs/build-guide.md).