CMake: Refactor UNITTESTS CMake #14426

rajkan01 · 2021-03-15T10:28:25Z

Summary of changes

All the subs cpp files are grouped into their respective libraries like mbed-stubs-connectivity, mbed-stubs-drivers, mbed-stubs-events, mbed-stubs-hal, mbed-stubs-platform, mbed-stubs-rtos, mbed-stubs-storage so all libraries(drivers, platform etc.,) unittest CMakeLists.txt target link aginst to specifically required stub lib for their test or use mbed-stubs libraries which intern target linked to all stub libraries
All the Mbed OS headers are also grouped into header libraries like mbed-headers-base, mbed-headers-platform, mbed-headers-connectivity, mbed-headers-storage, mbed-headers-drivers, mbed-headers-hal, mbed-headers-events, mbed-headers-rtos so all libraries (drivers, platform etc.,) unit test CMakeLists.txt target link aginst to specifically required header interface lib or use mbed-headers libraries which intern target linked with all specific headers libraries
Add the command line argument BUILD_TESTING to ON/OFF Mbed OS unittest build

Impact of changes

As part of this PR, we migrated to use the latest version CMake 3.19 or above version to build unit tests and the user must update their CMake to the latest version stated above

Migration actions required

Previous unittest build	New way of unittest build
The unittest CMake build discover the library unittest by searching the unittest.cmake file across Mbed OS and build	Every library CMakeLists.txt ( For ex: mbed-os/platform/CMakeLists.txt) will also add the UNITTESTS directory into the build based on NOT CMAKE_CROSSCOMPILING guard and normally all library directory gets added from mbed os root.
Every library unittest unittest.cmake used to set includes, sources, test sources, test flags using unittest-includes, unittest-sources, unittest-test-sources, unittest-test-flags CMake variable	Every library unittest has its CMakeLists.txt file a kind of application build CMake where it calls CMake standard calls target_sources, target_link_libraries, add_executable etc.,

Note:
Refer to official Mbed OS latest debug-test/unit-testing documentation Example section which explains how to write a unittest for example class in a new way

Documentation

ARMmbed/mbed-os-5-docs#1427

Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@hugueskamba @0xc0170 @evedon @LDong-Arm @pan- @paul-szczepanek-arm @jamesbeyond @rwalton-arm @Patater @donatieng

ciarmcom · 2021-03-15T10:30:25Z

@rajkan01, thank you for your changes.
@hugueskamba @rwalton-arm @0xc0170 @Patater @pan- @jamesbeyond @paul-szczepanek-arm @evedon @donatieng @LDong-Arm @ARMmbed/mbed-os-connectivity @ARMmbed/mbed-os-test @ARMmbed/mbed-os-hal @ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

paul-szczepanek-arm

Herculean task (akin to cleaning up the Augean stables).

UNITTESTS/CMakeLists.txt

storage/kvstore/tdbstore/tests/UNITTESTS/TDBStore/CMakeLists.txt

Pull request has been modified.

0xc0170 · 2021-05-12T14:21:13Z

Also worth mentioning users need CMake 3.19 or above to build unit tests, even if they use Mbed CLI 1.

sounds like it should be in "Impact of changes". @rajkan01 Please let us know once the fist comment is updated, we should review before merging - there could be lot of useful information our users need.

donatieng

Thanks @rajkan01! Lots of good stuff here.

One slight concern from my standpoint: stubs and headers are defined "globally" across the OS, as opposed to the relevant component directory. The risk is that it becomes harder to maintain these components (as you have to change the stubs which are in a different place - which is not obvious, and there is duplication in terms of listing the relevant header files for instance).

Otherwise I see you've dropped the documentation, would be good to add something back except if I've missed something else.

Finally, more of a question - what's the impact (if any) on CLI1?

CMakeLists.txt

donatieng · 2021-05-13T14:36:24Z

CMakeLists.txt

+if(${CMAKE_CROSSCOMPILING})
+    add_subdirectory(targets)
+    # The directories below contain optional target libraries
+    add_subdirectory(connectivity EXCLUDE_FROM_ALL)


Same, I understand why you're doing that but it's adding a bit of un-necessary complexity, as we should assume that most code we maintain will eventually have unit tests. Any reason why we can't remove EXCLUDE_FROM_ALL in any case?

Yes most of the components have unit test and we want to include to the build when we invoke unit test cmake command but in case of application build (Cross compiling), we would like to exclude to the build by default. When the application cmake target_link_libraries to any of the excluded component then it gets added into the build.

I see why but this has implications beyond unit tests. I would suggest the specific unit testing targets are excluded from the all target in the appropriate places, not at the top level here.

If we remove EXCLUDE_FROM_ALL then all the added CMake targets will be compiled when we try to build the CMake all target (mbed-tools tries to build the all target as it has no knowledge of CMake target names). Using EXCLUDE_FROM_ALL here allows us to specify which components of Mbed are built by using target_link_libraries in the consuming application. This isn't an ideal set-up but it was a pre-existing issue. I'd suggest we add an issue to the CMake milestone to tidy up here.

I'd suggest we add an issue to the CMake milestone to tidy up here.

@rwalton-arm can you create an issue please for this?

UNITTESTS/mbed_unittest.py

UNITTESTS/stubs/CMakeLists.txt

rajkan01 · 2021-05-14T09:49:44Z

Thanks @rajkan01! Lots of good stuff here.

One slight concern from my standpoint: stubs and headers are defined "globally" across the OS, as opposed to the relevant component directory. The risk is that it becomes harder to maintain these components (as you have to change the stubs which are in a different place - which is not obvious, and there is duplication in terms of listing the relevant header files for instance).

Otherwise I see you've dropped the documentation, would be good to add something back except if I've missed something else.

Finally, more of a question - what's the impact (if any) on CLI1?

@donatieng Thanks for your review comments
Readme in the mbed-os/UNITTESTS is a duplicate copy of official mbed documentation https://github.com/ARMmbed/mbed-os-5-docs/blob/development/docs/debugging-testing/testing/unit_testing.md so I just removed mbed-os/UNITTESTS/Readme.md and user can refer directly https://os.mbed.com/docs/mbed-os/v6.8/debug-test/unit-testing.html

No impact in Mbed CLI1 as mbed CLI 1 just directly calls the python modules in mbed-os/UNITTESTS/*.py for build and run the test when the user calls mbed test -unittests command

0xc0170 · 2021-05-18T12:41:07Z

Back to review. Hopefully we can merge this once we release this week and unblock master.

Pull request has been modified.

donatieng

Thanks for the additional comments and rework @rajkan01 and @rwalton-arm.
Let's get this in as it's a good starting point, but please raise issues for what needs to happen next :). Great work!

rajkan01 · 2021-05-21T11:28:38Z

Thanks for the additional comments and rework @rajkan01 and @rwalton-arm.
Let's get this in as it's a good starting point, but please raise issues for what needs to happen next :). Great work!

Thanks, Don for reviewing and approval this big PR

0xc0170 · 2021-05-21T12:41:52Z

CI restarted

mbed-ci · 2021-05-21T13:10:29Z

Jenkins CI Test : ✔️ SUCCESS

Build Number: 4 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_greentea-test	✔️

0xc0170 · 2021-05-21T13:25:14Z

We will merge this once master is unblocked (release made) - it should be on Monday

mergify · 2021-05-24T12:05:04Z

This PR does not contain release version label after merging.

rajkan01 requested a review from lorjala March 15, 2021 10:28

ciarmcom requested review from 0xc0170, donatieng, evedon, hugueskamba, jamesbeyond, LDong-Arm, pan-, Patater, paul-szczepanek-arm, rwalton-arm and a team March 15, 2021 10:30

ciarmcom added needs: review component: tools component: core component: connectivity component: test component: hal labels Mar 15, 2021

mergify bot added needs: work and removed needs: review labels Mar 15, 2021

paul-szczepanek-arm previously approved these changes Mar 16, 2021

View reviewed changes

UNITTESTS/CMakeLists.txt Show resolved Hide resolved

mergify bot added needs: CI and removed needs: work labels Mar 16, 2021

paul-szczepanek-arm reviewed Mar 16, 2021

View reviewed changes

storage/kvstore/tdbstore/tests/UNITTESTS/TDBStore/CMakeLists.txt Outdated Show resolved Hide resolved

0xc0170 removed the request for review from a team March 16, 2021 10:26

ghost mentioned this pull request May 13, 2021

[BLE] When pairing with Android device (Android 10) the pairing request has to be confirmed twice #14654

Open

donatieng previously requested changes May 13, 2021

View reviewed changes

mergify bot added needs: work and removed needs: review labels May 13, 2021

rajkan01 requested a review from donatieng May 14, 2021 09:50

0xc0170 added needs: review and removed needs: work labels May 18, 2021

CMake: Move CMAKE_CROSSCOMPILING check inside library CMake

6d4caad

rajkan01 force-pushed the feature_unittest_refactor branch from bf2ed7d to 6d4caad Compare May 20, 2021 13:26

donatieng approved these changes May 21, 2021

View reviewed changes

mergify bot added needs: CI and removed needs: review labels May 21, 2021

mergify bot added ready for merge and removed needs: CI labels May 21, 2021

adbridge merged commit 4e586a9 into master May 24, 2021

adbridge removed the ready for merge label May 24, 2021

mergify bot added the release version missing When PR does not contain release version, bot should label it and we fix it afterwards label May 24, 2021

0xc0170 added release-version: 6.10.0 and removed release version missing When PR does not contain release version, bot should label it and we fix it afterwards labels Jun 22, 2021

0xc0170 deleted the feature_unittest_refactor branch October 5, 2021 08:23

CMake: Refactor UNITTESTS CMake #14426

CMake: Refactor UNITTESTS CMake #14426

Uh oh!

Conversation

rajkan01 commented Mar 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

ciarmcom commented Mar 15, 2021

Uh oh!

paul-szczepanek-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

0xc0170 commented May 12, 2021

Uh oh!

donatieng left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

donatieng May 13, 2021

Choose a reason for hiding this comment

Uh oh!

rajkan01 May 14, 2021

Choose a reason for hiding this comment

Uh oh!

donatieng May 18, 2021

Choose a reason for hiding this comment

Uh oh!

rwalton-arm May 20, 2021

Choose a reason for hiding this comment

Uh oh!

0xc0170 May 21, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rajkan01 commented May 14, 2021

Uh oh!

0xc0170 commented May 18, 2021

Uh oh!

donatieng left a comment

Choose a reason for hiding this comment

Uh oh!

rajkan01 commented May 21, 2021

Uh oh!

0xc0170 commented May 21, 2021

Uh oh!

mbed-ci commented May 21, 2021

Jenkins CI Test : ✔️ SUCCESS

Build Number: 4 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

Uh oh!

0xc0170 commented May 21, 2021

Uh oh!

mergify bot commented May 24, 2021

Uh oh!

Uh oh!

rajkan01 commented Mar 15, 2021 •

edited

Loading