-
Notifications
You must be signed in to change notification settings - Fork 3k
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
BLE: Add cmake unittest fakes for BLE and events #14737
Conversation
replaces: #14515 |
Used by ARMmbed/mbed-os-experimental-ble-services#61 to successfully run unittests, logs available here: |
@paul-szczepanek-arm, thank you for your changes. |
There was a problem hiding this 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.
UNITTESTS/fakes/ble/BLE.cpp
Outdated
|
||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new line missing
UNITTESTS/fakes/ble/ble_mocks.h
Outdated
|
||
} | ||
|
||
#endif // BLE_MOCKS_H |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, new line
Added newlines and a readme. |
UNITTESTS/README.md
Outdated
|
||
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`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
UNITTESTS/README.md
Outdated
|
||
### 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. |
There was a problem hiding this comment.
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
.
#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 |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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]>
UNITTESTS/README.md
Outdated
@@ -0,0 +1,65 @@ | |||
## Stubs, mocks and fakes in Mbed OS |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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) ?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
OK, I removed the readme, is this good to go now? |
CI started |
Jenkins CI Test : ✔️ SUCCESSBuild Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
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
Test results
Reviewers