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

Add moduletests for FileSystemStore #12652

Merged
merged 1 commit into from
Mar 19, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 111 additions & 0 deletions UNITTESTS/moduletests/storage/kvstore/FileSystemStore/moduletest.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
/* Copyright (c) 2020 ARM Limited
* SPDX-License-Identifier: Apache-2.0
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#include "gtest/gtest.h"
#include "features/storage/blockdevice/HeapBlockDevice.h"
#include "features/storage/kvstore/filesystemstore/FileSystemStore.h"
#include "features/storage/filesystem/littlefs/LittleFileSystem.h"
#include "mbed_error.h"
#include <stdlib.h>

#define HEAPBLOCK_SIZE (4096)

using namespace mbed;

class FileSystemStoreModuleTest : public testing::Test {
protected:
HeapBlockDevice heap{HEAPBLOCK_SIZE};
LittleFileSystem *fs;
FileSystemStore *store;

virtual void SetUp()
{
fs = new LittleFileSystem("kvstore", &heap);
if(fs->mount(&heap) != MBED_SUCCESS) {
EXPECT_EQ(fs->reformat(&heap), MBED_SUCCESS);
}
store = new FileSystemStore(fs);
EXPECT_EQ(store->init(), MBED_SUCCESS);
}

virtual void TearDown()
{
EXPECT_EQ(store->deinit(), MBED_SUCCESS);
delete store;
EXPECT_EQ(fs->unmount(), MBED_SUCCESS);
delete fs;
}
};

TEST_F(FileSystemStoreModuleTest, init)
{
EXPECT_EQ(store->deinit(), MBED_SUCCESS);
EXPECT_EQ(store->init(), MBED_SUCCESS);
EXPECT_EQ(store->init(), MBED_SUCCESS);
}

TEST_F(FileSystemStoreModuleTest, set_get)
{
char buf[100];
size_t size;
EXPECT_EQ(store->set("key", "data", 5, 0), MBED_SUCCESS);
EXPECT_EQ(store->get("key", buf, 100, &size), MBED_SUCCESS);
EXPECT_EQ(size, 5);
EXPECT_STREQ("data", buf);
}

TEST_F(FileSystemStoreModuleTest, erased_set_get)
{
EXPECT_EQ(store->deinit(), MBED_SUCCESS);
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume these test can't be run in random order.

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 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.

Copy link
Contributor

@LDong-Arm LDong-Arm Apr 1, 2021

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do you want to deallocate the heap buffer on erase? That doesn't sound like correct behavior.

Copy link
Contributor

@LDong-Arm LDong-Arm Apr 12, 2021

Choose a reason for hiding this comment

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

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@LDong-Arm LDong-Arm Apr 13, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, that explains it. LGTM.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cheers

EXPECT_EQ(heap.init(), MBED_SUCCESS);
EXPECT_EQ(heap.erase(0, heap.size()), MBED_SUCCESS);
EXPECT_EQ(heap.deinit(), MBED_SUCCESS);
EXPECT_EQ(store->init(), MBED_SUCCESS);
char buf[100];
size_t size;
EXPECT_EQ(store->set("key", "data", 5, 0), MBED_SUCCESS);
EXPECT_EQ(store->get("key", buf, 100, &size), MBED_SUCCESS);
EXPECT_EQ(size, 5);
EXPECT_STREQ("data", buf);
}

TEST_F(FileSystemStoreModuleTest, set_deinit_init_get)
{
char buf[100];
size_t size;
for (int i = 0; i < 100; ++i) {
EXPECT_EQ(store->set("key", "data", 5, 0), MBED_SUCCESS);
EXPECT_EQ(store->deinit(), MBED_SUCCESS);
EXPECT_EQ(store->init(), MBED_SUCCESS);
EXPECT_EQ(store->get("key", buf, 100, &size), MBED_SUCCESS);
EXPECT_EQ(size, 5);
EXPECT_STREQ("data", buf);
EXPECT_EQ(store->remove("key"), MBED_SUCCESS);
}
}

TEST_F(FileSystemStoreModuleTest, set_multiple_iterate)
{
char buf[100];
KVStore::iterator_t iterator;
EXPECT_EQ(store->set("primary_key", "data", 5, 0), MBED_SUCCESS);
EXPECT_EQ(store->set("primary_second_key", "value", 6, 0), MBED_SUCCESS);
EXPECT_EQ(store->iterator_open(&iterator, "primary"), MBED_SUCCESS);
EXPECT_EQ(store->iterator_next(iterator, buf, 100), MBED_SUCCESS);
EXPECT_EQ(store->iterator_next(iterator, buf, 100), MBED_SUCCESS);
EXPECT_EQ(store->iterator_next(iterator, buf, 100), MBED_ERROR_ITEM_NOT_FOUND);
EXPECT_EQ(store->iterator_close(iterator), MBED_SUCCESS);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
####################
# UNIT TESTS
####################

set(unittest-includes ${unittest-includes}
.
..
../features/frameworks/mbed-trace/mbed-trace
)

set(unittest-sources
../features/storage/blockdevice/HeapBlockDevice.cpp
../features/storage/kvstore/filesystemstore/FileSystemStore.cpp
../features/storage/filesystem/littlefs/LittleFileSystem.cpp
../features/storage/filesystem/Dir.cpp
../features/storage/filesystem/File.cpp
../features/storage/filesystem/FileSystem.cpp
../features/frameworks/mbed-trace/source/mbed_trace.c
../features/storage/filesystem/littlefs/littlefs/lfs_util.c
../features/storage/filesystem/littlefs/littlefs/lfs.c
../platform/source/FileBase.cpp
../platform/source/FileSystemHandle.cpp
../platform/source/FileHandle.cpp
)

set(unittest-test-sources
moduletests/storage/kvstore/FileSystemStore/moduletest.cpp
stubs/mbed_atomic_stub.c
stubs/mbed_assert_stub.cpp
stubs/mbed_error.c
stubs/kv_config_stub.cpp
stubs/mbed_retarget_stub.cpp
)

set(unittest-test-flags
-DMBED_LFS_READ_SIZE=64
-DMBED_LFS_PROG_SIZE=64
-DMBED_LFS_BLOCK_SIZE=512
-DMBED_LFS_LOOKAHEAD=512
)
23 changes: 23 additions & 0 deletions UNITTESTS/stubs/kv_config_stub.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/*
* Copyright (c) , Arm Limited and affiliates.
* SPDX-License-Identifier: Apache-2.0
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#include "features/storage/kvstore/conf/kv_config.h"

const char *get_filesystemstore_folder_path()
{
return "kvstore";
}
26 changes: 26 additions & 0 deletions UNITTESTS/stubs/mbed_retarget_stub.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/*
* Copyright (c) , Arm Limited and affiliates.
* SPDX-License-Identifier: Apache-2.0
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#include "platform/mbed_retarget.h"
#include "platform/FileHandle.h"

namespace mbed {
void remove_filehandle(FileHandle *file)
{
(void)file;
}
}
60 changes: 60 additions & 0 deletions UNITTESTS/target_h/platform/mbed_retarget.h
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,49 @@ namespace mbed {
#define _IFCHR 0020000 //< character special
#define _IFIFO 0010000 //< fifo special

#ifndef S_IFMT
#define S_IFMT _IFMT //< type of file
#endif
#ifndef S_IFSOCK
#define S_IFSOCK _IFSOCK //< socket
#endif
#ifndef S_IFLNK
#define S_IFLNK _IFLNK //< symbolic link
#endif
#ifndef S_IFREG
#define S_IFREG _IFREG //< regular
#endif
#ifndef S_IFBLK
#define S_IFBLK _IFBLK //< block special
#endif
#ifndef S_IFDIR
#define S_IFDIR _IFDIR //< directory
#endif
#ifndef S_IFCHR
#define S_IFCHR _IFCHR //< character special
#endif
#ifndef S_IFIFO
#define S_IFIFO _IFIFO //< fifo special
#endif

#ifndef S_IRWXU
#define S_IRWXU (S_IRUSR | S_IWUSR | S_IXUSR)
#define S_IRUSR 0000400 ///< read permission, owner
#define S_IWUSR 0000200 ///< write permission, owner
#define S_IXUSR 0000100 ///< execute/search permission, owner
#endif /* S_IRWXU */
#ifndef S_IRWXG
#define S_IRWXG (S_IRGRP | S_IWGRP | S_IXGRP)
#define S_IRGRP 0000040 ///< read permission, group
#define S_IWGRP 0000020 ///< write permission, group
#define S_IXGRP 0000010 ///< execute/search permission, group
#endif /* S_IRWXG */
#ifndef S_IRWXO
#define S_IRWXO (S_IROTH | S_IWOTH | S_IXOTH)
#define S_IROTH 0000004 ///< read permission, other
#define S_IWOTH 0000002 ///< write permission, other
#define S_IXOTH 0000001 ///< execute/search permission, other
#endif /* S_IRWXO */

#define O_RDONLY 0 ///< Open for reading
#define O_WRONLY 1 ///< Open for writing
Expand All @@ -319,6 +362,23 @@ namespace mbed {
#define STDOUT_FILENO 1
#define STDERR_FILENO 2

#ifndef _STAT_VER
struct stat {
dev_t st_dev; ///< Device ID containing file
ino_t st_ino; ///< File serial number
mode_t st_mode; ///< Mode of file
nlink_t st_nlink; ///< Number of links to file

uid_t st_uid; ///< User ID
gid_t st_gid; ///< Group ID

off_t st_size; ///< Size of file in bytes

time_t st_atime; ///< Time of last access
time_t st_mtime; ///< Time of last data modification
time_t st_ctime; ///< Time of last status change
};
#endif

struct statvfs {
unsigned long f_bsize; ///< Filesystem block size
Expand Down