Datamodel markers should ignore marker value as zero #253
Conversation
Signed-off-by: PriyaDharshini_Kathiravan <PriyaDharshini_Kathiravan@comcast.com>
There was a problem hiding this comment.
Pull request overview
This PR implements functionality to ignore datamodel marker values when they equal "0", aligning with legacy behavior for absolute/split markers. This prevents reporting of markers with zero values in telemetry reports.
Changes:
- Modified the
checkForEmptyStringfunction to treat "0" as an empty/invalid value that should be ignored - Added debug logging to indicate when marker values are empty, zero, or NULL
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
source/reportgen/reportgen.c
Outdated
| // Consider "NULL" string value as qualified for empty string | ||
| if(strlen(valueString) < 1 || !strncmp(valueString, " ", 1) || !strncmp(valueString, "NULL", 4)) | ||
| // If the string has zero value it should be ignored | ||
| if(strlen(valueString) < 1 || !strncmp(valueString, " ", 1) || !strncmp(valueString, "0", 1) || !strncmp(valueString, "NULL", 4)) |
There was a problem hiding this comment.
The check !strncmp(valueString, "0", 1) will match any string starting with "0", not just the string "0". This means valid values like "01", "012345", "0.5", or "0x1A" would also be incorrectly treated as empty. Consider using strcmp(valueString, "0") == 0 or strlen(valueString) == 1 && valueString[0] == '0' to match only the exact string "0".
| if(strlen(valueString) < 1 || !strncmp(valueString, " ", 1) || !strncmp(valueString, "0", 1) || !strncmp(valueString, "NULL", 4)) | |
| if(strlen(valueString) < 1 || !strncmp(valueString, " ", 1) || strcmp(valueString, "0") == 0 || !strncmp(valueString, "NULL", 4)) |
source/reportgen/reportgen.c
Outdated
| // Consider "NULL" string value as qualified for empty string | ||
| if(strlen(valueString) < 1 || !strncmp(valueString, " ", 1) || !strncmp(valueString, "NULL", 4)) | ||
| // If the string has zero value it should be ignored | ||
| if(strlen(valueString) < 1 || !strncmp(valueString, " ", 1) || !strncmp(valueString, "0", 1) || !strncmp(valueString, "NULL", 4)) |
There was a problem hiding this comment.
The test CheckForEmptyString in source/test/reportgen/reportgenTest.cpp needs to be updated to include test cases for the new "0" check. Currently, the test covers NULL, empty string, space, and "NULL" literal, but doesn't test the newly added "0" behavior to verify it works as intended.
Signed-off-by: PriyaDharshini_Kathiravan <PriyaDharshini_Kathiravan@comcast.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| rbus_set_data("Device.DeviceInfo.X_RDKCENTRAL-COM.IUI.Version", "string", "0") | ||
| kill_telemetry(29) | ||
| sleep(5) | ||
| assert "IUI_accum" not in grep_T2logs("cJSON Report") | ||
| assert "Test_datamodel_1" in grep_T2logs("cJSON Report") |
There was a problem hiding this comment.
grep_T2logs("cJSON Report") returns all matching lines from the entire log file. Since earlier reports in this test already contain IUI_accum, the assertion "IUI_accum" not in grep_T2logs("cJSON Report") can fail (or pass for the wrong reason) unless the logs are cleared or the search is restricted to the most recent report. Consider clearing the log right before triggering the report after setting the value to "0", or changing the helper/usage to only inspect the latest cJSON Report entry (e.g., tail the file / track report boundaries).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pytest.mark.run(order=15) | ||
| def test_xconf_datamodel(): |
There was a problem hiding this comment.
pytest.mark.run(...) is missing the @ decorator prefix here, so the mark is never applied (it just evaluates an unused expression at import time). This likely breaks the intended ordered execution for this test.
| pytest.mark.run(order=14) | ||
| def test_xconf_split_markers(): |
There was a problem hiding this comment.
pytest.mark.run(...) is missing the @ decorator prefix here, so the mark is never applied and test ordering will not follow the configured order value.
| sleep(7) | ||
| assert "IUI_accum\":\"0.1" in grep_T2logs("cJSON Report") | ||
| rbus_set_data("Device.DeviceInfo.X_RDKCENTRAL-COM.IUI.Version", "string", "0") | ||
| sleep(2) |
There was a problem hiding this comment.
This negative assertion will almost always fail because the same test previously asserted that an earlier report contained IUI_accum, and grep_T2logs() returns all matching lines from the entire log file (it does not scope to the latest report). Clear the log (or extract/assert only the last cJSON Report) before checking that IUI_accum is absent after setting the value to "0".
| sleep(2) | |
| sleep(2) | |
| clear_T2logs() |
| // If the string has zero value it should be ignored | ||
| if(strlen(valueString) < 1 || !strncmp(valueString, " ", 1) || !strcmp(valueString, "0") || !strncmp(valueString, "NULL", 4)) | ||
| { | ||
| isEmpty = true ; | ||
| T2Debug("Marker Value is empty or zero or NULL\n"); | ||
| } |
There was a problem hiding this comment.
checkForEmptyString() now treats the string "0" as empty, but the existing unit test for this helper (in source/test/reportgen/reportgenTest.cpp, TEST(CheckForEmptyString, AllBranchesAreCovered)) does not cover the new branch. Please add a test case asserting that "0" returns true to prevent regressions.
2 participants
No description provided.