Add moduletests for FileSystemStore #12652
Conversation
Baseline for the tests is similar to the TDBStore. Differing from TDBStore moduletests, FileSystemStore doesn't include tests for reserved data or corrupting the blockdevice, as it corrupts the filesystem also.
|
|
||
| TEST_F(FileSystemStoreModuleTest, erased_set_get) | ||
| { | ||
| EXPECT_EQ(store->deinit(), MBED_SUCCESS); |
There was a problem hiding this comment.
I assume these test can't be run in random order.
There was a problem hiding this comment.
I think the tests are run in the order they are written. But that shouldn't matter as the tests are separate and setup and teardown are ran for each test.
There was a problem hiding this comment.
@jarlamsa This test case fails if BlockDevice::erase() actually performs an erase, unless we call fs->reformat() afterwards. We didn't see this issue because HeapBlockDevice::erase() is essentially a no-op. But now I'm working on de-allocating the heap buffer on erase, and get the failure. (Same happens, if we memset the buffer to 0xFF instead of de-allocate it.)
I can add the reformat to make it pass. But erasing the underlying BlockDevice sounds like restarting everything from scratch? Could you please explain me the use case we're trying to test here? Thanks in advance.
There was a problem hiding this comment.
Why do you want to deallocate the heap buffer on erase? That doesn't sound like correct behavior.
There was a problem hiding this comment.
If deallocating the heap is not correct, then what about a BlockDevice that does an actual erase (e.g. like many flash devices) and sets values to 0xFF, for example? If I didn't miss anything, this test case relies on the assumption that data is unaltered when BlockDevice::erase() is called?
In my opinion, FileSystemStore should be compatible with any block device that's compliant with BlockDevice API requirements, rather than tied to specific behaviours? (I mean, I personally think a test case is only meaningful if it tests something generally applicable.)
There was a problem hiding this comment.
Referring to this test case, I think it can be safely deleted, erasing the whole heap underneath the filesystem just corrupts the filesystem.
But regarding the deallocation of the allocated memory from the heap when erasing, it really sounds just wrong. Now the allocation for HeapBlockDevice is done on init and deallocation is done on destructor, how do you plan to change this?
There was a problem hiding this comment.
HeapBlockDevice::init() only allocates an array of null pointers to blocks, not individual blocks. Only when a block is used in HeapBlockDevice::program(), the particular blocks being programmed is allocated and added to the array of pointers.
My HeapBlockDevice::erase() change only frees blocks being erased. The destructor, as before, frees all block and the array of pointers. So in terms of behaviour, it's still self-consistent, I think.
But if the change looks redundant I can remove it.
There was a problem hiding this comment.
Ok, that explains it. LGTM.
|
CI started |
Test run: SUCCESSSummary: 1 of 1 test jobs passed |
Summary of changes
Baseline for the tests is similar to the TDBStore.
Differing from TDBStore moduletests, FileSystemStore doesn't include
tests for reserved data or corrupting the blockdevice, as it corrupts
the filesystem also.
Impact of changes
Migration actions required
Documentation
None
Pull request type
Test results
Reviewers
@SeppoTakalo @VeijoPesonen