From 10210ba4f29813047d5eae3025cc9b97aa5c4c8e Mon Sep 17 00:00:00 2001
From: Sergey Yakubov <sergey.yakubov@desy.de>
Date: Fri, 8 Dec 2017 12:19:01 +0100
Subject: [PATCH] get rid of memory leaks
---
CMakeModules/check_test.sh | 2 +-
CMakeModules/testing_cpp.cmake | 32 +++++++++++++++----
common/cpp/src/system_io_linux.cpp | 2 +-
tests/system_io/read_folder_content.cpp | 5 ++-
worker/api/cpp/include/worker/data_broker.h | 1 +
worker/api/cpp/src/data_broker.cpp | 2 +-
worker/api/cpp/src/folder_data_broker.h | 1 +
.../api/cpp/unittests/test_folder_broker.cpp | 3 +-
worker/api/cpp/unittests/test_worker_api.cpp | 2 +-
9 files changed, 37 insertions(+), 13 deletions(-)
diff --git a/CMakeModules/check_test.sh b/CMakeModules/check_test.sh
index 5ab34057c..ff6f8c5bf 100755
--- a/CMakeModules/check_test.sh
+++ b/CMakeModules/check_test.sh
@@ -17,7 +17,7 @@ if (( coverage < HIDRA2_MINIMUM_COVERAGE )); then
echo
echo "*****"
echo
- echo $TARGET coverage is ${coverage}% - less than required ${HIDRA2_MINIMUM_COVERAGE}%
+ echo ${TARGET} coverage is ${coverage}% - less than required ${HIDRA2_MINIMUM_COVERAGE}%
echo
echo "*****"
echo
diff --git a/CMakeModules/testing_cpp.cmake b/CMakeModules/testing_cpp.cmake
index d3b2ed023..ab96564a5 100644
--- a/CMakeModules/testing_cpp.cmake
+++ b/CMakeModules/testing_cpp.cmake
@@ -33,16 +33,31 @@ function(gtest target test_source_files test_libraries)
set(CMAKE_CXX_FLAGS ${CMAKE_CXX_FLAGS} PARENT_SCOPE)
endif ()
- if (MEMORYCHECK_COMMAND)
- set(memcheck_args ${MEMORYCHECK_COMMAND_OPTIONS})
- separate_arguments(memcheck_args)
- add_test(NAME memcheck-${target} COMMAND ${MEMORYCHECK_COMMAND} ${memcheck_args}
- ${CMAKE_CURRENT_BINARY_DIR}/test-${target})
- set_tests_properties(memcheck-${target} PROPERTIES LABELS "memcheck;all")
- endif()
+ add_memory_test(${target} test-${target} "" "" "unit")
+
endif ()
endfunction()
+function(add_memory_test target executable commandargs fixture label)
+ if (MEMORYCHECK_COMMAND)
+ set(memcheck_args ${MEMORYCHECK_COMMAND_OPTIONS})
+ separate_arguments(memcheck_args)
+ set( args ${commandargs} )
+ separate_arguments(args)
+ add_test(NAME memcheck-${target} COMMAND ${MEMORYCHECK_COMMAND} ${memcheck_args}
+ ${CMAKE_CURRENT_BINARY_DIR}/${executable} ${args})
+ set_tests_properties(memcheck-${target} PROPERTIES
+ LABELS "memcheck_${label};all"
+ DEPENDS test-${target}
+ )
+ if (NOT ${fixture} STREQUAL "")
+ set_tests_properties(memcheck-${target} PROPERTIES
+ FIXTURES_REQUIRED ${fixture}
+ )
+ endif()
+
+ endif()
+endfunction()
function(add_test_setup_cleanup exename)
if (BUILD_TESTS)
@@ -62,5 +77,8 @@ function(add_integration_test exename testname commandargs)
LABELS "integration;all"
FIXTURES_REQUIRED test-${exename}-fixture
)
+ add_memory_test(${exename}-${testname} ${exename}
+ "${commandargs}" test-${exename}-fixture
+ "integration")
endif ()
endfunction()
diff --git a/common/cpp/src/system_io_linux.cpp b/common/cpp/src/system_io_linux.cpp
index 5e39b413b..e74694106 100644
--- a/common/cpp/src/system_io_linux.cpp
+++ b/common/cpp/src/system_io_linux.cpp
@@ -40,7 +40,7 @@ bool IsDirectory(const struct dirent* entity) {
system_clock::time_point GetTimePointFromFile(const string& fname, IOErrors* err) {
- struct stat t_stat;
+ struct stat t_stat{};
int res = stat(fname.c_str(), &t_stat);
if (res < 0) {
*err = IOErrorFromErrno();
diff --git a/tests/system_io/read_folder_content.cpp b/tests/system_io/read_folder_content.cpp
index cb4b24c10..8391c73bc 100644
--- a/tests/system_io/read_folder_content.cpp
+++ b/tests/system_io/read_folder_content.cpp
@@ -8,7 +8,7 @@ using hidra2::IOErrors;
void M_AssertEq(const std::string& expected, const std::string& got) {
- if (expected.compare(got) != 0) {
+ if (expected != got) {
std::cerr << "Assert failed:\n"
<< "Expected:\t'" << expected << "'\n"
<< "Obtained:\t'" << got << "'\n";
@@ -40,6 +40,9 @@ int main(int argc, char* argv[]) {
case IOErrors::PERMISSIONS_DENIED:
result = "noaccess";
break;
+ default:
+ result = "";
+ break;
}
M_AssertEq(expect, result);
diff --git a/worker/api/cpp/include/worker/data_broker.h b/worker/api/cpp/include/worker/data_broker.h
index 38a263f36..df931e4ac 100644
--- a/worker/api/cpp/include/worker/data_broker.h
+++ b/worker/api/cpp/include/worker/data_broker.h
@@ -18,6 +18,7 @@ enum class WorkerErrorCode {
class DataBroker {
public:
virtual WorkerErrorCode Connect() = 0;
+ virtual ~DataBroker() = default; // needed for unique_ptr to delete itself
};
class DataBrokerFactory {
diff --git a/worker/api/cpp/src/data_broker.cpp b/worker/api/cpp/src/data_broker.cpp
index f412eaf1c..cdcd96684 100644
--- a/worker/api/cpp/src/data_broker.cpp
+++ b/worker/api/cpp/src/data_broker.cpp
@@ -13,7 +13,7 @@ std::unique_ptr<DataBroker> DataBrokerFactory::Create(const std::string& source_
std::unique_ptr<DataBroker> p = nullptr;
try {
- p = (std::unique_ptr<DataBroker>) new FolderDataBroker(source_name);
+ p.reset(new FolderDataBroker(source_name));
*return_code = WorkerErrorCode::ERR__NO_ERROR;
} catch (...) { // we do not test this part
*return_code = WorkerErrorCode::ERR__MEMORY_ERROR;
diff --git a/worker/api/cpp/src/folder_data_broker.h b/worker/api/cpp/src/folder_data_broker.h
index bdecc943d..564751158 100644
--- a/worker/api/cpp/src/folder_data_broker.h
+++ b/worker/api/cpp/src/folder_data_broker.h
@@ -15,6 +15,7 @@ class FolderDataBroker final : public hidra2::DataBroker {
explicit FolderDataBroker(const std::string& source_name);
WorkerErrorCode Connect() override;
std::unique_ptr<hidra2::IO> io__; // modified in testings to mock system calls,otherwise do not touch
+
private:
std::string base_path_;
std::vector<FileInfo> filelist_;
diff --git a/worker/api/cpp/unittests/test_folder_broker.cpp b/worker/api/cpp/unittests/test_folder_broker.cpp
index 27952a410..a8b891b1e 100644
--- a/worker/api/cpp/unittests/test_folder_broker.cpp
+++ b/worker/api/cpp/unittests/test_folder_broker.cpp
@@ -21,7 +21,8 @@ namespace {
TEST(FolderDataBroker, SetCorrectIO) {
auto data_broker = new FolderDataBroker("test");
- ASSERT_THAT(dynamic_cast<hidra2::SystemIO*>(data_broker->io__.release()), Ne(nullptr));
+ ASSERT_THAT(dynamic_cast<hidra2::SystemIO*>(data_broker->io__.get()), Ne(nullptr));
+ delete data_broker;
}
diff --git a/worker/api/cpp/unittests/test_worker_api.cpp b/worker/api/cpp/unittests/test_worker_api.cpp
index ca87f1226..681a99462 100644
--- a/worker/api/cpp/unittests/test_worker_api.cpp
+++ b/worker/api/cpp/unittests/test_worker_api.cpp
@@ -20,7 +20,7 @@ TEST(DataBrokerFactoryTests, CreateFolderDataSource) {
auto data_broker = DataBrokerFactory::Create("path/to/file", &return_code);
ASSERT_THAT(return_code, Eq(WorkerErrorCode::ERR__NO_ERROR));
- ASSERT_THAT(dynamic_cast<FolderDataBroker*>(data_broker.release()), Ne(nullptr));
+ ASSERT_THAT(dynamic_cast<FolderDataBroker*>(data_broker.get()), Ne(nullptr));
}
TEST(DataBrokerFactoryTests, FailCreateDataSourceWithEmptySource) {
--
GitLab