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>
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive test coverage for the report profiles functionality in the telemetry system. The changes introduce new unit tests for reportprofiles.c and expand existing tests in profileTest.cpp, along with the necessary mock infrastructure and build system configuration.
Changes:
- Added new test file
reportprofilesTest.cppwith tests for report profile processing (msgpack and JSON blob handling) - Created mock infrastructure (
reportprofileMockandprofileMock) to support testing of HTTP reporting and RBUS functionality - Enhanced existing
profileTest.cppwith additional test cases for profile management, callbacks, and edge cases - Updated build configuration to enable test compilation with proper linker wrapping for mock functions
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| test/run_ut.sh | Added reportprofiles_gtest.bin to the test execution list |
| source/test/bulkdata/reportprofilesTest.cpp | New test file with 6 test cases covering report profile blob processing |
| source/test/bulkdata/reportprofileMock.h | Mock header for isRbusEnabled and HTTP reporting functions |
| source/test/bulkdata/reportprofileMock.cpp | Mock implementation with _wrap functions for linker wrapping |
| source/test/bulkdata/profileMock.h | Mock header for HTTP reporting functions used in profile tests |
| source/test/bulkdata/profileMock.cpp | Mock implementation for HTTP functions |
| source/test/bulkdata/profileTest.cpp | Added 11 new test cases and helper function calls for enhanced coverage |
| source/test/bulkdata/Makefile.am | Fixed syntax error, added GTEST_ENABLE flags, configured new test binary with linker wrapping |
| source/ccspinterface/rbusInterface.c | Added DCMAGENT guard for GTEST_ENABLE callback function |
| source/bulkdata/reportprofiles.c | Added GTEST_ENABLE callback functions, removed unnecessary blank lines |
| source/bulkdata/profilexconf.c | Added test helper function to set reportThreadExits for testing |
| source/bulkdata/profile.c | Added macro definitions to redirect HTTP functions to _wrap versions for testing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| void SetUp() override { | ||
| g_reportprofileMock = new reportprofileMock(); | ||
| g_systemMock = new SystemMock(); | ||
| g_fileIOMock = new FileMock(); | ||
| m_rdklogMock = new rdklogMock(); | ||
| g_rbusMock = new rbusMock(); | ||
| g_vectorMock = new VectorMock(); | ||
| } |
There was a problem hiding this comment.
The test fixture does not initialize g_schedulerMock in SetUp, but declares it as extern. If tests using this fixture call functions that depend on g_schedulerMock (which is defined elsewhere), there may be issues if it's null. Consider whether this mock needs to be initialized here or if the extern declaration is sufficient for your use case.
| .Times(::testing::AtMost(5)) | ||
| .WillRepeatedly(Return(T2ERROR_SUCCESS)); | ||
|
|
||
| EXPECT_EQ(ReportProfiles_uninit(), T2ERROR_FAILURE); |
There was a problem hiding this comment.
This test expects ReportProfiles_uninit to return T2ERROR_FAILURE, which seems counterintuitive for a unit test. Tests typically verify successful operation unless specifically testing error handling. Consider whether the expected return value is correct or if the test setup needs adjustment to allow successful uninitialization.
| EXPECT_EQ(ReportProfiles_uninit(), T2ERROR_FAILURE); | |
| EXPECT_EQ(ReportProfiles_uninit(), T2ERROR_SUCCESS); |
| #include "t2log_wrapper.h" | ||
| #include "msgpack.h" | ||
| #include <glib.h> | ||
| #include <glib/gi18n.h> |
There was a problem hiding this comment.
Trailing whitespace found on this line after the include statement. This should be removed to maintain code cleanliness.
| #include <glib/gi18n.h> | |
| #include <glib/gi18n.h> |
| ProfileXConf profile; | ||
| EXPECT_CALL(*g_vectorMock, Vector_Size(_)) | ||
| .Times(::testing::AtMost(1)) | ||
| .WillRepeatedly(Return(0)); // Return 1 to indicate only one profile (no duplicates) |
There was a problem hiding this comment.
The comment on this line is incorrect. The mock is set to return 0 (indicating an empty list), not 1 (one profile). The comment should be updated to match the actual behavior or removed if it's a copy-paste error.
| g_reportprofileMock = new reportprofileMock(); | ||
| g_systemMock = new SystemMock(); | ||
| g_fileIOMock = new FileMock(); | ||
| m_rdklogMock = new rdklogMock(); | ||
| g_rbusMock = new rbusMock(); | ||
| g_vectorMock = new VectorMock(); | ||
| } | ||
| void TearDown() override { | ||
| delete g_reportprofileMock; | ||
| delete g_systemMock; | ||
| delete g_fileIOMock; | ||
| delete m_rdklogMock; | ||
| delete g_rbusMock; | ||
| delete g_vectorMock; | ||
| g_reportprofileMock = nullptr; |
There was a problem hiding this comment.
Inconsistent indentation: Lines 61, 69, and 75 use tabs while the surrounding lines use spaces. This should be corrected to match the indentation style of the rest of the file.
| 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.
Memory leak: The allocated memory for 'msg' and 'webConfigString' is not freed before the test ends. This should be cleaned up to prevent memory leaks in the test suite.
| ReportProfiles_ProcessReportProfilesMsgPackBlob(msg->msgpack_blob, msg->msgpack_blob_size); | |
| ReportProfiles_ProcessReportProfilesMsgPackBlob(msg->msgpack_blob, msg->msgpack_blob_size); | |
| g_free(msg->msgpack_blob); | |
| free(msg); |
| //#ifndef SOURCE_TEST_MOCKS_SYSTEMMOCK_H_ | ||
| //#define SOURCE_TEST_MOCKS_SYSTEMMOCK_H_ | ||
|
|
There was a problem hiding this comment.
Commented-out code should be removed. If this code is not needed, it should be deleted rather than left as a comment. Version control systems can track historical code if needed.
| //#ifndef SOURCE_TEST_MOCKS_SYSTEMMOCK_H_ | |
| //#define SOURCE_TEST_MOCKS_SYSTEMMOCK_H_ |
| gsProfile->execCounter = 0; | ||
| } | ||
| profile->grepSeekProfile = gsProfile; | ||
| //profile->grepSeekProfile = nullptr; |
There was a problem hiding this comment.
Commented-out code should be removed. This line sets a property that is subsequently commented out, indicating uncertainty about the test setup. Either use it or remove it.
| //profile->grepSeekProfile = nullptr; |
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.