pkg/fff: Add fake functions framework package #17076
Conversation
jenswet
requested review from
aabadie,
cgundogan,
fjmolinas,
jia200x,
leandrolanzieri,
MichelRottleuthner,
miri64 and
smlng
as code owners
github-actions
bot
added
Area: doc
|
Looks good, please squash ! |
|
Hmm, I tried to build manually and it doesn't build on native and esp32 platforms: native (in docker)native (not in docker, with gcc 11.2)esp32It builds fine on arm, riscv and avr. |
Hmm okay. I only tried avr and arm so far. It works on ARM because all irq functions are defined inline. Because of that replacing the original header by our mock header in the test directory is enough. On other platforms these IRQ functions are obviously not defined inline. To mock them we'd either need the possibility to avoid compiling the original C files or make the functions weak. Any suggestions on what to do? For some cases the framework is still very helpful. E.g. for many peripherals, drivers, etc it's possible to avoid building their C files by not adding the corresponding USEMODULE. For others we'd need to adjust the build process to support mocking better. Should I just add different example for the package test or should we exclude some platforms from running the test? Thanks. |
|
The issue on ESP is that Conceptionally fff provides a mock implementation of a function instead of the real one. But with Maybe it is better to let the test example use some periph API without actually using that implementation? That would make sure that there are not two implementations of that functions available. To get this working with unmodified drivers, something like this could help: diff --git a/makefiles/features_modules.inc.mk b/makefiles/features_modules.inc.mk
index abde96c438..cb98b29872 100644
--- a/makefiles/features_modules.inc.mk
+++ b/makefiles/features_modules.inc.mk
@@ -11,7 +11,7 @@ USEMODULE += $(PERIPH_FEATURES)
# Add all USED periph_% init modules unless they are blacklisted
ifneq (,$(filter periph_init, $(USEMODULE)))
- PERIPH_IGNORE_MODULES := \
+ PERIPH_IGNORE_MODULES += \
periph_init% \
periph_common \
periph_rtc_rtt \And in the |
This will not work for Regarding the issues with Also: I think this might actually be a false positive from the preprocessor in the docker, rather than really |
I think that's probably better since IMHO fff can be really useful when testing a driver without the hardware. I agree that adding a way to exclude the mocked module from the build system is probably the best solution but I'm not sure if it should be done by extending |
|
+1 for |
|
I'd like to have @fjmolinas's opinion on |
I adjusted the example to mock some The new example compiles for native and esp. I tried both. I ran the test on a Nucleo board and that works too. Is that a better example? It doesn't require any Makefile changes so far. I also like the
Where would I need to add that for the package? Additionally I've added a note to the |
I would be in favor of a simpler solution that would fit this use case, and then open a separate PR/ISSUE to discuss this generic solution. There are already cases of mocked modules in the tree, and this will make this PR more complicated than it needs. |
Yes that's fine. My updated |
Note that diff --git a/Makefile.include b/Makefile.include
index f93bbb740a..4311e5b628 100644
--- a/Makefile.include
+++ b/Makefile.include
@@ -642,7 +642,7 @@ endif
LINKFLAGPREFIX ?= -Wl,
# Also build external modules
-DIRS += $(EXTERNAL_MODULE_PATHS)
+DIRS += $(filter-out $(PSEUDOMODULES),$(EXTERNAL_MODULE_PATHS))
# Define dependencies required for building (headers, downloading source files,)
BUILDDEPS += $(RIOTBUILD_CONFIG_HEADER_C) |
MrKevinWeiss
added
the
CI: ready for build
|
Lets see what murdock says now :) |
|
Two compilation failures still. One is failing to link due to too little RAM - just add that board to The other fails with two definitions of |
The Should we just exclude both boards from the build process? Is there only I can use a different mock example too, but it might be difficult the perfect one. |
|
You can use For the one with too little RAM use |
It would be preferable to use |
|
(unless of course it is, IISC as it is the case in this instance, just a single board from that CPU type that breaks the mold... Might point to a bug in its definitions actually, but in your case then |
|
Should I squash? |
Seem like it was the case at the end ;) |
Go ahead |
jenswet
force-pushed
the
feature/add-fff-package
branch
from
e6f5703
to
4f1d29f
Compare
Yes you had the right feeling. 😄
Done 👍 |
|
I was wondering something: instead of blacklisting boards that pull in periph_i2c by default, would it work to blacklist the i2c feature instead (with |
On a second thought, the proposal is stupid: that will exclude all boards that provide periph_i2c, so not a good idea at all :) |
It should only exclude boards that provide and use that feature. In fact, blacklisting the feature will result in the feature not being used on the blacklisted board, so this is indeed the better approach. (Optional features will only get included if they are not blacklisted and provided.) |
jenswet
force-pushed
the
feature/add-fff-package
branch
from
4f1d29f
to
14f90f3
Compare
|
Even though |
Maybe just blacklist that package on esp32 that would skip it from both build and test. |
2a13f07 broke the test. This prevents any UART output. Hello world is also unsuccessful on the ESP32 platform with this flag. What was the reason to add it? |
|
Any app that never calls |
17066: sys/irq: Add C++ wrapper using RAII r=maribu a=jenswet ### Contribution description This adds a C++ wrapper around the `irq.h` API. The wrapper uses RAII to accomplish a convenient and bug resistent use. A little background: I'm currently writing my master thesis on using C++ for embedded development, at the working group that `@maribu` is part of. For that I will try to add better C++ support to several parts of RIOT and then do some benchmarking and metrics to compare it with the C implementation. For example, I also plan to add a wrapper around i2c, a std::cout drop-in replacement and probably some more about networks or threads. ### Testing procedure I've added a unit test to verify that the IRQ wrapper calls the original `irq` functions as expected. As C++ and wrapper testing isn't done much so far in this project, I've added two additional headers to ease testing: 1. #17076 - fake functions framework, already merged 2. As there is no framework for C++ unit tests yet, I've added something for this too. Unfortunately the existing frameworks like GoogleTest, CppUTest or CppUnit don't easily compile for embedded or are difficult to integrate in to the RIOT build process. That's why I wrote some (simple) helper functions and macros inspired by the above frameworks. That allows to create C++ tests based on a fixture class with set up and tear down methods. It also allows some simple assertions and is easily extendable for other use cases. It wraps some of the fff functionality too. Both of this is obviously not required for the initial reason of this PR. But I'd like to provide unit tests for the features that I suggest to introduce where possible. So I'd appreciate some feedback on that too. If you'd prefer a PR without or different tests please let me know. You can run the test `irq_cpp` locally or on the CI to test the implementation. Please feel free to give feedback or suggest improvements! Co-authored-by: Jens Wetterich <jens@wetterich-net.de>
Contribution description
Add the fake functions framework for mocks in unit tests.
I'd like to use this in #17066. @aabadie suggested to make this a separate PR.
I need feedback on the
Kconfigsince I have no experience with that. It was copied from other packages.Testing procedure
Please have a look at the sample test
fff_test.Issues/PRs references
See also #17066.