Fix and re-enable Zephyr native tests #37333
Open
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
* Don't dereference address pointed by lfp in for loop expression as it may be a nullptr. * Add simple test that registers and deregisters single error formatter. * Deregister all formatters at the end of CheckRegisterDeregisterErrorFormatter. Signed-off-by: Adrian Gielniewski <[email protected]>
* Add function to deregister CHIP layer error formatter. * Deregister CHIP error formatter at the end of TestSystemErrorStr to not interfere with other tests. Signed-off-by: Adrian Gielniewski <[email protected]>
Add temporary workaround for Zephyr native link issue. This will be fixed with the next NCS release. Signed-off-by: Adrian Gielniewski <[email protected]>
Signed-off-by: Adrian Gielniewski <[email protected]>
Signed-off-by: Adrian Gielniewski <[email protected]>
Signed-off-by: Adrian Gielniewski <[email protected]>
Wipe all key slots during each test case SetUp to avoid reusing slot previously used by other tests. This affects heap memory usage calculation as when the slot is reused it frees the memory. Signed-off-by: Adrian Gielniewski <[email protected]>
|
Review changes with |
github-actions
bot
added
system
nrf connect
lib
crypto
app
github
workflows
support
core
test driver
icd
|
PR #37333: Size comparison from 3044eeb to e3e10cf Full report (82 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
| void ValidateHmac128(const Crypto::Hmac128KeyHandle & saved, const Crypto::Hmac128KeyHandle & loaded) | ||
| { | ||
| #if CHIP_CRYPTO_PSA | ||
| EXPECT_NE(memcmp(saved.As<Crypto::Symmetric128BitsKeyByteArray>(), loaded.As<Crypto::Symmetric128BitsKeyByteArray>(), |
There was a problem hiding this comment.
You shouldn't cast the handles to Symmetric128BitsKeyByteArray if they are actually psa_key_id_ts. Should this be EXPECT_EQ(saved.As<psa_key_id_t>(), loaded.As<psa_key_id_t>())? If both keys are already persisted, I would expect the same ID after loading the entry.
| #if CHIP_CRYPTO_PSA | ||
| psa_crypto_init(); | ||
| psa_wipe_all_key_slots(); |
There was a problem hiding this comment.
Is this needed to support multiple runs when using the flash simulator which lefts flash.bin behind? Note that you can just run zephyr.exe with --flash_rm to remove the file automatically after the test run.
There was a problem hiding this comment.
Without this change it fails on first run. We have some slots used by other tests, then some of them have to be resused and as a result we free the memory allocated before HeapChecker is constructed.
| EXPECT_EQ(loopHandler.trace, std::string("1AP2HP3R4")); | ||
| #else | ||
| EXPECT_EQ(loopHandler.trace, std::string("1APHP2HPHP3R4")); |
There was a problem hiding this comment.
Can you comment where the difference comes from? Is this expected?
There was a problem hiding this comment.
There is a difference in how WakeEvent is handled:
https://github.com/project-chip/connectedhomeip/blob/master/src/system/SystemLayerImplSelect.cpp#L142-L150
In Zephyr Notify is called and loop has to spin one more time to process the event.
Maybe it would be better to use CHIP_SYSTEM_CONFIG_POSIX_LOCKING instead of CHIP_SYSTEM_CONFIG_USE_POSIX_PIPE.
There was a problem hiding this comment.
I changed the define and updated commit description.
Expect correct sequence for different LayerImplSelect::Signal implementations. When CHIP_SYSTEM_CONFIG_POSIX_LOCKING is not defined, WakeEvent::Notify is called and loop has to spin one more time to process the event. Signed-off-by: Adrian Gielniewski <[email protected]>
adigie
force-pushed
the
fix-zephyr-native-tests
branch
from
e3e10cf to
72ca79e
Compare
|
PR #37333: Size comparison from 4f956ec to 72ca79e Full report (83 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
There was a problem hiding this comment.
Seems like this includes all of #37254
Would be better to get that one merged, then update this PR so it's clear what's actually changing here.
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.
This PR:
Testing
Tested using automated tests.