tests: unittest: Added unittests for ble_bas #306

marenges · 2025-08-22T08:13:46Z

Added unit tests for ble_bas service

github-actions · 2025-08-22T08:14:31Z

You can find the documentation preview for this PR here.

tests/subsys/bluetooth/services/ble_bas/src/unity_test.c

CODEOWNERS

tests/subsys/bluetooth/services/ble_bas/CMakeLists.txt

lemrey · 2025-09-08T11:34:54Z

No milestone required for tests

lemrey

This looks good but I have an important comment.

We should not try to peek into or manually modify struct ble_bas.
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).

lemrey · 2025-09-24T10:27:31Z

tests/subsys/bluetooth/services/ble_bas/src/unity_test.c

+	__cmock_sd_ble_gatts_hvx_ExpectAnyArgsAndReturn(NRF_ERROR_INVALID_STATE);
+	ret = ble_bas_battery_level_update(&ble_bas, conn_handle, battery_level);
+	TEST_ASSERT_EQUAL(-EPIPE, ret);
+	TEST_ASSERT_EQUAL(battery_level, ble_bas.battery_level);


We shouldn't test this (you are testing inside ble_bas)

lemrey · 2025-09-24T10:27:37Z

tests/subsys/bluetooth/services/ble_bas/src/unity_test.c

+	TEST_ASSERT_EQUAL(BLE_GATT_HVX_NOTIFICATION, p_hvx_params->type);
+	TEST_ASSERT_EQUAL(0, p_hvx_params->offset);
+	TEST_ASSERT_EQUAL(sizeof(uint8_t), *(p_hvx_params->p_len));
+	TEST_ASSERT_EQUAL(ble_bas.battery_level, *(uint8_t *)(p_hvx_params->p_data));


lemrey · 2025-09-24T10:27:44Z

tests/subsys/bluetooth/services/ble_bas/src/unity_test.c

+	/* Battery level hasn't change 'Nothing to do' */
+	ret = ble_bas_battery_level_update(&ble_bas, conn_handle, battery_level);
+	TEST_ASSERT_EQUAL(0, ret);
+	TEST_ASSERT_EQUAL(battery_level, ble_bas.battery_level);


lemrey · 2025-09-24T10:29:48Z

tests/subsys/bluetooth/services/ble_bas/src/unity_test.c

+	TEST_ASSERT_EQUAL(SERVICE_HANDLE, ble_bas.service_handle);
+	TEST_ASSERT_EQUAL(VALUE_HANDLE, ble_bas.battery_level_handles.value_handle);
+	TEST_ASSERT_EQUAL(REPORT_REF_HANDLE, ble_bas.report_ref_handle);


You can't test these like that-- but you can try to call some API that would pass these values as a parameter to a SoftDevice call to verify that they were set as expected during ble_bas_init()

For example ble_bas_battery_level_notify() passes the field battery_level_handles.value_handle to the sd_ble_gatts_hvx() function

lemrey · 2025-09-24T10:35:28Z

tests/subsys/bluetooth/services/ble_bas/src/unity_test.c

+	ret = ble_bas_init(&ble_bas, &bas_cfg);
+	TEST_ASSERT_EQUAL(0, ret);
+	TEST_ASSERT_EQUAL(SERVICE_HANDLE, ble_bas.service_handle);
+	TEST_ASSERT_EQUAL(VALUE_HANDLE, ble_bas.battery_level_handles.value_handle);


Here too, you can call the ble_bas_battery_level_notify() function to see that the hvx parameters have the correct handle for the value.

lemrey · 2025-09-24T10:39:35Z

tests/subsys/bluetooth/services/ble_bas/src/unity_test.c

+	/* Change battery level, and ble_bas should update but not notify */
+	battery_level = 42;
+	__cmock_sd_ble_gatts_value_set_ExpectAnyArgsAndReturn(NRF_SUCCESS);
+	ret = ble_bas_battery_level_update(&ble_bas, conn_handle, battery_level);
+	TEST_ASSERT_EQUAL(0, ret);
+	TEST_ASSERT_EQUAL(battery_level, ble_bas.battery_level);


Instead of accepting any arguments to sd_ble_gatts_value_set() you should check that the implementation is passing the value that the user provided when calling ble_bas_battery_level_updated(). The implementation may pass one value to sd_ble_gatts_value_set() and a set different value in ble_bas.battery_level (due to a bug).

MirkoCovizzi · 2025-09-30T13:42:26Z

tests/subsys/bluetooth/services/ble_bas/src/unity_test.c

+#include "ble_gatts.h"
+#include "cmock_ble_gatts.h"
+
+#define SERVICE_HANDLE		0x1234


The alignment here could be improved a bit.

I see that the alignment is different in my editor. It seems like tabsize is set to 4 in this PR

MirkoCovizzi · 2025-09-30T13:42:43Z

tests/subsys/bluetooth/services/ble_bas/src/unity_test.c

+#define REPORT_REF_HANDLE	0xF8EE
+#define INVALID_HANDLE		0xFFFF
+
+BLE_BAS_DEF(ble_bas);


Add a newline under this line.

MirkoCovizzi · 2025-09-30T13:43:01Z

tests/subsys/bluetooth/services/ble_bas/src/unity_test.c

+static uint8_t battery_level;
+static int evt_handler_called;
+static int hvx_stub_called;
+static struct {


Add some space in between these structs.

MirkoCovizzi · 2025-09-30T13:44:12Z

tests/subsys/bluetooth/services/ble_bas/src/unity_test.c

+	bas_cfg.can_notify = false;
+	bas_init(&bas_cfg);
+
+	/* Battery level hasn't change 'Nothing to do' */


Suggested change

/* Battery level hasn't change 'Nothing to do' */

/* Battery level hasn't changed 'Nothing to do' */

MirkoCovizzi · 2025-09-30T13:45:39Z

tests/subsys/bluetooth/services/ble_bas/src/unity_test.c

+{
+	memset(&ble_bas, 0, sizeof(ble_bas));
+	evt_handler_called = false;
+	battery_level = 55;


If 55 is a default/reference value, maybe it can be a macro?

Added unit tests for ble_bas service Signed-off-by: Martin Engesvold <[email protected]> Co-authored-by: Andreas Moltumyr <[email protected]>

marenges added this to the v0.8.0 milestone Aug 22, 2025

marenges requested review from a team as code owners August 22, 2025 08:13

marenges self-assigned this Aug 22, 2025

marenges force-pushed the ble_bas_unittest branch 2 times, most recently from bc3f68a to 6b6e791 Compare August 22, 2025 08:40

eivindj-nordic reviewed Aug 22, 2025

View reviewed changes

tests/subsys/bluetooth/services/ble_bas/src/unity_test.c Outdated Show resolved Hide resolved

tests/subsys/bluetooth/services/ble_bas/src/unity_test.c Outdated Show resolved Hide resolved

marenges force-pushed the ble_bas_unittest branch from 6b6e791 to 6dab329 Compare August 22, 2025 11:23

MirkoCovizzi reviewed Aug 25, 2025

View reviewed changes

CODEOWNERS Outdated Show resolved Hide resolved

tests/subsys/bluetooth/services/ble_bas/CMakeLists.txt Show resolved Hide resolved

marenges force-pushed the ble_bas_unittest branch from 6dab329 to 3faff88 Compare August 25, 2025 10:37

eivindj-nordic removed this from the v0.8.0 milestone Aug 26, 2025

marenges force-pushed the ble_bas_unittest branch 3 times, most recently from a353f5d to 7d114c2 Compare August 26, 2025 13:43

eivindj-nordic added this to the v0.9.0 milestone Aug 27, 2025

marenges force-pushed the ble_bas_unittest branch from 7d114c2 to 2b0df21 Compare September 8, 2025 07:04

lemrey removed this from the v0.9.0 milestone Sep 8, 2025

lemrey reviewed Sep 12, 2025

View reviewed changes

marenges force-pushed the ble_bas_unittest branch from 2b0df21 to ad6a36b Compare September 16, 2025 12:38

lemrey reviewed Sep 24, 2025

View reviewed changes

marenges force-pushed the ble_bas_unittest branch 2 times, most recently from 2eba186 to 9e7f915 Compare September 30, 2025 10:36

MirkoCovizzi reviewed Sep 30, 2025

View reviewed changes

tests: unittest: Added unittests for ble_bas

101275b

Added unit tests for ble_bas service Signed-off-by: Martin Engesvold <[email protected]> Co-authored-by: Andreas Moltumyr <[email protected]>

marenges force-pushed the ble_bas_unittest branch from 9e7f915 to 101275b Compare October 9, 2025 06:49

	/* Battery level hasn't change 'Nothing to do' */
	/* Battery level hasn't changed 'Nothing to do' */

tests: unittest: Added unittests for ble_bas #306

Are you sure you want to change the base?

tests: unittest: Added unittests for ble_bas #306

Conversation

marenges commented Aug 22, 2025

Uh oh!

github-actions bot commented Aug 22, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lemrey commented Sep 8, 2025

Uh oh!

lemrey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!