Increase MCPServer coverage - #227
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThe PR adds comprehensive test coverage to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 52-72: The helper readTransportMessageNonBlocking uses POSIX fcntl
and must be made platform-safe: wrap the current implementation in a `#ifndef`
_WIN32 guard and add a Windows branch that sets non-blocking mode via
ioctlsocket/FIONBIO (or returns an empty QByteArray and lets tests skip on
Windows); ensure you restore the original blocking state (fcntl on POSIX,
ioctlsocket on Windows) and reference readTransportMessageNonBlocking in any
test skip logic so Windows CI either uses the ioctlsocket-based path or the
tests that call readTransportMessageNonBlocking are skipped.
- Around line 14-15: The code includes the POSIX header <fcntl.h> and uses
fcntl() in readTransportMessageNonBlocking which will fail to compile on
Windows; wrap the `#include` <fcntl.h> and the POSIX-specific implementation of
readTransportMessageNonBlocking with a platform guard (e.g. `#ifndef` _WIN32 /
`#else` / `#endif`), and provide a Windows-specific alternative implementation or
stub that uses the appropriate Windows non-blocking APIs (e.g. ioctlsocket/WSA*
for sockets) or returns a clear unsupported/error path; modify the function
(readTransportMessageNonBlocking) and any uses of fcntl to be conditionally
compiled so the build succeeds on both POSIX and Windows.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| #include <unistd.h> | ||
| #include <fcntl.h> |
There was a problem hiding this comment.
Platform compatibility: fcntl.h is POSIX-only and will not compile on Windows.
The include and the readTransportMessageNonBlocking function below use POSIX fcntl() which is unavailable on Windows. As per coding guidelines, platform-specific APIs must be guarded.
🛠️ Proposed fix with platform guard
`#include` <unistd.h>
+#ifndef Q_OS_WIN
`#include` <fcntl.h>
+#endif📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #include <unistd.h> | |
| #include <fcntl.h> | |
| `#include` <unistd.h> | |
| `#ifndef` Q_OS_WIN | |
| `#include` <fcntl.h> | |
| `#endif` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MCPServer_test.cpp` around lines 14 - 15, The code includes the POSIX
header <fcntl.h> and uses fcntl() in readTransportMessageNonBlocking which will
fail to compile on Windows; wrap the `#include` <fcntl.h> and the POSIX-specific
implementation of readTransportMessageNonBlocking with a platform guard (e.g.
`#ifndef` _WIN32 / `#else` / `#endif`), and provide a Windows-specific alternative
implementation or stub that uses the appropriate Windows non-blocking APIs (e.g.
ioctlsocket/WSA* for sockets) or returns a clear unsupported/error path; modify
the function (readTransportMessageNonBlocking) and any uses of fcntl to be
conditionally compiled so the build succeeds on both POSIX and Windows.
| static QByteArray readTransportMessageNonBlocking(int readFd) | ||
| { | ||
| const int flags = fcntl(readFd, F_GETFL, 0); | ||
| if (flags == -1) { | ||
| return {}; | ||
| } | ||
|
|
||
| if (fcntl(readFd, F_SETFL, flags | O_NONBLOCK) == -1) { | ||
| return {}; | ||
| } | ||
|
|
||
| QByteArray response; | ||
| char buffer[4096]; | ||
| ssize_t bytesRead = read(readFd, buffer, sizeof(buffer)); | ||
| if (bytesRead > 0) { | ||
| response.append(buffer, bytesRead); | ||
| } | ||
|
|
||
| fcntl(readFd, F_SETFL, flags); | ||
| return response; | ||
| } |
There was a problem hiding this comment.
Platform compatibility: fcntl() is POSIX-only.
This function uses POSIX fcntl() which is unavailable on Windows. Guard the implementation or provide a Windows alternative.
🛠️ Proposed fix with platform guard
static QByteArray readTransportMessageNonBlocking(int readFd)
{
+#ifdef Q_OS_WIN
+ // fcntl not available on Windows; return empty to indicate no data
+ Q_UNUSED(readFd);
+ return {};
+#else
const int flags = fcntl(readFd, F_GETFL, 0);
if (flags == -1) {
return {};
}
if (fcntl(readFd, F_SETFL, flags | O_NONBLOCK) == -1) {
return {};
}
QByteArray response;
char buffer[4096];
ssize_t bytesRead = read(readFd, buffer, sizeof(buffer));
if (bytesRead > 0) {
response.append(buffer, bytesRead);
}
fcntl(readFd, F_SETFL, flags);
return response;
+#endif
}Note: Tests relying on this helper will need to be skipped on Windows or the Windows implementation enhanced using ioctlsocket() with FIONBIO.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static QByteArray readTransportMessageNonBlocking(int readFd) | |
| { | |
| const int flags = fcntl(readFd, F_GETFL, 0); | |
| if (flags == -1) { | |
| return {}; | |
| } | |
| if (fcntl(readFd, F_SETFL, flags | O_NONBLOCK) == -1) { | |
| return {}; | |
| } | |
| QByteArray response; | |
| char buffer[4096]; | |
| ssize_t bytesRead = read(readFd, buffer, sizeof(buffer)); | |
| if (bytesRead > 0) { | |
| response.append(buffer, bytesRead); | |
| } | |
| fcntl(readFd, F_SETFL, flags); | |
| return response; | |
| } | |
| static QByteArray readTransportMessageNonBlocking(int readFd) | |
| { | |
| `#ifdef` Q_OS_WIN | |
| // fcntl not available on Windows; return empty to indicate no data | |
| Q_UNUSED(readFd); | |
| return {}; | |
| `#else` | |
| const int flags = fcntl(readFd, F_GETFL, 0); | |
| if (flags == -1) { | |
| return {}; | |
| } | |
| if (fcntl(readFd, F_SETFL, flags | O_NONBLOCK) == -1) { | |
| return {}; | |
| } | |
| QByteArray response; | |
| char buffer[4096]; | |
| ssize_t bytesRead = read(readFd, buffer, sizeof(buffer)); | |
| if (bytesRead > 0) { | |
| response.append(buffer, bytesRead); | |
| } | |
| fcntl(readFd, F_SETFL, flags); | |
| return response; | |
| `#endif` | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MCPServer_test.cpp` around lines 52 - 72, The helper
readTransportMessageNonBlocking uses POSIX fcntl and must be made platform-safe:
wrap the current implementation in a `#ifndef` _WIN32 guard and add a Windows
branch that sets non-blocking mode via ioctlsocket/FIONBIO (or returns an empty
QByteArray and lets tests skip on Windows); ensure you restore the original
blocking state (fcntl on POSIX, ioctlsocket on Windows) and reference
readTransportMessageNonBlocking in any test skip logic so Windows CI either uses
the ioctlsocket-based path or the tests that call
readTransportMessageNonBlocking are skipped.
|
1 participant
Summary
Testing
Summary by CodeRabbit