Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BLE: Add cmake unittest fakes for BLE and events #14737

Merged
merged 8 commits into from
Jun 11, 2021

Conversation

paul-szczepanek-arm
Copy link
Member

Summary of changes

This adds fakes for unittests for BLE and events. This requires a change in roles configurations for the roles to default to on so that unittests don't need to manually set the configs.

Impact of changes

Migration actions required

Documentation

none


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)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[x] Tests / results supplied as part of this PR

Reviewers


@paul-szczepanek-arm
Copy link
Member Author

replaces: #14515

@paul-szczepanek-arm
Copy link
Member Author

paul-szczepanek-arm commented Jun 6, 2021

Used by ARMmbed/mbed-os-experimental-ble-services#61 to successfully run unittests, logs available here:
https://github.com/ARMmbed/mbed-os-experimental-ble-services/pull/61/checks?check_run_id=2756965842

@paul-szczepanek-arm paul-szczepanek-arm changed the title Add cmake unittest fakes for BLE and events BLE: Add cmake unittest fakes for BLE and events Jun 6, 2021
@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Jun 6, 2021
@ciarmcom ciarmcom requested a review from a team June 6, 2021 11:30
@ciarmcom
Copy link
Member

ciarmcom commented Jun 6, 2021

@paul-szczepanek-arm, thank you for your changes.
@ARMmbed/mbed-os-maintainers please review.

0xc0170
0xc0170 previously requested changes Jun 7, 2021
Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 styling nits, otherwise looks good to me.

As these are first "fakes", is it worth adding Readme to fakes or everyone knows what is there and how it works? Just wondering as we haven't got these before in the tree.


}

}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new line missing


}

#endif // BLE_MOCKS_H
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, new line

@mergify mergify bot dismissed 0xc0170’s stale review June 7, 2021 15:33

Pull request has been modified.

@paul-szczepanek-arm
Copy link
Member Author

Added newlines and a readme.


All the libraries available are in the `CMakeLists.txt` files in `mbed-os/UNITTESTS/stubs` and `mbed-os/UNITTESTS/fakes`.

The most common ones you might need are `mbed-os-fakes-ble` and `mbed-os-fakes-event-queue`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The most common ones you might need are `mbed-os-fakes-ble` and `mbed-os-fakes-event-queue`.

Shall we just remove this, I would add just that libraries are prefixed with mbed-os-fakes in the next section.


### Fakes

While stubs are self explanatory and don't offer anything beyond empty implementations, fakes allow for more complex unittests that can simulate whole subsystems.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a note, fakes library should be prefixed with mbed-os-fakes.

UNITTESTS/fakes/events/events/EventQueue.h Outdated Show resolved Hide resolved
#include "SecurityManagerImpl_mock.h"

/***
* You must use delete_mocks() at the end of the test. BLE::Instance(), ble::gap() etc. inits the mocks. Do not cache
Copy link
Contributor

@chrisswinchatt-arm chrisswinchatt-arm Jun 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the mocks be deleted automatically with RAII (or something else)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Due to how BLE instantiation is done which the mocks replace.

Co-authored-by: Chris Swinchatt <[email protected]>
@@ -0,0 +1,65 @@
## Stubs, mocks and fakes in Mbed OS
Copy link
Contributor

@rajkan01 rajkan01 Jun 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better we move this readme content into our official documentation https://github.com/ARMmbed/mbed-os-5-docs/blob/development/docs/debugging-testing/testing/unit_testing.md page

Note:
Already this PR ARMmbed/mbed-os-5-docs#1427 for updating unit testing content based on new unittest CMake build system

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already have a PR for documentation of ble unit testing: https://github.com/ARMmbed/mbed-os-5-docs/pull/1399/files

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I requested Readme to introduce stubs and fakes (I was thinking putting it into stubs/fakes folders). It can contain basic info and reference to our docs pages. Basically if I fetch the repo and start browsing sources, I can read readme and browse files and contribute.

@paul-szczepanek-arm Lets move this readme to your 1399 PR. It contains valuable information.

@rajkan01 Are you proposing to completely remove this readme (move everything to docs) ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so you don't want the readme in mbed-os?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@0xc0170 @paul-szczepanek-arm Yes, I have only removed the existing Readme from the UNITTESTS directory as part of Refactor unittest cmake PR as content gets duplicated and also official vs local readme content mismatch so better to move this readme content into the official Mbed OS documentation

@paul-szczepanek-arm
Copy link
Member Author

OK, I removed the readme, is this good to go now?

@0xc0170
Copy link
Contributor

0xc0170 commented Jun 11, 2021

CI started

@mbed-ci
Copy link

mbed-ci commented Jun 11, 2021

Jenkins CI Test : ✔️ SUCCESS

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

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-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-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️

@0xc0170 0xc0170 merged commit df12718 into ARMmbed:master Jun 11, 2021
@mergify mergify bot removed the ready for merge label Jun 11, 2021
@mbedmain mbedmain added release-version: 6.12.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Jun 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants