test: add memory check for unit tests in ci#558
Merged
Miou-zora merged 7 commits intoApr 5, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis pull request introduces automated memory leak detection into the CI pipeline using macOS's native Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 |
|
![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: 3
🧹 Nitpick comments (1)
tools/xmake/check_leaks.lua (1)
17-17: Suppressing leaks output prevents debugging.Redirecting both stdout and stderr to
/dev/nullhides the actual leak information when leaks are detected. Consider showing output on failure or removing the suppression entirely.♻️ Proposed fix: show output or capture for failures
Option 1 - Remove suppression to always show output:
- local return_value = os.execv(leaks_tool, {"--atExit", "--", bin_path}, {try = true, stdout = os.nuldev(), stderr = os.nuldev()}) + local return_value = os.execv(leaks_tool, {"--atExit", "--", bin_path}, {try = true})Option 2 - Only show output when leaks are found (run twice):
- local return_value = os.execv(leaks_tool, {"--atExit", "--", bin_path}, {try = true, stdout = os.nuldev(), stderr = os.nuldev()}) + local return_value = os.execv(leaks_tool, {"--atExit", "--", bin_path}, {try = true, stdout = os.nuldev(), stderr = os.nuldev()}) if return_value ~= 0 then + print("Leak details for " .. target_name .. ":") + os.execv(leaks_tool, {"--atExit", "--", bin_path}, {try = true}) table.insert(failing_targets, target_name) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/xmake/check_leaks.lua` at line 17, The exec call to run the leaks tool (os.execv(leaks_tool, {"--atExit", "--", bin_path}, {try = true, stdout = os.nuldev(), stderr = os.nuldev()})) suppresses all output and hides useful leak diagnostics; modify the invocation in check_leaks.lua so stdout/stderr are not discarded — either remove the {stdout = os.nuldev(), stderr = os.nuldev()} options so leaks_tool prints directly, or capture the output (e.g., not discarding streams), check return_value, and on non-zero/failed runs print or log the captured stdout/stderr so leak details are visible; ensure you update the os.execv call around the leaks_tool and bin_path usage and handle return_value accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 149-153: The CI macOS step "Check memory leaks (macOS)" invokes
"xmake check_leaks" but the underlying script check_leaks.lua does not fail the
process when leaks are detected; update check_leaks.lua so that when a leak is
found it calls os.raise() (or otherwise raises an error) instead of merely
logging, ensuring the xmake check_leaks target returns a nonzero status and
makes the workflow step fail; locate the leak-detection branch/handler in
check_leaks.lua (the function or block that parses leak results) and replace the
silent/log-only behavior with os.raise() including an informative message.
In `@tools/xmake/check_leaks.lua`:
- Around line 57-64: The script currently only prints leaking targets but
doesn't signal failure; update the end of the check (the block that inspects
failing_targets) so that when `#failing_targets` > 0 it not only prints the list
but also exits non‑zero (e.g., call os.exit(1) or error with a clear message) to
fail the CI job; modify the code around the failing_targets handling (the block
that prints "Targets with memory leaks found:" and iterates over
failing_targets) to perform the non‑zero exit after printing.
- Line 9: The bin_path construction is incorrect because it appends a hardcoded
"debug" directory to target:targetdir() and target:filename(); replace that
logic to obtain the binary path from the target itself by using
target:targetfile() (or an equivalent API) instead of path.join(os.projectdir(),
target:targetdir(), "debug", target:filename()); update the code that sets
bin_path to call target:targetfile() so it works for debug/release and
platform-specific target layouts.
---
Nitpick comments:
In `@tools/xmake/check_leaks.lua`:
- Line 17: The exec call to run the leaks tool (os.execv(leaks_tool,
{"--atExit", "--", bin_path}, {try = true, stdout = os.nuldev(), stderr =
os.nuldev()})) suppresses all output and hides useful leak diagnostics; modify
the invocation in check_leaks.lua so stdout/stderr are not discarded — either
remove the {stdout = os.nuldev(), stderr = os.nuldev()} options so leaks_tool
prints directly, or capture the output (e.g., not discarding streams), check
return_value, and on non-zero/failed runs print or log the captured
stdout/stderr so leak details are visible; ensure you update the os.execv call
around the leaks_tool and bin_path usage and handle return_value accordingly.
🪄 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: a0daf330-1286-4ca4-b6e6-616143cfa6ab
📒 Files selected for processing (6)
.github/workflows/ci.yml.github/workflows/commit_norm_check.ymlsrc/engine/xmake.luatools/xmake/check_leaks.luatools/xmake/groups.luaxmake.lua
💤 Files with no reviewable changes (1)
- xmake.lua
Miou-zora
deleted the
447-feature-add-memory-checks-valgrind-leaks-for-unit-tests-in-ci
branch
This was referenced Apr 6, 2026
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.
Pull Request
Description
I added a way to check memory through
leakson macOS and put it in CI. For now it is not a mandatory to pass the CI.Related Issues (Put "None" if there are no related issues)
close #447
Type of Change
Please delete options that are not relevant.
Changes Made
List the main changes in this PR:
Testing
Describe the tests you ran to verify your changes. Please delete options that are not relevant.
xmake test)Test Environment
Screenshots/Videos (Put "None" if there are no related issues)
None
Documentation
Please delete options that are not relevant.
Checklist (Don't delete any options)
Breaking Changes (Put "None" if there are no related issues)
None
Additional Notes (Put "None" if there are no related issues)
None
Summary by CodeRabbit
Chores
New Features
Improvements