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

tests: mcuboot: add "depends_on: mcuboot" #79924

Closed

Conversation

butok
Copy link
Collaborator

@butok butok commented Oct 16, 2024

  • Adds "depends_on: mcuboot" to the test_mcuboot testcase.yaml.
  • Adds "supported: mcuboot" to the board yaml files, found in the testcase.yaml platform_allow list.
  • Avoid appending of the platform_allow list for new platforms.

- Adds "depends_on: mcuboot" to the test_mcuboot testcase.yaml.
- Adds "supported: mcuboot" to the board yaml files,
  found in the testcase.yaml platform_allow list.
- Avoid appending of the platform_allow list for new platforms.

Signed-off-by: Andrej Butok <andrey.butok@nxp.com>
Copy link
Collaborator

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

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

No let's not add a tag that has nothing to do with the boards

@butok
Copy link
Collaborator Author

butok commented Oct 16, 2024

Hi @nordicjm

No let's not add a tag that has nothing to do with the boards

Nothing wrong. This the supported board feature.
For example, it's possible to find other SW features as "nvs" and "tinycrypt" in board yaml files.

This is the way how avoid growing platform_allow list for mcuboot specific samples/tests that was blocked by @nashif here #78944

So @nashif and @nordicjm please provide your solution and unblock any of PRs.

@nordicjm
Copy link
Collaborator

Hi @nordicjm

No let's not add a tag that has nothing to do with the boards

Nothing wrong. This the supported board feature. For example, it's possible to find other SW features as "nvs" and "tinycrypt" in board yaml files.

This is the way how avoid growing platform_allow list for mcuboot specific samples/tests that was blocked by @nashif here #78944

So @nashif and @nordicjm please provide your solution and unblock any of PRs.

He said there that you are adding a lot of boards to a sysbuild test, which adds a strain to the CI because it then has to build all of them. You're doing the same thing here, so I'm not sure why you think if that got rejected that this would be accepted?

@butok
Copy link
Collaborator Author

butok commented Oct 16, 2024

Hi @nordicjm

No let's not add a tag that has nothing to do with the boards

Nothing wrong. This the supported board feature. For example, it's possible to find other SW features as "nvs" and "tinycrypt" in board yaml files.
This is the way how avoid growing platform_allow list for mcuboot specific samples/tests that was blocked by @nashif here #78944
So @nashif and @nordicjm please provide your solution and unblock any of PRs.

He said there that you are adding a lot of boards to a sysbuild test, which adds a strain to the CI because it then has to build all of them. You're doing the same thing here, so I'm not sure why you think if that got rejected that this would be accepted?

Yes, this is the goal:

  • to test supported boards.
  • It is used by west, otherwise boards are skipped and not tested (+ @hakehuang)
  • It can also be used to filter examples supported by a board.
    Guess, this function is used by the Nordic nRFConnect VSCode sample filter.

@butok butok requested review from hakehuang and nashif and removed request for marwaiehm-st and mathieuchopstm October 16, 2024 12:59
@danieldegrasse
Copy link
Collaborator

No let's not add a tag that has nothing to do with the boards

If I understand @nashif's block on #78944, the issue there is that we are attempting to use a sample as a test, right? For this PR though, the goal is to expand the test coverage of the MCUBoot test. Personally I would love if we had a better solution than adding mcuboot to the platform's support tags, but we can't use twister filters here reliably (since boards that don't support MCUBoot will fail during the devicetree stage of cmake, before the filter runs)

I'm unclear what the better solution here would be. The goal of the mcuboot test is to provide a contained testcase that verifies the platform can boot an image with MCUBoot, and run an image upgrade. Why would we want to limit the platform coverage for that testcase?

@hakehuang
Copy link
Collaborator

  • It is used by west, otherwise boards are skipped and not tested (+ @hakehuang)

I agree this will help a lot. and simply the tracking problem.

@PerMac
Copy link
Member

PerMac commented Oct 17, 2024

I find supported/depends_on a better solution than platform_allow. It has a better scaling - 1 entry in platform yaml vs adding platform to every allow list.

@butok
Copy link
Collaborator Author

butok commented Oct 17, 2024

I find supported/depends_on a better solution than platform_allow. It has a better scaling - 1 entry in platform yaml vs adding platform to every allow list.

Thank you.

The proposed approach is clean,
and will avoid updating paltform_allow lists for each new platform for all mcuboot dependent samples/tests.

@decsny
Copy link
Member

decsny commented Oct 17, 2024

For what it's worth, I also don't understand what's the issue with increasing coverage of the sample, even if it's not used for testing, what is wrong with making sure the sample works on all those platforms? It's pretty important actually for vendors since users will go to the sample to see how to use something and try out the board. And if there is some uncaught issue related to the sample only for some platform, that we don't know about, because somebody blocked adding coverage of more platforms to that sample over some philosophy and an aesthetic distaste for the new content in the yaml file, that's a problem.

@butok
Copy link
Collaborator Author

butok commented Oct 18, 2024

Hi @nordicjm
This is currently the best way to filter mcuboot examples.
Waiting for your unlock to proceed (twister yaml are used for our regular testing on real platforms).

@nashif
Copy link
Member

nashif commented Oct 18, 2024

For example, it's possible to find other SW features as "nvs" and "tinycrypt" in board yaml files.

just because someone made a mistake and added something in a board yaml file, does not make it right and does not make it a rule. tinycrypt does not make any sense in a board as a supported feature for example, same for NVS probably. need to clean this.

This is the way how avoid growing platform_allow list for mcuboot specific samples/tests that was blocked by @nashif here #78944

Isn't like evey other board in zephyr potentially bootable with mcuboot? Are we going to have majority of boards adding this?

I welcome the removal of the long platform_allow and in general I think this is good, however I want to pause here and have us think of a better way to do this that does not involvie putting 'mucboot' as a supported feature of a board, but can go with this if there are no other choices.

@butok
Copy link
Collaborator Author

butok commented Oct 18, 2024

I welcome the removal of the long platform_allow and in general I think this is good, however I want to pause here and have us think of a better way to do this that does not involvie putting 'mucboot' as a supported feature of a board, but can go with this if there are no other choices.

Thank you @nashif for your feedback, looking forward for your final decision.

@hakehuang, as the chair of the Zephyr Testing Working Group, could you also give your feedback:

  • If "supported: list of features supported by this board" can contain "mcuboot" as a feature?
  • If there is another better solution to avoid the growing platform_allow list?

@butok
Copy link
Collaborator Author

butok commented Oct 18, 2024

same for NVS probably. need to clean this.

Hi @nashif
please decide what to do with the board's "nvs" (non-volatile storage) feature.
There are already samples and tests that depend on it:

\zephyr\samples\modules\canopennode\sample.yaml 
	Line 18:     depends_on: nvs
	Line 21:     depends_on: nvs
\zephyr\samples\subsys\nvs\sample.yaml 
	Line  7:     depends_on: nvs
\zephyr\tests\subsys\settings\nvs\testcase.yaml 
	Line 3:     depends_on: nvs

@danieldegrasse
Copy link
Collaborator

Isn't like evey other board in zephyr potentially bootable with mcuboot? Are we going to have majority of boards adding this?

I welcome the removal of the long platform_allow and in general I think this is good, however I want to pause here and have us think of a better way to do this that does not involvie putting 'mucboot' as a supported feature of a board, but can go with this if there are no other choices.

The core issue here (as far as I understand) is that twister's filtering step attempts to run the CMake phase of the build, to generate a .config and zephyr.dts file to use for filtering. MCUBoot will fail at the CMake phase for any board that does not define a set of flash partitions, as the zephyr,code-partition is set by an app overlay for MCUBoot. So we can't just use a filter currently.

One option I believe we considered in the past was allowing twister to filter based on a "base" build, without overlays. This introduces a new set of issues though, because some tests do need board overlays pulled in to satisfy the filter settings.

@nordicjm
Copy link
Collaborator

nordicjm commented Oct 22, 2024

Isn't like evey other board in zephyr potentially bootable with mcuboot? Are we going to have majority of boards adding this?
I welcome the removal of the long platform_allow and in general I think this is good, however I want to pause here and have us think of a better way to do this that does not involvie putting 'mucboot' as a supported feature of a board, but can go with this if there are no other choices.

The core issue here (as far as I understand) is that twister's filtering step attempts to run the CMake phase of the build, to generate a .config and zephyr.dts file to use for filtering. MCUBoot will fail at the CMake phase for any board that does not define a set of flash partitions, as the zephyr,code-partition is set by an app overlay for MCUBoot. So we can't just use a filter currently.

One option I believe we considered in the past was allowing twister to filter based on a "base" build, without overlays. This introduces a new set of issues though, because some tests do need board overlays pulled in to satisfy the filter settings.

Why does filtering on slot0 and slot1 partitions existing not work or has this not been tried? E.g.

filter: dt_nodelabel_enabled("slot0_partition") and dt_nodelabel_enabled("slot1_partition")

@danieldegrasse
Copy link
Collaborator

Why does filtering on slot0 and slot1 partitions existing not work or has this not been tried? E.g.

Twister runs the cmake configuration phase in order to run filtering, right? If we run the cmake configuration phase for mcuboot on a board that does not define flash partitions in devicetree, it will fail. For example: west build -p always -b faze ../bootloader/mcuboot/boot/zephyr --cmake-only (faze board has no flash partitions)

@nordicjm
Copy link
Collaborator

nordicjm commented Oct 23, 2024

Why does filtering on slot0 and slot1 partitions existing not work or has this not been tried? E.g.

Twister runs the cmake configuration phase in order to run filtering, right? If we run the cmake configuration phase for mcuboot on a board that does not define flash partitions in devicetree, it will fail. For example: west build -p always -b faze ../bootloader/mcuboot/boot/zephyr --cmake-only (faze board has no flash partitions)

That is not a test in zephyr and that's not the test being changed here, I made this changes:

diff --git a/tests/boot/test_mcuboot/testcase.yaml b/tests/boot/test_mcuboot/testcase.yaml
index 630e3ac6b63..21873857e6d 100644
--- a/tests/boot/test_mcuboot/testcase.yaml
+++ b/tests/boot/test_mcuboot/testcase.yaml
@@ -12,44 +12,4 @@ common:
 tests:
   bootloader.mcuboot:
     tags: mcuboot
-    platform_allow:
-      - frdm_k22f
-      - frdm_k64f
-      - frdm_k82f
-      - frdm_ke17z
-      - frdm_ke17z512
-      - rddrone_fmuk66
-      - twr_ke18f
-      - twr_kv58f220m
-      - frdm_mcxn947/mcxn947/cpu0
-      - lpcxpresso55s06
-      - lpcxpresso55s16
-      - lpcxpresso55s28
-      - lpcxpresso55s36
-      - lpcxpresso55s69/lpc55s69/cpu0
-      - mimxrt1010_evk
-      - mimxrt1015_evk
-      - mimxrt1020_evk
-      - mimxrt1024_evk
-      - mimxrt1040_evk
-      - mimxrt1050_evk
-      - mimxrt1060_evk
-      - mimxrt1062_fmurt6
-      - mimxrt1064_evk
-      - mimxrt1160_evk/mimxrt1166/cm7
-      - mimxrt1170_evk/mimxrt1176/cm7
-      - vmu_rt1170/mimxrt1176/cm7
-      - mimxrt595_evk/mimxrt595s/cm33
-      - mimxrt685_evk/mimxrt685s/cm33
-      - nrf52840dk/nrf52840
-      - rd_rw612_bga
-      - nucleo_wba55cg
-    integration_platforms:
-      - frdm_k64f
-      - nrf52840dk/nrf52840
-  bootloader.mcuboot.assert:
-    tags: mcuboot
-    platform_allow:
-      - b_u585i_iot02a
-    extra_configs:
-      - CONFIG_ASSERT=y
+    filter: dt_nodelabel_enabled("slot0_partition") and dt_nodelabel_enabled("slot1_partition") and dt_nodelabel_enabled("boot_partition")

And ran it:

/tmp/aa/zephyr/scripts/twister -T . -G
...
INFO    - Writing JSON report /tmp/aa/zephyr/tests/boot/test_mcuboot/twister-out/testplan.json
INFO    - JOBS: 8
INFO    - Adding tasks to the queue...
INFO    - Added initial list of jobs to queue
ERROR   - Cmake build failure: /tmp/aa/zephyr/tests/boot/test_mcuboot for native_sim
ERROR   - native_sim                tests/boot/test_mcuboot/bootloader.mcuboot          ERROR : Cmake build failure
ERROR   - see: /tmp/aa/zephyr/tests/boot/test_mcuboot/twister-out/native_sim/tests/boot/test_mcuboot/bootloader.mcuboot/build.log
INFO    - Total complete:   41/  41  100%  skipped:   40, failed:    0, error:    1
INFO    - 1 test scenarios (41 test instances) selected, 40 configurations skipped (15 by static filter, 25 at runtime).
INFO    - 0 of 41 test configurations passed (0.00%), 0 failed, 1 errored, 40 skipped with 0 warnings in 71.25 seconds
INFO    - In total 1 test cases were executed, 40 skipped on 41 out of total 852 platforms (4.81%)
INFO    - 0 test configurations executed on platforms, 1 test configurations were only built.
INFO    - Saving reports...
INFO    - Writing JSON report /tmp/aa/zephyr/tests/boot/test_mcuboot/twister-out/twister.json
INFO    - Writing xunit report /tmp/aa/zephyr/tests/boot/test_mcuboot/twister-out/twister.xml...
INFO    - Writing xunit report /tmp/aa/zephyr/tests/boot/test_mcuboot/twister-out/twister_report.xml...
INFO    - -+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
INFO    - The following issues were found (showing the top 10 items):
INFO    - 1) tests/boot/test_mcuboot/bootloader.mcuboot on native_sim error (Cmake build failure)
INFO    - 

So excluding that platform, seems like this works? Although 41 boards seems like a low number

@nordicjm
Copy link
Collaborator

Using the -l instead of -G switch fixes it. So with the above change this now builds that 851 board targets, this seems a bit ridiculous for an already high load CI

@danieldegrasse
Copy link
Collaborator

That is not a test in zephyr and that's not the test being changed here, I made this changes:

Thanks for looking at this more- I took another look here, and it looks like we can indeed use the filter.

What I was missing is that we now default to using "filter stages" for the CMake filtering stage when possible- filter stages will not run sysbuild. The issue I expected to find was that when we ran sysbuild and tried to generate a DTS for MCUBoot, the CMake filtering stage would fail. However, since we use filter stages this is not the case.

Given that, I think we should make a change like so:

diff --git a/tests/boot/test_mcuboot/testcase.yaml b/tests/boot/test_mcuboot/testcase.yaml
index 3de1f70426a..fd6a61c21ce 100644
--- a/tests/boot/test_mcuboot/testcase.yaml
+++ b/tests/boot/test_mcuboot/testcase.yaml
@@ -11,46 +11,6 @@ common:
       - "Swapped application booted on (.*)"
 tests:
   bootloader.mcuboot:
+    sysbuild: true
     tags: mcuboot
-    platform_allow:
-      - frdm_k22f
-      - frdm_k64f
-      - frdm_k82f
-      - frdm_ke17z
-      - frdm_ke17z512
-      - rddrone_fmuk66
-      - twr_ke18f
-      - twr_kv58f220m
-      - frdm_mcxn947/mcxn947/cpu0
-      - lpcxpresso55s06
-      - lpcxpresso55s16
-      - lpcxpresso55s28
-      - lpcxpresso55s36
-      - lpcxpresso55s69/lpc55s69/cpu0
-      - mimxrt1010_evk
-      - mimxrt1015_evk
-      - mimxrt1020_evk
-      - mimxrt1024_evk
-      - mimxrt1040_evk
-      - mimxrt1050_evk
-      - mimxrt1060_evk
-      - mimxrt1062_fmurt6
-      - mimxrt1064_evk
-      - mimxrt1160_evk/mimxrt1166/cm7
-      - mimxrt1170_evk/mimxrt1176/cm7
-      - vmu_rt1170/mimxrt1176/cm7
-      - mimxrt595_evk/mimxrt595s/cm33
-      - mimxrt685_evk/mimxrt685s/cm33
-      - nrf52840dk/nrf52840
-      - rd_rw612_bga
-      - nucleo_wba55cg
-      - frdm_mcxw71
-    integration_platforms:
-      - frdm_k64f
-      - nrf52840dk/nrf52840
-  bootloader.mcuboot.assert:
-    tags: mcuboot
-    platform_allow:
-      - b_u585i_iot02a
-    extra_configs:
-      - CONFIG_ASSERT=y
+    filter: dt_nodelabel_enabled("slot0_partition") and dt_nodelabel_enabled("slot1_partition") and dt_nodelabel_enabled("boot_partition")

Using the -l instead of -G switch fixes it. So with the above change this now builds that 851 board targets, this seems a bit ridiculous for an already high load CI

I mean what is the alternative here? We want to build (and execute, if hardware is available) MCUBoot on the platforms that support it. It is a high CI load, but I don't really see another way to validate this. Maybe we only test one platform per flash driver? Not sure how else to narrow things down.

@nordicjm
Copy link
Collaborator

nordicjm commented Oct 24, 2024

Would be up to @stephanosio and @nashif if having this run 851 tests is something that is wanted (to put it into perspective, when CI runs for a PR and the tests are split up and ran on node, this is equivalent to having over 1 more complete sub-job). The CI for this PR for example, https://github.com/zephyrproject-rtos/zephyr/actions/runs/11365562284/job/31621002358 shows 736 jobs for that and 7 sub-jobs, so this would add more builds to CI that one full sub-job there has

@butok
Copy link
Collaborator Author

butok commented Oct 24, 2024

Would be up to @stephanosio and @nashif if having this run 851 tests is something that is wanted (to put it into perspective, when CI runs for a PR and the tests are split up and ran on node, this is equivalent to having over 1 more complete sub-job). The CI for this PR for example, https://github.com/zephyrproject-rtos/zephyr/actions/runs/11365562284/job/31621002358 shows 736 jobs for that and 7 sub-jobs, so this would add more builds to CI that one full sub-job there has

Twister .yaml should allow building/running tests/samples on all platforms that support those tests/samples.
At least the Zephyr documentation does not demand any limitations.

Proposal:
If CI has capacity issues, it can test only integration platforms, or use a predefined platform subset.

@kartben
Copy link
Collaborator

kartben commented Dec 12, 2024

Twister .yaml should allow building/running tests/samples on all platforms that support those tests/samples.

Samples should not be treated as tests.

At least the Zephyr documentation does not demand any limitations.

It is actually pretty clear:

"Although being able to run a sample successfully does verify the functionality is working as expected, this should not replace dedicated and comprehensive tests with full coverage to be submitted into the tests/ folder. In a sample, the only thing you test is buildability and, in some cases, if a sample is performing as expected, i.e. you are testing the sample, not the subsystem it builds on top."

"The tests should verify the default configuration of the sample and any optional features documented in the sample’s README file. Sample should never be used to test functionality of the subsystem or module the sample belongs to."

"Use platform_allow to limit test to the platforms the sample was actually verified on. Those platforms should be listed in the sample’s README file."

https://docs.zephyrproject.org/latest/samples/sample_definition_and_criteria.html#sample-criteria

@butok
Copy link
Collaborator Author

butok commented Dec 12, 2024

Twister .yaml should allow building/running tests/samples on all platforms that support those tests/samples.

Samples should not be treated as tests.

At least the Zephyr documentation does not demand any limitations.

It is actually pretty clear:

"Although being able to run a sample successfully does verify the functionality is working as expected, this should not replace dedicated and comprehensive tests with full coverage to be submitted into the tests/ folder. In a sample, the only thing you test is buildability and, in some cases, if a sample is performing as expected, i.e. you are testing the sample, not the subsystem it builds on top."

"The tests should verify the default configuration of the sample and any optional features documented in the sample’s README file. Sample should never be used to test functionality of the subsystem or module the sample belongs to."

"Use platform_allow to limit test to the platforms the sample was actually verified on. Those platforms should be listed in the sample’s README file."

https://docs.zephyrproject.org/latest/samples/sample_definition_and_criteria.html#sample-criteria

Hi @kartben

OK, lets find a compromise.

We can rephrase it as Twister .yaml should allow building/running tests on all platforms that support those tests.
Is it OK now? ;)

Also, this PR adds "depend on" to the test (not sample).

@masz-nordic masz-nordic removed their request for review December 18, 2024 15:56
@nordicjm
Copy link
Collaborator

Ping @nashif

@butok
Copy link
Collaborator Author

butok commented Jan 30, 2025

Hi @nashif could you clarify a board.yaml usage.

According to https://docs.zephyrproject.org/latest/build/dts/design.html#example-remaining-work :
Zephyr’s Test Runner (Twister)] currently use board.yaml files to determine the hardware supported by a board. This should be obtained from devicetree instead.

Does it mean that we must avoid "depends_on" and use DTS "filter" instead? => Close this PR?

@dleach02
Copy link
Member

dleach02 commented Feb 5, 2025

I like this approach. I find the "platforms_allowed" to be a sledge hammer approach when we have the ability to basically have flags to drive things. For the overloaded CI, that is what the integration_platforms is for.

Copy link
Member

@nashif nashif left a comment

Choose a reason for hiding this comment

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

using filter is the right(*) approach, supported in the board yaml file is reserved for HW features and should be kept this way.

So one of the changes suggested by @nordicjm or @danieldegrasse could be applied.

(*) It is not optimal yet, but this how are things are right now, how twister selects platforms in CI and when/how filters are evaluated etc is subject to change and we are looking at different options.

@butok
Copy link
Collaborator Author

butok commented Mar 28, 2025

No common conclusion. Closing.

@erwango
Copy link
Member

erwango commented Mar 31, 2025

No common conclusion. Closing.

@butok Still open. Is that on purpose ?

@butok butok closed this Mar 31, 2025
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.