RDK-60304: Adding L1 unit test cases for reportgen #237

rosemarybennyy · 2026-01-13T15:57:16Z

Reason for change: Adding L1 unit test cases for reportgen
Test Procedure: Tested and verified
Risks: Medium
Priority: P1
Signed-off-by: Rose Mary Benny RoseMary_Benny@comcast.com

Copilot

Pull request overview

This pull request adds comprehensive L1 unit test cases for the reportgen component to improve code coverage. The changes focus on testing various error handling paths and edge cases that weren't previously covered.

Changes:

Added unit tests for convertVectorToJson, prepareHttpUrl, encodeGrepResultInJSON, encodeEventMarkersInJSON, encodeTopResultInJSON, and tagReportAsCached functions
Extended mock interfaces to support testing of cJSON and regex operations (cJSON_IsArray, cJSON_InsertItemInArray, cJSON_GetObjectItemCaseSensitive, applyRegexToValue)
Added callback functions in reportgen.c to expose static functions for testing under GTEST_ENABLE flag

Reviewed changes

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

File	Description
source/test/reportgen/reportgenTest.cpp	Added approximately 30+ new test cases covering error paths, null argument handling, and edge cases for various reportgen functions
source/test/reportgen/reportgenMock.h	Extended mock interface with cJSON and regex-related methods needed for new test cases
source/test/reportgen/reportgenMock.cpp	Implemented mock wrappers for newly added interface methods
source/reportgen/reportgen.c	Added callback functions under GTEST_ENABLE to expose static functions for testing

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

source/test/reportgen/reportgenTest.cpp

Copilot

Pull request overview

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

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

Copilot · 2026-01-14T08:50:24Z

source/test/reportgen/reportgenTest.cpp

+    Vector_Destroy(topMarkerList, nullptr);
+    if(valArray) free(valArray);
+}
+TEST_F(reportgenTestFixture, encodeTopResultInJSON_cpuMem_cjsonCreateObjectFail)


Similarly, this test name has inconsistent naming. It should be 'encodeTopResultInJSON_cpuMem_CreateObjectFails' to match the naming pattern used elsewhere in the file (e.g., lines 633, 660, 1729). The inconsistency in casing and abbreviation ('cjsonCreateObjectFail' vs 'CreateObjectFails') could lead to confusion.

Suggested change

TEST_F(reportgenTestFixture, encodeTopResultInJSON_cpuMem_cjsonCreateObjectFail)

TEST_F(reportgenTestFixture, encodeTopResultInJSON_cpuMem_CreateObjectFails)

Copilot · 2026-01-14T08:50:24Z

source/test/reportgen/reportgenTest.cpp

+    char url[] = "http://rdk.central";
+    httpStruct.URL = url;
+    httpStruct.RequestURIparamList = paramList;


The test uses a stack-allocated array with mutable char literals for paramRefFail, paramRefEmpty, paramName, and url, which is good practice. However, the httpStruct.URL assignment (line 293) assigns a stack-allocated array to the URL pointer. While the test only reads from this pointer and doesn't free it, this could be fragile. It would be safer to use strdup for URL as well, or add a comment explaining why a stack-allocated array is acceptable here (since prepareHttpUrl internally calls strdup on http->URL at line 1205).

Copilot · 2026-01-14T08:50:24Z

source/test/reportgen/reportgenTest.cpp

+            ::testing::SetArgPointee<1>(nullptr),
+            ::testing::Return(T2ERROR_FAILURE)
+        ));
+    char* emptyValue = strdup("");  // allocate so code under test can free() it


The comment says 'allocate so code under test can free() it' but the production code at line 1235 in reportgen.c checks 'if (paramValue[0] == '\0')' and then frees paramValue. However, when an empty string "" is passed, paramValue[0] is '\0', so the free will happen. This is correct behavior, but the comment could be more explicit about explaining that the production code will free this empty string when it detects it's empty.

Suggested change

char* emptyValue = strdup(""); // allocate so code under test can free() it

char* emptyValue = strdup(""); // allocate an empty string so the code under test can detect paramValue[0] == '\0' and free() it

Copilot · 2026-01-14T08:50:25Z

source/test/reportgen/reportgenTest.cpp

+TEST_F(reportgenTestFixture, tagReportAsCached_NoArrayErrorBranch) {
+    char* fakeJson = strdup("{\"foo\":42}");

+    cJSON* mockRoot = (cJSON*)0x1000;
+    cJSON* mockReportTypeObj = (cJSON*)0x3000;
+
+    // Parse
+    EXPECT_CALL(*m_reportgenMock, cJSON_Parse(_))
+        .WillOnce(::testing::Return(mockRoot));
+    EXPECT_CALL(*m_reportgenMock, cJSON_CreateObject())
+        .WillOnce(::testing::Return(mockReportTypeObj));
+    EXPECT_CALL(*m_reportgenMock, cJSON_AddStringToObject(
+        mockReportTypeObj,
+        ::testing::StrEq("REPORT_TYPE"),
+        ::testing::StrEq("CACHED"))
+    )
+        .WillOnce(::testing::Return(mockReportTypeObj));
+    // Try "searchResult" (not found)
+    EXPECT_CALL(*m_reportgenMock, cJSON_GetObjectItemCaseSensitive(mockRoot, ::testing::StrEq("searchResult")))
+        .WillOnce(::testing::Return(nullptr));
+    // Try "Report" (not found)
+    EXPECT_CALL(*m_reportgenMock, cJSON_GetObjectItemCaseSensitive(mockRoot, ::testing::StrEq("Report")))
+        .WillOnce(::testing::Return(nullptr));
+    // Act
+    tagReportAsCached(&fakeJson);
+
+    // Assert: unchanged input
+    EXPECT_STREQ("{\"foo\":42}", fakeJson);
+    free(fakeJson);
+}


This test has a bug: it doesn't expect the cleanup calls that should happen when neither 'searchResult' nor 'Report' arrays are found. Looking at the production code (lines 1326-1330 in reportgen.c), when no valid array is found, the code calls destroyJSONReport on both reportTypeEntry and jsonReportObj before returning. This test should expect cJSON_Delete to be called for both mockReportTypeObj and mockRoot, but currently it doesn't mock the cleanup of mockReportTypeObj.

Copilot · 2026-01-14T08:50:25Z

source/test/reportgen/reportgenTest.cpp

+    char* emptyValue = strdup("");  // allocate so code under test can free() it
+    EXPECT_CALL(*m_reportgenMock, getParameterValue(::testing::StrEq(paramRefEmpty), ::testing::_))
+        .WillOnce(::testing::DoAll(
+            ::testing::SetArgPointee<1>(emptyValue),
+            ::testing::Return(T2ERROR_SUCCESS)
+        ));


In this test case, when getParameterValue fails or returns an empty value, prepareHttpUrl continues to process more parameters and then returns a strdup'd copy of the URL. The test correctly frees this result, but there's a potential memory leak: the test calls strdup("") for emptyValue which is freed by the production code when paramValue[0] equals '\0' (line 1235 in reportgen.c), but the test doesn't verify this cleanup is actually happening. While the test will work, it would be clearer to add a comment explaining that the production code will free emptyValue.

Copilot · 2026-01-14T08:50:25Z

source/test/reportgen/reportgenTest.cpp

+        .WillOnce(::testing::Return(nullptr));
+    // Try "Report" (not found)
+    EXPECT_CALL(*m_reportgenMock, cJSON_GetObjectItemCaseSensitive(mockRoot, ::testing::StrEq("Report")))
+        .WillOnce(::testing::Return(nullptr));


This test is missing an EXPECT_CALL for cJSON_Delete to cleanup mockReportTypeObj. Looking at the production code path (line 1328 in reportgen.c), when neither array is found, destroyJSONReport is called on reportTypeEntry, which should trigger cJSON_Delete on mockReportTypeObj. Without this expectation, if the production code correctly calls the cleanup, the test will fail due to unexpected mock calls.

Suggested change

.WillOnce(::testing::Return(nullptr));

.WillOnce(::testing::Return(nullptr));

// Expect cleanup of the created report type object

EXPECT_CALL(*m_reportgenMock, cJSON_Delete(mockReportTypeObj));

Copilot · 2026-01-14T08:50:26Z

source/test/reportgen/reportgenTest.cpp

+    Vector_Destroy(topMarkerList, nullptr);
+    if(valArray) free(valArray);
+}
+TEST_F(reportgenTestFixture, encodeTopResultInJSON_loadAverage_cjsonCreateObjectFail)


The naming of this test is inconsistent. The test function is named 'encodeTopResultInJSON_loadAverage_cjsonCreateObjectFail' but it should follow the naming pattern more consistently as 'encodeTopResultInJSON_loadAverage_CreateObjectFails' to match the pattern used in other similar tests (e.g., line 633: encodeGrepResultInJSON_CreateObjectFails, line 660: encodeGrepResultInJSON_CreateArrayFails). The inconsistency in casing ('cjsonCreateObjectFail' vs 'CreateObjectFails') and the full spelling could lead to confusion.

Suggested change

TEST_F(reportgenTestFixture, encodeTopResultInJSON_loadAverage_cjsonCreateObjectFail)

TEST_F(reportgenTestFixture, encodeTopResultInJSON_loadAverage_CreateObjectFails)

Reason for change: Adding L1 unit test cases for reportgen Test Procedure: Tested and verified Risks: Medium Priority: P1 Signed-off-by: Rose Mary Benny <RoseMary_Benny@comcast.com>

Copilot

Pull request overview

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

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

Copilot · 2026-01-19T17:56:58Z

source/test/reportgen/reportgenTest.cpp

+TEST_F(reportgenTestFixture, tagReportAsCached_NoArrayErrorBranch) {
+    char* fakeJson = strdup("{\"foo\":42}");

+    cJSON* mockRoot = (cJSON*)0x1000;
+    cJSON* mockReportTypeObj = (cJSON*)0x3000;
+
+    // Parse
+    EXPECT_CALL(*m_reportgenMock, cJSON_Parse(_))
+        .WillOnce(::testing::Return(mockRoot));
+    EXPECT_CALL(*m_reportgenMock, cJSON_CreateObject())
+        .WillOnce(::testing::Return(mockReportTypeObj));
+    EXPECT_CALL(*m_reportgenMock, cJSON_AddStringToObject(
+        mockReportTypeObj,
+        ::testing::StrEq("REPORT_TYPE"),
+        ::testing::StrEq("CACHED"))
+    )
+        .WillOnce(::testing::Return(mockReportTypeObj));
+    // Try "searchResult" (not found)
+    EXPECT_CALL(*m_reportgenMock, cJSON_GetObjectItemCaseSensitive(mockRoot, ::testing::StrEq("searchResult")))
+        .WillOnce(::testing::Return(nullptr));
+    // Try "Report" (not found)
+    EXPECT_CALL(*m_reportgenMock, cJSON_GetObjectItemCaseSensitive(mockRoot, ::testing::StrEq("Report")))
+        .WillOnce(::testing::Return(nullptr));
+    // Act
+    tagReportAsCached(&fakeJson);
+
+    // Assert: unchanged input
+    EXPECT_STREQ("{\"foo\":42}", fakeJson);
+    free(fakeJson);
+}


Missing mock expectation for cJSON_Delete on mockReportTypeObj. When the production code at reportgen.c:1328 calls destroyJSONReport(reportTypeEntry), it will invoke cJSON_Delete(reportTypeEntry). The test should include an expectation for cJSON_Delete(mockReportTypeObj) before the cJSON_Delete(mockRoot) expectation to properly verify this error path.

Copilot · 2026-01-19T17:56:59Z

source/test/reportgen/reportgenTest.cpp

+TEST_F(reportgenTestFixture, prepareHttpUrl_CurlInitFails) {
+    T2HTTP httpStruct;
+    T2HTTP* http = &httpStruct;
+
+    // Simulate curl_easy_init() failing
+    EXPECT_CALL(*m_reportgenMock, curl_easy_init())
+        .WillOnce(::testing::Return(nullptr));
+
+    // Optionally: you could check for error logging if T2Error is mockable
+
+    // Call function under test
+    char* result = prepareHttpUrl(http);
+
+    // Expect NULL return
+    EXPECT_EQ(result, nullptr);
+}
+
+TEST_F(reportgenTestFixture, prepareHttpUrl_getParameterValueFailsAndEmptyValue) {
+    T2HTTP httpStruct = {0};
+    HTTPReqParam httpParam1 = {0};
+    HTTPReqParam httpParam2 = {0};
+    Vector* paramList = NULL;
+    Vector_Create(&paramList);
+
+    // Use mutable char arrays or cast string literals to char*
+    char paramRefFail[] = "Test.Fail";
+    char paramRefEmpty[] = "Test.Empty";
+    char paramName[] = "testParam";
+    char url[] = "http://rdk.central";
+    httpStruct.URL = url;
+    httpStruct.RequestURIparamList = paramList;
+
+    httpParam1.HttpName = paramName;
+    httpParam1.HttpValue = nullptr;
+    httpParam1.HttpRef = paramRefFail;
+    httpParam2.HttpName = paramName;
+    httpParam2.HttpValue = nullptr;
+    httpParam2.HttpRef = paramRefEmpty;
+
+    Vector_PushBack(paramList, &httpParam1);
+    Vector_PushBack(paramList, &httpParam2);
+
+    EXPECT_CALL(*m_reportgenMock, curl_easy_init())
+        .WillOnce(::testing::Return(reinterpret_cast<CURL*>(0x1)));
+    EXPECT_CALL(*m_reportgenMock, getParameterValue(::testing::StrEq(paramRefFail), ::testing::_))
+        .WillOnce(::testing::DoAll(
+            ::testing::SetArgPointee<1>(nullptr),
+            ::testing::Return(T2ERROR_FAILURE)
+        ));
+    char* emptyValue = strdup("");  // allocate so code under test can free() it
+    EXPECT_CALL(*m_reportgenMock, getParameterValue(::testing::StrEq(paramRefEmpty), ::testing::_))
+        .WillOnce(::testing::DoAll(
+            ::testing::SetArgPointee<1>(emptyValue),
+            ::testing::Return(T2ERROR_SUCCESS)
+        ));
+    EXPECT_CALL(*m_reportgenMock, curl_easy_cleanup(reinterpret_cast<CURL*>(0x1))).Times(1);
+
+    char* result = prepareHttpUrl(&httpStruct);
+
+    ASSERT_NE(result, nullptr);
+    EXPECT_STREQ(result, url); // No new params on fail/empty
+    free(result);
+
+    Vector_Destroy(paramList,nullptr);
+}
+
+// This tests the branch: getParameterValue returns success, paramValue[0] != '\0', curl_easy_escape is called, paramValue is freed
+
+TEST_F(reportgenTestFixture, prepareHttpUrl_ParamValueIsEscapedAndFreed) {


Inconsistent naming convention for test case names. While existing tests use PascalCase (e.g., PrepareHttpUrl, PrepareJSONReport1), the newly added tests use snake_case with descriptive suffixes (e.g., prepareHttpUrl_CurlInitFails, encodeGrepResultInJSON_CreateObjectFails). Consider using a consistent naming convention throughout the test file. Either align new tests with the existing PascalCase pattern or update all tests to use the more descriptive snake_case pattern.

Copilot AI review requested due to automatic review settings January 13, 2026 15:57

rosemarybennyy requested a review from a team as a code owner January 13, 2026 15:57

Copilot started reviewing on behalf of rosemarybennyy January 13, 2026 15:57 View session

Copilot AI reviewed Jan 13, 2026

View reviewed changes

rosemarybennyy force-pushed the topic/RDK-60304 branch from 840160d to a7e473f Compare January 13, 2026 16:03

Copilot AI review requested due to automatic review settings January 14, 2026 08:44

rosemarybennyy force-pushed the topic/RDK-60304 branch from a7e473f to 07e7d6b Compare January 14, 2026 08:44

Copilot started reviewing on behalf of rosemarybennyy January 14, 2026 08:45 View session

Copilot AI reviewed Jan 14, 2026

View reviewed changes

RDK-60304: Adding L1 unit test cases to improve code coverage

8ee327f

Reason for change: Adding L1 unit test cases for reportgen Test Procedure: Tested and verified Risks: Medium Priority: P1 Signed-off-by: Rose Mary Benny <RoseMary_Benny@comcast.com>

rosemarybennyy force-pushed the topic/RDK-60304 branch from 07e7d6b to 8ee327f Compare January 14, 2026 09:02

rosemarybennyy changed the title ~~RDK-60304: Adding L1 unit test cases to improve code coverage~~ RDK-60304: Adding L1 unit test cases for reportgen Jan 14, 2026

Merge branch 'develop' into topic/RDK-60304

d24d2be

Copilot AI review requested due to automatic review settings January 19, 2026 17:52

Copilot started reviewing on behalf of scthunderbolt January 19, 2026 17:53 View session

Copilot AI reviewed Jan 19, 2026

View reviewed changes

Merge branch 'develop' into topic/RDK-60304

0d12b32

shibu-kv approved these changes Jan 20, 2026

View reviewed changes

shibu-kv merged commit 4db019b into develop Jan 20, 2026
15 checks passed

github-actions bot locked and limited conversation to collaborators Jan 20, 2026

	TEST_F(reportgenTestFixture, encodeTopResultInJSON_cpuMem_cjsonCreateObjectFail)
	TEST_F(reportgenTestFixture, encodeTopResultInJSON_cpuMem_CreateObjectFails)

	char* emptyValue = strdup(""); // allocate so code under test can free() it
	char* emptyValue = strdup(""); // allocate an empty string so the code under test can detect paramValue[0] == '\0' and free() it

RDK-60304: Adding L1 unit test cases for reportgen #237

RDK-60304: Adding L1 unit test cases for reportgen #237

Conversation

rosemarybennyy commented Jan 13, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants