Conversation
* RDK-60805: Adding L1 unit test cases for reportprofiles Reason for change: Adding L1 unit test cases for reportprofiles Test Procedure: Tested and verified Risks: Medium Priority: P1 Signed-off-by: Rose Mary Benny <RoseMary_Benny@comcast.com> * Adding Apache banners to new files * added GTEST_ENABLE flag for bulkdata --------- Signed-off-by: Rose Mary Benny <RoseMary_Benny@comcast.com> Co-authored-by: shibu-kv <shibu.kakkoth@gmail.com>
* RDK-60476: Reduce default connection pool size to 1 Update L1 and L2 testcases for fork elimination --------- Signed-off-by: Yogeswaran K <yogeswaransky@gmail.com> Co-authored-by: shibu-kv <shibu.kakkoth@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR prepares release 1.8.1 with optimizations to the connection pool implementation and expanded test coverage for report profiles functionality. The changes focus on reducing resource usage on low-end devices while maintaining reliability through improved test infrastructure and mTLS support.
Changes:
- Reduced default CURL connection pool size from 2 to 1 to optimize resource usage on low-end devices
- Added comprehensive L1 unit tests for reportprofiles functionality with new test mocks and fixtures
- Enhanced test infrastructure with mTLS certificate support and improved L2 test failure reporting
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| source/protocol/http/multicurlinterface.c | Reduced default pool size to 1; replaced condition variable waiting with polling mechanism for better timeout handling |
| source/protocol/http/curlinterface.c | Added conditional compilation guard for LIBRDKCONFIG_BUILD; updated log levels from Info to Debug |
| source/xconf-client/xconfclient.c | Added null/empty string check before strdup in timezone handling; added conditional compilation guard |
| source/utils/t2MtlsUtils.c | Added conditional compilation guard for LIBRDKCONFIG_BUILD |
| source/bulkdata/reportprofiles.c | Added GTEST_ENABLE callback functions for testing static functions |
| source/bulkdata/profilexconf.c | Added test helper function to set reportThreadExits flag |
| source/bulkdata/profile.c | Added wrap directives for sendReportOverHTTP functions in test builds |
| source/test/bulkdata/reportprofilesTest.cpp | New unit test file with test cases for report profiles message pack processing |
| source/test/bulkdata/reportprofileMock.{h,cpp} | New mock implementation for report profile functions |
| source/test/bulkdata/profileMock.{h,cpp} | New mock implementation for profile protocol functions |
| source/test/bulkdata/profileTest.cpp | Enhanced tests with new callback tests and improved profile setup |
| source/test/bulkdata/Makefile.am | Added reportprofiles_gtest.bin target; fixed typo in profilexconf source assignment |
| source/test/xconf-client/xconfclientTest.cpp | Updated tests to use connection pool instead of fork-based approach |
| source/test/protocol/ProtocolTest.cpp | Updated tests for connection pool usage; removed obsolete fork-based test code |
| source/test/mocks/FileioMock.{h,cpp} | Added curl_easy_setopt and curl_easy_getinfo mock implementations |
| test/functional-tests/tests/helper_functions.py | Added mTLS certificate configuration for test client requests |
| test/functional-tests/tests/report_profiles.py | Fixed typo in URL from dataLookeMock to dataLakeMockXconf |
| test/run_l2.sh | Improved test result collection and final exit status reporting |
| test/run_ut.sh | Added reportprofiles_gtest.bin to unit test execution |
| test/test-artifacts/mockxconf/xconf-dcm-response2.json | Removed unused test artifact file |
| source/xconf-client/Makefile.am | Added -lrdkconfig linker flag for rdkcertselector builds |
| source/protocol/http/Makefile.am | Added -lrdkconfig linker flag for rdkcertselector builds |
| source/ccspinterface/rbusInterface.c | Added DCMAGENT conditional guard for GTEST callback function |
| build_inside_container.sh | Added ENABLE_MTLS flag and --enable-rdkcertselector=yes configure option |
| .github/workflows/L2-tests.yml | Added ENABLE_MTLS=true environment variable to Docker containers; removed xconf-dcm-response2.json copy step |
| CHANGELOG.md | Added 1.8.1 release notes with PR references |
| source/docs/protocol/curl_usage_architecture.md | New comprehensive documentation of CURL connection pool architecture |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| struct __msgpack__ *msg = (struct __msgpack__*)malloc(sizeof(struct __msgpack__)); | ||
| msg->msgpack_blob = (char*)webConfigString; | ||
| msg->msgpack_blob_size = (int)decodedDataLen; | ||
|
|
||
| EXPECT_CALL(*g_vectorMock, Vector_Size(_)) | ||
| .Times(::testing::AtMost(1)) | ||
| .WillRepeatedly(Return(0)); | ||
| EXPECT_CALL(*g_vectorMock, Vector_Destroy(_, _)).Times(::testing::AtMost(1)) | ||
| .WillRepeatedly(Return(T2ERROR_SUCCESS)); | ||
|
|
||
| ReportProfiles_ProcessReportProfilesMsgPackBlob(msg->msgpack_blob, msg->msgpack_blob_size); | ||
| } |
There was a problem hiding this comment.
The dynamically allocated memory for msg is never freed, causing a memory leak. Add free(msg); after line 108 to release the allocated memory.
| EXPECT_CALL(*g_vectorMock, Vector_Destroy(_, _)).Times(::testing::AtMost(1)) | ||
| .WillRepeatedly(Return(T2ERROR_SUCCESS)); | ||
|
|
||
| ReportProfiles_ProcessReportProfilesMsgPackBlob(msg->msgpack_blob, msg->msgpack_blob_size); |
There was a problem hiding this comment.
The memory allocated by g_base64_decode for webConfigString is never freed, causing a memory leak. GLib-allocated memory should be freed using g_free(webConfigString); after it's no longer needed (after line 108).
| ReportProfiles_ProcessReportProfilesMsgPackBlob(msg->msgpack_blob, msg->msgpack_blob_size); | |
| ReportProfiles_ProcessReportProfilesMsgPackBlob(msg->msgpack_blob, msg->msgpack_blob_size); | |
| g_free(webConfigString); | |
| free(msg); |
| ProfileXConf* profile = (ProfileXConf*)malloc(sizeof(ProfileXConf)); | ||
| memset(profile, 0, sizeof(ProfileXConf)); | ||
| profile->name = strdup("TestProfile"); | ||
| profile->eMarkerList = nullptr; | ||
| profile->gMarkerList = nullptr; | ||
| profile->topMarkerList = nullptr; | ||
| profile->paramList = nullptr; | ||
| profile->cachedReportList = nullptr; | ||
| profile->protocol = strdup("HTTP"); | ||
| profile->encodingType = strdup("JSON"); | ||
| profile->t2HTTPDest = nullptr; | ||
| profile->grepSeekProfile = nullptr; | ||
| profile->jsonReportObj = nullptr; // types now match | ||
| profile->checkPreviousSeek = true; | ||
|
|
||
| profile->t2HTTPDest = (T2HTTP *)malloc(sizeof(T2HTTP)); | ||
| profile->t2HTTPDest->URL = strdup("https://mock1xconf:50051/dataLakeMockXconf"); | ||
| profile->isUpdated = true; | ||
| GrepSeekProfile *gsProfile = (GrepSeekProfile *)malloc(sizeof(GrepSeekProfile)); | ||
| if (gsProfile) | ||
| { | ||
| gsProfile->logFileSeekMap = hash_map_create(); | ||
| gsProfile->execCounter = 0; | ||
| } | ||
| profile->grepSeekProfile = gsProfile; | ||
| //profile->grepSeekProfile = nullptr; | ||
| profile->reportInProgress = false; | ||
| profile->isUpdated = false; | ||
|
|
||
| EXPECT_CALL(*g_vectorMock, Vector_Size(_)) | ||
| .Times(::testing::AtMost(3)) | ||
| .WillRepeatedly(Return(0)); // Return 1 to indicate one profile in the list | ||
| EXPECT_CALL(*g_vectorMock, Vector_At(_, 0)) | ||
| .Times(::testing::AtMost(1)) | ||
| .WillRepeatedly(Return((void*)strdup("PROFILE_1"))); | ||
| EXPECT_CALL(*g_vectorMock, Vector_Create(_)) | ||
| .Times(::testing::AtMost(3)) // 1 for local test configlist, 1 for global profileList, 1 for configList in loadReportProfilesFromDisk | ||
| .WillRepeatedly(Return(T2ERROR_SUCCESS)); | ||
| EXPECT_CALL(*g_vectorMock, Vector_PushBack(_, _)) | ||
| .Times(::testing::AtMost(3)) | ||
| .WillRepeatedly(Return(T2ERROR_SUCCESS)); | ||
|
|
||
| // Scheduler mock expectations - ProfileXConf_set calls registerProfileWithScheduler | ||
| EXPECT_CALL(*g_schedulerMock, registerProfileWithScheduler(_, _, _, _, _, _, _, _)) | ||
| .Times(::testing::AtMost(1)) | ||
| .WillRepeatedly(Return(T2ERROR_SUCCESS)); | ||
|
|
||
| EXPECT_EQ(ProfileXConf_set(profile), T2ERROR_SUCCESS); | ||
| EXPECT_TRUE(ProfileXConf_isSet()); | ||
|
|
||
|
|
||
| // Get name | ||
| char* name = ProfileXconf_getName(); | ||
| ASSERT_NE(name, nullptr); | ||
| EXPECT_STREQ(name, "TestProfile"); | ||
| free(name); | ||
|
|
||
| // Clean up - ProfileXConf_uninit calls unregisterProfileFromScheduler | ||
| EXPECT_CALL(*g_schedulerMock, unregisterProfileFromScheduler(_)) | ||
| .Times(::testing::AtMost(1)) | ||
| .WillRepeatedly(Return(T2ERROR_SUCCESS)); | ||
|
|
||
| test_set_reportThreadExits(true); | ||
| generateDcaReport(false,true); | ||
| EXPECT_CALL(*g_schedulerMock, SendInterruptToTimeoutThread(_)) | ||
| .Times(::testing::AtMost(1)); | ||
|
|
||
| ReportProfiles_Interrupt(); | ||
|
|
||
| EXPECT_EQ(ProfileXConf_uninit(), T2ERROR_SUCCESS); |
There was a problem hiding this comment.
Potential memory leak: The profile structure and its allocated fields (name, protocol, encodingType, t2HTTPDest, grepSeekProfile) are manually allocated but it's unclear if ProfileXConf_uninit() properly frees all this memory. If ProfileXConf_uninit does not take ownership of and free the profile structure, this test will leak memory. Consider explicitly freeing the profile structure after uninit, or verify that ProfileXConf_set takes ownership.
| #ifndef LIBRDKCERTSEL_BUILD | ||
| #ifdef LIBRDKCONFIG_BUILD | ||
| #include "rdkconfig.h" | ||
| #endif | ||
| #endif |
There was a problem hiding this comment.
The nested preprocessor directive structure should be simplified for clarity. Consider flattening the conditional compilation logic. The pattern #ifndef LIBRDKCERTSEL_BUILD wrapping #ifdef LIBRDKCONFIG_BUILD is confusing. It would be clearer to use a single conditional check like #if defined(LIBRDKCONFIG_BUILD) && !defined(LIBRDKCERTSEL_BUILD) or restructure the logic to avoid nested conditionals.
| #ifndef LIBRDKCERTSEL_BUILD | ||
| #ifdef LIBRDKCONFIG_BUILD | ||
| #include "rdkconfig.h" | ||
| #endif | ||
| #endif |
There was a problem hiding this comment.
The nested preprocessor directive structure should be simplified for clarity. Consider flattening the conditional compilation logic. The pattern #ifndef LIBRDKCERTSEL_BUILD wrapping #ifdef LIBRDKCONFIG_BUILD is confusing. It would be clearer to use a single conditional check like #if defined(LIBRDKCONFIG_BUILD) && !defined(LIBRDKCERTSEL_BUILD) or restructure the logic to avoid nested conditionals.
| #ifndef LIBRDKCERTSEL_BUILD | |
| #ifdef LIBRDKCONFIG_BUILD | |
| #include "rdkconfig.h" | |
| #endif | |
| #endif | |
| #if defined(LIBRDKCONFIG_BUILD) && !defined(LIBRDKCERTSEL_BUILD) | |
| #include "rdkconfig.h" | |
| #endif |
| #ifndef LIBRDKCERTSEL_BUILD | ||
| #ifdef LIBRDKCONFIG_BUILD | ||
| #include "rdkconfig.h" | ||
| #endif | ||
| #endif |
There was a problem hiding this comment.
The nested preprocessor directive structure should be simplified for clarity. Consider flattening the conditional compilation logic. The pattern #ifndef LIBRDKCERTSEL_BUILD wrapping #ifdef LIBRDKCONFIG_BUILD is confusing. It would be clearer to use a single conditional check like #if defined(LIBRDKCONFIG_BUILD) && !defined(LIBRDKCERTSEL_BUILD) or restructure the logic to avoid nested conditionals.
| #ifndef LIBRDKCERTSEL_BUILD | |
| #ifdef LIBRDKCONFIG_BUILD | |
| #include "rdkconfig.h" | |
| #endif | |
| #endif | |
| #if defined(LIBRDKCONFIG_BUILD) && !defined(LIBRDKCERTSEL_BUILD) | |
| #include "rdkconfig.h" | |
| #endif |
Code Coverage Summary |
Coverity Issue - Data race conditionAccessing "profile->grepSeekProfile->execCounter" without holding lock "plMutex". Elsewhere, "_GrepSeekProfile.execCounter" is written to with "plMutex" held 2 out of 2 times. Medium Impact, CWE-366 Issue locationThis issue was discovered outside the diff for this Pull Request. You can find it at: |
No description provided.