From ae116e037c1106473a3fd87e4cb3655b89e34e8e Mon Sep 17 00:00:00 2001 From: Marcos Bento Date: Tue, 28 Nov 2023 13:52:05 +0000 Subject: [PATCH] Avoid reusing same temporary file amongst tests When several tests use the same temporary file there is an effective race condition between them, which causes the tests to occasionally fail. The goal of the changes is to create unique temporary file names to be used by the tests. --- ANode/CMakeLists.txt | 6 +++ ANode/parser/test/ParseTimer.cpp | 7 ++- ANode/parser/test/PersistHelper.cpp | 58 ++++++++++-------------- ANode/parser/test/TemporaryFile.cpp | 26 +++++++++++ ANode/parser/test/TemporaryFile.hpp | 34 ++++++++++++++ ANode/parser/test/TestSingleDefsFile.cpp | 17 ++----- 6 files changed, 95 insertions(+), 53 deletions(-) create mode 100644 ANode/parser/test/TemporaryFile.cpp create mode 100644 ANode/parser/test/TemporaryFile.hpp diff --git a/ANode/CMakeLists.txt b/ANode/CMakeLists.txt index eb350b250..01a95d61f 100644 --- a/ANode/CMakeLists.txt +++ b/ANode/CMakeLists.txt @@ -246,8 +246,10 @@ target_clangformat(u_anode set(test_srcs # Headers parser/test/PersistHelper.hpp + parser/test/TemporaryFile.hpp # Sources parser/test/PersistHelper.cpp + parser/test/TemporaryFile.cpp parser/test/TestAutoAddExterns.cpp parser/test/TestDefsStructurePersistAndReload.cpp parser/test/TestMementoPersistAndReload.cpp @@ -304,8 +306,10 @@ if (ENABLE_ALL_TESTS) set(test_srcs # Headers parser/test/PersistHelper.hpp + parser/test/TemporaryFile.hpp # Sources parser/test/PersistHelper.cpp + parser/test/TemporaryFile.cpp parser/test/TestParserPerformance_main.cpp # test entry point parser/test/TestSingleDefsFile.cpp ) @@ -331,9 +335,11 @@ if (ENABLE_ALL_TESTS) set(test_srcs # Headers parser/test/PersistHelper.hpp + parser/test/TemporaryFile.hpp # Sources parser/test/ParseTimer.cpp parser/test/PersistHelper.cpp + parser/test/TemporaryFile.cpp ) # The following is not technically a test (as it makes no checks), diff --git a/ANode/parser/test/ParseTimer.cpp b/ANode/parser/test/ParseTimer.cpp index 28652710d..ab3c12e44 100644 --- a/ANode/parser/test/ParseTimer.cpp +++ b/ANode/parser/test/ParseTimer.cpp @@ -36,6 +36,7 @@ #include "Suite.hpp" #include "System.hpp" #include "Task.hpp" +#include "TemporaryFile.hpp" using namespace std; using namespace ecf; @@ -133,14 +134,12 @@ int main(int argc, char* argv[]) { } { // Test time for persisting to defs file only - std::string tmpFilename = "tmp.def"; + TemporaryFile temporary("tmp_%%%%-%%%%-%%%%-%%%%.def"); timer.start(); - defs.save_as_checkpt(tmpFilename); + defs.save_as_checkpt(temporary.path()); cout << " Save as DEFS checkpoint, time taken = " << timer.format(3, Str::cpu_timer_format()) << endl; - - std::remove(tmpFilename.c_str()); } { diff --git a/ANode/parser/test/PersistHelper.cpp b/ANode/parser/test/PersistHelper.cpp index 88592ab8b..e9341f662 100644 --- a/ANode/parser/test/PersistHelper.cpp +++ b/ANode/parser/test/PersistHelper.cpp @@ -22,6 +22,7 @@ #include "Defs.hpp" #include "Ecf.hpp" #include "File.hpp" +#include "TemporaryFile.hpp" namespace fs = boost::filesystem; using namespace std; @@ -34,19 +35,16 @@ bool PersistHelper::test_persist_and_reload(const Defs& theInMemoryDefs, errorMsg_.clear(); file_size_ = 0; -#ifdef DEBUG - std::string tmpFilename = "tmp_d.def"; -#else - std::string tmpFilename = "tmp.def"; -#endif + TemporaryFile temporary("tmp_%%%%-%%%%-%%%%-%%%%.def"); + { // The file MUST be written in the *SAME* form that it was read, Otherwise they will not compare: - theInMemoryDefs.save_as_filename(tmpFilename, file_type_on_disk); + theInMemoryDefs.save_as_filename(temporary.path(), file_type_on_disk); } // Reload the file we just persisted and compare with in memory defs Defs savedDef; - return reload_from_defs_file(theInMemoryDefs, savedDef, tmpFilename, do_compare); + return reload_from_defs_file(theInMemoryDefs, savedDef, temporary.path(), do_compare); } bool PersistHelper::test_defs_checkpt_and_reload(const Defs& theInMemoryDefs, bool do_compare) { @@ -54,19 +52,16 @@ bool PersistHelper::test_defs_checkpt_and_reload(const Defs& theInMemoryDefs, bo errorMsg_.clear(); file_size_ = 0; -#ifdef DEBUG - std::string tmpFilename = "tmp_d.def"; -#else - std::string tmpFilename = "tmp.def"; -#endif + TemporaryFile temporary("tmp_%%%%-%%%%-%%%%-%%%%.def"); + { // The file MUST be written in the *SAME* form that it was read, Otherwise they will not compare: - theInMemoryDefs.save_as_checkpt(tmpFilename); + theInMemoryDefs.save_as_checkpt(temporary.path()); } // Reload the file we just persisted and compare with in memory defs Defs savedDef; - bool reload_result = reload_from_defs_file(theInMemoryDefs, savedDef, tmpFilename, do_compare); + bool reload_result = reload_from_defs_file(theInMemoryDefs, savedDef, temporary.path(), do_compare); if (reload_result) return savedDef.checkInvariants(errorMsg_); return false; @@ -97,22 +92,19 @@ bool PersistHelper::test_state_persist_and_reload_with_checkpt(const Defs& theIn DebugEquality debug_equality; // only as affect in DEBUG build -#ifdef DEBUG - std::string tmpFilename = "tmp_d.def"; -#else - std::string tmpFilename = "tmp.def"; -#endif + TemporaryFile temporary("tmp_%%%%-%%%%-%%%%-%%%%.def"); + { // The file MUST be written in the *SAME* form that it was read, Otherwise they will not compare: - theInMemoryDefs.save_as_checkpt(tmpFilename); // will save edit history + theInMemoryDefs.save_as_checkpt(temporary.path()); // will save edit history } Defs reload_strings_def; { // Open file, and parse as a string. std::string defs_as_string; - if (!File::open(tmpFilename, defs_as_string)) { - errorMsg_ += "Could not file file: " + tmpFilename; + if (!File::open(temporary.path(), defs_as_string)) { + errorMsg_ += "Could not file file: " + temporary.path(); return false; } std::string error_msg, warning; @@ -127,7 +119,7 @@ bool PersistHelper::test_state_persist_and_reload_with_checkpt(const Defs& theIn // Reload the file we just persisted and compare with in memory defs Defs reloaded_defs; - if (!reload_from_defs_file(theInMemoryDefs, reloaded_defs, tmpFilename)) { + if (!reload_from_defs_file(theInMemoryDefs, reloaded_defs, temporary.path())) { return false; } if (!reloaded_defs.checkInvariants(errorMsg_)) { @@ -271,22 +263,19 @@ bool PersistHelper::reload_from_defs_file(const Defs& theInMemoryDefs, } bool PersistHelper::reload_from_cereal_checkpt_file(const Defs& theInMemoryDefs, Defs& reloaded_defs, bool do_compare) { - // make sure edit history is saved -#ifdef DEBUG - std::string tmpCheckPt_file = "tmp.check_debug"; -#else - std::string tmpCheckPt_file = "tmp.check"; -#endif - theInMemoryDefs.cereal_save_as_checkpt(tmpCheckPt_file); + // make sure edit history is stored + TemporaryFile temporary("tmp.check_%%%%-%%%%-%%%%-%%%%"); - DebugEquality debug_equality; // only as affect in DEBUG build + theInMemoryDefs.cereal_save_as_checkpt(temporary.path()); + + DebugEquality debug_equality; // only as effect in DEBUG build try { // Parse the file we just persisted and load the defs file into memory. - reloaded_defs.cereal_restore_from_checkpt(tmpCheckPt_file); + reloaded_defs.cereal_restore_from_checkpt(temporary.path()); if (do_compare) { - // Make sure the checkpoint file file we just parsed match's the one we persisted + // Make sure the checkpoint the file we just parsed corresponds to the one we stored bool match = reloaded_defs == theInMemoryDefs; if (!match) { std::stringstream ss; @@ -322,8 +311,7 @@ bool PersistHelper::reload_from_cereal_checkpt_file(const Defs& theInMemoryDefs, errorMsg_ = "PersistHelper::reload_from_cereal_checkpt_file: " + string(e.what()); } - file_size_ = fs::file_size(tmpCheckPt_file); - std::remove(tmpCheckPt_file.c_str()); + file_size_ = temporary.size(); return errorMsg_.empty(); } diff --git a/ANode/parser/test/TemporaryFile.cpp b/ANode/parser/test/TemporaryFile.cpp new file mode 100644 index 000000000..b0a5f26fd --- /dev/null +++ b/ANode/parser/test/TemporaryFile.cpp @@ -0,0 +1,26 @@ +/* + * Copyright 2023- ECMWF. + * + * This software is licensed under the terms of the Apache Licence version 2.0 + * which can be obtained at http://www.apache.org/licenses/LICENSE-2.0. + * In applying this licence, ECMWF does not waive the privileges and immunities + * granted to it by virtue of its status as an intergovernmental organisation + * nor does it submit to any jurisdiction. + */ + +#include "TemporaryFile.hpp" + +TemporaryFile::TemporaryFile() : path_{fs::unique_path("tmp_%%%%-%%%%-%%%%-%%%%")} { +} + +TemporaryFile::TemporaryFile(const std::string& pattern) : path_{fs::unique_path(pattern)} { +} + +TemporaryFile::~TemporaryFile() { + try { + fs::remove(path_); + } + catch (...) { + // Nothing to do... + } +} diff --git a/ANode/parser/test/TemporaryFile.hpp b/ANode/parser/test/TemporaryFile.hpp new file mode 100644 index 000000000..ff7197ddc --- /dev/null +++ b/ANode/parser/test/TemporaryFile.hpp @@ -0,0 +1,34 @@ +/* + * Copyright 2023- ECMWF. + * + * This software is licensed under the terms of the Apache Licence version 2.0 + * which can be obtained at http://www.apache.org/licenses/LICENSE-2.0. + * In applying this licence, ECMWF does not waive the privileges and immunities + * granted to it by virtue of its status as an intergovernmental organisation + * nor does it submit to any jurisdiction. + */ + +#ifndef ECFLOW_NODE_PARSER_TEST_TEMPORARYFILE_HPP +#define ECFLOW_NODE_PARSER_TEST_TEMPORARYFILE_HPP + +#include + +#include + +namespace fs = boost::filesystem; + +class TemporaryFile { +public: + TemporaryFile(); + explicit TemporaryFile(const std::string& pattern); + + ~TemporaryFile(); + + [[nodiscard]] inline std::string path() const { return path_.string(); } + [[nodiscard]] inline size_t size() const { return fs::file_size(path_); } + +private: + fs::path path_; +}; + +#endif diff --git a/ANode/parser/test/TestSingleDefsFile.cpp b/ANode/parser/test/TestSingleDefsFile.cpp index 3e55ef494..3b25eff7e 100644 --- a/ANode/parser/test/TestSingleDefsFile.cpp +++ b/ANode/parser/test/TestSingleDefsFile.cpp @@ -18,10 +18,8 @@ #include #include -#include #include #include - #include #include @@ -39,6 +37,7 @@ #include "Suite.hpp" #include "System.hpp" #include "Task.hpp" +#include "TemporaryFile.hpp" namespace fs = boost::filesystem; using namespace std; @@ -173,15 +172,11 @@ BOOST_AUTO_TEST_CASE(test_single_defs) { { // Test time for persisting to defs file only -#ifdef DEBUG - std::string tmpFilename = "tmp_d.def"; -#else - std::string tmpFilename = "tmp.def"; -#endif + TemporaryFile temporary("tmp_%%%%-%%%%-%%%%-%%%%.def"); timer.start(); PrintStyle style(PrintStyle::DEFS); - std::ofstream ofs(tmpFilename.c_str()); + std::ofstream ofs(temporary.path()); ofs << defs; BOOST_CHECK_MESSAGE(get_seconds(timer.elapsed().user) < expectedTimeForDefsPersistOnly, "Performance regression, expected < " << expectedTimeForDefsPersistOnly @@ -189,8 +184,6 @@ BOOST_AUTO_TEST_CASE(test_single_defs) { << timer.format(3, Str::cpu_timer_format())); cout << " Persist only, time taken = " << timer.format(3, Str::cpu_timer_format()) << " < limit(" << expectedTimeForDefsPersistOnly << ")" << endl; - - std::remove(tmpFilename.c_str()); } { @@ -331,10 +324,6 @@ BOOST_AUTO_TEST_CASE(test_single_defs) { // Explicitly destroy, To keep valgrind happy Log::destroy(); System::destroy(); - - // cout << "Printing Defs \n"; - // PrintStyle style(PrintStyle::DEFS); - // std::cout << defs; } BOOST_AUTO_TEST_SUITE_END()