Development/incremental test coverage expansion [Test Branch]

[smanes0213]

No description provided.

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 8 comments.

Comments suppressed due to low confidence (2)

Tests/unit/core/test_websocket_protocol.cpp:24

• RunTextServer takes a std::function, but this file does not include <functional>, which can cause a build failure depending on transitive includes. Add the missing standard header explicitly.

#include <atomic>
#include <condition_variable>
#include <mutex>
#include <thread>
#include <unistd.h>

Tests/unit/core/test_comrpc.cpp:893

• Core::IUnknown/IReferenceCounted implementations in Thunder conventionally return error codes (e.g., ERROR_NONE / ERROR_COMPOSIT_OBJECT / ERROR_DESTRUCTION_SUCCEEDED) rather than the numeric refcount. Returning the refcount from AddRef() here is inconsistent with that convention, and returning ERROR_DESTRUCTION_SUCCEEDED from a stack-allocated object is misleading since nothing is destroyed. Track the count but return a composite error code instead.

        // 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);
        }

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (7)

Tests/unit/core/test_comrpc.cpp:893

• Thunder's Core::IUnknown AddRef/Release convention returns Core::ERROR_NONE / Core::ERROR_DESTRUCTION_SUCCEEDED (not the reference count). Returning (++_refCount) from AddRef is inconsistent and could trip callers that check for ERROR_NONE.

        // 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_url.cpp:557

• Same TCHAR sizing issue here: prefer _tcslen(source) over strlen(source) for correctness across TCHAR configurations.

        const TCHAR source[] = "abc123XYZ";
        TCHAR dest[64] = {};

        uint16_t len = ::Thunder::Core::URL::Encode(source, static_cast<uint16_t>(strlen(source)), dest, sizeof(dest));
        string encoded(dest, len);

Tests/unit/core/test_url.cpp:568

• Same TCHAR sizing issue here: strlen() is not correct for TCHAR when _UNICODE is enabled; use _tcslen(source).

        const TCHAR source[] = "a=b&c";
        TCHAR dest[64] = {};

        uint16_t len = ::Thunder::Core::URL::Encode(source, static_cast<uint16_t>(strlen(source)), dest, sizeof(dest));
        string encoded(dest, len);

Tests/unit/core/test_url.cpp:583

• Same TCHAR sizing issue here: use _tcslen(source) instead of strlen(source) when passing a TCHAR length to URL::Decode.

        const TCHAR source[] = "hello%20world";
        TCHAR dest[64] = {};

        uint16_t len = ::Thunder::Core::URL::Decode(source, static_cast<uint16_t>(strlen(source)), dest, sizeof(dest));
        string decoded(dest, len);

Tests/unit/core/test_url.cpp:593

• Same TCHAR sizing issue here: use _tcslen(source) instead of strlen(source) when passing a TCHAR length to URL::Decode.

        const TCHAR source[] = "hello+world";
        TCHAR dest[64] = {};

        uint16_t len = ::Thunder::Core::URL::Decode(source, static_cast<uint16_t>(strlen(source)), dest, sizeof(dest));
        string decoded(dest, len);

Tests/unit/core/test_url.cpp:605

• Same TCHAR sizing issue here: Encode() expects a TCHAR length; strlen() is only correct for narrow builds. Use _tcslen(source).

        const TCHAR source[] = "key=hello world&other=a/b";
        TCHAR encoded[128] = {};
        TCHAR decoded[128] = {};

        uint16_t encLen = ::Thunder::Core::URL::Encode(source, static_cast<uint16_t>(strlen(source)), encoded, sizeof(encoded));
        ASSERT_GT(encLen, 0u);

Tests/unit/core/test_url.cpp:545

• This uses strlen() to size a TCHAR buffer. Thunder defines _tcslen for TCHAR, so this should use _tcslen(source) to remain correct if TCHAR is wchar_t (e.g. _UNICODE builds).

        const TCHAR source[] = "hello world";
        TCHAR dest[64] = {};

        uint16_t len = ::Thunder::Core::URL::Encode(source, static_cast<uint16_t>(strlen(source)), dest, sizeof(dest));
        EXPECT_GT(len, 0u);

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

Tests/unit/core/test_comrpc.cpp:893

• IRemoteConnection::INotification inherits Core::IUnknown, where AddRef()/Release() are expected to return Thunder error codes (e.g., Core::ERROR_NONE / Core::ERROR_DESTRUCTION_SUCCEEDED), not the incremented refcount value. Returning ++_refCount here can be interpreted as a failure by callers that check for ERROR_NONE (see e.g. Source/core/Proxy.h:131-154). For a stack-allocated tracker used only for registration, make these no-op and always return Core::ERROR_NONE.

        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);
        }

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

Tests/unit/core/test_websocket_protocol.cpp:26

• RunTextServer takes a std::function, but this file does not include <functional>, which can break compilation (no other included header here guarantees it).

#include <atomic>
#include <condition_variable>
#include <mutex>
#include <thread>
#include <unistd.h>

#include <gtest/gtest.h>

Tests/unit/core/test_comrpc.cpp:893

• AddRef()/Release() here return the incremented refcount / an error code, which is inconsistent with the pattern used in this codebase where AddRef()/Release() return Thunder error codes (e.g., Core::ERROR_NONE, Core::ERROR_DESTRUCTION_SUCCEEDED, or Core::ERROR_COMPOSIT_OBJECT). For a stack-allocated notification sink, returning Core::ERROR_COMPOSIT_OBJECT avoids implying ownership/deletion semantics and matches Core::SinkType behavior (Source/core/Services.h:151-161).

        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);
        }

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

Tests/unit/core/test_nodeid.cpp:109

• The DefaultMask() expectations for big-endian are reversed: NodeId::DefaultMask() returns 24/16/8 based on the top bits of s_addr, not 8/16/24. Also, __BYTE_ORDER__ is not available on all toolchains (e.g. MSVC), so this should be guarded to keep the test portable.

        // DefaultMask() interprets s_addr (network byte order) as a native
        // unsigned long and checks bits 31/30.  On little-endian hosts this
        // inspects the LAST octet, not the first, so all addresses ending in
        // .1 yield 24.  On big-endian hosts it inspects the FIRST octet and
        // returns classful defaults: Class A=8, Class B=16, Class C=24.
#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__

Tests/unit/core/test_assertionunit.cpp:20

• This test uses std::atomic but does not include <atomic> directly, unlike other unit tests (e.g. Tests/unit/core/test_socketport.cpp:21). Adding the explicit include avoids relying on transitive includes from core/core.h.

#include <gtest/gtest.h>