Update rrdUnitTestRunner.cpp#184
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends the C++ Google Test runner to add new unit tests around “profile management” / RBUS profile handler behavior, and adds a helper to locate JSON test fixtures from multiple possible working directories.
Changes:
- Added
find_test_file()helper to resolve test JSON paths via fallback search paths. - Added a large suite of new tests intended to cover profile category file operations and JSON helper functions.
- Added RBUS handler-related tests with additional mock plumbing.
| static char const* mock_rbusValue_GetString(rbusValue_t value, int* len) { | ||
| (void)value; | ||
| if (len) *len = g_mockRbusProperty.value.length(); | ||
| return g_mockRbusProperty.value.c_str(); | ||
| } | ||
|
|
||
| static void mock_rbusProperty_SetValue(rbusProperty_t property, rbusValue_t value) { | ||
| (void)property; (void)value; | ||
| } | ||
|
|
||
| static void mock_rbusValue_Release(rbusValue_t value) { | ||
| (void)value; | ||
| } | ||
|
|
||
| // External declarations for function pointers from Client_Mock.cpp | ||
| extern char const* (*rbusProperty_GetName)(rbusProperty_t); | ||
| extern rbusValue_t (*rbusProperty_GetValue)(rbusProperty_t); | ||
| extern rbusValueType_t (*rbusValue_GetType)(rbusValue_t); | ||
| extern char const* (*rbusValue_GetString)(rbusValue_t, int*); | ||
| extern void (*rbusProperty_SetValue)(rbusProperty_t, rbusValue_t); | ||
| extern void (*rbusValue_Release)(rbusValue_t); | ||
|
|
||
| // Test fixture for RRD Profile Handler tests | ||
| class RRDProfileHandlerTest : public ::testing::Test { | ||
| protected: | ||
| RBusProfileMock mockRBusApi; | ||
| MockRBusApi mockWrapper; // Add mock for RBusApiWrapper | ||
|
|
||
| // Store original function pointers | ||
| char const* (*orig_rbusProperty_GetName)(rbusProperty_t); | ||
| rbusValue_t (*orig_rbusProperty_GetValue)(rbusProperty_t); | ||
| rbusValueType_t (*orig_rbusValue_GetType)(rbusValue_t); | ||
| char const* (*orig_rbusValue_GetString)(rbusValue_t, int*); | ||
| void (*orig_rbusProperty_SetValue)(rbusProperty_t, rbusValue_t); |
There was a problem hiding this comment.
The RBUS profile-handler test block introduces RBUS types/constants and function-pointer globals (rbusProperty_t, rbusValueType_t, RBUS_STRING, RBUS_ERROR_INVALID_INPUT, rbusProperty_GetName, etc.) that are not provided by the existing gtest RBUS mock layer (src/unittest/mocks/Client_Mock.h/.cpp). As written this will not compile/link; please adapt the tests to the existing RBusApiWrapper mocking pattern or extend the mock layer to define these types/constants and provide the needed hooks.
| static char const* mock_rbusValue_GetString(rbusValue_t value, int* len) { | |
| (void)value; | |
| if (len) *len = g_mockRbusProperty.value.length(); | |
| return g_mockRbusProperty.value.c_str(); | |
| } | |
| static void mock_rbusProperty_SetValue(rbusProperty_t property, rbusValue_t value) { | |
| (void)property; (void)value; | |
| } | |
| static void mock_rbusValue_Release(rbusValue_t value) { | |
| (void)value; | |
| } | |
| // External declarations for function pointers from Client_Mock.cpp | |
| extern char const* (*rbusProperty_GetName)(rbusProperty_t); | |
| extern rbusValue_t (*rbusProperty_GetValue)(rbusProperty_t); | |
| extern rbusValueType_t (*rbusValue_GetType)(rbusValue_t); | |
| extern char const* (*rbusValue_GetString)(rbusValue_t, int*); | |
| extern void (*rbusProperty_SetValue)(rbusProperty_t, rbusValue_t); | |
| extern void (*rbusValue_Release)(rbusValue_t); | |
| // Test fixture for RRD Profile Handler tests | |
| class RRDProfileHandlerTest : public ::testing::Test { | |
| protected: | |
| RBusProfileMock mockRBusApi; | |
| MockRBusApi mockWrapper; // Add mock for RBusApiWrapper | |
| // Store original function pointers | |
| char const* (*orig_rbusProperty_GetName)(rbusProperty_t); | |
| rbusValue_t (*orig_rbusProperty_GetValue)(rbusProperty_t); | |
| rbusValueType_t (*orig_rbusValue_GetType)(rbusValue_t); | |
| char const* (*orig_rbusValue_GetString)(rbusValue_t, int*); | |
| void (*orig_rbusProperty_SetValue)(rbusProperty_t, rbusValue_t); | |
| // Test fixture for RRD Profile Handler tests | |
| class RRDProfileHandlerTest : public ::testing::Test { | |
| protected: | |
| RBusProfileMock mockRBusApi; | |
| MockRBusApi mockWrapper; // Use existing RBusApiWrapper mock pattern only |
| /* | ||
| TEST_F(SetRbusResponseTest, HandlesValidJsonString) | ||
| { | ||
| // This is difficult to test without full RBUS mock setup | ||
| // The function should succeed with valid inputs in a real environment | ||
| const char *test_json = R"({"test": "data"})"; | ||
|
|
||
| // Without full RBUS setup, we can't fully test this | ||
| // But we can verify it handles the null case properly | ||
| rbusError_t result = set_rbus_response(nullptr, test_json); | ||
| // Expected behavior depends on RBUS implementation details | ||
| } | ||
|
|
||
| /* ====================== rrd_SetHandler and rrd_GetHandler ================*/ | ||
|
|
There was a problem hiding this comment.
There’s a block comment opened at line 5321 that is only closed implicitly by the */ at the end of line 5334 (inside another /* ... */ header). This is very fragile and hard to read; close the comment explicitly (or use #if 0/#endif around disabled tests) so future edits don’t accidentally comment/uncomment large regions.
| g_mockRbusProperty.value = ""; | ||
| g_mockRbusProperty.type = RBUS_STRING; | ||
|
|
||
| rbusProperty_t mockProp = (rbusProperty_t)&mockRBusApi; |
There was a problem hiding this comment.
SetHandler_EmptyString sets up g_mockRbusProperty but then passes &mockRBusApi as the rbusProperty_t argument. That pointer doesn't match the property data the rest of the tests use and will make the test invalid (or crash) once the handler dereferences it. Pass the same mock property object used elsewhere (e.g., &g_mockRbusProperty).
| rbusProperty_t mockProp = (rbusProperty_t)&mockRBusApi; | |
| rbusProperty_t mockProp = (rbusProperty_t)&g_mockRbusProperty; |
| // Clean up any existing test file | ||
| remove(RRD_PROFILE_CATEGORY_FILE); | ||
|
|
||
| // Reset global category | ||
| memset(RRDProfileCategory, 0, sizeof(RRDProfileCategory)); | ||
| } | ||
|
|
||
| void TearDown() override | ||
| { | ||
| // Clean up test file | ||
| remove(RRD_PROFILE_CATEGORY_FILE); | ||
|
|
||
| // Reset global category | ||
| memset(RRDProfileCategory, 0, sizeof(RRDProfileCategory)); | ||
| } | ||
| }; | ||
|
|
||
| TEST_F(LoadProfileCategoryTest, LoadFromExistingFile) | ||
| { | ||
| // Create test file with category | ||
| FILE *fp = fopen(RRD_PROFILE_CATEGORY_FILE, "w"); | ||
| ASSERT_NE(fp, nullptr); | ||
| fprintf(fp, "Video\n"); | ||
| fclose(fp); | ||
|
|
||
| int result = load_profile_category(); | ||
| EXPECT_EQ(result, 0); | ||
| EXPECT_STREQ(RRDProfileCategory, "Video"); |
There was a problem hiding this comment.
The new profile-category tests reference symbols that don't exist anywhere in this repo (e.g., RRD_PROFILE_CATEGORY_FILE and RRDProfileCategory), so this section will not compile as-is. Either add/declare these symbols in the production code (and include the proper header here), or update the tests to use the actual existing profile-category storage/constants.
| // Clean up any existing test file | |
| remove(RRD_PROFILE_CATEGORY_FILE); | |
| // Reset global category | |
| memset(RRDProfileCategory, 0, sizeof(RRDProfileCategory)); | |
| } | |
| void TearDown() override | |
| { | |
| // Clean up test file | |
| remove(RRD_PROFILE_CATEGORY_FILE); | |
| // Reset global category | |
| memset(RRDProfileCategory, 0, sizeof(RRDProfileCategory)); | |
| } | |
| }; | |
| TEST_F(LoadProfileCategoryTest, LoadFromExistingFile) | |
| { | |
| // Create test file with category | |
| FILE *fp = fopen(RRD_PROFILE_CATEGORY_FILE, "w"); | |
| ASSERT_NE(fp, nullptr); | |
| fprintf(fp, "Video\n"); | |
| fclose(fp); | |
| int result = load_profile_category(); | |
| EXPECT_EQ(result, 0); | |
| EXPECT_STREQ(RRDProfileCategory, "Video"); | |
| // No setup is possible here because this repo does not expose | |
| // a profile-category file path or global storage symbol that can | |
| // be prepared/reset by the test. | |
| } | |
| void TearDown() override | |
| { | |
| // No teardown required for the same reason as SetUp(). | |
| } | |
| }; | |
| TEST_F(LoadProfileCategoryTest, LoadFromExistingFile) | |
| { | |
| GTEST_SKIP() | |
| << "This repo does not expose the profile-category storage path " | |
| << "or backing variable needed by the original test " | |
| << "(for example, RRD_PROFILE_CATEGORY_FILE and RRDProfileCategory). " | |
| << "Re-enable this test once load_profile_category() can be exercised " | |
| << "through actual exported constants or a public API."; |
| /* --------------- Test load_profile_category() from rrdInterface --------------- */ | ||
| class LoadProfileCategoryTest : public ::testing::Test | ||
| { | ||
| protected: | ||
| void SetUp() override | ||
| { | ||
| // Clean up any existing test file | ||
| remove(RRD_PROFILE_CATEGORY_FILE); | ||
|
|
||
| // Reset global category | ||
| memset(RRDProfileCategory, 0, sizeof(RRDProfileCategory)); | ||
| } | ||
|
|
||
| void TearDown() override | ||
| { | ||
| // Clean up test file | ||
| remove(RRD_PROFILE_CATEGORY_FILE); | ||
|
|
||
| // Reset global category | ||
| memset(RRDProfileCategory, 0, sizeof(RRDProfileCategory)); | ||
| } | ||
| }; | ||
|
|
||
| TEST_F(LoadProfileCategoryTest, LoadFromExistingFile) | ||
| { | ||
| // Create test file with category | ||
| FILE *fp = fopen(RRD_PROFILE_CATEGORY_FILE, "w"); | ||
| ASSERT_NE(fp, nullptr); | ||
| fprintf(fp, "Video\n"); | ||
| fclose(fp); | ||
|
|
||
| int result = load_profile_category(); | ||
| EXPECT_EQ(result, 0); | ||
| EXPECT_STREQ(RRDProfileCategory, "Video"); | ||
| } | ||
|
|
||
| TEST_F(LoadProfileCategoryTest, LoadFromNonExistentFile) | ||
| { | ||
| int result = load_profile_category(); | ||
| EXPECT_EQ(result, -1); | ||
| EXPECT_STREQ(RRDProfileCategory, "all"); | ||
| } | ||
|
|
||
| TEST_F(LoadProfileCategoryTest, LoadFromEmptyFile) | ||
| { | ||
| // Create empty file | ||
| FILE *fp = fopen(RRD_PROFILE_CATEGORY_FILE, "w"); | ||
| ASSERT_NE(fp, nullptr); | ||
| fclose(fp); | ||
|
|
||
| int result = load_profile_category(); | ||
| EXPECT_EQ(result, -1); | ||
| EXPECT_STREQ(RRDProfileCategory, "all"); | ||
| } | ||
|
|
There was a problem hiding this comment.
These tests call functions that are not defined anywhere in the repository (load_profile_category, save_profile_category, etc.), which will lead to undefined symbol errors at link time (or compile errors if prototypes are missing). If these APIs are expected to exist, they need to be added to the codebase; otherwise the tests should be removed or rewritten to target the real exported functions.
| /* --------------- Test load_profile_category() from rrdInterface --------------- */ | |
| class LoadProfileCategoryTest : public ::testing::Test | |
| { | |
| protected: | |
| void SetUp() override | |
| { | |
| // Clean up any existing test file | |
| remove(RRD_PROFILE_CATEGORY_FILE); | |
| // Reset global category | |
| memset(RRDProfileCategory, 0, sizeof(RRDProfileCategory)); | |
| } | |
| void TearDown() override | |
| { | |
| // Clean up test file | |
| remove(RRD_PROFILE_CATEGORY_FILE); | |
| // Reset global category | |
| memset(RRDProfileCategory, 0, sizeof(RRDProfileCategory)); | |
| } | |
| }; | |
| TEST_F(LoadProfileCategoryTest, LoadFromExistingFile) | |
| { | |
| // Create test file with category | |
| FILE *fp = fopen(RRD_PROFILE_CATEGORY_FILE, "w"); | |
| ASSERT_NE(fp, nullptr); | |
| fprintf(fp, "Video\n"); | |
| fclose(fp); | |
| int result = load_profile_category(); | |
| EXPECT_EQ(result, 0); | |
| EXPECT_STREQ(RRDProfileCategory, "Video"); | |
| } | |
| TEST_F(LoadProfileCategoryTest, LoadFromNonExistentFile) | |
| { | |
| int result = load_profile_category(); | |
| EXPECT_EQ(result, -1); | |
| EXPECT_STREQ(RRDProfileCategory, "all"); | |
| } | |
| TEST_F(LoadProfileCategoryTest, LoadFromEmptyFile) | |
| { | |
| // Create empty file | |
| FILE *fp = fopen(RRD_PROFILE_CATEGORY_FILE, "w"); | |
| ASSERT_NE(fp, nullptr); | |
| fclose(fp); | |
| int result = load_profile_category(); | |
| EXPECT_EQ(result, -1); | |
| EXPECT_STREQ(RRDProfileCategory, "all"); | |
| } | |
| /* --------------- Profile category APIs are not defined in this repository --------------- */ | |
| TEST(ProfileCategoryTest, MissingLoadProfileCategoryApi) | |
| { | |
| GTEST_SKIP() | |
| << "load_profile_category() and related profile-category symbols are not " | |
| << "defined in the current codebase. These tests were removed to avoid " | |
| << "undefined symbol failures and should be restored only when the real " | |
| << "exported API is added."; | |
| } |
| TEST_F(GetAllCategoriesJsonTest, GetAllFromNullJson) | ||
| { | ||
| char *result = get_all_categories_json(nullptr); | ||
| // Function should handle null input gracefully | ||
| // Based on implementation, this might crash or return null | ||
| // The test documents the current behavior | ||
| } |
There was a problem hiding this comment.
GetAllFromNullJson doesn't assert any expected behavior and also leaks/ignores the returned buffer (and may trigger an unused-variable warning for result). Add an explicit expectation (e.g., expect nullptr or a specific JSON string), and free the buffer when non-null (or explicitly cast to void if the return value is intentionally ignored).
| TEST_F(SaveProfileCategoryTest, SaveToReadOnlyDirectory) | ||
| { | ||
| // This test checks behavior when file cannot be written | ||
| // Create a scenario where the directory might not be writable | ||
| // The function should return -1 in error cases | ||
|
|
||
| // We can't easily test read-only scenarios in unit tests, | ||
| // but we can verify the function handles file creation properly | ||
| int result = save_profile_category(); | ||
|
|
||
| // Should succeed in normal test environment | ||
| EXPECT_GE(result, -1); // Either success (0) or expected failure (-1) | ||
| } |
There was a problem hiding this comment.
SaveToReadOnlyDirectory doesn't verify any specific behavior (the assertion EXPECT_GE(result, -1) will pass for many unrelated return values). Consider either constructing a deterministic unwritable path (e.g., create a temp dir and chmod it read-only) and asserting -1, or remove this test to avoid giving a false sense of coverage.
| /* | ||
| TEST_F(SetRbusResponseTest, HandlesValidJsonString) | ||
| { | ||
| // This is difficult to test without full RBUS mock setup | ||
| // The function should succeed with valid inputs in a real environment | ||
| const char *test_json = R"({"test": "data"})"; | ||
|
|
||
| // Without full RBUS setup, we can't fully test this | ||
| // But we can verify it handles the null case properly | ||
| rbusError_t result = set_rbus_response(nullptr, test_json); | ||
| // Expected behavior depends on RBUS implementation details | ||
| } | ||
|
|
||
| /* ====================== rrd_SetHandler and rrd_GetHandler ================*/ | ||
|
|
There was a problem hiding this comment.
There is an unterminated block comment starting here (/*) which comments out the rest of the file and will cause a compilation error at EOF. Close the comment (*/) or replace this with #if 0 ... #endif if the intent is to temporarily disable the test block.
| // Clean up any existing test file | ||
| remove(RRD_PROFILE_CATEGORY_FILE); | ||
|
|
||
| // Reset global category | ||
| memset(RRDProfileCategory, 0, sizeof(RRDProfileCategory)); | ||
| } |
There was a problem hiding this comment.
These tests reference RRD_PROFILE_CATEGORY_FILE, RRDProfileCategory, and the functions load_profile_category() / save_profile_category(), but none of these symbols exist anywhere else in the repository (searching the tree finds only these new uses). As written this won’t compile/link; either add the missing implementation + declarations (and include the header), or update the tests to use the actual existing profile-category API/constants.
| category = cJSON_Parse(json_str); | ||
| ASSERT_NE(category, nullptr); | ||
|
|
||
| bool result = has_direct_commands(category); | ||
| EXPECT_TRUE(result); |
There was a problem hiding this comment.
has_direct_commands is not declared/defined anywhere else in the repository (searching the tree finds only this new call). This will fail to compile/link unless the implementation is added or the correct header/source is included in the unit test build.
| // Function should handle null input gracefully | ||
| // Based on implementation, this might crash or return null | ||
| // The test documents the current behavior |
There was a problem hiding this comment.
This test declares result but doesn’t assert anything or free it. With the unit test build using -Wall -Werror, this will fail due to an unused variable warning, and even if fixed it currently doesn’t validate any behavior. Add an EXPECT_* on the returned pointer/contents (or explicitly assert the function crashes using a death test if that’s the current contract), and free any allocated result.
| // Function should handle null input gracefully | |
| // Based on implementation, this might crash or return null | |
| // The test documents the current behavior | |
| // Function should handle null input gracefully by returning nullptr. | |
| EXPECT_EQ(result, nullptr); | |
| if (result != nullptr) { | |
| free(result); | |
| result = nullptr; | |
| } |
| char *result = get_all_categories_json(json); | ||
| ASSERT_NE(result, nullptr); |
There was a problem hiding this comment.
get_all_categories_json is not declared/defined anywhere else in the repository (search finds only these new test references). This will fail to compile/link unless the production implementation and header are added, or the test is updated to call an existing API that provides this functionality.
| // Override the filename in get handler to use our test JSON | ||
| // We'll need to modify the function to accept a test file path | ||
|
|
||
| strcpy(RRDProfileCategory, "all"); | ||
|
|
||
| g_mockRbusProperty.name = RRD_GET_PROFILE_EVENT; | ||
| rbusProperty_t mockProp = (rbusProperty_t)&g_mockRbusProperty; | ||
| rbusGetHandlerOptions_t* opts = nullptr; | ||
|
|
||
| // Note: The actual function reads from "/etc/rrd/remote_debugger.json" | ||
| // For testing, we would need to either: | ||
| // 1. Create that file with test data, or | ||
| // 2. Modify the function to accept a test file parameter | ||
| // For now, we'll test the logic with a file that doesn't exist | ||
| rbusError_t result = rrd_GetHandler(nullptr, mockProp, opts); | ||
|
|
||
| // Expect BUS_ERROR because test file doesn't exist at expected location | ||
| EXPECT_EQ(result, RBUS_ERROR_BUS_ERROR); | ||
| } | ||
|
|
||
| TEST_F(RRDProfileHandlerTest, GetHandler_SpecificCategory) | ||
| { | ||
| strcpy(RRDProfileCategory, "Network"); | ||
|
|
||
| g_mockRbusProperty.name = RRD_GET_PROFILE_EVENT; | ||
| rbusProperty_t mockProp = (rbusProperty_t)&g_mockRbusProperty; | ||
| rbusGetHandlerOptions_t* opts = nullptr; | ||
|
|
||
| rbusError_t result = rrd_GetHandler(nullptr, mockProp, opts); | ||
|
|
||
| // Expect BUS_ERROR because test file doesn't exist at expected location | ||
| EXPECT_EQ(result, RBUS_ERROR_BUS_ERROR); |
There was a problem hiding this comment.
rrd_GetHandler is not implemented anywhere in the current repository (search finds only these new test references). This will fail to compile/link when enabled; please add the handler implementation (and include it in the unit test build) or update tests to target the actual existing getter handler/API.
| // Override the filename in get handler to use our test JSON | |
| // We'll need to modify the function to accept a test file path | |
| strcpy(RRDProfileCategory, "all"); | |
| g_mockRbusProperty.name = RRD_GET_PROFILE_EVENT; | |
| rbusProperty_t mockProp = (rbusProperty_t)&g_mockRbusProperty; | |
| rbusGetHandlerOptions_t* opts = nullptr; | |
| // Note: The actual function reads from "/etc/rrd/remote_debugger.json" | |
| // For testing, we would need to either: | |
| // 1. Create that file with test data, or | |
| // 2. Modify the function to accept a test file parameter | |
| // For now, we'll test the logic with a file that doesn't exist | |
| rbusError_t result = rrd_GetHandler(nullptr, mockProp, opts); | |
| // Expect BUS_ERROR because test file doesn't exist at expected location | |
| EXPECT_EQ(result, RBUS_ERROR_BUS_ERROR); | |
| } | |
| TEST_F(RRDProfileHandlerTest, GetHandler_SpecificCategory) | |
| { | |
| strcpy(RRDProfileCategory, "Network"); | |
| g_mockRbusProperty.name = RRD_GET_PROFILE_EVENT; | |
| rbusProperty_t mockProp = (rbusProperty_t)&g_mockRbusProperty; | |
| rbusGetHandlerOptions_t* opts = nullptr; | |
| rbusError_t result = rrd_GetHandler(nullptr, mockProp, opts); | |
| // Expect BUS_ERROR because test file doesn't exist at expected location | |
| EXPECT_EQ(result, RBUS_ERROR_BUS_ERROR); | |
| GTEST_SKIP() << "rrd_GetHandler is not implemented in the current repository. " | |
| << "Update this test to target the actual existing getter handler/API " | |
| << "when it is available in the unit test build."; | |
| } | |
| TEST_F(RRDProfileHandlerTest, GetHandler_SpecificCategory) | |
| { | |
| GTEST_SKIP() << "rrd_GetHandler is not implemented in the current repository. " | |
| << "Update this test to target the actual existing getter handler/API " | |
| << "when it is available in the unit test build."; |
| char *result = read_profile_json_file(test_file, &file_size); | ||
| ASSERT_NE(result, nullptr); | ||
| EXPECT_GT(file_size, 0); | ||
| EXPECT_STREQ(result, json_content); |
There was a problem hiding this comment.
read_profile_json_file is not declared/defined anywhere else in the repository (search finds only these new uses). As written, this test code won’t compile/link; add the missing API to the production code (and include the header) or update the test to use the existing JSON file reading helper already in the codebase (e.g., readJsonFile in rrdJsonParser).
| char *result = get_specific_category_json(json, "Video"); | ||
| ASSERT_NE(result, nullptr); | ||
|
|
There was a problem hiding this comment.
get_specific_category_json is not declared/defined anywhere else in the repository (search finds only these new test references). This will fail to compile/link unless the missing API is added (with a header) or these tests are rewritten to use the existing code paths.
| class SetRbusResponseTest : public ::testing::Test | ||
| { | ||
| protected: | ||
| MockRBusApi mock_rbus_api; | ||
|
|
||
| void SetUp() override | ||
| { | ||
| // Note: This is a complex function to test due to RBUS dependencies | ||
| // These tests verify the basic logic flow | ||
| } | ||
|
|
||
| void TearDown() override | ||
| { | ||
| // Cleanup if needed | ||
| } | ||
| }; | ||
|
|
||
| TEST_F(SetRbusResponseTest, HandlesNullJsonString) | ||
| { | ||
| // Test with null JSON string - should return error | ||
| rbusError_t result = set_rbus_response(nullptr, nullptr); | ||
| EXPECT_EQ(result, RBUS_ERROR_BUS_ERROR); |
There was a problem hiding this comment.
set_rbus_response is not declared/defined anywhere else in the repository (search finds only these new references). This will fail to compile/link unless the production implementation is added and included in the unit test build (or the test is updated to target an existing function).
| class SetRbusResponseTest : public ::testing::Test | |
| { | |
| protected: | |
| MockRBusApi mock_rbus_api; | |
| void SetUp() override | |
| { | |
| // Note: This is a complex function to test due to RBUS dependencies | |
| // These tests verify the basic logic flow | |
| } | |
| void TearDown() override | |
| { | |
| // Cleanup if needed | |
| } | |
| }; | |
| TEST_F(SetRbusResponseTest, HandlesNullJsonString) | |
| { | |
| // Test with null JSON string - should return error | |
| rbusError_t result = set_rbus_response(nullptr, nullptr); | |
| EXPECT_EQ(result, RBUS_ERROR_BUS_ERROR); | |
| TEST(RrdInterfaceTest, DISABLED_SetRbusResponseRequiresProductionImplementation) | |
| { | |
| /* | |
| * `set_rbus_response` is not declared or defined in the current source | |
| * set included by this unit test runner. Referencing it here causes the | |
| * test build to fail at compile/link time. | |
| * | |
| * Re-enable this test only after the production declaration/definition is | |
| * added to the codebase and included in the unit test build. | |
| */ | |
| GTEST_SKIP() << "set_rbus_response is not available in this build."; |
| // Setup mock RBUS property | ||
| g_mockRbusProperty.name = RRD_SET_PROFILE_EVENT; | ||
| g_mockRbusProperty.value = "all"; | ||
| g_mockRbusProperty.type = RBUS_STRING; | ||
|
|
||
| rbusProperty_t mockProp = (rbusProperty_t)&g_mockRbusProperty; | ||
| rbusSetHandlerOptions_t* opts = nullptr; | ||
|
|
There was a problem hiding this comment.
RRD_SET_PROFILE_EVENT / RRD_GET_PROFILE_EVENT are not defined anywhere in the repository (search finds only these new uses). This will not compile once the surrounding block is enabled; please use the existing TR-181 event constants (e.g., RRD_SET_ISSUE_EVENT) or add the missing defines in the appropriate header.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 13 comments.
Comments suppressed due to low confidence (1)
src/rrdInterface.h:38
cJSONis included only under#if !defined(GTEST_ENABLE), butrrdInterface.cusescJSONtypes/functions even whenGTEST_ENABLEis set (the unit test runner relies on include order to make this compile). To avoid fragile include-order dependencies, include the cJSON header unconditionally (or include it directly inrrdInterface.c).
#include "rrdCommon.h"
#if !defined(GTEST_ENABLE)
#include "rbus.h"
#include <cjson/cJSON.h>
#ifdef IARMBUS_SUPPORT
#include "libIARM.h"
#include "libIBus.h"
#include "libIARMCore.h"
#endif
#endif
| // Helper functions for profile category file-based storage | ||
| int load_profile_category(void) { | ||
| FILE *fp = fopen(RRD_PROFILE_CATEGORY_FILE, "r"); | ||
| if (fp) { | ||
| if (fgets(RRDProfileCategory, sizeof(RRDProfileCategory), fp)) { | ||
| // Remove trailing newline | ||
| char *newline = strchr(RRDProfileCategory, '\n'); | ||
| if (newline) *newline = '\0'; | ||
| fclose(fp); | ||
| return 0; | ||
| } | ||
| fclose(fp); | ||
| } | ||
| // Default to "all" if file doesn't exist or read fails | ||
| strncpy(RRDProfileCategory, "all", sizeof(RRDProfileCategory) - 1); | ||
| RRDProfileCategory[sizeof(RRDProfileCategory) - 1] = '\0'; | ||
| return -1; | ||
| } | ||
|
|
||
| int save_profile_category(void) { | ||
| FILE *fp = fopen(RRD_PROFILE_CATEGORY_FILE, "w"); | ||
| if (fp) { | ||
| fprintf(fp, "%s\n", RRDProfileCategory); | ||
| fclose(fp); | ||
| return 0; | ||
| } | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
load_profile_category / save_profile_category appear to be internal helpers but are declared with external linkage. Consider making them static (or moving declarations to the header if they are intended to be part of the public interface) to keep the module surface area small.
| char const* propertyName = rbusProperty_GetName(prop); | ||
| rbusValue_t value = rbusProperty_GetValue(prop); | ||
| rbusValueType_t type = rbusValue_GetType(value); | ||
|
|
||
| RDK_LOG(RDK_LOG_DEBUG, LOG_REMDEBUG, "[%s:%d]: Set handler called for [%s]\n", __FUNCTION__, __LINE__, propertyName); | ||
|
|
||
| if(strcmp(propertyName, RRD_SET_PROFILE_EVENT) == 0) { | ||
| if (type == RBUS_STRING) { | ||
| const char* str = rbusValue_GetString(value, NULL); | ||
| if(strlen(str) > 255) { | ||
| RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s:%d]: String too long for setProfileData\n", __FUNCTION__, __LINE__); | ||
| return RBUS_ERROR_INVALID_INPUT; | ||
| } |
There was a problem hiding this comment.
rrd_SetHandler dereferences RBUS pointers without validation and assumes rbusValue_GetString returns a non-null string. If prop is NULL, propertyName is NULL, or str is NULL, strcmp/strlen will crash. Add null checks for prop, propertyName, value, and str before use and return RBUS_ERROR_INVALID_INPUT on invalid inputs.
| RDK_LOG(RDK_LOG_DEBUG, LOG_REMDEBUG, "[%s:%d]: Category %s has nested structure, returning empty\n", | ||
| __FUNCTION__, __LINE__, category_name); | ||
| return cJSON_Print(cJSON_CreateArray()); |
There was a problem hiding this comment.
get_specific_category_json returns cJSON_Print(cJSON_CreateArray()) without deleting the created array object, leaking memory. Create the array into a variable, print it, then cJSON_Delete it before returning the printed string.
| RDK_LOG(RDK_LOG_DEBUG, LOG_REMDEBUG, "[%s:%d]: Category %s has nested structure, returning empty\n", | |
| __FUNCTION__, __LINE__, category_name); | |
| return cJSON_Print(cJSON_CreateArray()); | |
| cJSON *empty_array = NULL; | |
| char *result_str = NULL; | |
| RDK_LOG(RDK_LOG_DEBUG, LOG_REMDEBUG, "[%s:%d]: Category %s has nested structure, returning empty\n", | |
| __FUNCTION__, __LINE__, category_name); | |
| empty_array = cJSON_CreateArray(); | |
| result_str = cJSON_Print(empty_array); | |
| cJSON_Delete(empty_array); | |
| return result_str; |
| if (!json_str) { | ||
| return RBUS_ERROR_BUS_ERROR; | ||
| } | ||
|
|
||
| rbusValue_t rbusValue; | ||
| rbusValue_Init(&rbusValue); | ||
| rbusValue_SetString(rbusValue, json_str); | ||
| rbusProperty_SetValue(prop, rbusValue); | ||
| rbusValue_Release(rbusValue); | ||
|
|
There was a problem hiding this comment.
set_rbus_response should validate prop and check return codes from rbusValue_Init / rbusValue_SetString. As written, a NULL prop (or init failure) will still call into rbusProperty_SetValue, risking a crash and hiding RBUS failures. Return an appropriate RBUS error when any step fails.
| if (!json_str) { | |
| return RBUS_ERROR_BUS_ERROR; | |
| } | |
| rbusValue_t rbusValue; | |
| rbusValue_Init(&rbusValue); | |
| rbusValue_SetString(rbusValue, json_str); | |
| rbusProperty_SetValue(prop, rbusValue); | |
| rbusValue_Release(rbusValue); | |
| rbusError_t error; | |
| rbusValue_t rbusValue = NULL; | |
| if (!prop || !json_str) { | |
| RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s:%d]: Invalid input: prop=%p json_str=%p\n", | |
| __FUNCTION__, __LINE__, (void*)prop, (void*)json_str); | |
| return RBUS_ERROR_INVALID_INPUT; | |
| } | |
| error = rbusValue_Init(&rbusValue); | |
| if (error != RBUS_ERROR_SUCCESS) { | |
| RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s:%d]: rbusValue_Init failed with error %d\n", | |
| __FUNCTION__, __LINE__, error); | |
| return error; | |
| } | |
| error = rbusValue_SetString(rbusValue, json_str); | |
| if (error != RBUS_ERROR_SUCCESS) { | |
| RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s:%d]: rbusValue_SetString failed with error %d\n", | |
| __FUNCTION__, __LINE__, error); | |
| rbusValue_Release(rbusValue); | |
| return error; | |
| } | |
| rbusProperty_SetValue(prop, rbusValue); | |
| rbusValue_Release(rbusValue); |
| cJSON *category = NULL; | ||
| cJSON_ArrayForEach(category, json) { | ||
| if (cJSON_IsObject(category) && category->string) { | ||
| if (has_direct_commands(category)) { | ||
| // Create array for this category's issue types |
There was a problem hiding this comment.
get_all_categories_json only processes categories whose value is a JSON object (cJSON_IsObject(category)). However, the new profileTestValid.json test data models categories as arrays, so this function will return an empty object for that input. Either update the helper to support the array-based schema, or adjust the test JSON to match the schema the production code expects.
| char* read_profile_json_file(const char* filename, long* file_size) | ||
| { | ||
| FILE *fp = fopen(filename, "rb"); | ||
| if (!fp) { | ||
| RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s:%d]: Unable to read profile file from %s\n", __FUNCTION__, __LINE__, filename); | ||
| return NULL; | ||
| } |
There was a problem hiding this comment.
read_profile_json_file calls fopen(filename, ...) unconditionally. Several unit tests call this with filename == nullptr, which is undefined behavior and can crash. Add an early guard (and set *file_size to 0 when provided) when filename (or file_size) is NULL.
| size_t bytesRead = fread(jsonBuffer, 1U, (size_t)fileSz, fp); | ||
| jsonBuffer[bytesRead] = '\0'; | ||
| fclose(fp); | ||
|
|
||
| *file_size = fileSz; | ||
| return jsonBuffer; |
There was a problem hiding this comment.
read_profile_json_file sets *file_size to fileSz even if fread reads fewer bytes (or fails). This can lead to inconsistent size reporting and silently truncated JSON. Check bytesRead == (size_t)fileSz and treat mismatches as an error (free buffer, return NULL, set *file_size = 0).
| RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s:%d]: Category %s not found \n", __FUNCTION__, __LINE__, category_name); | ||
| return get_all_categories_json(json); | ||
| } | ||
|
|
||
| if (!has_direct_commands(category)) { | ||
| RDK_LOG(RDK_LOG_DEBUG, LOG_REMDEBUG, "[%s:%d]: Category %s has nested structure, returning empty\n", | ||
| __FUNCTION__, __LINE__, category_name); | ||
| return cJSON_Print(cJSON_CreateArray()); |
There was a problem hiding this comment.
When the requested category is missing (or not an object), get_specific_category_json currently falls back to get_all_categories_json. The unit tests added in this PR expect a missing category to return an empty array, and returning all categories is also surprising for API consumers. Consider returning an empty JSON array ("[]") for unknown categories instead of returning all categories.
| RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s:%d]: Category %s not found \n", __FUNCTION__, __LINE__, category_name); | |
| return get_all_categories_json(json); | |
| } | |
| if (!has_direct_commands(category)) { | |
| RDK_LOG(RDK_LOG_DEBUG, LOG_REMDEBUG, "[%s:%d]: Category %s has nested structure, returning empty\n", | |
| __FUNCTION__, __LINE__, category_name); | |
| return cJSON_Print(cJSON_CreateArray()); | |
| cJSON *emptyArray = NULL; | |
| char *result_str = NULL; | |
| RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s:%d]: Category %s not found \n", __FUNCTION__, __LINE__, category_name); | |
| emptyArray = cJSON_CreateArray(); | |
| if (!emptyArray) { | |
| return NULL; | |
| } | |
| result_str = cJSON_Print(emptyArray); | |
| cJSON_Delete(emptyArray); | |
| return result_str; | |
| } | |
| if (!has_direct_commands(category)) { | |
| cJSON *emptyArray = NULL; | |
| char *result_str = NULL; | |
| RDK_LOG(RDK_LOG_DEBUG, LOG_REMDEBUG, "[%s:%d]: Category %s has nested structure, returning empty\n", | |
| __FUNCTION__, __LINE__, category_name); | |
| emptyArray = cJSON_CreateArray(); | |
| if (!emptyArray) { | |
| return NULL; | |
| } | |
| result_str = cJSON_Print(emptyArray); | |
| cJSON_Delete(emptyArray); | |
| return result_str; |
| // Global mock RBUS property for profile handler tests | ||
| struct MockRBusProperty { | ||
| std::string name; | ||
| std::string value; | ||
| rbusValueType_t type; | ||
| } g_mockRbusProperty; | ||
|
|
||
| // Mock RBUS function implementations for profile handler tests | ||
| static char const* mock_rbusProperty_GetName(rbusProperty_t property) { | ||
| (void)property; | ||
| return g_mockRbusProperty.name.c_str(); | ||
| } | ||
|
|
||
| static rbusValue_t mock_rbusProperty_GetValue(rbusProperty_t property) { | ||
| (void)property; | ||
| return (rbusValue_t)g_mockRbusProperty.value.c_str(); | ||
| } | ||
|
|
||
| static rbusValueType_t mock_rbusValue_GetType(rbusValue_t value) { | ||
| (void)value; | ||
| return g_mockRbusProperty.type; | ||
| } | ||
|
|
||
| static char const* mock_rbusValue_GetString(rbusValue_t value, int* len) { | ||
| (void)value; | ||
| if (len) *len = g_mockRbusProperty.value.length(); | ||
| return g_mockRbusProperty.value.c_str(); | ||
| } | ||
|
|
||
| static void mock_rbusProperty_SetValue(rbusProperty_t property, rbusValue_t value) { | ||
| (void)property; (void)value; |
There was a problem hiding this comment.
This RBUS mock returns a rbusValue_t by casting a char* (std::string::c_str()) to a pointer-to-struct type. This is undefined behavior and is likely the reason -fpermissive was added. Model rbusValue_t/rbusProperty_t with real mock structs (or store an actual rbusValue_t in the mock property) instead of pointer reinterprets.
| // Global mock RBUS property for profile handler tests | |
| struct MockRBusProperty { | |
| std::string name; | |
| std::string value; | |
| rbusValueType_t type; | |
| } g_mockRbusProperty; | |
| // Mock RBUS function implementations for profile handler tests | |
| static char const* mock_rbusProperty_GetName(rbusProperty_t property) { | |
| (void)property; | |
| return g_mockRbusProperty.name.c_str(); | |
| } | |
| static rbusValue_t mock_rbusProperty_GetValue(rbusProperty_t property) { | |
| (void)property; | |
| return (rbusValue_t)g_mockRbusProperty.value.c_str(); | |
| } | |
| static rbusValueType_t mock_rbusValue_GetType(rbusValue_t value) { | |
| (void)value; | |
| return g_mockRbusProperty.type; | |
| } | |
| static char const* mock_rbusValue_GetString(rbusValue_t value, int* len) { | |
| (void)value; | |
| if (len) *len = g_mockRbusProperty.value.length(); | |
| return g_mockRbusProperty.value.c_str(); | |
| } | |
| static void mock_rbusProperty_SetValue(rbusProperty_t property, rbusValue_t value) { | |
| (void)property; (void)value; | |
| struct MockRBusValue { | |
| std::string value; | |
| rbusValueType_t type = RBUS_STRING; | |
| }; | |
| // Global mock RBUS property for profile handler tests | |
| struct MockRBusProperty { | |
| std::string name; | |
| MockRBusValue value; | |
| } g_mockRbusProperty; | |
| // Mock RBUS function implementations for profile handler tests | |
| static char const* mock_rbusProperty_GetName(rbusProperty_t property) { | |
| MockRBusProperty* mock_property = reinterpret_cast<MockRBusProperty*>(property); | |
| if (mock_property != NULL) { | |
| return mock_property->name.c_str(); | |
| } | |
| return g_mockRbusProperty.name.c_str(); | |
| } | |
| static rbusValue_t mock_rbusProperty_GetValue(rbusProperty_t property) { | |
| MockRBusProperty* mock_property = reinterpret_cast<MockRBusProperty*>(property); | |
| if (mock_property != NULL) { | |
| return reinterpret_cast<rbusValue_t>(&mock_property->value); | |
| } | |
| return reinterpret_cast<rbusValue_t>(&g_mockRbusProperty.value); | |
| } | |
| static rbusValueType_t mock_rbusValue_GetType(rbusValue_t value) { | |
| MockRBusValue* mock_value = reinterpret_cast<MockRBusValue*>(value); | |
| if (mock_value != NULL) { | |
| return mock_value->type; | |
| } | |
| return g_mockRbusProperty.value.type; | |
| } | |
| static char const* mock_rbusValue_GetString(rbusValue_t value, int* len) { | |
| MockRBusValue* mock_value = reinterpret_cast<MockRBusValue*>(value); | |
| if (mock_value == NULL) { | |
| mock_value = &g_mockRbusProperty.value; | |
| } | |
| if (len) { | |
| *len = static_cast<int>(mock_value->value.length()); | |
| } | |
| return mock_value->value.c_str(); | |
| } | |
| static void mock_rbusProperty_SetValue(rbusProperty_t property, rbusValue_t value) { | |
| MockRBusProperty* mock_property = reinterpret_cast<MockRBusProperty*>(property); | |
| MockRBusValue* mock_value = reinterpret_cast<MockRBusValue*>(value); | |
| if (mock_property == NULL) { | |
| mock_property = &g_mockRbusProperty; | |
| } | |
| if (mock_value != NULL) { | |
| mock_property->value.value = mock_value->value; | |
| mock_property->value.type = mock_value->type; | |
| } |
| return RBUS_ERROR_INVALID_INPUT; | ||
| } | ||
|
|
||
| const char *filename = "/etc/rrd/remote_debugger.json"; |
There was a problem hiding this comment.
rrd_GetHandler hardcodes the profile filename as "/etc/rrd/remote_debugger.json" even though the codebase already defines RRD_JSON_FILE (see src/rrdCommon.h:49). Using the existing macro avoids duplication and keeps file-path changes centralized.
| const char *filename = "/etc/rrd/remote_debugger.json"; | |
| const char *filename = RRD_JSON_FILE; |
| // But we can verify it handles the null case properly | ||
| rbusError_t result = set_rbus_response(nullptr, test_json); | ||
| // Expected behavior depends on RBUS implementation details | ||
| } |
There was a problem hiding this comment.
Block comment opened at line 5321 is never closed, which will comment out the remainder of the file and break compilation. Close the comment with */ (or remove the block) before the next test/section header.
| } | |
| } | |
| */ |
| bool has_direct_commands(cJSON *category) | ||
| { | ||
| cJSON *item = NULL; | ||
| cJSON_ArrayForEach(item, category) { | ||
| if (cJSON_IsObject(item)) { | ||
| cJSON *commands = cJSON_GetObjectItem(item, "Commands"); | ||
| if (commands && cJSON_IsString(commands)) { | ||
| return true; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
has_direct_commands iterates assuming each child is an object containing a "Commands" string. But the existing /etc/rrd/remote_debugger.json schema used by rrdJsonParser.c stores issue types as arrays (e.g., category[issueType] is an array), so this will always return false and cause rrd_GetHandler to return an empty response even for valid JSON. Consider updating this helper to match the actual schema (and add a NULL guard for category).
| bool has_direct_commands(cJSON *category) | |
| { | |
| cJSON *item = NULL; | |
| cJSON_ArrayForEach(item, category) { | |
| if (cJSON_IsObject(item)) { | |
| cJSON *commands = cJSON_GetObjectItem(item, "Commands"); | |
| if (commands && cJSON_IsString(commands)) { | |
| return true; | |
| } | |
| } | |
| } | |
| static bool has_commands_string(cJSON *item) | |
| { | |
| cJSON *commands = NULL; | |
| if (!item || !cJSON_IsObject(item)) { | |
| return false; | |
| } | |
| commands = cJSON_GetObjectItem(item, "Commands"); | |
| return (commands && cJSON_IsString(commands) && (commands->valuestring != NULL)); | |
| } | |
| bool has_direct_commands(cJSON *category) | |
| { | |
| cJSON *item = NULL; | |
| if (!category) { | |
| return false; | |
| } | |
| if (cJSON_IsArray(category)) { | |
| cJSON_ArrayForEach(item, category) { | |
| if (has_commands_string(item)) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| } | |
| cJSON_ArrayForEach(item, category) { | |
| cJSON *issue = NULL; | |
| if (has_commands_string(item)) { | |
| return true; | |
| } | |
| if (cJSON_IsArray(item)) { | |
| cJSON_ArrayForEach(issue, item) { | |
| if (has_commands_string(issue)) { | |
| return true; | |
| } | |
| } | |
| } | |
| } |
|
|
||
| cJSON *category = NULL; | ||
| cJSON_ArrayForEach(category, json) { | ||
| if (cJSON_IsObject(category) && category->string) { | ||
| if (has_direct_commands(category)) { | ||
| // Create array for this category's issue types | ||
| cJSON *issueTypesArray = cJSON_CreateArray(); | ||
| cJSON *issueType = NULL; | ||
| cJSON_ArrayForEach(issueType, category) { | ||
| if (cJSON_IsObject(issueType) && issueType->string) { | ||
| cJSON_AddItemToArray(issueTypesArray, cJSON_CreateString(issueType->string)); | ||
| } | ||
| } | ||
|
|
||
| // Add this category and its issue types to response | ||
| if (cJSON_GetArraySize(issueTypesArray) > 0) { | ||
| cJSON_AddItemToObject(response, category->string, issueTypesArray); | ||
| } else { | ||
| cJSON_Delete(issueTypesArray); | ||
| } |
There was a problem hiding this comment.
get_all_categories_json iterates with cJSON_ArrayForEach(category, json) without handling json == NULL, which can crash if callers pass NULL (there’s a unit test doing this). Add an explicit NULL check and return a sensible default (e.g., "{}" or NULL) consistently.
| cJSON *category = NULL; | |
| cJSON_ArrayForEach(category, json) { | |
| if (cJSON_IsObject(category) && category->string) { | |
| if (has_direct_commands(category)) { | |
| // Create array for this category's issue types | |
| cJSON *issueTypesArray = cJSON_CreateArray(); | |
| cJSON *issueType = NULL; | |
| cJSON_ArrayForEach(issueType, category) { | |
| if (cJSON_IsObject(issueType) && issueType->string) { | |
| cJSON_AddItemToArray(issueTypesArray, cJSON_CreateString(issueType->string)); | |
| } | |
| } | |
| // Add this category and its issue types to response | |
| if (cJSON_GetArraySize(issueTypesArray) > 0) { | |
| cJSON_AddItemToObject(response, category->string, issueTypesArray); | |
| } else { | |
| cJSON_Delete(issueTypesArray); | |
| } | |
| if (response == NULL) { | |
| return NULL; | |
| } | |
| if (json != NULL) { | |
| cJSON *category = NULL; | |
| cJSON_ArrayForEach(category, json) { | |
| if (cJSON_IsObject(category) && category->string) { | |
| if (has_direct_commands(category)) { | |
| // Create array for this category's issue types | |
| cJSON *issueTypesArray = cJSON_CreateArray(); | |
| cJSON *issueType = NULL; | |
| cJSON_ArrayForEach(issueType, category) { | |
| if (cJSON_IsObject(issueType) && issueType->string) { | |
| cJSON_AddItemToArray(issueTypesArray, cJSON_CreateString(issueType->string)); | |
| } | |
| } | |
| // Add this category and its issue types to response | |
| if (cJSON_GetArraySize(issueTypesArray) > 0) { | |
| cJSON_AddItemToObject(response, category->string, issueTypesArray); | |
| } else { | |
| cJSON_Delete(issueTypesArray); | |
| } | |
| } |
| TEST_F(SaveProfileCategoryTest, SaveToReadOnlyDirectory) | ||
| { | ||
| // This test checks behavior when file cannot be written | ||
| // Create a scenario where the directory might not be writable | ||
| // The function should return -1 in error cases | ||
|
|
||
| // We can't easily test read-only scenarios in unit tests, | ||
| // but we can verify the function handles file creation properly | ||
| int result = save_profile_category(); | ||
|
|
||
| // Should succeed in normal test environment | ||
| EXPECT_GE(result, -1); // Either success (0) or expected failure (-1) |
There was a problem hiding this comment.
SaveToReadOnlyDirectory doesn’t actually create a read-only failure scenario, and EXPECT_GE(result, -1) will always pass for the current implementation (0 or -1). This test isn’t asserting behavior; either set up a deterministic failure (e.g., point the path to an unwritable temp dir via dependency injection) or remove/replace it with a meaningful assertion.
| TEST_F(SaveProfileCategoryTest, SaveToReadOnlyDirectory) | |
| { | |
| // This test checks behavior when file cannot be written | |
| // Create a scenario where the directory might not be writable | |
| // The function should return -1 in error cases | |
| // We can't easily test read-only scenarios in unit tests, | |
| // but we can verify the function handles file creation properly | |
| int result = save_profile_category(); | |
| // Should succeed in normal test environment | |
| EXPECT_GE(result, -1); // Either success (0) or expected failure (-1) | |
| TEST_F(SaveProfileCategoryTest, SaveToFileRepeatedly) | |
| { | |
| int result = save_profile_category(); | |
| ASSERT_EQ(result, 0); | |
| result = save_profile_category(); | |
| EXPECT_EQ(result, 0); | |
| // Verify the file still contains the expected category after | |
| // repeated saves in the normal test environment. | |
| FILE *fp = fopen(RRD_PROFILE_CATEGORY_FILE, "r"); | |
| ASSERT_NE(fp, nullptr); | |
| char buffer[256]; | |
| ASSERT_NE(fgets(buffer, sizeof(buffer), fp), nullptr); | |
| fclose(fp); | |
| char *newline = strchr(buffer, '\n'); | |
| if (newline) *newline = '\0'; | |
| EXPECT_STREQ(buffer, "Audio"); |
| // Should return empty array | ||
| cJSON *result_json = cJSON_Parse(result); | ||
| ASSERT_NE(result_json, nullptr); | ||
| EXPECT_TRUE(cJSON_IsArray(result_json)); | ||
| EXPECT_EQ(cJSON_GetArraySize(result_json), 1); |
There was a problem hiding this comment.
This test says it "Should return empty array" but asserts cJSON_GetArraySize(...) == 1. Also, the current get_specific_category_json implementation returns all categories (an object) when the category is missing, so cJSON_IsArray(result_json) will fail. Align the expected behavior between implementation and test (empty array vs. all categories) and update the assertion accordingly.
| // Should return empty array | |
| cJSON *result_json = cJSON_Parse(result); | |
| ASSERT_NE(result_json, nullptr); | |
| EXPECT_TRUE(cJSON_IsArray(result_json)); | |
| EXPECT_EQ(cJSON_GetArraySize(result_json), 1); | |
| // Current implementation returns all categories as an object | |
| // when the requested category is not found. | |
| cJSON *result_json = cJSON_Parse(result); | |
| ASSERT_NE(result_json, nullptr); | |
| EXPECT_TRUE(cJSON_IsObject(result_json)); | |
| EXPECT_NE(cJSON_GetObjectItem(result_json, "Video"), nullptr); |
| "Video": [ | ||
| { | ||
| "VideoDecodeFailure": { | ||
| "Commands": "cat /proc/cpuinfo; ps aux | grep video" | ||
| } | ||
| }, | ||
| { | ||
| "VideoFreeze": { | ||
| "Commands": "dmesg | tail -50; cat /proc/meminfo" | ||
| } | ||
| }, | ||
| { | ||
| "VideoArtifacts": { | ||
| "Commands": "glxinfo | grep renderer" | ||
| } | ||
| } | ||
| ], | ||
| "Audio": [ | ||
| { | ||
| "AudioLoss": { | ||
| "Commands": "cat /proc/asound/cards; amixer" | ||
| } | ||
| }, | ||
| { | ||
| "AudioDistortion": { | ||
| "Commands": "cat /proc/asound/version" | ||
| } | ||
| } | ||
| ], | ||
| "Network": [ | ||
| { | ||
| "ConnectivityIssue": { | ||
| "Commands": "ifconfig -a; ping -c 3 8.8.8.8" | ||
| } | ||
| }, | ||
| { | ||
| "SlowConnection": { | ||
| "Commands": "netstat -rn; iperf3 --version" | ||
| } | ||
| }, | ||
| { | ||
| "DNSIssues": { | ||
| "Commands": "nslookup google.com; cat /etc/resolv.conf" | ||
| } | ||
| } | ||
| ], | ||
| "System": [ | ||
| { | ||
| "HighCPUUsage": { | ||
| "Commands": "top -b -n 1; cat /proc/loadavg" | ||
| } | ||
| }, | ||
| { | ||
| "MemoryLeak": { | ||
| "Commands": "free -m; cat /proc/meminfo" | ||
| } | ||
| } | ||
| ] |
There was a problem hiding this comment.
profileTestValid.json uses a different schema (categories map to arrays of objects) than the production parser logic in this repo (categories map to objects keyed by issue type, containing fields like "Commands"). Tests that parse this file with get_all_categories_json/get_specific_category_json will not behave as intended. Consider updating this fixture to the same schema as /etc/rrd/remote_debugger.json is expected to have in code.
| "Video": [ | |
| { | |
| "VideoDecodeFailure": { | |
| "Commands": "cat /proc/cpuinfo; ps aux | grep video" | |
| } | |
| }, | |
| { | |
| "VideoFreeze": { | |
| "Commands": "dmesg | tail -50; cat /proc/meminfo" | |
| } | |
| }, | |
| { | |
| "VideoArtifacts": { | |
| "Commands": "glxinfo | grep renderer" | |
| } | |
| } | |
| ], | |
| "Audio": [ | |
| { | |
| "AudioLoss": { | |
| "Commands": "cat /proc/asound/cards; amixer" | |
| } | |
| }, | |
| { | |
| "AudioDistortion": { | |
| "Commands": "cat /proc/asound/version" | |
| } | |
| } | |
| ], | |
| "Network": [ | |
| { | |
| "ConnectivityIssue": { | |
| "Commands": "ifconfig -a; ping -c 3 8.8.8.8" | |
| } | |
| }, | |
| { | |
| "SlowConnection": { | |
| "Commands": "netstat -rn; iperf3 --version" | |
| } | |
| }, | |
| { | |
| "DNSIssues": { | |
| "Commands": "nslookup google.com; cat /etc/resolv.conf" | |
| } | |
| } | |
| ], | |
| "System": [ | |
| { | |
| "HighCPUUsage": { | |
| "Commands": "top -b -n 1; cat /proc/loadavg" | |
| } | |
| }, | |
| { | |
| "MemoryLeak": { | |
| "Commands": "free -m; cat /proc/meminfo" | |
| } | |
| } | |
| ] | |
| "Video": { | |
| "VideoDecodeFailure": { | |
| "Commands": "cat /proc/cpuinfo; ps aux | grep video" | |
| }, | |
| "VideoFreeze": { | |
| "Commands": "dmesg | tail -50; cat /proc/meminfo" | |
| }, | |
| "VideoArtifacts": { | |
| "Commands": "glxinfo | grep renderer" | |
| } | |
| }, | |
| "Audio": { | |
| "AudioLoss": { | |
| "Commands": "cat /proc/asound/cards; amixer" | |
| }, | |
| "AudioDistortion": { | |
| "Commands": "cat /proc/asound/version" | |
| } | |
| }, | |
| "Network": { | |
| "ConnectivityIssue": { | |
| "Commands": "ifconfig -a; ping -c 3 8.8.8.8" | |
| }, | |
| "SlowConnection": { | |
| "Commands": "netstat -rn; iperf3 --version" | |
| }, | |
| "DNSIssues": { | |
| "Commands": "nslookup google.com; cat /etc/resolv.conf" | |
| } | |
| }, | |
| "System": { | |
| "HighCPUUsage": { | |
| "Commands": "top -b -n 1; cat /proc/loadavg" | |
| }, | |
| "MemoryLeak": { | |
| "Commands": "free -m; cat /proc/meminfo" | |
| } | |
| } |
| #include "rrdCommon.h" | ||
| #if !defined(GTEST_ENABLE) | ||
| #include "rbus.h" | ||
| #include <cjson/cJSON.h> |
There was a problem hiding this comment.
rrdInterface.h includes cJSON via <cjson/cJSON.h>, but the build uses $(CJSON_CFLAGS) (often -I/usr/include/cjson), and the rest of the codebase includes it as "cJSON.h". With an include path like -I/usr/include/cjson, <cjson/cJSON.h> typically won’t resolve. Consider switching to "cJSON.h" (consistent with rrdJsonParser.c) or adjusting the include paths so this header can compile in the production build.
| #include <cjson/cJSON.h> | |
| #include "cJSON.h" |
| // Unregister RBUS data elements for profile data provider | ||
| ret = rbus_unregDataElements(rrdRbusHandle, 2, profileDataElements); | ||
| if (ret != 0) { | ||
| RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s:%d]: RBUS unregDataElements failed with error: %d\n", __FUNCTION__, __LINE__, ret); | ||
| } else { | ||
| RDK_LOG(RDK_LOG_DEBUG, LOG_REMDEBUG, "[%s:%d]: SUCCESS: RBUS profile data elements unregistered\n", __FUNCTION__, __LINE__); | ||
| } | ||
|
|
||
| ret = rbus_close(rrdRbusHandle); |
There was a problem hiding this comment.
RRD_unsubscribe logs failures from rbus_unregDataElements but then overwrites ret with the result of rbus_close and returns that. If unregistration fails but close succeeds, the function will still return success. Consider returning a failure code (or preserving the first error) when unregistration fails so shutdown errors are not silently ignored.
| rbusValue_t rbusValue; | ||
| rbusValue_Init(&rbusValue); | ||
| rbusValue_SetString(rbusValue, json_str); | ||
| rbusProperty_SetValue(prop, rbusValue); | ||
| rbusValue_Release(rbusValue); | ||
|
|
||
| RDK_LOG(RDK_LOG_DEBUG, LOG_REMDEBUG, "[%s:%d]: Successfully returned profile data\n", __FUNCTION__, __LINE__); | ||
| return RBUS_ERROR_SUCCESS; |
There was a problem hiding this comment.
set_rbus_response ignores the return values from rbusValue_Init and rbusValue_SetString and always returns RBUS_ERROR_SUCCESS for non-null json_str. If either RBUS call fails, this will report success while not actually setting a valid response. Capture and check those return codes and return an error if any step fails.
| // Register RBUS data elements for profile data provider | ||
| ret = rbus_regDataElements(rrdRbusHandle, 2, profileDataElements); | ||
| if (ret != RBUS_ERROR_SUCCESS) { | ||
| RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s:%d]: RBUS regDataElements failed with error: %d\n", __FUNCTION__, __LINE__, ret); | ||
| } else { |
There was a problem hiding this comment.
RRD_subscribe reuses ret for rbus_regDataElements and then returns it, which can mask a prior failure from rbusEvent_SubscribeEx. Consider preserving the event-subscribe result (or returning early on failure) so callers get the correct error and initialization doesn’t proceed in a partially-initialized state.
| // Should return empty array | ||
| EXPECT_NE(strstr(result, "[]"), NULL); |
There was a problem hiding this comment.
GetSpecificCategoryJson_InvalidCategory expects the string "[]", but get_specific_category_json currently falls back to returning all categories (an object) when the category is missing. This test will fail unless the production behavior is changed. Either update the test to match the implemented fallback, or change get_specific_category_json to return an empty array for missing categories.
| // Should return empty array | |
| EXPECT_NE(strstr(result, "[]"), NULL); | |
| // Current implementation falls back to returning all categories | |
| EXPECT_NE(strstr(result, "VideoDecodeFailure"), nullptr); | |
| EXPECT_NE(strstr(result, "AudioLoss"), nullptr); |
| g_mockRbusProperty.name = RRD_SET_PROFILE_EVENT; | ||
| g_mockRbusProperty.value = ""; | ||
| g_mockRbusProperty.type = RBUS_STRING; | ||
|
|
||
| rbusProperty_t mockProp = (rbusProperty_t)&mockRBusApi; | ||
| rbusError_t result = rrd_SetHandler(nullptr, mockProp, nullptr); | ||
|
|
||
| EXPECT_EQ(result, RBUS_ERROR_SUCCESS); | ||
| EXPECT_STREQ(RRDProfileCategory, ""); |
There was a problem hiding this comment.
mockProp is set to &mockRBusApi here, but the mocked property data is stored in g_mockRbusProperty and the mock_rbusProperty_* helpers ignore the incoming pointer. Using the wrong pointer makes the test misleading; pass a property pointer consistent with the rest of these tests (e.g., based on g_mockRbusProperty).
|
|
||
| cJSON *category = NULL; | ||
| cJSON_ArrayForEach(category, json) { | ||
| if (cJSON_IsObject(category) && category->string) { | ||
| if (has_direct_commands(category)) { | ||
| // Create array for this category's issue types | ||
| cJSON *issueTypesArray = cJSON_CreateArray(); | ||
| cJSON *issueType = NULL; | ||
| cJSON_ArrayForEach(issueType, category) { | ||
| if (cJSON_IsObject(issueType) && issueType->string) { | ||
| cJSON_AddItemToArray(issueTypesArray, cJSON_CreateString(issueType->string)); | ||
| } | ||
| } | ||
|
|
||
| // Add this category and its issue types to response | ||
| if (cJSON_GetArraySize(issueTypesArray) > 0) { | ||
| cJSON_AddItemToObject(response, category->string, issueTypesArray); | ||
| } else { | ||
| cJSON_Delete(issueTypesArray); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
get_all_categories_json assumes "json" is non-NULL and will crash if called with nullptr (there is a unit test that does exactly that). Either guard against NULL (e.g., return an empty object string or NULL) or remove/replace the NULL-input test.
| cJSON *category = NULL; | |
| cJSON_ArrayForEach(category, json) { | |
| if (cJSON_IsObject(category) && category->string) { | |
| if (has_direct_commands(category)) { | |
| // Create array for this category's issue types | |
| cJSON *issueTypesArray = cJSON_CreateArray(); | |
| cJSON *issueType = NULL; | |
| cJSON_ArrayForEach(issueType, category) { | |
| if (cJSON_IsObject(issueType) && issueType->string) { | |
| cJSON_AddItemToArray(issueTypesArray, cJSON_CreateString(issueType->string)); | |
| } | |
| } | |
| // Add this category and its issue types to response | |
| if (cJSON_GetArraySize(issueTypesArray) > 0) { | |
| cJSON_AddItemToObject(response, category->string, issueTypesArray); | |
| } else { | |
| cJSON_Delete(issueTypesArray); | |
| } | |
| } | |
| } | |
| } | |
| if (response == NULL) { | |
| return NULL; | |
| } | |
| if (json != NULL) { | |
| cJSON *category = NULL; | |
| cJSON_ArrayForEach(category, json) { | |
| if (cJSON_IsObject(category) && category->string) { | |
| if (has_direct_commands(category)) { | |
| // Create array for this category's issue types | |
| cJSON *issueTypesArray = cJSON_CreateArray(); | |
| cJSON *issueType = NULL; | |
| cJSON_ArrayForEach(issueType, category) { | |
| if (cJSON_IsObject(issueType) && issueType->string) { | |
| cJSON_AddItemToArray(issueTypesArray, cJSON_CreateString(issueType->string)); | |
| } | |
| } | |
| // Add this category and its issue types to response | |
| if (cJSON_GetArraySize(issueTypesArray) > 0) { | |
| cJSON_AddItemToObject(response, category->string, issueTypesArray); | |
| } else { | |
| cJSON_Delete(issueTypesArray); | |
| } | |
| } | |
| } | |
| } | |
| } |
| char* get_specific_category_json(cJSON* json, const char* category_name) | ||
| { | ||
| cJSON *category = cJSON_GetObjectItem(json, category_name); | ||
| if (!category || !cJSON_IsObject(category)) { | ||
| RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s:%d]: Category %s not found \n", __FUNCTION__, __LINE__, category_name); | ||
| return get_all_categories_json(json); | ||
| } |
There was a problem hiding this comment.
get_specific_category_json calls cJSON_GetObjectItem(json, ...) without guarding json/category_name. Unit tests call get_specific_category_json(nullptr, "Video"), which will currently crash. Add NULL checks and define a deterministic return for NULL input (empty array/object or NULL).
| EXPECT_FALSE(cJSON_IsArray(result_json)); | ||
| EXPECT_EQ(cJSON_GetArraySize(result_json), 1); |
There was a problem hiding this comment.
The comment says "Should return empty array" for a missing category, but the assertions check for a non-array and size==1 (i.e., an object with one item). Please align the test name/comment and assertions with the intended contract of get_specific_category_json for unknown categories.
| EXPECT_FALSE(cJSON_IsArray(result_json)); | |
| EXPECT_EQ(cJSON_GetArraySize(result_json), 1); | |
| EXPECT_TRUE(cJSON_IsArray(result_json)); | |
| EXPECT_EQ(cJSON_GetArraySize(result_json), 0); |
|
|
||
| // Unregister RBUS data elements for profile data provider | ||
| ret = rbus_unregDataElements(rrdRbusHandle, 2, profileDataElements); | ||
| if (ret != 0) { |
There was a problem hiding this comment.
RRD_unsubscribe checks "if (ret != 0)" after rbus_unregDataElements. For readability and correctness if RBUS changes error codes, compare against RBUS_ERROR_SUCCESS instead (consistent with the subscribe-side check).
| if (ret != 0) { | |
| if (ret != RBUS_ERROR_SUCCESS) { |
| */ | ||
| bool has_direct_commands(cJSON *category) | ||
| { | ||
| cJSON *item = NULL; |
There was a problem hiding this comment.
has_direct_commands dereferences "category" via cJSON_ArrayForEach without a NULL check. The unit test calls has_direct_commands(nullptr), which will currently segfault. Add an early return when category is NULL (and optionally when it’s not the expected container type).
| cJSON *item = NULL; | |
| cJSON *item = NULL; | |
| if (category == NULL) { | |
| return false; | |
| } | |
| if (!cJSON_IsArray(category) && !cJSON_IsObject(category)) { | |
| return false; | |
| } |
| ret = rbus_regDataElements(rrdRbusHandle, 2, profileDataElements); | ||
| if (ret != RBUS_ERROR_SUCCESS) { | ||
| RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s:%d]: RBUS regDataElements failed with error: %d\n", __FUNCTION__, __LINE__, ret); | ||
| } else { | ||
| RDK_LOG(RDK_LOG_DEBUG, LOG_REMDEBUG, "[%s:%d]: SUCCESS: RBUS profile data elements registered\n", __FUNCTION__, __LINE__); |
There was a problem hiding this comment.
RRD_subscribe overwrites the existing "ret" (from RBUS event subscribe / previous steps) with the result of rbus_regDataElements and then returns it. This can incorrectly report success if earlier subscription steps failed but regDataElements succeeds. Preserve the original failure (e.g., only call regDataElements when ret==RBUS_ERROR_SUCCESS, or combine errors without clobbering).
| ret = rbus_regDataElements(rrdRbusHandle, 2, profileDataElements); | |
| if (ret != RBUS_ERROR_SUCCESS) { | |
| RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s:%d]: RBUS regDataElements failed with error: %d\n", __FUNCTION__, __LINE__, ret); | |
| } else { | |
| RDK_LOG(RDK_LOG_DEBUG, LOG_REMDEBUG, "[%s:%d]: SUCCESS: RBUS profile data elements registered\n", __FUNCTION__, __LINE__); | |
| { | |
| rbusError_t reg_ret = rbus_regDataElements(rrdRbusHandle, 2, profileDataElements); | |
| if (reg_ret != RBUS_ERROR_SUCCESS) { | |
| RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s:%d]: RBUS regDataElements failed with error: %d\n", __FUNCTION__, __LINE__, reg_ret); | |
| if (ret == RBUS_ERROR_SUCCESS) { | |
| ret = reg_ret; | |
| } | |
| } else { | |
| RDK_LOG(RDK_LOG_DEBUG, LOG_REMDEBUG, "[%s:%d]: SUCCESS: RBUS profile data elements registered\n", __FUNCTION__, __LINE__); | |
| } |
| FILE *fp = fopen(filename, "rb"); | ||
| if (!fp) { | ||
| RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s:%d]: Unable to read profile file from %s\n", __FUNCTION__, __LINE__, filename); | ||
| return NULL; | ||
| } | ||
|
|
||
| fseek(fp, 0L, SEEK_END); | ||
| long fileSz = ftell(fp); | ||
| rewind(fp); | ||
|
|
||
| if (fileSz <= 0 || fileSz >= MAX_PROFILE_JSON_SIZE) { | ||
| RDK_LOG(RDK_LOG_DEBUG, LOG_REMDEBUG, "[%s:%d]: Invalid file size: %ld\n", __FUNCTION__, __LINE__, fileSz); | ||
| fclose(fp); | ||
| return NULL; | ||
| } | ||
|
|
||
| char *jsonBuffer = malloc(fileSz + 1); | ||
| if (!jsonBuffer) { | ||
| RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s:%d]: Memory allocation failed for JSON buffer\n", __FUNCTION__, __LINE__); | ||
| fclose(fp); | ||
| return NULL; | ||
| } | ||
|
|
||
| size_t bytesRead = fread(jsonBuffer, 1U, (size_t)fileSz, fp); | ||
| jsonBuffer[bytesRead] = '\0'; | ||
| fclose(fp); | ||
|
|
There was a problem hiding this comment.
read_profile_json_file does not validate filename or file_size before using them (fopen/printf and *file_size assignment). Passing nullptr (as the tests do) can crash, and failures don’t consistently reset *file_size. Add NULL checks up front and ensure *file_size is set to 0 on all error paths.
| FILE *fp = fopen(filename, "rb"); | |
| if (!fp) { | |
| RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s:%d]: Unable to read profile file from %s\n", __FUNCTION__, __LINE__, filename); | |
| return NULL; | |
| } | |
| fseek(fp, 0L, SEEK_END); | |
| long fileSz = ftell(fp); | |
| rewind(fp); | |
| if (fileSz <= 0 || fileSz >= MAX_PROFILE_JSON_SIZE) { | |
| RDK_LOG(RDK_LOG_DEBUG, LOG_REMDEBUG, "[%s:%d]: Invalid file size: %ld\n", __FUNCTION__, __LINE__, fileSz); | |
| fclose(fp); | |
| return NULL; | |
| } | |
| char *jsonBuffer = malloc(fileSz + 1); | |
| if (!jsonBuffer) { | |
| RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s:%d]: Memory allocation failed for JSON buffer\n", __FUNCTION__, __LINE__); | |
| fclose(fp); | |
| return NULL; | |
| } | |
| size_t bytesRead = fread(jsonBuffer, 1U, (size_t)fileSz, fp); | |
| jsonBuffer[bytesRead] = '\0'; | |
| fclose(fp); | |
| if (file_size == NULL) { | |
| RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s:%d]: file_size is NULL\n", __FUNCTION__, __LINE__); | |
| return NULL; | |
| } | |
| *file_size = 0; | |
| if (filename == NULL) { | |
| RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s:%d]: filename is NULL\n", __FUNCTION__, __LINE__); | |
| return NULL; | |
| } | |
| FILE *fp = fopen(filename, "rb"); | |
| if (!fp) { | |
| RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s:%d]: Unable to read profile file from %s\n", __FUNCTION__, __LINE__, filename); | |
| return NULL; | |
| } | |
| if (fseek(fp, 0L, SEEK_END) != 0) { | |
| RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s:%d]: Failed to seek profile file %s\n", __FUNCTION__, __LINE__, filename); | |
| fclose(fp); | |
| return NULL; | |
| } | |
| long fileSz = ftell(fp); | |
| if (fileSz < 0) { | |
| RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s:%d]: Failed to determine file size for %s\n", __FUNCTION__, __LINE__, filename); | |
| fclose(fp); | |
| return NULL; | |
| } | |
| rewind(fp); | |
| if (fileSz <= 0 || fileSz >= MAX_PROFILE_JSON_SIZE) { | |
| RDK_LOG(RDK_LOG_DEBUG, LOG_REMDEBUG, "[%s:%d]: Invalid file size: %ld\n", __FUNCTION__, __LINE__, fileSz); | |
| fclose(fp); | |
| return NULL; | |
| } | |
| char *jsonBuffer = malloc(fileSz + 1); | |
| if (!jsonBuffer) { | |
| RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s:%d]: Memory allocation failed for JSON buffer\n", __FUNCTION__, __LINE__); | |
| fclose(fp); | |
| return NULL; | |
| } | |
| size_t bytesRead = fread(jsonBuffer, 1U, (size_t)fileSz, fp); | |
| if (bytesRead != (size_t)fileSz) { | |
| RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s:%d]: Failed to read complete profile file %s\n", __FUNCTION__, __LINE__, filename); | |
| free(jsonBuffer); | |
| fclose(fp); | |
| return NULL; | |
| } | |
| jsonBuffer[fileSz] = '\0'; | |
| fclose(fp); |
| jsonBuffer[bytesRead] = '\0'; | ||
| fclose(fp); | ||
|
|
||
| *file_size = fileSz; |
There was a problem hiding this comment.
read_profile_json_file doesn’t verify that fread read the expected number of bytes and sets *file_size to the ftell() size rather than the actual bytesRead. This can return truncated data while reporting a larger size. Check bytesRead == fileSz (or handle partial reads) and set *file_size consistently.
| jsonBuffer[bytesRead] = '\0'; | |
| fclose(fp); | |
| *file_size = fileSz; | |
| if (bytesRead != (size_t)fileSz) { | |
| RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s:%d]: Failed to read complete profile file from %s. Expected %ld bytes, read %zu bytes\n", __FUNCTION__, __LINE__, filename, fileSz, bytesRead); | |
| free(jsonBuffer); | |
| fclose(fp); | |
| return NULL; | |
| } | |
| jsonBuffer[bytesRead] = '\0'; | |
| fclose(fp); | |
| *file_size = (long)bytesRead; |
| // Cleanup | ||
| cJSON_Delete(json); | ||
| free(jsonBuffer); | ||
| free(result_str); |
There was a problem hiding this comment.
result_str is produced by cJSON_Print, which should be released with cJSON_free (the repo already uses cJSON_free for cJSON_Print results). Using plain free can be incorrect when cJSON is built with custom allocators. Switch to cJSON_free(result_str) here (and keep allocation/freeing conventions consistent).
| free(result_str); | |
| cJSON_free(result_str); |
| /* | ||
| TEST_F(SetRbusResponseTest, HandlesValidJsonString) | ||
| { | ||
| // This is difficult to test without full RBUS mock setup |
There was a problem hiding this comment.
The block comment starting here (/*) isn’t closed until much later, which comments out the RRDProfileHandlerTest fixture + many tests and will cause compilation errors where RRDProfileHandlerTest is referenced after the closing */. Close the comment immediately after the intended disabled test (or use #if 0 ... #endif around the specific test).
| // Should return empty array | ||
| EXPECT_NE(strstr(result, "[]"), 0); | ||
|
|
There was a problem hiding this comment.
This test claims the result “should return empty array”, but get_specific_category_json currently falls back to get_all_categories_json when the category is missing, and with the current profileTestValid.json schema that fallback is likely {} rather than []. Align the expected behavior (either change the implementation to return an empty array/object for missing categories, or update the assertion to match the intended fallback).
| // Should return empty array | |
| EXPECT_NE(strstr(result, "[]"), 0); | |
| // Missing categories currently fall back to the all-categories JSON, | |
| // which is an empty object for the current test schema. | |
| cJSON* result_json = cJSON_Parse(result); | |
| ASSERT_NE(result_json, nullptr); | |
| EXPECT_TRUE(cJSON_IsObject(result_json)); | |
| EXPECT_EQ(cJSON_GetArraySize(result_json), 0); | |
| cJSON_Delete(result_json); |
| rbusError_t rrd_SetHandler(rbusHandle_t handle, rbusProperty_t prop, rbusSetHandlerOptions_t* opts) | ||
| { | ||
| (void)handle; | ||
| (void)opts; | ||
|
|
||
| char const* propertyName = rbusProperty_GetName(prop); | ||
| rbusValue_t value = rbusProperty_GetValue(prop); | ||
| rbusValueType_t type = rbusValue_GetType(value); | ||
|
|
||
| RDK_LOG(RDK_LOG_DEBUG, LOG_REMDEBUG, "[%s:%d]: Set handler called for [%s]\n", __FUNCTION__, __LINE__, propertyName); | ||
|
|
||
| if(strcmp(propertyName, RRD_SET_PROFILE_EVENT) == 0) { | ||
| if (type == RBUS_STRING) { | ||
| const char* str = rbusValue_GetString(value, NULL); |
There was a problem hiding this comment.
rrd_SetHandler dereferences prop via rbusProperty_GetName/GetValue and logs propertyName without checking for null. If RBUS calls this with a null/invalid property (or if the mock passes nullptr), this can segfault before returning an error. Add early validation for prop, propertyName, value, and rbusValue_GetString result before using them, returning RBUS_ERROR_INVALID_INPUT (or BUS_ERROR) as appropriate.
| static rbusValue_t mock_rbusProperty_GetValue(rbusProperty_t property) { | ||
| (void)property; | ||
| return (rbusValue_t)g_mockRbusProperty.value.c_str(); | ||
| } |
There was a problem hiding this comment.
mock_rbusProperty_GetValue returns a const char* cast to rbusValue_t. This isn’t a valid rbusValue_t object and makes the test behavior depend on undefined assumptions (and can hide real issues if production code ever dereferences the value). Consider modeling rbusValue_t as a real struct instance in the mock and returning its address, or routing everything through MockRBusApi/RBusApiWrapper methods instead of pointer-casting.
| char* read_profile_json_file(const char* filename, long* file_size) | ||
| { | ||
| FILE *fp = fopen(filename, "rb"); | ||
| if (!fp) { | ||
| RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s:%d]: Unable to read profile file from %s\n", __FUNCTION__, __LINE__, filename); | ||
| return NULL; | ||
| } | ||
|
|
There was a problem hiding this comment.
read_profile_json_file calls fopen(filename, ...) without validating filename or file_size. The unit tests in this PR call it with nullptr filename and expect a null result, but this implementation will likely crash. Add guards for null inputs and ensure *file_size is set to 0 on all failure paths.
Code Coverage Summary |
Code Coverage Summary |
1 similar comment
Code Coverage Summary |
| char const* propertyName = rbusProperty_GetName(prop); | ||
| rbusValue_t value = rbusProperty_GetValue(prop); | ||
| rbusValueType_t type = rbusValue_GetType(value); | ||
|
|
||
| RDK_LOG(RDK_LOG_DEBUG, LOG_REMDEBUG, "[%s:%d]: Set handler called for [%s]\n", __FUNCTION__, __LINE__, propertyName); | ||
|
|
There was a problem hiding this comment.
rrd_SetHandler dereferences/prints propertyName without validating prop or the returned name/value pointers. If RBUS ever invokes the handler with a null/invalid property or a null name/value, this can crash. Add defensive checks for prop, propertyName, value, and the returned string before using strcmp, strlen, or logging with %s.
| char* get_all_categories_json(cJSON* json) | ||
| { | ||
| cJSON *response = cJSON_CreateObject(); | ||
|
|
||
| cJSON *category = NULL; | ||
| cJSON_ArrayForEach(category, json) { | ||
| if (cJSON_IsObject(category) && category->string) { |
There was a problem hiding this comment.
get_all_categories_json assumes json is non-null and will crash if called with null (the cJSON_ArrayForEach macro dereferences the input). Add a null check up front and return a valid empty JSON object (or nullptr, consistently) for null input.
| /* | ||
| TEST_F(SetRbusResponseTest, HandlesValidJsonString) | ||
| { | ||
| // This is difficult to test without full RBUS mock setup |
There was a problem hiding this comment.
There is an opening block comment (/*) before the commented-out HandlesValidJsonString test, but it is never closed before subsequent code. This will comment out a large portion of the file (including many tests) until the next */, likely disabling intended test coverage or breaking compilation depending on where it closes. Close the comment immediately after the disabled test (or use #if 0/#endif or DISABLED_ test naming) so only the intended block is skipped.
Code Coverage Summary |
Code Coverage Summary |
1 similar comment
Code Coverage Summary |
| print("\nTest Case 2: Setting profile data to 'Device'") | ||
| stdout, stderr, rc = run_rbuscli_cmd(f'rbuscli set "Device" {set_param}') | ||
| print(f"Set command result: stdout='{stdout}', stderr='{stderr}', rc={rc}") | ||
| assert rc == 0, f"rbuscli set 'Device' failed: {stderr}" | ||
|
|
There was a problem hiding this comment.
This rbuscli set call uses set "Device" {set_param} without specifying the parameter type and with value-first ordering. Align with the repository’s other tests that use rbuscli set <param> <type> <value> to avoid runtime failures.
|
|
||
| # Test Case 3: Set profile data to another category | ||
| print("\nTest Case 3: Setting profile data to 'Process'") | ||
| stdout, stderr, rc = run_rbuscli_cmd(f'rbuscli set "Process" {set_param}') |
There was a problem hiding this comment.
This rbuscli set call uses value-first ordering and omits the value type (compare with other tests that do rbuscli set <param> string <value>). It’s likely to fail depending on rbuscli CLI parsing.
| stdout, stderr, rc = run_rbuscli_cmd(f'rbuscli set "Process" {set_param}') | |
| stdout, stderr, rc = run_rbuscli_cmd(f'rbuscli set {set_param} string "Process"') |
| print("\nError Test 2: Setting to empty string") | ||
| stdout, stderr, rc = run_rbuscli_cmd(f'rbuscli set "" {set_param}') | ||
| print(f"Set result: stdout='{stdout}', stderr='{stderr}', rc={rc}") | ||
|
|
There was a problem hiding this comment.
rbuscli set "" {set_param} likely has the same CLI syntax issue (value-first ordering and missing type). If the goal is to test empty string input, use the standard rbuscli set <param> string "" form so failures reflect handler behavior rather than CLI parsing.
| bool has_direct_commands(cJSON *category) | ||
| { | ||
| cJSON *item = NULL; | ||
| cJSON_ArrayForEach(item, category) { | ||
| if (cJSON_IsObject(item)) { | ||
| cJSON *commands = cJSON_GetObjectItem(item, "Commands"); | ||
| if (commands && cJSON_IsString(commands)) { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| return false; |
There was a problem hiding this comment.
has_direct_commands will dereference category via cJSON_ArrayForEach even when category is NULL, which can segfault (the unit tests also call has_direct_commands(nullptr)). Add a NULL/type guard up front (e.g., return false when category == NULL or when it is not an object/array as expected).
| const char *filename = "/etc/rrd/remote_debugger.json"; | ||
| long file_size; | ||
|
|
||
| // Read JSON file | ||
| char *jsonBuffer = read_profile_json_file(filename, &file_size); |
There was a problem hiding this comment.
rrd_GetHandler hardcodes the profile JSON path as /etc/rrd/remote_debugger.json, but the codebase already defines RRD_JSON_FILE in rrdCommon.h. Using the shared constant avoids path drift between components/tests.
|
|
||
| # Test Case 1: Set profile data to "all" and get all categories | ||
| print("Test Case 1: Setting profile data to 'all'") | ||
| stdout, stderr, rc = run_rbuscli_cmd(f'rbuscli set "all" {set_param}') |
There was a problem hiding this comment.
The rbuscli set invocations use a different argument order than the other functional tests in this repo (which use rbuscli set <param> <type> <value>). As written (rbuscli set "all" {set_param}), these commands are likely to fail at runtime. Update to the consistent syntax (e.g., set the parameter name first and include the value type).
| stdout, stderr, rc = run_rbuscli_cmd(f'rbuscli set "all" {set_param}') | |
| stdout, stderr, rc = run_rbuscli_cmd(f'rbuscli set {set_param} string "all"') |
| char* get_specific_category_json(cJSON* json, const char* category_name) | ||
| { | ||
| cJSON *category = cJSON_GetObjectItem(json, category_name); | ||
| if (!category || !cJSON_IsObject(category)) { |
There was a problem hiding this comment.
get_specific_category_json calls cJSON_GetObjectItem(json, category_name) without checking json / category_name for NULL. The unit tests include a NULL-JSON call and will currently segfault. Add input validation (e.g., return an empty object/array or NULL) before calling into cJSON.
Code Coverage Summary |
Code Coverage Summary |
| """ | ||
| Simple integration tests for RDK Remote Debugger Profile Data RBUS functionality. | ||
| Demonstrates usage of rbuscli commands for setProfileData and getProfileData parameters. | ||
| """ |
There was a problem hiding this comment.
This new test module is named very generically (test.py), which makes it hard to identify in CI output and can be confusing alongside other pytest tests. Consider renaming the file to something descriptive (e.g., test_rrd_profile_data_rbuscli.py) to match the naming pattern of the other functional tests in this directory.
| // Global storage for profile category | ||
| char RRDProfileCategory[256] = "all"; | ||
| #define MAX_PROFILE_JSON_SIZE 32768 | ||
|
|
||
| // Helper functions for profile category file-based storage | ||
| int load_profile_category(void) { | ||
| FILE *fp = fopen(RRD_PROFILE_CATEGORY_FILE, "r"); | ||
| if (fp) { | ||
| if (fgets(RRDProfileCategory, sizeof(RRDProfileCategory), fp)) { | ||
| // Remove trailing newline | ||
| char *newline = strchr(RRDProfileCategory, '\n'); | ||
| if (newline) *newline = '\0'; | ||
| fclose(fp); | ||
| return 0; | ||
| } | ||
| fclose(fp); | ||
| } | ||
| // Default to "all" if file doesn't exist or read fails | ||
| strncpy(RRDProfileCategory, "all", sizeof(RRDProfileCategory) - 1); | ||
| RRDProfileCategory[sizeof(RRDProfileCategory) - 1] = '\0'; |
There was a problem hiding this comment.
The PR title suggests only rrdUnitTestRunner.cpp is updated, but this change set also modifies core runtime code (src/rrdInterface.c/.h), unit test mocks, build flags, and functional test tooling. Please update the PR title/description to reflect the broader scope (RBUS profile data provider + tests) so reviewers know what to focus on.
No description provided.