Development/incremental test coverage expansion [Test Branch]

[smanes0213]

No description provided.

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Suppressed comments (6)

Tests/unit/core/test_controller.cpp:126

• These tests assume an external "Dictionary" plugin is available in the ThunderTestRuntime environment. If it is not configured/installed, Controller.activate will return Core::ERROR_UNKNOWN_KEY (see Source/Thunder/Controller.cpp:1009-1011) and this test will fail deterministically. Consider skipping when Dictionary is unavailable so the unit test suite is self-contained and not environment-dependent.

    TEST_F(ControllerTest, ActivateDictionary_TransitionsToActivated)
    {
        // Ensure Dictionary starts deactivated.

        string response;
        uint32_t result = _runtime.Invoke(
            "Controller.activate",
            R"({"callsign":"Dictionary"})",
            response);

        EXPECT_EQ(result, Core::ERROR_NONE);
    }

Tests/unit/core/test_controller.cpp:139

• This test assumes the "Dictionary" plugin exists in the runtime. If it is not configured/installed, Controller.deactivate will return Core::ERROR_UNKNOWN_KEY and the test will fail. Skip the test when Dictionary is not present to avoid environment-dependent failures.

    TEST_F(ControllerTest, DeactivateDictionary_TransitionsToDeactivated)
    {
        // Ensure Dictionary starts activated.

        string response;
        uint32_t result = _runtime.Invoke(
            "Controller.deactivate",
            R"({"callsign":"Dictionary"})",
            response);

        EXPECT_EQ(result, Core::ERROR_NONE);
    }

Tests/unit/core/test_websocket_protocol.cpp:234

• RunTextServer() unlinks the Unix socket path before starting, but never removes it after the test completes. Because each test uses a unique /tmp path, this can leave behind many stale socket files across runs. Consider unlinking the connector path in stopServer() after joining the server thread.

        // Ensure the server thread is always joined before returning
        auto stopServer = [&]() {
            clientDone = true;
            if (serverThread.joinable()) {
                serverThread.join();
            }
            ::Thunder::Core::Singleton::Dispose();
        };

Tests/unit/core/test_websocket_protocol.cpp:391

• This test also leaves the Unix domain socket file behind (it unlinks before Open(), but never unlinks at the end). Unlinking in stopServer() keeps /tmp clean and avoids accumulating stale socket files.

        // RAII guard: always signal and join the server thread on exit
        auto stopServer = [&]() {
            clientsDone = true;
            if (serverThread.joinable()) {
                serverThread.join();
            }
            ::Thunder::Core::Singleton::Dispose();
        };

Tests/unit/core/test_controller.cpp:166

• This configuration test hard-codes Dictionary-specific configuration keys/values. If the Dictionary plugin is not configured/installed, Controller.1.configuration@Dictionary will fail (Core::ERROR_UNKNOWN_KEY). Skip when Dictionary is unavailable to keep the unit test suite robust.

    TEST_F(ControllerTest, ConfigurationQuery_ReturnsDictionaryConfiguration)
    {
        string response;

        const uint32_t result = _runtime.Invoke(
            "Controller.1.configuration@Dictionary",
            "{}",
            response);

        ASSERT_EQ(result, Core::ERROR_NONE);
        ASSERT_FALSE(response.empty());

        JsonObject configuration;
        ASSERT_TRUE(configuration.FromString(response));

        EXPECT_EQ(configuration["storage"].String(), "DataModel.json");
        EXPECT_EQ(configuration["lingertime"].Number(), 10);
    }

Tests/unit/core/test_tls.cpp:396

• CertificateStore::Add returns void and Thunder is typically built with exceptions disabled, so EXPECT_NO_THROW doesn't provide meaningful coverage here (it becomes a no-op when exceptions are off). Consider calling Add() directly and relying on the subsequent X509_verify_cert() assertions to validate success.

        // Add generated cert into a writable store.
        EXPECT_NO_THROW(store.Add(cert));

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

Tests/unit/core/test_comrpc.cpp:894

• IRemoteConnection::INotification inherits Core::IUnknown, so Release() is expected to return the new reference count, not an error code. Returning Core::ERROR_* values violates the interface contract and makes the ref-counting semantics misleading in this test.

        uint32_t AddRef() const override { return (++_refCount); }
        uint32_t Release() const override {
            uint32_t result = (--_refCount);
            return (result == 0 ? ::Thunder::Core::ERROR_DESTRUCTION_SUCCEEDED : ::Thunder::Core::ERROR_NONE);
        }

Tests/unit/core/test_websocket_protocol.cpp:240

• The wait predicate only becomes true when serverReady is true, so if server.Open() fails quickly the main thread still waits the full 10s timeout even though serverOpenResult is already set and the condition variable has been notified. Waiting for serverOpenResult to change from its sentinel value avoids the unnecessary delay and yields the correct failure message.

            ready = readyCV.wait_for(lk, std::chrono::seconds(10),
                [&]{ return serverReady.load(); });

Tests/unit/core/test_websocket_protocol.cpp:367

• Avoid calling GoogleTest assertions from the server thread. GTest assertions are intended to run on the test thread; reporting failures from worker threads can be unreliable. The parent thread already checks readiness and fails the test if the server did not open.

            bool opened = (server.Open(maxWait) == ::Thunder::Core::ERROR_NONE);
            EXPECT_TRUE(opened) << "server.Open() failed";

Tests/unit/core/test_socketport.cpp:1404

• Using ASSERT_EQ on server.Close() before signaling the parent means a close failure will abort the child callback and the parent will block in Wait() until the futex timeout. Prefer a non-fatal check here so the closure signal is still sent and the test can fail without an extra 16s delay.

            ASSERT_EQ(server.Close(maxWaitTimeMs), ::Thunder::Core::ERROR_NONE);

            // Signal parent that server has closed.
            ASSERT_EQ(testAdmin.Signal(initHandshakeValue, maxRetries),
                      ::Thunder::Core::ERROR_NONE);

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Suppressed comments (4)

Tests/unit/core/test_assertionunit.cpp:28

• std::atomic is used but <atomic> is not included, which can cause a compile error depending on transitive includes.

#include <gtest/gtest.h>

#ifndef MODULE_NAME
#include "../Module.h"
#endif

#include <core/core.h>
#include <unistd.h>

Tests/unit/core/test_websocket_protocol.cpp:232

• UniqueSocketPath() creates a new socket filename per run, but the path is only unlinked before creating the server. This leaves a growing number of /tmp/wpe_ws_* entries across repeated test runs. Consider unlinking the socket path during teardown as well.

        auto stopServer = [&]() {
            clientDone = true;
            if (serverThread.joinable()) {
                serverThread.join();
            }

Tests/unit/core/test_tls.cpp:414

• EXPECT_NO_THROW expands to try/catch; this repo is built with -fno-exceptions by default, so this assertion can fail to compile. Since CertificateStore::Add() is void and the test already verifies the cert via X509_verify_cert, just call store.Add(cert) here.

        // Add generated cert into a writable store.
        EXPECT_NO_THROW(store.Add(cert));

Tests/unit/core/test_websocket_protocol.cpp:389

• Same as above: this test uses a unique Unix socket path and only unlinks it before starting the server, so repeated runs can leave many stale socket files under /tmp. Unlink the socket path in stopServer() after the server thread is joined.

        auto stopServer = [&]() {
            clientsDone = true;
            if (serverThread.joinable()) {
                serverThread.join();
            }

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Suppressed comments (3)

Tests/unit/core/test_comrpc.cpp:894

• ConnectionNotificationTracker::AddRef() returns the incremented refcount, but Thunder's IReferenceCounted implementations conventionally return Core::ERROR_NONE (see Source/com/IUnknown.h:214-219). Returning a non-zero value can be misinterpreted as an error code by callers that check the return value.

        // IReferenceCounted — stack-allocated, use a simple counter
        uint32_t AddRef() const override { return (++_refCount); }
        uint32_t Release() const override {
            uint32_t result = (--_refCount);
            return (result == 0 ? ::Thunder::Core::ERROR_DESTRUCTION_SUCCEEDED : ::Thunder::Core::ERROR_NONE);
        }

Tests/unit/core/test_controller.cpp:126

• These tests expect activating/deactivating a "Dictionary" plugin to succeed, but ThunderTestRuntime is initialized with an empty plugins list, so only the Controller service is configured. Controller::Activate/Deactivate returns ERROR_UNKNOWN_KEY for unknown callsigns (Source/Thunder/Controller.cpp:1009-1011), so this will fail in CI unless Dictionary is configured externally.

    TEST_F(ControllerTest, ActivateDictionary_TransitionsToActivated)
    {
        // Ensure Dictionary starts deactivated.

        string response;
        uint32_t result = _runtime.Invoke(
            "Controller.activate",
            R"({"callsign":"Dictionary"})",
            response);

        EXPECT_EQ(result, Core::ERROR_NONE);
    }

Tests/unit/core/test_controller.cpp:166

• Same issue as ActivateDictionary_*: this test assumes the Dictionary service exists and is configured with specific settings, but ThunderTestRuntime is initialized with no plugins. If Dictionary isn't configured/installed in the test environment, this assertion will fail with ERROR_UNKNOWN_KEY.

    TEST_F(ControllerTest, ConfigurationQuery_ReturnsDictionaryConfiguration)
    {
        string response;

        const uint32_t result = _runtime.Invoke(
            "Controller.1.configuration@Dictionary",
            "{}",
            response);

        ASSERT_EQ(result, Core::ERROR_NONE);
        ASSERT_FALSE(response.empty());

        JsonObject configuration;
        ASSERT_TRUE(configuration.FromString(response));

        EXPECT_EQ(configuration["storage"].String(), "DataModel.json");
        EXPECT_EQ(configuration["lingertime"].Number(), 10);
    }

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Suppressed comments (5)

Tests/unit/core/test_comrpc.cpp:894

• ConnectionNotificationTracker::AddRef() returns the incremented refcount, but most Thunder Core::IUnknown implementations return Core::ERROR_NONE and use Core::ERROR_DESTRUCTION_SUCCEEDED from Release() as the only meaningful return value (e.g., Source/core/Proxy.h:131-153). Returning the refcount here is inconsistent and can confuse/ break code that interprets non-zero as an error code.

        uint32_t AddRef() const override { return (++_refCount); }
        uint32_t Release() const override {
            uint32_t result = (--_refCount);
            return (result == 0 ? ::Thunder::Core::ERROR_DESTRUCTION_SUCCEEDED : ::Thunder::Core::ERROR_NONE);
        }

Tests/unit/core/test_websocket_protocol.cpp:234

• The Unix-domain socket path created in RunTextServer() is unlinked before bind, but never cleaned up after the test completes. Since UniqueSocketPath() generates a new path per test run, this can leave many stale /tmp/wpe_ws_* files behind on CI/dev machines.

        auto stopServer = [&]() {
            clientDone = true;
            if (serverThread.joinable()) {
                serverThread.join();
            }
            ::Thunder::Core::Singleton::Dispose();
        };

Tests/unit/core/test_websocket_protocol.cpp:241

• RunTextServer() waits up to 10s for serverReady to become true, but serverReady is only set on success. If server.Open() fails quickly, the test still waits the full timeout before failing, slowing CI. Consider waking based on “Open finished” (e.g., serverOpenResult changed from its sentinel value) and then checking the result.

            std::unique_lock<std::mutex> lk(readyMutex);
            ready = readyCV.wait_for(lk, std::chrono::seconds(10),
                [&]{ return serverReady.load(); });
        }

Tests/unit/core/test_controller.cpp:138

• Controller.deactivate can return ERROR_ILLEGAL_STATE or ERROR_INPROGRESS depending on current plugin state (not only ERROR_NONE). With a shared runtime across the fixture this makes the test flaky if the Dictionary plugin is already inactive or deactivation is still progressing.

        uint32_t result = _runtime.Invoke(
            "Controller.deactivate",
            R"({"callsign":"Dictionary"})",
            response);

        EXPECT_EQ(result, Core::ERROR_NONE);

Tests/unit/core/test_controller.cpp:125

• Controller.activate can return ERROR_ILLEGAL_STATE, ERROR_INPROGRESS, or ERROR_PENDING_CONDITIONS depending on current plugin state (not only ERROR_NONE). With a shared runtime across the fixture this makes the test flaky if the Dictionary plugin is already active or activation is still progressing.

        uint32_t result = _runtime.Invoke(
            "Controller.activate",
            R"({"callsign":"Dictionary"})",
            response);

        EXPECT_EQ(result, Core::ERROR_NONE);

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Suppressed comments (4)

Tests/unit/core/test_websocket_protocol.cpp:241

• The condition_variable predicate only returns true on success (serverReady == true). If server.Open() fails, the notify happens but the wait keeps sleeping until timeout. Wait for "open completed" (via a sentinel value) and then check the result separately.

            std::unique_lock<std::mutex> lk(readyMutex);
            ready = readyCV.wait_for(lk, std::chrono::seconds(10),
                [&]{ return serverReady.load(); });
        }

Tests/unit/core/test_websocket_protocol.cpp:362

• MultipleClientsSequential(): serverReady is only set true on success, but the wait predicate also only unblocks on serverReady==true. If server.Open() fails, the server thread still notifies, yet the wait blocks until timeout. A tri-state (e.g. -1/0/1) lets the wait unblock on both success and failure and then assert success explicitly.

        std::atomic<bool> serverReady{false};
        std::mutex readyMutex;
        std::condition_variable readyCV;
        std::atomic<bool> clientsDone{false};

Tests/unit/core/test_websocket_protocol.cpp:399

• MultipleClientsSequential(): once serverReady becomes tri-state, the wait predicate should unblock when the server thread has signaled (serverReady != -1), then fail fast if serverReady != 1. As written, the predicate only unblocks on success.

            std::unique_lock<std::mutex> lk(readyMutex);
            ready = readyCV.wait_for(lk, std::chrono::seconds(10),
                [&]{ return serverReady.load(); });
        }
        if (!ready || !serverReady.load()) {

Tests/unit/core/test_websocket_protocol.cpp:200

• RunTextServer() waits on serverReady (which is only set true on success), so if server.Open() fails the parent thread will still block until the full timeout and then report a misleading "did not become ready" error. Using serverOpenResult as the wait completion signal avoids the 10s stall and makes failures report immediately.

This issue also appears in the following locations of the same file:

• line 238
• line 358
• line 395

        std::atomic<uint32_t> serverOpenResult{::Thunder::Core::ERROR_GENERAL};

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

Tests/unit/core/test_websocket_protocol.cpp:241

• This wait predicate should track completion of server.Open() rather than success-only readiness; otherwise, a fast Open() failure still blocks for up to 10 seconds. After switching to a completion flag (see earlier hunk), update the predicate here accordingly.

        bool ready = false;
        {
            std::unique_lock<std::mutex> lk(readyMutex);
            ready = readyCV.wait_for(lk, std::chrono::seconds(10),
                [&]{ return serverReady.load(); });
        }