Refactor hal greentea cmake #15056
Refactor hal greentea cmake #15056
Conversation
Initially, every library greentea test has its project() creation in their CMake. As running greentea using CTest move all greentea test suite under one global project mbed-os and MBED_CONFIG_PATH set at the root mbed os CMake under the condition BUILD_GREENTEA_TESTS check so refactored mbed_hal greentea CMake accordingly.
Initially, every library greentea test has its project() creation in their CMake. As running greentea using CTest move all greentea test suite under one global project mbed-os and MBED_CONFIG_PATH set at the root mbed os CMake under the condition BUILD_GREENTEA_TESTS check so refactored mbed_hal_fpga_ci_test_shield greentea CMake accordingly.
Initially, every library greentea test has its project() creation in their CMake. As running greentea using CTest move all greentea test suite under one global project mbed-os and MBED_CONFIG_PATH set at the root mbed os CMake under the condition BUILD_GREENTEA_TESTS check so refactored mbed_timing_fpga_ci_test_shield greentea CMake accordingly.
Added CMake ctest support for pin_names greentea test
rajkan01
force-pushed
the
refactor_hal_greentea_cmake
branch
from
0545df4 to
44057a7
Compare
ciarmcom
added
the
release-type: patch
|
@rajkan01, thank you for your changes. |
Jenkins CI Test : ✔️ SUCCESSBuild Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
|
@ARMmbed/mbed-os-core Please review |
| set(TEST_SKIPPED "FPGA CI Test Shield is needed to run this test") | ||
| elseif(NOT("TARGET_FF_ARDUINO" IN_LIST MBED_TARGET_DEFINITIONS OR | ||
| "TARGET_FF_ARDUINO_UNO" IN_LIST MBED_TARGET_DEFINITIONS) AND | ||
| NOT DEFINED MBED_CONF_TARGET_DEFAULT_FORM_FACTOR) |
There was a problem hiding this comment.
| NOT DEFINED MBED_CONF_TARGET_DEFAULT_FORM_FACTOR) | |
| NOT "MBED_CONF_TARGET_DEFAULT_FORM_FACTOR" IN_LIST MBED_TARGET_DEFINITIONS) |
It's an Mbed configuration macro, not a CMake variable. You can only use DEFINED on CMake variables. Same comments for the other files.
| # This test is enabled via CMake command-line argument -DMBED_HAL_PIN_NAMES_GENERIC_TEST=ON. | ||
| if(NOT MBED_HAL_PIN_NAMES_GENERIC_TEST) | ||
| set(TEST_SKIPPED "Pin names generic test is not enabled for this Target") | ||
| endif() |
There was a problem hiding this comment.
@ARMmbed/mbed-os-core As per the PR description, this test can't be built or run if a target has no LED or button. Such check is not available to CMake, because pins are defined in a header file. As we okay with the idea of an additional CMake variable?
If we want to do this,
- Available options need to be documented in a place that's easier for users to see. I don't know if CMake: Update README.md with CTest/greentea instructions #15001 is a suitable place?
- We can add
option(MBED_HAL_PIN_NAMES_GENERIC_TEST "Enable pin names generic test" OFF)above, for readability.
There was a problem hiding this comment.
@ARMmbed/mbed-os-core As per the PR description, this test can't be built or run if a target has no LED or button. Such check is not available to CMake, because pins are defined in a header file. As we okay with the idea of an additional CMake variable?
If we want to do this,
Available options need to be documented in a place that's easier for users to see. I don't know if CMake: Update README.md with CTest/greentea instructions #15001 is a suitable place?
We can add
option(MBED_HAL_PIN_NAMES_GENERIC_TEST "Enable pin names generic test" OFF)above, for readability.
I had a look at this with @rajkan01 yesterday. Initially I suggested setting SKIP_REGULAR_EXPRESSION and emitting a trace message from the test to set the skipped state. However, @rajkan01 ran into a problem with this approach and it didn't work as expected. In that case an additional variable seemed reasonable (another option would be to add another field to targets.json to say "has_button" or similar, which didn't seem ideal at the time either).
Anyway, now I see the code, I'm not entirely convinced that forcing the user to set this is the best approach. One potential issue: how will CI know when to pass this cache variable? Maybe we have a solution for that, I don't know.
At this point I'm wondering if we should add another field to targets.json, so we can automate skipping/enabling this test. It's not the cleanest solution but it might be the best we can do. It wouldn't really be "test specific" config, as we're describing the available peripherals on the device. This particular test only runs if LED or BUTTON pins are available.
If we don't want to add an extra field to targets.json, and we decide the current approach is valid, i.e we want to add extra "user-facing" variables, we should of course document this clearly. We should also add the option so it shows up in ccmake and cmake-gui.
|
This pull request has automatically been marked as stale because it has had no recent activity. @rajkan01, please carry out any necessary work to get the changes merged. Thank you for your contributions. |
Summary of changes
Fixes #14947
Initially, every library greentea test has its project() creation in their CMake. As running greentea using CTest move all greentea test suite under one global project
mbed-osandMBED_CONFIG_PATHset at the root mbed os CMake under the conditionBUILD_GREENTEA_TESTScheck so refactored hal greentea CMake accordingly.Impact of changes
The pin_names generic greentea test previously skipped/included based on LED1 and BUTTON1 macros which are defined in targets pinNames.h header file but unfortunately this macro will not be visible at CMake configuration time to find out that test to skip/include, so introduced new CMake command-line argument MBED_HAL_PIN_NAMES_GENERIC_TEST to decide inclusion of pin_names generic test
Migration actions required
User requires to pass -D MBED_HAL_PIN_NAMES_GENERIC_TEST ON to the CMake command-line argument to enable pin_names generic test
Documentation
#15001
Pull request type
Test results
Reviewers