Bluetooth: controller: Llcp update unittests #52894

kruithofa · 2022-12-08T13:26:42Z

Update to the feature exchange unit tests

use the new ZTEST API
use the Zephyr Kconfig settings (and a prj.conf file), getting rid of the hack of having an include file for KConfig settings.
specify the helper functions and the mocks as a library instead of including them as a file list

Looking for feedback, in particular if we should have a file unit_sources.txt, and include that in each of the unit-tests (as per this PR), or copy the contents of this file to each CMakelists.txt in each unit-test.

fixes #52626

tejlmand

Thanks for this improvement, some minor nits and question / proposals regarding some of the choices made.

Overall impression is that this is a nice improvement 👍

tejlmand · 2022-12-20T10:31:10Z

tests/bluetooth/controller/ctrl_feature_exchange/Kconfig

+# some of the control procedures in the BT LL depend on
+# the following configs been set
+config SOC_COMPATIBLE_NRF
+       bool


no need to redefine the type here.
Actually not specifying the type not only minimize the file but also has the benefit of easily discover if the real symbol is renamed / removed.

Ref: https://docs.zephyrproject.org/latest/build/kconfig/tips.html#common-kconfig-shorthands

Suggested change

bool

Didn't know this, but makes sense. A cleanup of other Kconfig files may be required then, for example in tests/bluetooth/df/* (where this was copied from)

specifying the type has been removed from all other places as well

A cleanup of other Kconfig files may be required then, for example in tests/bluetooth/df/* (where this was copied from)

Yes, there's a bit of whack-a-mole here, because developers naturally copies files / code snippes around and adjust.

Thanks for cleaning up.

tejlmand · 2022-12-20T10:32:06Z

tests/bluetooth/controller/ctrl_feature_exchange/Kconfig

+       default y
+
+config SOC_SERIES_NRFX
+       bool


Suggested change

bool

tejlmand · 2022-12-20T10:32:13Z

tests/bluetooth/controller/ctrl_feature_exchange/Kconfig

+       default y
+
+config BT_CTLR_PHY_UPDATE_SUPPORT
+       bool


Suggested change

bool

tejlmand · 2022-12-20T10:32:17Z

tests/bluetooth/controller/ctrl_feature_exchange/Kconfig

+       default y
+
+config ENTROPY_NRF_FORCE_ALT
+       bool


Suggested change

bool

tejlmand · 2022-12-20T10:32:22Z

tests/bluetooth/controller/ctrl_feature_exchange/Kconfig

+       default n
+
+config DT_HAS_NORDIC_NRF_RNG_ENABLED
+       bool


Suggested change

bool

tejlmand · 2022-12-20T10:49:33Z

tests/bluetooth/controller/common/unit_sources.txt

+# Common include directories and source files for bluetooth unit tests
+#
+
+include_directories(


why exactly is include_directories() used here ?
cause in this particular case then those include paths will only be applied to testbinary afaict.

Generally target_include_directories() is recommended.

Creating a uut library removes the need for this file

tejlmand · 2022-12-20T10:50:48Z

tests/bluetooth/controller/ctrl_feature_exchange/Kconfig

@@ -15,6 +15,14 @@ config BT_CTLR_PHY_UPDATE_SUPPORT
       bool
       default y

+config BT_CTLR_PHY_CODED_SUPPORT
+	bool


Suggested change

bool

tejlmand · 2022-12-20T10:51:55Z

tests/bluetooth/controller/ctrl_feature_exchange/Kconfig

+	default y
+
+config BT_CTLR_PHY_2M_SUPPORT
+	bool


Suggested change

bool

tejlmand · 2022-12-20T10:56:37Z

tests/bluetooth/controller/common/CMakeLists.txt

+  ${ZEPHYR_BASE}/subsys/bluetooth/controller/include
+  ${ZEPHYR_BASE}/subsys/bluetooth/controller/util
+  ${ZEPHYR_BASE}/subsys/bluetooth/controller
+  ${ZEPHYR_BASE}/subsys/bluetooth
+  ${ZEPHYR_BASE}/subsys/bluetooth/controller/ll_sw
+  ${ZEPHYR_BASE}/subsys/bluetooth/controller/ll_sw/nordic/lll
+  ${ZEPHYR_BASE}/subsys/bluetooth/controller/ll_sw/nordic
+  ${ZEPHYR_BASE}/include/zephyr/bluetooth


those are also exported by the mocks library.

are both really needed or could there be a single interface library that both the mock and the testbinary can link to ?

I prefer listing the required include directories explicitly for each library; however the list was too large, so the list will be reduced to only the absolutely minimum required include directories
Arguably the directory 'common' should be renamed 'helper', but that will have to be part of a separate PR, where we move the unittests for the controller to a more logical place

tejlmand · 2022-12-20T11:03:56Z

tests/bluetooth/controller/common/defaults_cmake.txt

+add_definitions(-include kconfig.h)
+if(KCONFIG_OVERRIDE_FILE)
+  add_definitions(-include ${KCONFIG_OVERRIDE_FILE})
+endif()


we have kconfig support in unittesting so I don't think we should be using this Kconfig override principle.
We should be able to handle this properly in Kconfig files directly, so that unittests are following same pattern as samples.

Please let me know if there are reasons when the common Kconfig approach cannot be used.

100% agreed, and the reason for this PR is so that we can get rid of this hack. It is left here because the other unittests rely on this. Once this PR is approved (meaning that we agree on how the framework for the unittests for the controller should be) we can create another PR updating all the other unittests and getting rid of this hack. See #35099

tejlmand

Thanks. Definitely improved, making other things easier to spot.

tejlmand · 2023-01-02T14:17:43Z

tests/bluetooth/controller/mock_ctrl/CMakeLists.txt

+#
+
+add_library(mocks STATIC
+  ${ZEPHYR_BASE}/tests/bluetooth/controller/mock_ctrl/src/kernel.c


no need for full path, relative sources are picked up relative to CMAKE_CURRENT_SOURCE_DIR, so this is sufficient here and below:

src/kernel.c

tejlmand · 2023-01-02T14:19:55Z

tests/bluetooth/controller/mock_ctrl/CMakeLists.txt

+
+
+target_include_directories(mocks PUBLIC
+  ${ZEPHYR_BASE}/tests/bluetooth/controller/mock_ctrl/include


this line can be reduced to

Suggested change

${ZEPHYR_BASE}/tests/bluetooth/controller/mock_ctrl/include

include

as that include folder is below the CMAKE_CURRENT_SOURCE_DIR.
This also makes it clearer to the reader which folders are local to the mock lib, and which folders are picked up externally (as they use abs path)

tejlmand · 2023-01-02T14:20:39Z

tests/bluetooth/controller/uut/CMakeLists.txt

@@ -0,0 +1,35 @@
+#
+# CMakeLists.txt file for creating of mocks library.


Please adjust.

tejlmand · 2023-01-02T14:31:15Z

tests/bluetooth/controller/uut/CMakeLists.txt

+)
+
+target_include_directories(uut PUBLIC
+  ${ZEPHYR_BASE}/tests/bluetooth/controller/mock_ctrl/include


this line indicates that this library depends on the mock library created.

Should there be a target_link_libraries(uut PUBLIC mocks) here ?
That would ensure the include path gets inherited correctly, as well as generally making sense as I believe the sources in uut actually links to functions provided my the mock lib.

agreed; that makes the CMakeLists.txt in the ctrl_feature_exchange cleaner as well

so because you now link to the mocks library, then this line can be removed because it is inherited.

Suggested change

${ZEPHYR_BASE}/tests/bluetooth/controller/mock_ctrl/include

this line is still here.
The mock_ctrl/include is an include native to the mock library, so it should be inherited.

The other includes, those with ${ZEPHYR_BASE}/subsys/bluetooth are not native to the mock library (they are rightfully native to the bluetooth controller library which we are not linking directly).
Therefor the ${ZEPHYR_BASE}/subsys/bluetooth are acceptable not to inherit, but the mock include must be inherited. The uut itself should not know about internals of the mock lib.

tejlmand

Nice, also some minor nits.

tejlmand · 2023-01-12T09:30:48Z

tests/bluetooth/controller/uut/CMakeLists.txt

+)
+
+target_include_directories(uut PUBLIC
+  ${ZEPHYR_BASE}/tests/bluetooth/controller/mock_ctrl/include


so because you now link to the mocks library, then this line can be removed because it is inherited.

Suggested change

${ZEPHYR_BASE}/tests/bluetooth/controller/mock_ctrl/include

tejlmand · 2023-01-12T09:39:19Z

tests/bluetooth/controller/uut/CMakeLists.txt

+  ${ZEPHYR_BASE}/subsys/bluetooth/controller/include
+  ${ZEPHYR_BASE}/subsys/bluetooth/controller
+  ${ZEPHYR_BASE}/subsys/bluetooth
+  ${ZEPHYR_BASE}/subsys/bluetooth/controller/ll_sw
+  ${ZEPHYR_BASE}/subsys/bluetooth/controller/ll_sw/nordic
+  ${ZEPHYR_BASE}/include/zephyr/bluetooth


FYI: Those include paths are also defined by the mocks library, which in practice means you can remove them from here.

But as the uut library refers to source files directly in the bluetooth controller library then it also make sense to keep those includes explicitly here, as well as in the mocks lib.

I prefer listing them here explicitly, due to the direct reference to bluetooth controller files

The consequence is however that we also need to leave the ${ZEPHYR_BASE}/tests/bluetooth/controller/mock_ctrl/include in place to ensure that we include the mock include-files.
i.e. either we remove all include paths, or we leave all of them as is

tejlmand · 2023-01-12T09:40:58Z

tests/bluetooth/controller/uut/CMakeLists.txt

+  ${ZEPHYR_BASE}/include/zephyr/bluetooth
+)
+
+add_subdirectory(${ZEPHYR_BASE}/tests/bluetooth/controller/mock_ctrl mocks)


Comment for future reference.

This is not ideal, as we should strive to have a straight forward CMake hierarchy.
But as this is an improvement on the overall test structure, then it is accepted in this case.

Created #53741 for followup

tejlmand · 2023-01-12T09:42:34Z

tests/bluetooth/controller/common/CMakeLists.txt

+#
+# SPDX-License-Identifier: Apache-2.0
+#
+# CMakeLists.txt file for creating of mocks library.


Please adjust, this file doesn't create the mocks library.

carlescufi · 2023-01-16T16:09:19Z

@tejlmand please take another look

tejlmand · 2023-01-16T16:18:42Z

tests/bluetooth/controller/uut/CMakeLists.txt

+)
+
+target_include_directories(uut PUBLIC
+  ${ZEPHYR_BASE}/tests/bluetooth/controller/mock_ctrl/include


this line is still here.
The mock_ctrl/include is an include native to the mock library, so it should be inherited.

The other includes, those with ${ZEPHYR_BASE}/subsys/bluetooth are not native to the mock library (they are rightfully native to the bluetooth controller library which we are not linking directly).
Therefor the ${ZEPHYR_BASE}/subsys/bluetooth are acceptable not to inherit, but the mock include must be inherited. The uut itself should not know about internals of the mock lib.

tejlmand · 2023-01-16T16:20:51Z

tests/bluetooth/controller/mock_ctrl/CMakeLists.txt

+
+target_include_directories(mocks PUBLIC
+  include
+  ${ZEPHYR_BASE}/subsys/bluetooth/controller/include


as you have decided that test lib themselves should not inherit includes under ${ZEPHYR_BASE}/subsys/bluetooth, then those should be made PRIVATE as to avoid mix of principles.

Suggested change

${ZEPHYR_BASE}/subsys/bluetooth/controller/include

PRIVATE

${ZEPHYR_BASE}/subsys/bluetooth/controller/include

Update the unittests for feature exchange to use the new ZTEST API Here we only update the feature exchange procedure. The remaining unittests will be done in a next PR Mocks, helper routines and the Unit Under Test are splitted out as a library instead of adding them to the file list Signed-off-by: Andries Kruithof <andries.kruithof@nordicsemi.no>

tejlmand

lgtm, thanks for the effort.

carlescufi · 2023-01-19T11:57:46Z

@thoh-ot @erbr-ot could you please review this one?

erbr-ot

This looks good - thanks, Andries!
One nit-pick - the tittle of the PR could reflect this is feature exchange only.

kruithofa requested review from tejlmand, thoh-ot and erbr-ot December 8, 2022 13:26

kruithofa force-pushed the llcp_update_unittests branch from b1af50b to 29eb3dc Compare December 12, 2022 09:59

kruithofa mentioned this pull request Dec 15, 2022

Bluetooth: controller: update remaining llcp unittests for new ZTEST API #53099

Merged

kruithofa marked this pull request as ready for review December 19, 2022 12:31

kruithofa requested review from cvinayak and sjanc as code owners December 19, 2022 12:31

zephyrbot added area: Bluetooth Controller area: Bluetooth labels Dec 19, 2022

zephyrbot requested review from carlescufi, mtpr-ot and wopu-ot December 19, 2022 12:32

zephyrbot assigned cvinayak Dec 19, 2022

kruithofa force-pushed the llcp_update_unittests branch 4 times, most recently from a78b032 to 9f77d52 Compare December 20, 2022 08:01

tejlmand requested changes Dec 20, 2022

View reviewed changes

kruithofa force-pushed the llcp_update_unittests branch 2 times, most recently from 049e545 to ff07730 Compare December 22, 2022 12:28

kruithofa requested a review from tejlmand December 22, 2022 12:49

tejlmand requested changes Jan 2, 2023

View reviewed changes

kruithofa force-pushed the llcp_update_unittests branch 5 times, most recently from 2dc3e63 to 802eabf Compare January 9, 2023 14:04

kruithofa requested a review from tejlmand January 9, 2023 15:12

tejlmand requested changes Jan 12, 2023

View reviewed changes

tejlmand mentioned this pull request Jan 12, 2023

unittest mock improvement and reusability. #53741

Open

kruithofa force-pushed the llcp_update_unittests branch from 802eabf to dc70d3c Compare January 12, 2023 13:10

kruithofa requested a review from tejlmand January 12, 2023 13:19

tejlmand requested changes Jan 16, 2023

View reviewed changes

kruithofa force-pushed the llcp_update_unittests branch from dc70d3c to 9767788 Compare January 17, 2023 10:38

kruithofa requested a review from tejlmand January 17, 2023 11:20

tejlmand approved these changes Jan 19, 2023

View reviewed changes

erbr-ot approved these changes Jan 19, 2023

View reviewed changes

thoh-ot approved these changes Jan 20, 2023

View reviewed changes

carlescufi merged commit f0c610c into zephyrproject-rtos:main Jan 20, 2023



		target_include_directories(mocks PUBLIC
		${ZEPHYR_BASE}/tests/bluetooth/controller/mock_ctrl/include

	${ZEPHYR_BASE}/tests/bluetooth/controller/mock_ctrl/include
	include

		@@ -0,0 +1,35 @@
		#
		# CMakeLists.txt file for creating of mocks library.

	${ZEPHYR_BASE}/subsys/bluetooth/controller/include
	PRIVATE
	${ZEPHYR_BASE}/subsys/bluetooth/controller/include

Bluetooth: controller: Llcp update unittests #52894

Bluetooth: controller: Llcp update unittests #52894

Conversation

kruithofa commented Dec 8, 2022 • edited Loading

tejlmand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tejlmand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tejlmand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlescufi commented Jan 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tejlmand left a comment

Choose a reason for hiding this comment

carlescufi commented Jan 19, 2023

erbr-ot left a comment

Choose a reason for hiding this comment

kruithofa commented Dec 8, 2022 •

edited

Loading