Unit tests: Move target_h/ stubs into libraries' test doubles #14929
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ciarmcom
added
the
release-type: patch
|
@LDong-Arm, thank you for your changes. |
LDong-Arm
force-pushed
the
move_target_h
branch
2 times, most recently
from
e399ebf
to
079538a
Compare
|
Fixed astyle. |
rajkan01
previously approved these changes
Jul 19, 2021
There was a problem hiding this comment.
Thanks for the changes @LDong-Arm , it looks good to me and needs to fix CI
rwalton-arm
previously approved these changes
Jul 20, 2021
|
The "pinvalidate" test is failing with the following output: |
mergify
bot
dismissed stale reviews from rajkan01 and rwalton-arm
Pull request has been modified.
LDong-Arm
commented
Jul 20, 2021
There was a problem hiding this comment.
@rajkan01 @rwalton-arm: Thanks for the reviews. The unit test stub PinNames.h didn't satisfy the latest requirements on pin names. I've added a commit to fix that, hopefully Travis will pass!
|
Rebased PR and fixed an actual issue in the PinName.h stub that failed Travis. |
|
@rajkan01 @rwalton-arm Travis now passes after I rebased the PR and fixed the |
|
This PR cannot be merged due to conflicts. Please rebase to resolve them. |
|
Rebased again |
Stubs previously in UNITTESTS/target_h/ have the same names as regular Mbed OS headers, intending to override the latter directly. We move platform target_h stubs into platform/tests/UNITTESTS/doubles/platform/. Note: nvic_wrapper.h is normally implemented and used by Mbed targets as needed. But as unit tests do not have a real target, we treat it as a stub for the platform.
The header `mbed.h` is a convenient wrapper that pre-includes some platform headers, for use by user applications. Libraries and tests internal to Mbed OS should not use it, and they should explicitly include headers they need. So a stub is not needed.
The stub header randLIB.h overrides the header of the same name in platform/randlib/ which is an external repository vendored into the mbed-os codebase. As the repository is synchronized regularly, it is better not to put overrides there, so we put the randLIB.h stub into the regular platform doubles directory.
Stubs previously in UNITTESTS/target_h/ have the same names as regular Mbed OS headers, intending to override the latter directly. We move hal target_h stubs into hal/tests/UNITTESTS/doubles/. Note: In Mbed OS, the standard include format requires each header to be prefixed with its module name, for example "hal/gpio_api.h". This requires headers to be organized in a module directory. But unit tests stubs for hal correspond to what a real Mbed target would have implemented (in a non-test scenario), and targets do not currently put headers in hal/, so we similary put stub headers directly in hal/tests/UNITTESTS/doubles/ instead of add a hal/ subdirectory there.
Individual libraries' `target_h` stub headers have now all been moved from `mbed-headers-base` to `mbed-headers-<library>`. Note: Even though headers previously in `target_h` are technically stubs/fakes too, they are used by not only unit tests but also regular libraries when compiled for unit tests, because no target-specific HAL implementation exists in this case. In order for regular library sources to pick up `target_h` headers, those headers must * have the same names as regular headers * appear first in include paths This is why those headers are part of `mbed-headers-<library>` and not `mbed-stubs-<library>`. Before this refactoring, `mbed-headers-base` was the first in unit tests' include paths.
The test script pinvalidate.py requires the following which are missing in the unit test stub PinName.h: * A comment "MBED TARGET LIST" * `CONSOLE_TX` and `CONSOLE_RX` in the `PinName` enum This commit adds them.
Run pinvalidate.py with -vvv to print the cause of failure, making it easier for authors of pull requests to fix pin names.
LDong-Arm
force-pushed
the
move_target_h
branch
from
205d01f
to
81eb6cb
Compare
|
The CI failure was due to missing |
Jenkins CI Test : ❌ FAILEDBuild Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
|
CI started |
Jenkins CI Test : ✔️ SUCCESSBuild Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
Patater
approved these changes
Aug 3, 2021
0xc0170
approved these changes
Aug 3, 2021
mbedmain
added
release-version: 6.14.0
Release-pending
and removed
release-type: patch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of changes
Fixes: #14799
This PR moves stubs headers from
UNITTESTS/target_h/into individual libraries' ownUNITTESTS/doubles/, similar to what we did withUNITTESTS/stubs/. The CMake targets that contain those headers are changed frommbed-headers-basetombed-headers-<library>.Note: Even though headers previously in
target_hare technically stubs/fakes too, they are used by not only unit tests but also regular libraries when compiled for unit tests, because no target-specific HAL implementation exists in this case. In order for regular library sources to pick uptarget_hheaders, those headers mustThis is why those headers are part of
mbed-headers-<library>and notmbed-stubs-<library>. Before this refactoring,mbed-headers-basewhich containedtarget_hwas the first in all unit tests' include paths.To avoid duplicating the above explanation in every commit, it is only in the commit message of the last commit "Unit tests: Remove redundant CMake target mbed-headers-base".
Impact of changes
Migration actions required
Documentation
None
Pull request type
Test results
Reviewers
@ARMmbed/mbed-os-core