tests: ble_hrs: unit tests for ble_hrs #313
Conversation
|
You can find the documentation preview for this PR here. |
|
No milestone required for tests |
There was a problem hiding this comment.
This is good, a few comments regarding splitting the tests.
Most importantly, we should not try to peek into struct ble_hrs. In general that's only used for initial configuration (which may not be the best idea). Afterwards the user is meant to use only the API to interact with the service. So we can't really assume that the values in there have to be set to a certain value, only that the API returns the correct value. Also, we shouldn't manipulate that struct in our tests to put the library in a given state, rather, we should simulate input to the library via BLE events, to confirm that the library is in a certain state (by using API calls).
| /* Try to use null for hrs struct */ | ||
| err = ble_hrs_sensor_contact_detected_update(NULL, true); | ||
| TEST_ASSERT_EQUAL(-EFAULT, err); |
There was a problem hiding this comment.
There is a dedicated test for that test_ble_hrs_sensor_contact_supported_set_efault() so this could be removed from here.
| err = ble_hrs_init(NULL, NULL); | ||
| TEST_ASSERT_EQUAL(-EFAULT, err); |
There was a problem hiding this comment.
This could be a dedicated test
| __cmock_sd_ble_gatts_service_add_IgnoreAndReturn(NRF_ERROR_INVALID_ADDR); | ||
| err = ble_hrs_init(&hrs, &hrs_config); | ||
| TEST_ASSERT_EQUAL(-EINVAL, err); |
There was a problem hiding this comment.
This could be a dedicated test
| TEST_ASSERT_EQUAL_PTR(hrs_config.evt_handler, hrs.evt_handler); | ||
| TEST_ASSERT_EQUAL(BLE_CONN_HANDLE_INVALID, hrs.conn_handle); | ||
| TEST_ASSERT_EQUAL(0, hrs.rr_interval_count); | ||
| TEST_ASSERT_EQUAL(hrs_config.is_sensor_contact_supported, hrs.is_sensor_contact_supported); | ||
| TEST_ASSERT_FALSE(hrs.is_sensor_contact_detected); |
There was a problem hiding this comment.
If the function ble_hrs_init() returns an error, then we can't make any assumption to the contents of struct ble_hrs (whether they are correct or not). In general I don't think we should check what's in struct ble_hrs at all, since the user is not supposed to read or write directly into that, but use the API.
| err = ble_hrs_heart_rate_measurement_send(NULL, heart_rate_measurement); | ||
| TEST_ASSERT_EQUAL(-EFAULT, err); |
| __cmock_sd_ble_gatts_hvx_IgnoreAndReturn(BLE_ERROR_INVALID_CONN_HANDLE); | ||
| err = ble_hrs_heart_rate_measurement_send(&hrs, heart_rate_measurement); | ||
| TEST_ASSERT_EQUAL(-ENOTCONN, err); |
| __cmock_sd_ble_gatts_hvx_ExpectAndReturn(hrs.conn_handle, NULL, NRF_ERROR_INVALID_STATE); | ||
| __cmock_sd_ble_gatts_hvx_IgnoreArg_p_hvx_params(); | ||
| err = ble_hrs_heart_rate_measurement_send(&hrs, heart_rate_measurement); | ||
| TEST_ASSERT_EQUAL(-EPIPE, err); |
There was a problem hiding this comment.
All these are groups in this function are separate tests
| /* Try to set sensor contact supported while in connection | ||
| * Simulate being in a connection | ||
| */ | ||
| hrs.conn_handle = 1; |
There was a problem hiding this comment.
We should not do it like so-- here we assume that once the device enters a connection, the service correctly updates its conn_handle field. That may not be the case at all. Instead, we should dispatch a BLE_GAP_CONNECTED event and then attempt to change ble_hrs_sensor_contact_supported_set() to see if the function correctly returns an error.
| struct ble_hrs hrs = { | ||
| .evt_handler = ble_hrs_evt_handler, | ||
| .service_handle = 0, | ||
| .conn_handle = BLE_CONN_HANDLE_INVALID, | ||
| .rr_interval_count = 0, | ||
| .max_hrm_len = 0, | ||
| .is_sensor_contact_supported = false, | ||
| .is_sensor_contact_detected = false | ||
| }; | ||
| int err; | ||
|
|
||
| /* Initialize the heart rate service. */ | ||
| memset(&hrs, 0, sizeof(hrs)); |
There was a problem hiding this comment.
First you initialize hrs to something and then you zero it, which one do you want to do?
MechanicV1
force-pushed
the
ble_hrs_ut
branch
from
7d066e9 to
9e3e429
Compare
Adding unit tests to increase coverage for ble_hrs library Signed-off-by: Nirmal Krishna <[email protected]> Co-authored-by: Andreas Moltumyr <[email protected]>
MechanicV1
force-pushed
the
ble_hrs_ut
branch
from
9e3e429 to
33a4245
Compare
| #include "cmock_ble_gatts.h" | ||
|
|
||
| void ble_hrs_evt_handler(struct ble_hrs *hrs, const struct ble_hrs_evt *evt) | ||
| { |
There was a problem hiding this comment.
Would be good with some tests on what is received here.
There was a problem hiding this comment.
Some tests for ble_hrs_conn_params_evt should also improve the coverage.
Adding unit tests to increase coverage for
ble_hrs library