Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 3 additions & 6 deletions src/EditorViewport_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,6 @@ class EditorViewportTest : public ::testing::Test {
Manager::kill();
QThread::msleep(50);

// EditorViewport.cpp is excluded from LCOV and the suite is unstable under
// GitHub's headless Ogre/X11 setup, where it intermittently crashes with SIGSEGV.
if (qEnvironmentVariableIsSet("GITHUB_ACTIONS") || qEnvironmentVariableIsSet("CI")) {
GTEST_SKIP() << "Skipping: EditorViewport tests are unstable in headless CI";
}

try {
mainWindow = new MainWindow();
} catch (...) {
Expand All @@ -38,6 +32,9 @@ class EditorViewportTest : public ::testing::Test {
void TearDown() override {
delete mainWindow;
mainWindow = nullptr;
if (app) {
app->processEvents();
}
Manager::kill();
QThread::msleep(50);
}
Expand Down
223 changes: 223 additions & 0 deletions src/MCPServer_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,12 @@
#include <QTemporaryDir>
#include <memory>
#include <QMainWindow>
#include <unistd.h>

#define private public
#include "MCPServer.h"
#undef private
Comment on lines +14 to +18

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "POSIX transport APIs referenced from src/MCPServer_test.cpp:"
rg -n -C2 '<unistd\.h>|\b(pipe|read|write|close)\s*\(' src/MCPServer_test.cpp

echo
echo "Windows guards in src/MCPServer_test.cpp:"
rg -n -C1 'Q_OS_WIN' src/MCPServer_test.cpp || true

Repository: fernandotonon/QtMeshEditor

Length of output: 1578


Guard POSIX APIs with #ifndef Q_OS_WIN for Windows compatibility.

The file uses <unistd.h> and POSIX functions (pipe, read, write, close) without platform guards. These APIs are unavailable on Windows.

Wrap the include and all POSIX usage in:

  • Line 14: #include <unistd.h>
  • Lines 107–118: pipe() and close() in SetUp() and TearDown()
  • Lines 3749–3772: pipe(), write(), and close() in the OnReadyRead test

Either guard with #ifndef Q_OS_WIN or provide Windows-safe alternatives (e.g., named pipes or sockets).

🤖 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 - 18, Wrap POSIX-only code in
MCPServer_test.cpp with a Windows guard: surround the `#include` <unistd.h> and
every POSIX call (pipe(), read(), write(), close()) used in the SetUp() and
TearDown() methods and the OnReadyRead test with `#ifndef` Q_OS_WIN ... `#endif` so
Windows builds skip them; in places where behavior is required on Windows,
replace or provide an alternative implementation (e.g., use a named pipe or
socket mock) inside the same guarded blocks. Specifically update the include
near the top, the pipe()/close() usage in SetUp() and TearDown(), and the
pipe()/write()/close() sequence in the OnReadyRead test to be compiled only when
Q_OS_WIN is not defined, and add Windows-safe fallbacks or test skips for those
tests.


#include "Manager.h"
#include "MeshInfoOverlay.h"
#include "PrimitiveObject.h"
Expand All @@ -32,6 +37,32 @@ static bool isError(const QJsonObject &result)
return result["isError"].toBool(false);
}

static QByteArray readTransportMessage(int readFd)
{
QByteArray response;
char buffer[4096];
ssize_t bytesRead = read(readFd, buffer, sizeof(buffer));
if (bytesRead > 0) {
response.append(buffer, bytesRead);
}
return response;
}
Comment on lines +40 to +49

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Avoid blocking read() in transport helper; it can hang the test run.

readTransportMessage() does a single blocking read(). If processMessage() does not emit a frame (or emits later), processAndRead() can stall indefinitely and make CI flaky. Use non-blocking fd + bounded poll/read loop with timeout.

💡 Suggested fix
 static QByteArray readTransportMessage(int readFd)
 {
     QByteArray response;
     char buffer[4096];
-    ssize_t bytesRead = read(readFd, buffer, sizeof(buffer));
-    if (bytesRead > 0) {
-        response.append(buffer, bytesRead);
-    }
+    QElapsedTimer timer;
+    timer.start();
+    while (timer.elapsed() < 1000) {
+        ssize_t bytesRead = read(readFd, buffer, sizeof(buffer));
+        if (bytesRead > 0) {
+            response.append(buffer, bytesRead);
+            break;
+        }
+        QCoreApplication::processEvents(QEventLoop::AllEvents, 10);
+        QThread::msleep(5);
+    }
     return response;
 }

Also applies to: 126-130

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/MCPServer_test.cpp` around lines 40 - 49, readTransportMessage currently
does a single blocking read which can hang tests; change it to set readFd to
non-blocking, then use a bounded poll/select loop with a short timeout to wait
for readability and perform repeated reads appending into response until no more
data or the overall timeout elapses, handling EAGAIN/EWOULDBLOCK between reads,
and finally restore the original file flags; apply the same non-blocking
poll/read pattern to the other helper usages mentioned (lines 126-130 / related
helper functions) so processAndRead() cannot stall indefinitely.


static QJsonObject extractJsonBody(const QByteArray &transport)
{
const int headerEnd = transport.indexOf("\r\n\r\n");
EXPECT_NE(headerEnd, -1);
if (headerEnd == -1) {
return QJsonObject();
}

QJsonParseError error;
const QJsonDocument doc = QJsonDocument::fromJson(transport.mid(headerEnd + 4), &error);
EXPECT_EQ(error.error, QJsonParseError::NoError);
EXPECT_TRUE(doc.isObject());
return doc.object();
}

class MCPServerTest : public ::testing::Test
{
protected:
Expand Down Expand Up @@ -65,6 +96,49 @@ class MCPServerTest : public ::testing::Test
std::unique_ptr<MCPServer> server;
};

class MCPServerProtocolTest : public ::testing::Test
{
protected:
void SetUp() override
{
app = qobject_cast<QApplication*>(QCoreApplication::instance());
ASSERT_NE(app, nullptr);
server = std::make_unique<MCPServer>();
ASSERT_EQ(pipe(outputPipe), 0);
server->setOutputFd(outputPipe[1]);
}

void TearDown() override
{
server.reset();
if (outputPipe[0] != -1) {
close(outputPipe[0]);
}
if (outputPipe[1] != -1) {
close(outputPipe[1]);
}
Manager::kill();
if (app) {
app->processEvents();
}
}

QJsonObject processAndRead(const QByteArray &payload)
{
server->processMessage(payload);
return extractJsonBody(readTransportMessage(outputPipe[0]));
}

QByteArray makeTransport(const QByteArray &json)
{
return QByteArray("Content-Length: ") + QByteArray::number(json.size()) + "\r\n\r\n" + json;
}

QApplication* app = nullptr;
std::unique_ptr<MCPServer> server;
int outputPipe[2] = {-1, -1};
};

// --- Material tools ---

TEST_F(MCPServerTest, CreateMaterial)
Expand Down Expand Up @@ -3548,3 +3622,152 @@ TEST_F(MCPServerTest, OpenScene_ValidFile_LoadsEntities)
EXPECT_TRUE(resultText.contains("Scene loaded"));
EXPECT_TRUE(resultText.contains("scene node(s)"));
}

TEST_F(MCPServerProtocolTest, ProcessMessageRejectsInvalidJson)
{
const QJsonObject response = processAndRead("not-json");
ASSERT_TRUE(response.contains("error"));
EXPECT_EQ(response["error"].toObject()["code"].toInt(), -32700);
EXPECT_TRUE(response["error"].toObject()["message"].toString().contains("Parse error"));
}

TEST_F(MCPServerProtocolTest, ProcessMessageRejectsNonObjectRequest)
{
const QByteArray payload = QJsonDocument(QJsonArray{1, 2, 3}).toJson(QJsonDocument::Compact);
const QJsonObject response = processAndRead(payload);
ASSERT_TRUE(response.contains("error"));
EXPECT_EQ(response["error"].toObject()["code"].toInt(), -32600);
EXPECT_TRUE(response["error"].toObject()["message"].toString().contains("Invalid Request"));
}

TEST_F(MCPServerProtocolTest, ProcessMessageInitializeRespondsWithCapabilities)
{
const QJsonObject request{
{"jsonrpc", "2.0"},
{"id", 7},
{"method", "initialize"},
{"params", QJsonObject{}}
};

const QJsonObject response = processAndRead(QJsonDocument(request).toJson(QJsonDocument::Compact));
ASSERT_TRUE(response.contains("result"));
EXPECT_EQ(response["id"].toInt(), 7);
EXPECT_TRUE(server->m_initialized);

const QJsonObject result = response["result"].toObject();
EXPECT_EQ(result["protocolVersion"].toString(), "2024-11-05");
EXPECT_EQ(result["serverInfo"].toObject()["name"].toString(), "QtMeshEditor");
EXPECT_TRUE(result["capabilities"].toObject().contains("tools"));
EXPECT_TRUE(result["capabilities"].toObject().contains("resources"));
}

TEST_F(MCPServerProtocolTest, ProcessMessageUnknownNotificationLeavesServerStateUnchanged)
{
const QJsonObject request{
{"jsonrpc", "2.0"},
{"method", "notifications/custom"},
{"params", QJsonObject{{"value", 1}}}
};

EXPECT_FALSE(server->m_initialized);
server->processMessage(QJsonDocument(request).toJson(QJsonDocument::Compact));
EXPECT_FALSE(server->m_initialized);
EXPECT_TRUE(server->m_buffer.isEmpty());
}

TEST_F(MCPServerProtocolTest, ProcessMessageUnknownMethodReturnsMethodNotFound)
{
const QJsonObject request{
{"jsonrpc", "2.0"},
{"id", "abc"},
{"method", "totally/unknown"},
{"params", QJsonObject{}}
};

const QJsonObject response = processAndRead(QJsonDocument(request).toJson(QJsonDocument::Compact));
ASSERT_TRUE(response.contains("error"));
EXPECT_EQ(response["id"].toString(), "abc");
EXPECT_EQ(response["error"].toObject()["code"].toInt(), -32601);
EXPECT_TRUE(response["error"].toObject()["message"].toString().contains("Method not found"));
}

TEST_F(MCPServerProtocolTest, HandleResourcesListReturnsExpectedUris)
{
const QJsonObject result = server->handleResourcesList();
const QJsonArray resources = result["resources"].toArray();
ASSERT_EQ(resources.size(), 2);
EXPECT_EQ(resources[0].toObject()["uri"].toString(), "qtmesheditor://material/current");
EXPECT_EQ(resources[1].toObject()["uri"].toString(), "qtmesheditor://scene/info");
}

TEST_F(MCPServerProtocolTest, HandleResourcesReadCurrentMaterialWithoutMainWindowReturnsPlaceholder)
{
const QJsonObject result = server->handleResourcesRead(QJsonObject{{"uri", "qtmesheditor://material/current"}});
const QJsonArray contents = result["contents"].toArray();
ASSERT_EQ(contents.size(), 1);
EXPECT_EQ(contents[0].toObject()["mimeType"].toString(), "text/plain");
EXPECT_TRUE(contents[0].toObject()["text"].toString().contains("No material currently loaded"));
}

TEST_F(MCPServerProtocolTest, HandleResourcesReadSceneInfoReturnsSerializedText)
{
ASSERT_TRUE(tryInitOgre());
createStandardOgreMaterials();

Comment on lines +3712 to +3714

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Skip this OGRE-backed test when headless init is unavailable.

Every other OGRE-dependent test in this file skips on tryInitOgre() failure. Hard-failing here makes the protocol suite brittle on runners where the display stack is unavailable even though the protocol path itself is fine.

💡 Suggested change
-    ASSERT_TRUE(tryInitOgre());
-    createStandardOgreMaterials();
+    if (!tryInitOgre()) {
+        GTEST_SKIP() << "Skipping: Ogre initialization failed";
+    }
+    createStandardOgreMaterials();
Based on learnings, "Tests must work under Xvfb (headless X11) — avoid assumptions about a real display."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/MCPServer_test.cpp` around lines 3712 - 3714, The test currently
hard-fails on OGRE init via ASSERT_TRUE(tryInitOgre()); change it to skip when
OGRE cannot initialize: call tryInitOgre() and if it returns false, invoke the
test skip mechanism (e.g. GTEST_SKIP() with a short message) and return early so
the rest of the test (including createStandardOgreMaterials()) is not run;
update the block around tryInitOgre() / createStandardOgreMaterials()
accordingly.

const QJsonObject result = server->handleResourcesRead(QJsonObject{{"uri", "qtmesheditor://scene/info"}});
const QJsonArray contents = result["contents"].toArray();
ASSERT_EQ(contents.size(), 1);
EXPECT_EQ(contents[0].toObject()["mimeType"].toString(), "application/json");
EXPECT_TRUE(contents[0].toObject()["text"].toString().contains("Scene Information"));
}

TEST_F(MCPServerProtocolTest, BuildToolsListContainsCoreToolDefinitions)
{
const QJsonArray tools = server->buildToolsList();
EXPECT_GE(tools.size(), 25);

bool sawCreateMaterial = false;
bool sawOpenScene = false;
for (const QJsonValue &value : tools) {
const QJsonObject tool = value.toObject();
if (tool["name"].toString() == "create_material") {
sawCreateMaterial = true;
EXPECT_TRUE(tool.contains("inputSchema"));
}
if (tool["name"].toString() == "open_scene") {
sawOpenScene = true;
}
}

EXPECT_TRUE(sawCreateMaterial);
EXPECT_TRUE(sawOpenScene);
}

TEST_F(MCPServerProtocolTest, OnReadyReadRecoversAfterInvalidHeaderAndParsesMessage)
{
int inputPipe[2] = {-1, -1};
ASSERT_EQ(pipe(inputPipe), 0);

server->m_stdinFd = inputPipe[0];
server->m_stdinNotifier = new QSocketNotifier(server->m_stdinFd, QSocketNotifier::Read, server.get());

const QJsonObject request{
{"jsonrpc", "2.0"},
{"id", 3},
{"method", "ping"},
{"params", QJsonObject{}}
};
const QByteArray payload = QByteArray("garbage\r\n\r\n") + makeTransport(QJsonDocument(request).toJson(QJsonDocument::Compact));
ASSERT_GT(write(inputPipe[1], payload.constData(), payload.size()), 0);

Comment on lines +3760 to +3762

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Assert full pipe write to avoid truncated-frame flakes.

ASSERT_GT(write(...), 0) accepts partial writes; then onReadyRead() may parse a truncated payload intermittently. Assert written == payload.size().

💡 Suggested fix
-    ASSERT_GT(write(inputPipe[1], payload.constData(), payload.size()), 0);
+    const ssize_t written = write(inputPipe[1], payload.constData(), payload.size());
+    ASSERT_EQ(written, static_cast<ssize_t>(payload.size()));
📝 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.

Suggested change
const QByteArray payload = QByteArray("garbage\r\n\r\n") + makeTransport(QJsonDocument(request).toJson(QJsonDocument::Compact));
ASSERT_GT(write(inputPipe[1], payload.constData(), payload.size()), 0);
const QByteArray payload = QByteArray("garbage\r\n\r\n") + makeTransport(QJsonDocument(request).toJson(QJsonDocument::Compact));
const ssize_t written = write(inputPipe[1], payload.constData(), payload.size());
ASSERT_EQ(written, static_cast<ssize_t>(payload.size()));
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/MCPServer_test.cpp` around lines 3760 - 3762, The test currently uses
ASSERT_GT(write(inputPipe[1], payload.constData(), payload.size()), 0) which
permits partial writes and can cause onReadyRead() to receive truncated frames;
change the assertion to verify the full payload was written by capturing the
return value of write(...) into a variable (e.g., ssize_t written) and asserting
written == payload.size() so the test fails on partial writes and eliminates
flaky truncated-frame behavior when writing the QByteArray payload constructed
with makeTransport(QJsonDocument(request).toJson(...)).

server->onReadyRead();

const QJsonObject response = extractJsonBody(readTransportMessage(outputPipe[0]));
EXPECT_EQ(response["id"].toInt(), 3);
EXPECT_TRUE(response["result"].toObject().isEmpty());

delete server->m_stdinNotifier;
server->m_stdinNotifier = nullptr;
close(inputPipe[0]);
close(inputPipe[1]);
}
Loading