Unit tests for syscollector legacy decoder #15985
Conversation
| merror("Invalid message. Type not found."); // LCOV_EXCL_LINE | ||
| goto end; // LCOV_EXCL_LINE |
There was a problem hiding this comment.
Why are we not testing this and ignoring instead?
There was a problem hiding this comment.
Is not posible to achieve this section of code, because the only one entry point for decoder_XXX functions is by call on DecodeSyscollector().
There was a problem hiding this comment.
I think it is possible but we would need to mock this json_type = cJSON_GetObjectItem(logJSON, "type"); it would affect the entire file and probably not worth it. The team may decide this.
There was a problem hiding this comment.
Are you sure we can't send a JSON without type to force cJSON_GetObjectItem() to return NULL?
it would affect the entire file and probably not worth it.
Wrapping this function directly would indeed generate too many changes. Every test should be updated.
The only thing I can think of is updating __wrap_cJSON_GetObjectItem() to allow selecting when we want the real function and when we need the fake one.
Something like
if(test_mode) {
//fake method
...
return mock;
}
else {
//real method
__real_cJSON_GetObjectItem();
}
There was a problem hiding this comment.
Let's not apply changes around this, this is a candidate for refactoring, it doesn't make sense to pass a string for a check that is done from the caller. A boolean would suffice to identify the end message.
| merror("Invalid message. Type not found."); // LCOV_EXCL_LINE | ||
| goto end; // LCOV_EXCL_LINE |
There was a problem hiding this comment.
I would normalize this message using the approach of the one above by adding the function name. The same for all cases
There was a problem hiding this comment.
This refactoring exceeds the scope of the issue.
| \"timestamp\":\"2021/10/29 14:26:24\",\ | ||
| \"inventory\":{\ | ||
| \"board_serial\" : \"86\",\ | ||
| \"cpu_name\" : \"87\",\ |
There was a problem hiding this comment.
I would use a more representative example but not change requested.
| \"type\":\"hotfix\",\ | ||
| \"ID\":100,\ | ||
| \"timestamp\":\"2021/10/29 14:26:24\",\ | ||
| \"hotfix\":\"hotfix-version-test\"\ |
There was a problem hiding this comment.
I would use a more representative example but not change requested.
| \"ID\":100,\ | ||
| \"timestamp\":\"2021/10/29 14:26:24\",\ | ||
| \"iface\":{\ | ||
| \"name\" : \"86\",\ |
There was a problem hiding this comment.
I would use a more representative example but not change requested.
snaow
changed the title
tdrauncieal
force-pushed
the
dev-10368-UT-syscollector-legacy
branch
from
8db2785
to
6580a42
Compare
* Add 100% UT coverage on these functions:
decode_netinfo()
decode_osinfo()
decode_hardware()
decode_package()
decode_hotfix()
decode_port()
decode_process()
tdrauncieal
force-pushed
the
dev-10368-UT-syscollector-legacy
branch
from
6580a42
to
bb683c6
Compare
| merror("Invalid message. Type not found."); // LCOV_EXCL_LINE | ||
| goto end; // LCOV_EXCL_LINE |
There was a problem hiding this comment.
Are you sure we can't send a JSON without type to force cJSON_GetObjectItem() to return NULL?
it would affect the entire file and probably not worth it.
Wrapping this function directly would indeed generate too many changes. Every test should be updated.
The only thing I can think of is updating __wrap_cJSON_GetObjectItem() to allow selecting when we want the real function and when we need the fake one.
Something like
if(test_mode) {
//fake method
...
return mock;
}
else {
//real method
__real_cJSON_GetObjectItem();
}
Description
Add 100% UT coverage on these functions:
decode_netinfo()
decode_osinfo()
decode_hardware()
decode_package()
decode_hotfix()
decode_port()
decode_process()
Tests