Skip to content

Add test to ensure MuEditor is removed when flag is off (#359) #362

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
guptapiyush16 wants to merge 4 commits into
base: main
Choose a base branch
from
+50 −1
Open

Add test to ensure MuEditor is removed when flag is off (#359) #362

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4b4d074
Add test to ensure MuEditor is removed when flag is off (#359)
guptapiyush16 May 3, 2026
f0d9b9c
Update CMakeLists.txt
guptapiyush16 May 6, 2026
dd99b68
Update CMakeLists.txt
guptapiyush16 May 6, 2026
8b3f3fd
Update CMakeLists.txt
guptapiyush16 May 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/.gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
/*.dat
*.sdf
.tmp/
.tmp\
Copy link
Copy Markdown
Collaborator

@Mosch0512 Mosch0512 May 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

*.bak
*.sbr
*.tlog
Expand Down
2 changes: 2 additions & 0 deletions tests/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,3 +51,5 @@ function(mu_add_test)
endfunction()

add_subdirectory(text)

add_subdirectory(editor)
20 changes: 20 additions & 0 deletions tests/editor/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# Editor-specific tests only.
# Shared test harness setup (doctest include, wine setup, mu_test_main,
Comment on lines +1 to +2
Copy link
Copy Markdown
Collaborator

@Mosch0512 Mosch0512 May 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# and mu_add_test) must remain in the top-level tests/CMakeLists.txt.

# Add Editor Leak Test (only runs when ENABLE_EDITOR is OFF)
if(NOT ENABLE_EDITOR)
# Generate a text file containing all sources of the Main target
file(GENERATE
OUTPUT "${CMAKE_CURRENT_BINARY_DIR}/main_sources.txt"
CONTENT "$<JOIN:$<TARGET_PROPERTY:Main,SOURCES>,\n>")

# 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()
Comment on lines +14 to +17
Copy link
Copy Markdown
Collaborator

@Mosch0512 Mosch0512 May 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

message(STATUS "Skipping test_editor_leak: Python3 interpreter not found")
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot May 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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")

Comment on lines +12 to +18
Copy link
Copy Markdown
Collaborator

@Mosch0512 Mosch0512 May 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

endif()
endif()
28 changes: 28 additions & 0 deletions tests/editor/test_leak.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import sys
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot May 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
import sys
import sys
import os


def main():
if len(sys.argv) < 2:
print("Usage: test_leak.py <sources_file>")
sys.exit(1)

sources_file = sys.argv[1]

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('\\', '/')]
Comment on lines +13 to +16
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot May 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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('\\', '/')]

Comment on lines +13 to +16
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot May 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
# 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('/')]

Comment on lines +13 to +16
Copy link
Copy Markdown
Collaborator

@Mosch0512 Mosch0512 May 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_sources and add a one-line docstring saying it only covers path-level leaks.
  • Broaden the test to match its name: add a second assertion that _EDITOR is not in $<TARGET_PROPERTY:Main,COMPILE_DEFINITIONS> when ENABLE_EDITOR=OFF.

Either is fine - but the current name + scope mismatch will mislead future readers.


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}")
Comment on lines +10 to +21
sys.exit(1)
else:
print("PASS: No MuEditor files leaked into the build.")
sys.exit(0)

if __name__ == '__main__':
main()
Loading