Improve reliability of KVStore general tests #12060
Conversation
kyle-cypress
force-pushed
the
pr/kvstore-general-test-fix
branch
from
f8a15ef to
739455a
Compare
|
@kyle-cypress, thank you for your changes. |
| @@ -85,6 +85,8 @@ static void kvstore_init() | |||
| res = bd->init(); | |||
| TEST_ASSERT_EQUAL_ERROR_CODE(0, res); | |||
| int erase_val = bd->get_erase_value(); | |||
| // Clear out any stale data that might be left from a previous test | |||
| bd->erase(0, bd->size()); | |||
There was a problem hiding this comment.
This is a bit dangerous/time consuming.
On some platforms these tests run against SD card, so clearing the whole card might stale the whole test.
Maybe some sensible value should be used here.
There was a problem hiding this comment.
Any ideas for how to find a sensible value? For the SecureStore case the test explicitly specifies the amount of storage that it needs via the SlicingBlockDevice's that it creates. But I didn't see anything equivalent for the FSStore or bare TDBStore that would bound the portion of the block device that the kvstore might use.
There was a problem hiding this comment.
There is no easy answer to this.. could it help if we erase just one erase area?
There was a problem hiding this comment.
I don't think that is sufficient, particularly on internal flash with small block sizes where that might just erase 512 bytes.
Is the size calculation used for the SlicingBlockDevice roughly applicable to the other tests as well? If so I can try using that as a reasonable baseline.
(In my testing, erasing the two SecureStore SlicingBlockDevice instances was sufficient to resolve the failure I was seeing; I made the erase more generic in an attempt to address the class of problem rather than just one instance).
There was a problem hiding this comment.
Pushed up a new version that uses the SlicingBlockDevice size as an estimate for how much to erase (and rebased on latest master while I was at it).
0xc0170
added
the
release-version: 6.0.0-alpha-1
kyle-cypress
force-pushed
the
pr/kvstore-general-test-fix
branch
from
739455a to
3b9fe1a
Compare
In kvstore_init, prior to initializing the kvstore, erase the underlying block storage device. This ensures that each test run starts from a consistent state and avoids failures that can result if a previous test run left the storage in an inconsistent state.
kyle-cypress
force-pushed
the
pr/kvstore-general-test-fix
branch
from
3b9fe1a to
d8f47bd
Compare
0xc0170
added
devices: cypress
release-version: 6.0.0-alpha-2
|
CI started |
Test run: SUCCESSSummary: 5 of 5 test jobs passed |
Summary of changes
In kvstore_init, prior to initializing the kvstore, erase the underlying block storage device. This ensures that each test run starts from a consistent state and avoids failures that can result if a previous test run left the storage in an inconsistent state.
This change resolves cases we have seen where the SecureStore init fails with
TDBSTORE: Unable to read record at initbecause the existing memory contents are sufficiently recognizeable to not trigger a reinitialization of the storage, but not enough for init to find the key that it expects.This mostly occurs on boards with no external flash storage and internal flash with a 1:1 ratio of program to erase size - which increases the chance of old values remaining in flash because fewer areas are erased as a "side effect". However, the fix conceptually applies to all storage types and is therefore not conditioned on internal storage or low program to erase size ratio.
Impact of changes
Migration actions required
Documentation
Pull request type
Test results
CY8CKIT_062_BLE-GCC_ARM.txt
CYW9P62S1_43012EVB_01-GCC_ARM.txt
CYW9P62S1_43438EVB_01-GCC_ARM.txt
CYW943012P6EVB_01-GCC_ARM.txt
CY8CKIT_062_WIFI_BT-GCC_ARM.txt
CY8CKIT_062S2_43012-GCC_ARM.txt
CY8CPROTO_062_4343W_GCC_ARM.txt
CY8CPROTO_062S3_4343W-GCC_ARM.txt
CY8CPROTO_063_BLE_GCC_ARM.txt
Note 1: The failure of tests-mbed_drivers-timeout on CY8CPROTO_063_BLE appears to be a spurious failure. The test passed when re-run on its own, and this PR does not modify that test file (nor any other non-test code).
Note 2: To avoid false failures, testing was performed with the changes from #12050 applied (but the changes from that PR are not included in the commits for this PR, nor is there a dependency between the two).
Reviewers