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: net: lib: nrf_cloud: add fota_common tests #11734

Merged
merged 1 commit into from
Aug 3, 2023

Conversation

jayteemo
Copy link
Contributor

@jayteemo jayteemo commented Jul 6, 2023

Add test suite for nrf_cloud_fota_common.c.
IRIS-4790

@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jul 6, 2023
@jayteemo jayteemo requested a review from tony-le-24 July 6, 2023 22:50
@github-actions github-actions bot added ble mesh Label for ble mesh PRbot. Add this if PR is related to ble mesh and you need to get review. doc-required PR must not be merged without tech writer approval. manifest and removed changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels Jul 17, 2023
@NordicBuilder
Copy link
Contributor

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff
dragoon f870a908c06de77fd4913370e97a92684782cf76 8088a53e2c71384a8cb3dfc51ce37f68a8c4088b N/A
find-my https://github.com/nrfconnect/sdk-find-my/commit/68e82297568d9c8e1d244e87858b4bd332643605 https://github.com/nrfconnect/sdk-find-my/commit/c30e0186e6476e439ffdbf83ebb421662031cbd1 (main) nrfconnect/[email protected]
hostap nrfconnect/sdk-hostap@8e2e758 nrfconnect/sdk-hostap@42a92ba (main) nrfconnect/[email protected]
matter nrfconnect/sdk-connectedhomeip@6d0b631 nrfconnect/sdk-connectedhomeip@7f724e8 (master) nrfconnect/[email protected]
mcuboot nrfconnect/sdk-mcuboot@4e84f31 (upmerge-tmp) nrfconnect/sdk-mcuboot@823fd36 (main) nrfconnect/[email protected]
nrf-802154 https://github.com/nrfconnect/sdk-nrf-802154/commit/43a0e760e7e47589739da632f852237d6331dacf https://github.com/nrfconnect/sdk-nrf-802154/commit/109821b9e3a9b1d3536e2ab86b6d174e2af64743 nrfconnect/[email protected]
nrfxlib nrfconnect/sdk-nrfxlib@4eca399 nrfconnect/sdk-nrfxlib@16721d9 (main) nrfconnect/[email protected]
openthread nrfconnect/sdk-openthread@11a38a6 nrfconnect/sdk-openthread@5beae14 (main) nrfconnect/[email protected]
trusted-firmware-m nrfconnect/sdk-trusted-firmware-m@ccab64f nrfconnect/sdk-trusted-firmware-m@c5b393b nrfconnect/[email protected]
zephyr nrfconnect/sdk-zephyr@01e2e4c nrfconnect/sdk-zephyr@b37ce40 (upmerge-tmp) nrfconnect/[email protected]

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@github-actions github-actions bot added changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. and removed manifest ble mesh Label for ble mesh PRbot. Add this if PR is related to ble mesh and you need to get review. doc-required PR must not be merged without tech writer approval. labels Jul 17, 2023
@jayteemo jayteemo force-pushed the IRIS-4790 branch 3 times, most recently from 7e85a88 to 1b61a48 Compare July 21, 2023 18:54
@jayteemo jayteemo marked this pull request as ready for review July 21, 2023 18:57
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Jul 21, 2023

Test specification

CI/Jenkins/NRF

  • Integration Platforms

CI/Jenkins/integration

Test Module File based changes Manually selected West overwrite

Detailed information of selected test modules

Note: This message is automatically posted and updated by the CI

Copy link
Contributor

@glarsennordic glarsennordic left a comment

Choose a reason for hiding this comment

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

Stopping review here since I suspect with some added comments reviewing will go faster

Comment on lines +11 to +16
- CONFIG_MCUBOOT_IMG_MANAGER=y
- CONFIG_BOOTLOADER_MCUBOOT=y
- CONFIG_IMG_MANAGER=y
- CONFIG_STREAM_FLASH_ERASE=y
- CONFIG_NRF_CLOUD_FOTA_FULL_MODEM_UPDATE=y
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- CONFIG_MCUBOOT_IMG_MANAGER=y
- CONFIG_BOOTLOADER_MCUBOOT=y
- CONFIG_IMG_MANAGER=y
- CONFIG_STREAM_FLASH_ERASE=y
- CONFIG_NRF_CLOUD_FOTA_FULL_MODEM_UPDATE=y
- CONFIG_MCUBOOT_IMG_MANAGER=y
- CONFIG_BOOTLOADER_MCUBOOT=y
- CONFIG_IMG_MANAGER=y
- CONFIG_STREAM_FLASH_ERASE=y
- CONFIG_NRF_CLOUD_FOTA=y
- CONFIG_NRF_CLOUD_REST=y
- CONFIG_FOTA_DOWNLOAD=y
- CONFIG_NRF_CLOUD_FOTA_FULL_MODEM_UPDATE=y

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CONFIG_NRF_CLOUD_REST=y is not needed.

CONFIG_NRF_CLOUD_MQTT=y in the prj.conf results in the selection of CONFIG_NRF_CLOUD_FOTA and CONFIG_FOTA_DOWNLOAD, I will add comments.

tests/subsys/net/lib/nrf_cloud/fota_common/testcase.yaml Outdated Show resolved Hide resolved
{
Z_TEST_SKIP_IFNDEF(CONFIG_NRF_CLOUD_FOTA_FULL_MODEM_UPDATE);

#if defined(CONFIG_NRF_CLOUD_FOTA_FULL_MODEM_UPDATE)
Copy link
Contributor

Choose a reason for hiding this comment

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

Given how abundant these fmfu tests are, it seems worthwhile for them to have their own test folder so that you can avoid giving them all ZTEST_SKIP statements and conditionally defined bodies.

So, like, essentially an entire copy of the fota_common test folder, but named fota_fmfu and containing only the tests and fakes required for FMFU

If I understand correctly there would be very few, if any, duplicated fakes between fota_common and fota_fmfu

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i moved them into their own file.

/* Verify that nrf_cloud_pending_fota_job_process succeeds when a pending
* FMFU FOTA validation PASSES.
* The reboot flag should become true.
* Must be called after nrf_cloud_fota_fmfu_dev_set() is successful.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you point out which part of the test guarantees this? I'm guessing it is part of how you have the fakes configured, but it would be useful to have a comment in the test explaining why setting the fakes this way causes nrf_cloud_fota_fmfu_dev_set to succeed

It would also be good to document what entity internally calls nrf_cloud_fota_fmfu_dev_set in this case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By default the tests are sorted and ran in alphanumerical order. Test cases may be dependent on this sequence. - ztest
the name of the test guarantees this.

what entity internally calls nrf_cloud_fota_fmfu_dev_set in this case

the test test_07_nrf_cloud_fota_fmfu_dev_set_pass results in a successful call to nrf_cloud_fota_fmfu_dev_set

tests/subsys/net/lib/nrf_cloud/fota_common/src/main.c Outdated Show resolved Hide resolved
tests/subsys/net/lib/nrf_cloud/fota_common/src/main.c Outdated Show resolved Hide resolved
tests/subsys/net/lib/nrf_cloud/fota_common/src/main.c Outdated Show resolved Hide resolved
tests/subsys/net/lib/nrf_cloud/fota_common/src/main.c Outdated Show resolved Hide resolved
tests/subsys/net/lib/nrf_cloud/fota_common/src/main.c Outdated Show resolved Hide resolved
tests/subsys/net/lib/nrf_cloud/fota_common/src/main.c Outdated Show resolved Hide resolved
@jayteemo jayteemo force-pushed the IRIS-4790 branch 4 times, most recently from 029b7be to 59df208 Compare July 28, 2023 18:47
Copy link
Contributor

@tony-le-24 tony-le-24 left a comment

Choose a reason for hiding this comment

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

LGTM!

@jayteemo jayteemo force-pushed the IRIS-4790 branch 3 times, most recently from 63380be to e17b6a4 Compare August 1, 2023 22:16
Add test suite for nrf_cloud_fota_common.c.
IRIS-4790

Signed-off-by: Justin Morton <[email protected]>
@jayteemo jayteemo removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Aug 2, 2023
@nordicjm nordicjm merged commit e08b810 into nrfconnect:main Aug 3, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants