zephyr icon indicating copy to clipboard operation
zephyr copied to clipboard

cmake: kconfig: introduce dedicated unit testing board and support Kconfig for unit tests

Open tejlmand opened this issue 2 years ago • 9 comments

This PR introduces a dedicated unit testing board.

Today, a dedicated Zephyr unit testing scheme exists but is different from how a Zephyr build generally works.

For example Kconfig is not possible, resulting on various different hacks to pass Kconfig settings from test cases / testcase.yaml through CMake to the code.

Some directly as compile definitions, some as header files with forced inclusion on sources, some with wrapper flags which again results in different define being enabled. There is even cases where a second forced header inclusion undefines previous defines.

Unit test often does a manual check for the right boards, like this:

if (NOT BOARD STREQUAL unit_testing)
    message(FATAL_ERROR "This project can only be used with...")
endif()

Introducing a dedicated unit_testing board under tests/root allows us to use Kconfig in unit test samples, and thus proper prj.conf and extra Kconfig fragments. Generation of autoconf.h so the overall architecture follows regular Zephyr builds.

Proper and uniform error messages when invalid board is selected.

The unit_testing board and arch is located under: tests/root so that it is only available when find_package(Zephyr COMPONENTS unittest) is used, and not available for regular Zephyr builds.

Signed-off-by: Torsten Rasmussen [email protected]

tejlmand avatar Aug 05 '22 09:08 tejlmand

@ahmedmoheb-nordic cannot add you as reviewer, but please test this PR and give comments.

tejlmand avatar Aug 05 '22 09:08 tejlmand

@ppryga and @thoh-ot this should allow you to cleanup what was in the thread here: https://github.com/zephyrproject-rtos/zephyr/pull/40073#issuecomment-1041451330

Would be great if you can try it out just on a few existing test cases that was merged in #40073 so we can know if more is needed in this PR.

tejlmand avatar Aug 05 '22 09:08 tejlmand

@tejlmand I'll try to find some time to look at this PR and do some tests next week.

ppryga-nordic avatar Aug 05 '22 10:08 ppryga-nordic

@tejlmand Very awesome, I will give it a spin next week

thoh-ot avatar Aug 05 '22 10:08 thoh-ot

@tejlmand this is great, I was 1/2 way doimg the exact same thing. Thank you!!!!

yperess avatar Aug 05 '22 13:08 yperess

In c373900#:~:text=Kconfig%20file%2C%20that%27s-,there,-choice%20and%20they you meant their not there.

@jeremybettis I sure did. The good old classic of you fingers typing something else than your thinking. Fixed, and now your +1 is gone 😞

tejlmand avatar Aug 05 '22 19:08 tejlmand

@ahmedmoheb-nordic cannot add you as reviewer, but please test this PR and give comments.

@tejlmand I think there is a problem that not all defined configuration parameters were added as a compilation defines.

I added the following configurations with a unit testing project which should make CONFIG_BT_HCI_ACL_FLOW_CONTROL defined in source files

CONFIG_BT=y CONFIG_BT_HCI=y CONFIG_BT_PERIPHERAL=y CONFIG_BT_CENTRAL=y CONFIG_BT_HCI_ACL_FLOW_CONTROL=y

when I added the following code to one of the test function, it was compiled and passed without reporting any issues #ifdef CONFIG_BT_HCI_ACL_FLOW_CONTROL #error "This isn't allowed CONFIG_BT_HCI_ACL_FLOW_CONTROL" #endif

I tested the same approach with the twister integration project at /samples/subsys/testsuite/integration and the compilation process was teminated due to hitting #error

ahmedmoheb-nordic avatar Aug 09 '22 11:08 ahmedmoheb-nordic

@tejlmand I think there is a problem that not all defined configuration parameters were added as a compilation defines.

Seems autoconf.h is not applied on sources being compiled. Looking into this.

DNM for now.

tejlmand avatar Aug 09 '22 12:08 tejlmand

Seems autoconf.h is not applied on sources being compiled. Looking into this.

DNM for now.

fixed

tejlmand avatar Aug 10 '22 09:08 tejlmand

@tejlmand seems there are a few things missing from linking.

yperess avatar Aug 10 '22 15:08 yperess

@tejlmand seems there are a few things missing from linking.

yes, it seems that the autoconf.h is impacting test when building for 64 bit. Not had time to investigate fully.

tejlmand avatar Aug 10 '22 15:08 tejlmand

@tejlmand seems there are a few things missing from linking.

yes, it seems that the autoconf.h is impacting test when building for 64 bit. Not had time to investigate fully.

it was actually the assert, just got tricked because the tests I initially looked where this happened was 64bit builds of cbprintf, not it's 32 bit counterpart.

Should be fixed now.

tejlmand avatar Aug 11 '22 06:08 tejlmand

@tejlmand seems there are a few things missing from linking.

yes, it seems that the autoconf.h is impacting test when building for 64 bit. Not had time to investigate fully.

it was actually the assert, just got tricked because the tests I initially looked where this happened was 64bit builds of cbprintf, not it's 32 bit counterpart.

Should be fixed now.

Can we instead set the default to assert if the board is not the unit_test board?

yperess avatar Aug 11 '22 15:08 yperess

Can we instead set the default to assert if the board is not the unit_test board?

I was starting to think the same, so that is now done.

tejlmand avatar Aug 12 '22 10:08 tejlmand

Wonder if subsys/testsuite/{arch,board,soc} should be moved down one level. It looks weird if arch, board and soc appear alongside coverage and ztest.

dcpleung avatar Aug 12 '22 23:08 dcpleung

Wonder if subsys/testsuite/{arch,board,soc} should be moved down one level. It looks weird if arch, board and soc appear alongside coverage and ztest.

Good question, although i'm unsure what the name should be. Afterall, the real Zephyr folders arch|soc|boards are at the top-level of Zephyr repo.

I used /tests/root/[boards|arch|soc] in first version, but never really liked it (especially because the root part), so was happy with the /subsys/testsuite proposal.

tejlmand avatar Aug 15 '22 06:08 tejlmand

Seems autoconf.h is not applied on sources being compiled. Looking into this. DNM for now.

fixed

I tested it and it's working now

ahmedmoheb-nordic avatar Aug 15 '22 09:08 ahmedmoheb-nordic

Wonder if subsys/testsuite/{arch,board,soc} should be moved down one level. It looks weird if arch, board and soc appear alongside coverage and ztest.

Good question, although i'm unsure what the name should be. Afterall, the real Zephyr folders arch|soc|boards are at the top-level of Zephyr repo.

I used /tests/root/[boards|arch|soc] in first version, but never really liked it (especially because the root part), so was happy with the /subsys/testsuite proposal.

Maybe subsys/testsuite/overlay/{arch,board,soc}? These configs can be considered overlays on top of the top level arch,board,soc configs.

dcpleung avatar Aug 15 '22 15:08 dcpleung

Maybe subsys/testsuite/overlay/{arch,board,soc}? These configs can be considered overlays on top of the top level arch,board,soc configs.

But they are not overlays, not in any way. Starting to refer to them as overlays or in such folder will really add to confusion.

This is an independent arch, soc, boards tree.

If I try to build for a wrong board in a regular Zephyr CMake build, I will be printed with a list of valid boards under their respective archs like this:

-- Board: invalid_board
No board named 'invalid_board' found.

Please choose one of the following boards:

arc:
  em_starterkit
  em_starterkit_em11d
  em_starterkit_em7d
  em_starterkit_em7d_v22
....
arm:
  96b_aerocore2
  96b_argonkey
  96b_avenger96
...

but the board unit_testing is not in that list, because it's not a valid board in a normal project.

But it's a valid board in unit testing, so if I do the same on a unit testing project like: https://github.com/zephyrproject-rtos/zephyr/tree/main/tests/unit/crc I get this:

-- Board: invalid_board
No board named 'invalid_board' found.

Please choose one of the following boards:

unit_testing:
  unit_testing

and none of the regular boards are available.

That is because it's a completely independent tree of archs and boards.

tejlmand avatar Aug 16 '22 08:08 tejlmand

@yperess and @jeremybettis can I get your +1 back :smile:

tejlmand avatar Aug 16 '22 08:08 tejlmand

Ah... I thought the unit_testing board would appear in the general board list. In that case, I think we can go with the current location.

dcpleung avatar Aug 16 '22 14:08 dcpleung

@tejlmand I found another issue related to not including the autoconf.h while building sources.

For one of the unit test projects, I tried to build a simple library by adding one of the mocking files net_buf.c by adding the followings to one of the projects CMakeLists.txt

zephyr_library() zephyr_library_compile_definitions(ZTEST_UNITTEST) zephyr_library_include_directories(${CMAKE_BINARY_DIR}/zephyr/include/generated) zephyr_library_include_directories(${ZEPHYR_BASE}/subsys/testsuite/include) zephyr_library_include_directories(${ZEPHYR_BASE}/subsys/testsuite/include/zephyr) zephyr_library_include_directories(${ZEPHYR_BASE}/subsys/testsuite/ztest/include) zephyr_library_include_directories(${ZEPHYR_BASE}/subsys/testsuite/ztest/include/zephyr) zephyr_library_include_directories(${ZEPHYR_BASE}/include) zephyr_library_sources(${ZEPHYR_BASE}/tests/bluetooth/host/buf/mocks/net_buf.c)

Trying to compile, it gives several errors like

  • error: #error "You need to add CONFIG_ZTEST to your config file."
  • zephyrproject/zephyr/include/zephyr/toolchain/gcc.h:554:2: error: #error processor architecture not supported

I think the problem is that the file autoconf.h isn't added while compiling the file net_buf.c.

ahmedmoheb-nordic avatar Aug 16 '22 19:08 ahmedmoheb-nordic

I think the problem is that the file autoconf.h isn't added while compiling the file net_buf.c.

The unit_testing has always been a very special use-case / project, so it has never followed the regular Zephyr CMake approach. Therefore the zephyr_interface is probably not populated. Also before this PR, the Zephyr extension commands were not available either.

Wonder if we should populate zephyr_interface or leave the unit tests more pure, and thus request users creating libraries to append such flags like this:

target_compile_options(<library> PRIVATE -imacros ${AUTOCONF_H})

or

zephyr_library_compile_options(-imacros ${AUTOCONF_H})

This is not directly related to Kconfig and unit_testing board (this PR), so would like to do some thinking and create a dedicated PR for a proposal on populating zephyr_interface with such flags.

tejlmand avatar Aug 17 '22 07:08 tejlmand

@jeremybettis could you take another look please?

carlescufi avatar Aug 18 '22 10:08 carlescufi

@jeremybettis could you take another look please?

nvm, I see you already approved earlier.

carlescufi avatar Aug 18 '22 12:08 carlescufi