Skip to content

Conversation

@yogeswaransky
Copy link
Contributor

No description provided.

Signed-off-by: Yogeswaran K <yogeswaransky@gmail.com>
Copilot AI review requested due to automatic review settings February 11, 2026 13:14
@yogeswaransky yogeswaransky requested a review from a team as a code owner February 11, 2026 13:14
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates unit tests to align with the new “fork elimination” HTTP implementation (connection-pool + libcurl), replacing older pipe/fork/read-based expectations and expanding the test mocks to interpose additional libcurl calls.

Changes:

  • Enable previously disabled xconf-client and protocol HTTP tests and update them to use curl/pool expectations instead of fork()/pipe().
  • Extend FileioMock interposition to cover curl_easy_getinfo() and curl_easy_setopt().
  • Add helper logic intended to avoid deadlocks when GTest/logging writes to FILE* while file I/O is mocked.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File Description
source/test/xconf-client/xconfclientTest.cpp Enables/updates doHttpGet tests to mock curl/pool behavior; adds a helper macro intended to prevent GTest logging deadlocks.
source/test/protocol/ProtocolTest.cpp Updates protocol HTTP tests to mock curl/pool behavior; adds default curl behaviors in fixture setup and a helper macro/comment.
source/test/mocks/FileioMock.h Adds/adjusts mock method signatures for curl interposition (curl_easy_setopt).
source/test/mocks/FileioMock.cpp Adds interposed curl_easy_getinfo() and a new interposed curl_easy_setopt() implementation for tests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

.Times(::testing::AnyNumber()) \
.WillRepeatedly(::testing::Invoke( \
[](const void* ptr, size_t size, size_t nitems, FILE* stream) { \
return ::fwrite(ptr, size, nitems, stream); \
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

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

PREVENT_GTEST_LOGGING_DEADLOCK() configures FileMock::fwrite() to call ::fwrite(), but ::fwrite is interposed by FileioMock.cpp to call g_fileIOMock->fwrite() again when the mock is set. This can recurse indefinitely (and still won’t reach the real libc fwrite), risking hangs/stack overflow when any code/GTest writes output. Consider calling the real fwrite via the existing dlsym-backed function (or temporarily setting g_fileIOMock = nullptr around the real call) instead of calling ::fwrite directly.

Suggested change
return ::fwrite(ptr, size, nitems, stream); \
FileMock* previousMock = g_fileIOMock; \
g_fileIOMock = nullptr; \
size_t result = ::fwrite(ptr, size, nitems, stream); \
g_fileIOMock = previousMock; \
return result; \

Copilot uses AI. Check for mistakes.
Comment on lines +84 to 87
FileMock fileMock;
g_fileIOMock = &fileMock;
PREVENT_GTEST_LOGGING_DEADLOCK();
EXPECT_EQ(T2ERROR_FAILURE, doHttpGet(NULL, &data));
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

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

This test assigns g_fileIOMock to a stack-allocated FileMock. After the test returns, g_fileIOMock becomes a dangling pointer, which can cause cross-test flakiness/crashes when tests are shuffled or other code references g_fileIOMock before it’s reset. Prefer using a fixture-owned mock or reset g_fileIOMock to nullptr (or its previous value) before exiting the test.

Suggested change
FileMock fileMock;
g_fileIOMock = &fileMock;
PREVENT_GTEST_LOGGING_DEADLOCK();
EXPECT_EQ(T2ERROR_FAILURE, doHttpGet(NULL, &data));
FileMock fileMock;
FileMock* prevFileMock = g_fileIOMock;
g_fileIOMock = &fileMock;
PREVENT_GTEST_LOGGING_DEADLOCK();
EXPECT_EQ(T2ERROR_FAILURE, doHttpGet(NULL, &data));
g_fileIOMock = prevFileMock;

Copilot uses AI. Check for mistakes.
using ::testing::StrEq;

// Helper macro to prevent deadlock from mocked fwrite and fputs calls in the protocol code when GTest tries to log output, which can cause a deadlock if the logging functions are mocked without allowing real calls to them.
// Helper macro to prevent deadlock from mocked fwrite and fputs calls in the protocol code when GTest tries to log output, which can cause a deadlock if the logging functions are mocked without allowing real calls to them.
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

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

The helper macro comment is duplicated back-to-back. Please remove one copy to avoid noise and keep file headers concise.

Suggested change
// Helper macro to prevent deadlock from mocked fwrite and fputs calls in the protocol code when GTest tries to log output, which can cause a deadlock if the logging functions are mocked without allowing real calls to them.

Copilot uses AI. Check for mistakes.
.Times(::testing::AnyNumber())
.WillRepeatedly(::testing::Invoke(
[](const void* ptr, size_t size, size_t nitems, FILE* stream) {
return ::fwrite(ptr, size, nitems, stream);
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

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

The default fwrite mock behavior calls ::fwrite, but ::fwrite is interposed by FileioMock.cpp to route back into g_fileIOMock->fwrite when the mock is set, which can cause infinite recursion/hangs when anything writes to a FILE* (including GTest output). To actually delegate to the real libc fwrite, call the dlsym-resolved real function (or temporarily set g_fileIOMock = nullptr around the call) rather than ::fwrite.

Suggested change
return ::fwrite(ptr, size, nitems, stream);
auto* savedMock = g_fileIOMock;
g_fileIOMock = nullptr;
size_t written = ::fwrite(ptr, size, nitems, stream);
g_fileIOMock = savedMock;
return written;

Copilot uses AI. Check for mistakes.
Comment on lines +496 to +500
va_list args;
va_start(args, option);
void* param = va_arg(args, void*);
va_end(args);
return g_fileIOMock->curl_easy_setopt(handle, option, param);
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

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

curl_easy_setopt is variadic, but this wrapper unconditionally reads the first vararg as void*. Many real call sites pass long values and function pointers (e.g., CURLOPT_TIMEOUT, CURLOPT_HTTPGET, CURLOPT_WRITEFUNCTION), so reading them as void* is undefined behavior and can break tests on some platforms/ABIs. Consider not extracting the argument at all for the mocked path (if tests don’t assert on it), or implement option-type-aware extraction (long vs pointer vs function pointer), and/or provide a safe fallback that forwards to the real curl_easy_setopt when g_fileIOMock is null.

Suggested change
va_list args;
va_start(args, option);
void* param = va_arg(args, void*);
va_end(args);
return g_fileIOMock->curl_easy_setopt(handle, option, param);
/* Do not read variadic arguments here to avoid undefined behavior
* when the caller passes non-pointer types (e.g., long or function
* pointers). The mock implementation should not depend on the
* concrete value of the option parameter.
*/
return g_fileIOMock->curl_easy_setopt(handle, option, nullptr);

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant