From 16377441fa7ffcb056dbccf63ed7d4331178eba6 Mon Sep 17 00:00:00 2001 From: Paul Bauer Date: Thu, 23 Apr 2020 12:07:38 +0000 Subject: [PATCH] Fix parallel testing Change testing to be more robust in threaded setups. Also change on merge test to use real MPI to make sure this works correctly. Change-Id: Id78b213291bf7cc225fc2564a23f0d85362467fd --- admin/gitlab-ci/gromacs.gitlab-ci.yml | 2 ++ src/api/cpp/tests/testingconfiguration.h | 20 ++++++++++++- src/gromacs/compat/tests/pointers.cpp | 9 ++++-- .../coordinateio/tests/outputadapters.cpp | 2 +- .../coordinateio/tests/outputadapters.h | 8 ++--- src/gromacs/gmxpreprocess/tests/readir.cpp | 19 +++++++----- .../utility/tests/enumerationhelpers.cpp | 6 +++- .../mdrun/tests/densityfittingmodule.cpp | 8 +++-- src/testutils/testasserts.h | 29 +++++++++++++++++++ 9 files changed, 83 insertions(+), 20 deletions(-) diff --git a/admin/gitlab-ci/gromacs.gitlab-ci.yml b/admin/gitlab-ci/gromacs.gitlab-ci.yml index afffc9cfb4..418a646ced 100644 --- a/admin/gitlab-ci/gromacs.gitlab-ci.yml +++ b/admin/gitlab-ci/gromacs.gitlab-ci.yml @@ -263,6 +263,7 @@ gromacs:clang-9:configure: extends: - .gromacs:base:configure - .use-clang:base + - .use-mpi - .rules:merge-requests image: gromacs/cmake-3.15.7-llvm-9-openmpi:master variables: @@ -708,6 +709,7 @@ gromacs:clang-9:regressiontest: image: gromacs/cmake-3.15.7-llvm-9-openmpi:master variables: REGRESSIONTEST_DOUBLE: "-double" + REGRESSIONTEST_PARALLEL: "-np" needs: - job: gromacs:clang-9:build - job: regressiontests:prepare diff --git a/src/api/cpp/tests/testingconfiguration.h b/src/api/cpp/tests/testingconfiguration.h index f3bef5f6e0..2d4cf147f0 100644 --- a/src/api/cpp/tests/testingconfiguration.h +++ b/src/api/cpp/tests/testingconfiguration.h @@ -1,7 +1,7 @@ /* * This file is part of the GROMACS molecular simulation package. * - * Copyright (c) 2018,2019, by the GROMACS development team, led by + * Copyright (c) 2018,2019,2020, by the GROMACS development team, led by * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl, * and including many others, as listed in the AUTHORS file in the * top-level source directory and at http://www.gromacs.org. @@ -41,6 +41,8 @@ #include #include +#include "config.h" + #include "gromacs/gmxpreprocess/grompp.h" #include "gromacs/math/vec.h" #include "gromacs/utility/stringutil.h" @@ -56,6 +58,11 @@ namespace gmxapi namespace testing { +#if GMX_OPENMP || defined(DOXYGEN) +//! Number of OpenMP threads for child mdrun call. +static constexpr int g_numOpenMPThreads = 2; +#endif + /*! \brief Helper function to get step size as floating point number. * * A step size of 0.002ps is suitable for the simulation. @@ -127,6 +134,17 @@ public: mdArgs.emplace_back(runner_.edrFileName_); mdArgs.emplace_back("-cpo"); mdArgs.emplace_back(runner_.cptFileName_); +#if GMX_THREAD_MPI + /* This should be handled through the actual API we have for getting + * ranks, but currently this leads to data races right now */ + mdArgs.emplace_back("-ntmpi"); + mdArgs.emplace_back("1"); +#endif + +#if GMX_OPENMP + mdArgs.emplace_back("-ntomp"); + mdArgs.emplace_back(std::to_string(g_numOpenMPThreads)); +#endif return mdArgs; } diff --git a/src/gromacs/compat/tests/pointers.cpp b/src/gromacs/compat/tests/pointers.cpp index 8e7b3f8e4f..991f9a3172 100644 --- a/src/gromacs/compat/tests/pointers.cpp +++ b/src/gromacs/compat/tests/pointers.cpp @@ -1,7 +1,7 @@ /* * This file is part of the GROMACS molecular simulation package. * - * Copyright (c) 2018,2019, by the GROMACS development team, led by + * Copyright (c) 2018,2019,2020, by the GROMACS development team, led by * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl, * and including many others, as listed in the AUTHORS file in the * top-level source directory and at http://www.gromacs.org. @@ -42,11 +42,14 @@ #include "gromacs/compat/pointers.h" +#include "config.h" + #include #include #include +#include "testutils/testasserts.h" namespace gmx { @@ -65,10 +68,12 @@ TEST(NotNullConstruction, Works) * will not trigger the assert when using the pointer without * a valid object. This was needed due to an internal error * being triggered instead with the compiler under this condition. + * + * Death tests can also not be used safely in a parallel environment. */ # if !defined(__INTEL_COMPILER) || !(__INTEL_COMPILER == 1800 && __INTEL_COMPILER_UPDATE == 0) int* nullPointer = nullptr; - EXPECT_DEATH_IF_SUPPORTED(not_null invalidNullPointer(nullPointer), ""); + GMX_EXPECT_DEATH_IF_SUPPORTED(not_null invalidNullPointer(nullPointer), ""); # endif #endif diff --git a/src/gromacs/coordinateio/tests/outputadapters.cpp b/src/gromacs/coordinateio/tests/outputadapters.cpp index ec4023bd95..d71abf1d0e 100644 --- a/src/gromacs/coordinateio/tests/outputadapters.cpp +++ b/src/gromacs/coordinateio/tests/outputadapters.cpp @@ -1,7 +1,7 @@ /* * This file is part of the GROMACS molecular simulation package. * - * Copyright (c) 2019, by the GROMACS development team, led by + * Copyright (c) 2019,2020, by the GROMACS development team, led by * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl, * and including many others, as listed in the AUTHORS file in the * top-level source directory and at http://www.gromacs.org. diff --git a/src/gromacs/coordinateio/tests/outputadapters.h b/src/gromacs/coordinateio/tests/outputadapters.h index 5e0437da9e..37f24b6eb9 100644 --- a/src/gromacs/coordinateio/tests/outputadapters.h +++ b/src/gromacs/coordinateio/tests/outputadapters.h @@ -1,7 +1,7 @@ /* * This file is part of the GROMACS molecular simulation package. * - * Copyright (c) 2019, by the GROMACS development team, led by + * Copyright (c) 2019,2020, by the GROMACS development team, led by * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl, * and including many others, as listed in the AUTHORS file in the * top-level source directory and at http://www.gromacs.org. @@ -160,9 +160,9 @@ public: addOptionForSelection(&sel, false); setSelectionOptionValues(getOption(), &sel, false); - ASSERT_DEATH_IF_SUPPORTED(adapters.addAdapter(std::make_unique(sel), - CoordinateFileFlags::RequireCoordinateSelection), - "Need a valid selection out of simple atom indices"); + GMX_ASSERT_DEATH_IF_SUPPORTED(adapters.addAdapter(std::make_unique(sel), + CoordinateFileFlags::RequireCoordinateSelection), + "Need a valid selection out of simple atom indices"); } }; diff --git a/src/gromacs/gmxpreprocess/tests/readir.cpp b/src/gromacs/gmxpreprocess/tests/readir.cpp index b9d06c00b6..80e407451b 100644 --- a/src/gromacs/gmxpreprocess/tests/readir.cpp +++ b/src/gromacs/gmxpreprocess/tests/readir.cpp @@ -1,7 +1,7 @@ /* * This file is part of the GROMACS molecular simulation package. * - * Copyright (c) 2017,2018,2019, by the GROMACS development team, led by + * Copyright (c) 2017,2018,2019,2020, by the GROMACS development team, led by * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl, * and including many others, as listed in the AUTHORS file in the * top-level source directory and at http://www.gromacs.org. @@ -47,6 +47,8 @@ #include "gromacs/gmxpreprocess/readir.h" +#include "config.h" + #include #include @@ -61,8 +63,8 @@ #include "gromacs/utility/unique_cptr.h" #include "testutils/refdata.h" +#include "testutils/testasserts.h" #include "testutils/testfilemanager.h" - namespace gmx { namespace test @@ -139,7 +141,7 @@ TEST_F(GetIrTest, HandlesDifferentKindsOfMdpLines) TEST_F(GetIrTest, RejectsNonCommentLineWithNoEquals) { const char* inputMdpFile = "title simulation"; - EXPECT_DEATH_IF_SUPPORTED(runTest(inputMdpFile), "No '=' to separate"); + GMX_EXPECT_DEATH_IF_SUPPORTED(runTest(inputMdpFile), "No '=' to separate"); } TEST_F(GetIrTest, AcceptsKeyWithoutValue) @@ -152,13 +154,14 @@ TEST_F(GetIrTest, AcceptsKeyWithoutValue) TEST_F(GetIrTest, RejectsValueWithoutKey) { const char* inputMdpFile = "= -I/home/me/stuff"; - EXPECT_DEATH_IF_SUPPORTED(runTest(inputMdpFile), "No .mdp parameter name was found"); + GMX_EXPECT_DEATH_IF_SUPPORTED(runTest(inputMdpFile), "No .mdp parameter name was found"); } TEST_F(GetIrTest, RejectsEmptyKeyAndEmptyValue) { const char* inputMdpFile = " = "; - EXPECT_DEATH_IF_SUPPORTED(runTest(inputMdpFile), "No .mdp parameter name or value was found"); + GMX_EXPECT_DEATH_IF_SUPPORTED(runTest(inputMdpFile), + "No .mdp parameter name or value was found"); } TEST_F(GetIrTest, AcceptsDefineParametersWithValuesIncludingAssignment) @@ -198,8 +201,8 @@ TEST_F(GetIrTest, AcceptsElectricFieldOscillating) TEST_F(GetIrTest, RejectsDuplicateOldAndNewKeys) { const char* inputMdpFile[] = { "verlet-buffer-drift = 1.3", "verlet-buffer-tolerance = 2.7" }; - EXPECT_DEATH_IF_SUPPORTED(runTest(joinStrings(inputMdpFile, "\n")), - "A parameter is present with both"); + GMX_EXPECT_DEATH_IF_SUPPORTED(runTest(joinStrings(inputMdpFile, "\n")), + "A parameter is present with both"); } TEST_F(GetIrTest, AcceptsImplicitSolventNo) @@ -211,7 +214,7 @@ TEST_F(GetIrTest, AcceptsImplicitSolventNo) TEST_F(GetIrTest, RejectsImplicitSolventYes) { const char* inputMdpFile = "implicit-solvent = yes"; - EXPECT_DEATH_IF_SUPPORTED(runTest(inputMdpFile), "Invalid enum"); + GMX_EXPECT_DEATH_IF_SUPPORTED(runTest(inputMdpFile), "Invalid enum"); } TEST_F(GetIrTest, AcceptsMimic) diff --git a/src/gromacs/utility/tests/enumerationhelpers.cpp b/src/gromacs/utility/tests/enumerationhelpers.cpp index 3e6c504355..9d27cc96ac 100644 --- a/src/gromacs/utility/tests/enumerationhelpers.cpp +++ b/src/gromacs/utility/tests/enumerationhelpers.cpp @@ -42,12 +42,16 @@ #include "gromacs/utility/enumerationhelpers.h" +#include "config.h" + #include #include #include "gromacs/utility/arrayref.h" +#include "testutils/testasserts.h" + namespace gmx { namespace @@ -146,7 +150,7 @@ TEST(EnumerationHelpersTest, EnumerationArrayCountIsSafe) #ifndef NDEBUG // Tests (where possible) that those assertions do fire in a build // with debug behavior. - EXPECT_DEATH_IF_SUPPORTED(fooStrings[Foo::Count], "index out of range"); + GMX_EXPECT_DEATH_IF_SUPPORTED(fooStrings[Foo::Count], "index out of range"); #endif } diff --git a/src/programs/mdrun/tests/densityfittingmodule.cpp b/src/programs/mdrun/tests/densityfittingmodule.cpp index 5a2e068f8b..7149eee08b 100644 --- a/src/programs/mdrun/tests/densityfittingmodule.cpp +++ b/src/programs/mdrun/tests/densityfittingmodule.cpp @@ -1,7 +1,7 @@ /* * This file is part of the GROMACS molecular simulation package. * - * Copyright (c) 2019, by the GROMACS development team, led by + * Copyright (c) 2019,2020, by the GROMACS development team, led by * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl, * and including many others, as listed in the AUTHORS file in the * top-level source directory and at http://www.gromacs.org. @@ -42,6 +42,8 @@ */ #include "gmxpre.h" +#include "config.h" + #include #include @@ -171,8 +173,8 @@ TEST_F(DensityFittingTest, GromppErrorWhenEnergyEvaluationFrequencyMismatch) { runner_.useStringAsMdpFile(mdpMdDensfitYesUnsetValues + mdpEnergyAndDensityfittingIntervalMismatch_); - EXPECT_DEATH_IF_SUPPORTED(runner_.callGrompp(), - ".*is not a multiple of density-guided-simulation-nst.*"); + GMX_EXPECT_DEATH_IF_SUPPORTED(runner_.callGrompp(), + ".*is not a multiple of density-guided-simulation-nst.*"); } /* Fit a subset of three of twelve argon atoms into a reference density diff --git a/src/testutils/testasserts.h b/src/testutils/testasserts.h index f1baab8be7..3e377280f8 100644 --- a/src/testutils/testasserts.h +++ b/src/testutils/testasserts.h @@ -42,6 +42,7 @@ * - exceptions * - floating-point comparison * - comparison against NULL + * - death tests * * \if internal * \todo @@ -207,6 +208,34 @@ void processExpectedException(const std::exception& ex); */ #define ASSERT_NO_THROW_GMX(statement) GMX_TEST_NO_THROW_(statement, GTEST_FATAL_FAILURE_) +/*! \brief + * Wrapper around EXPECT_DEATH_IF_SUPPORTED gtest macro for thread safe execution. + * + * Makes sure that tests that are supposed to trigger an assertion are only executed + * in a threadsafe environment, and not when when running under e.g. MPI. + * + * \hideinitializer + */ +#define GMX_EXPECT_DEATH_IF_SUPPORTED(expr, msg) \ + if (!GMX_LIB_MPI) \ + { \ + EXPECT_DEATH_IF_SUPPORTED(expr, msg); \ + } + +/*! \brief + * Wrapper around ASSERT_DEATH_IF_SUPPORTED gtest macro for thread safe execution. + * + * Makes sure that tests that are supposed to trigger an assertion are only executed + * in a threadsafe environment when running under e.g. MPI. + * + * \hideinitializer + */ +#define GMX_ASSERT_DEATH_IF_SUPPORTED(expr, msg) \ + if (!GMX_LIB_MPI) \ + { \ + ASSERT_DEATH_IF_SUPPORTED(expr, msg); \ + } + //! \} /*! \libinternal \brief -- 2.22.0