[codex] expand MCPServer coverage tests - #263
Conversation
📝 WalkthroughWalkthroughThis PR expands the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf7c0de8f3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| f.close(); | ||
|
|
||
| // Remove all permissions to force open failure on read. | ||
| ASSERT_TRUE(QFile::setPermissions(filePath, QFileDevice::Permissions())); |
There was a problem hiding this comment.
ReadFile_CannotOpenFileReturnsError assumes that clearing all file permissions will make read_file fail, but that is not true when tests run as root/elevated user (common in containerized CI): the file remains readable, so server->callTool("read_file", ...) succeeds and this test fails spuriously. This makes the new MCPServer suite environment-dependent rather than deterministic; use a failure mode that does not depend on effective UID.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
src/MCPServer_test.cpp (1)
4073-4100:⚠️ Potential issue | 🟠 MajorGuard unprotected POSIX APIs for Windows compatibility.
The file includes
<unistd.h>and<fcntl.h>at file scope (lines 15–16) without platform guards, and newly added test functions at lines 4073–4100, 4102–4136, 4138–4165, and 4167–4186 usepipe(),write(),close(),read(), andfcntl()calls that are undefined on Windows. Wrap the includes at the top with#ifndef Q_OS_WINand guard these POSIX-dependent test functions (or helper functions) with the same guard to prevent Windows build failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MCPServer_test.cpp` around lines 4073 - 4100, The POSIX headers and APIs used in MCPServer_test.cpp are not guarded for Windows; wrap the top-of-file includes of <unistd.h> and <fcntl.h> with `#ifndef` Q_OS_WIN and similarly guard the tests that call pipe(), write(), read(), close(), and fcntl() so they are excluded on Windows. Specifically, surround the includes and the POSIX-dependent test cases (e.g., TEST_F named OnReadyReadRecoversAfterInvalidHeaderAndParsesMessage and the other newly added tests) or any helper functions they use with `#ifndef` Q_OS_WIN / `#endif` so Windows builds won't try to compile undefined POSIX symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/MCPServer_test.cpp`:
- Around line 4839-4855: The test uses
reinterpret_cast<MainWindow*>(&fakeWindow) which is undefined; replace that cast
with a real MainWindow instance: instantiate a MainWindow with new MainWindow(),
attach the MaterialEditorQML child to it (same way you did with fakeWindow),
call server->setMainWindow(theNewMainWindow) before calling
server->handleResourcesRead(...), and clean up by server->setMainWindow(nullptr)
and deleting the MainWindow after the test; this prevents undefined vtable
access in MCPServer::handleResourcesRead which calls
m_mainWindow->findChild<MaterialEditorQML*>() and other MainWindow-specific
methods.
---
Outside diff comments:
In `@src/MCPServer_test.cpp`:
- Around line 4073-4100: The POSIX headers and APIs used in MCPServer_test.cpp
are not guarded for Windows; wrap the top-of-file includes of <unistd.h> and
<fcntl.h> with `#ifndef` Q_OS_WIN and similarly guard the tests that call pipe(),
write(), read(), close(), and fcntl() so they are excluded on Windows.
Specifically, surround the includes and the POSIX-dependent test cases (e.g.,
TEST_F named OnReadyReadRecoversAfterInvalidHeaderAndParsesMessage and the other
newly added tests) or any helper functions they use with `#ifndef` Q_OS_WIN /
`#endif` so Windows builds won't try to compile undefined POSIX symbols.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| { | ||
| QMainWindow fakeWindow; | ||
| auto* matEditor = new MaterialEditorQML(&fakeWindow); | ||
| matEditor->setMaterialName("LiveMat"); | ||
| matEditor->setMaterialText("material LiveMat\n{\n}\n"); | ||
| server->setMainWindow(reinterpret_cast<MainWindow*>(&fakeWindow)); | ||
|
|
||
| const QJsonObject result = server->handleResourcesRead( | ||
| QJsonObject{{"uri", "qtmesheditor://material/current"}}); | ||
| const QJsonArray contents = result["contents"].toArray(); | ||
| ASSERT_EQ(contents.size(), 1); | ||
| const QString text = contents[0].toObject()["text"].toString(); | ||
| EXPECT_TRUE(text.contains("LiveMat")); | ||
| EXPECT_TRUE(text.contains("material")); | ||
|
|
||
| server->setMainWindow(nullptr); | ||
| delete matEditor; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate potentially unsafe downcasts introduced/used in this test file.
rg -n -C2 'QMainWindow fakeWindow|reinterpret_cast<MainWindow\*>\(&fakeWindow\)' src/MCPServer_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 1882
🏁 Script executed:
# Find MainWindow class definition
fd -e h -e cpp | xargs grep -l "class MainWindow" | head -5Repository: fernandotonon/QtMeshEditor
Length of output: 159
🏁 Script executed:
# Look at MainWindow.h to understand its inheritance
rg -A 5 "^class MainWindow" --type hRepository: fernandotonon/QtMeshEditor
Length of output: 1345
🏁 Script executed:
# Check how setMainWindow uses the MainWindow* parameter
rg -B 3 -A 10 "void.*setMainWindow" src/MCPServer.{h,cpp}Repository: fernandotonon/QtMeshEditor
Length of output: 1144
🏁 Script executed:
# Find how m_mainWindow is dereferenced/used in MCPServer.cpp
rg -n "m_mainWindow" src/MCPServer.cpp | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 1074
🏁 Script executed:
# Look for dereferencing of m_mainWindow in the specific test methods
rg -B 5 -A 15 "HandleResourcesReadCurrentMaterialUsesMaterialEditorTextWhenAvailable" src/MCPServer_test.cpp | grep -A 15 "HandleResourcesReadCurrentMaterialUsesMaterialEditorTextWhenAvailable"Repository: fernandotonon/QtMeshEditor
Length of output: 812
🏁 Script executed:
# Check if there are any legitimate MainWindow instances created in the tests or in other test files
rg "new MainWindow" src/ --type cppRepository: fernandotonon/QtMeshEditor
Length of output: 563
🏁 Script executed:
# Check if MCPServer_test.cpp has any legitimate MainWindow instances
rg -n "new MainWindow" src/MCPServer_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 116
Replace reinterpret_cast<MainWindow*>(&fakeWindow) with a real MainWindow instance in all affected tests.
The reinterpret_cast from a QMainWindow stack object to MainWindow* is undefined behavior. Code in MCPServer.cpp actively dereferences m_mainWindow with MainWindow-specific calls: handleResourcesRead() (line 541) calls m_mainWindow->findChild<MaterialEditorQML*>(), which will invoke undefined behavior when the vtable is read from the miscast object.
This pattern appears in 6 tests (lines 3009–3013, 4840–4844, 4890–4891, 4992–4993, 5032–5033, 5044–5051). Replace all instances with legitimate MainWindow instances using new MainWindow(), which is already used elsewhere in the same test file (line 199).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MCPServer_test.cpp` around lines 4839 - 4855, The test uses
reinterpret_cast<MainWindow*>(&fakeWindow) which is undefined; replace that cast
with a real MainWindow instance: instantiate a MainWindow with new MainWindow(),
attach the MaterialEditorQML child to it (same way you did with fakeWindow),
call server->setMainWindow(theNewMainWindow) before calling
server->handleResourcesRead(...), and clean up by server->setMainWindow(nullptr)
and deleting the MainWindow after the test; this prevents undefined vtable
access in MCPServer::handleResourcesRead which calls
m_mainWindow->findChild<MaterialEditorQML*>() and other MainWindow-specific
methods.
|
This reverts commit 05bbcc8.
1 participant
Summary
src/MCPServer_test.cppWhy
Coverage regressed after recent merges and MCPServer had many untested branches. This PR targets those missing paths directly with deterministic unit/integration-style tests.
Validation
xvfb-run -a ./build_cov_local/debug/UnitTests --gtest_filter='MCPServer*'gcovforsrc/MCPServer.cppimproved from ~90.34% to ~91.85% line coverage in local coverage buildSummary by CodeRabbit