From 1b2ea5853ec7e490acec32e0e028ce0e648ea202 Mon Sep 17 00:00:00 2001 From: Paul Bauer Date: Fri, 24 Apr 2020 11:27:15 +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 | 24 +++++++------- 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 | 21 ++++++++++++- .../mdrun/tests/densityfittingmodule.cpp | 8 +++-- src/testutils/testasserts.h | 31 ++++++++++++++++++- 9 files changed, 109 insertions(+), 33 deletions(-) diff --git a/admin/gitlab-ci/gromacs.gitlab-ci.yml b/admin/gitlab-ci/gromacs.gitlab-ci.yml index 1285c582c6..4f8e644cb9 100644 --- a/admin/gitlab-ci/gromacs.gitlab-ci.yml +++ b/admin/gitlab-ci/gromacs.gitlab-ci.yml @@ -261,15 +261,16 @@ gromacs:clang-8:configure: gromacs:clang-3.6:configure: extends: - - .gromacs:base:configure - - .use-clang:base - - .rules:merge-requests + - .gromacs:base:configure + - .use-clang:base + - .use-mpi + - .rules:merge-requests image: gromacs/cmake-3.9.6-llvm-3.6-amdopencl-openmpi:2020 variables: COMPILER_MAJOR_VERSION: "3.6" CMAKE_PRECISION_OPTIONS: "-DGMX_DOUBLE=ON" - CMAKE_MPI_OPTIONS: "-DGMX_OPENMP=OFF" CMAKE_SIMD_OPTIONS: "-DGMX_SIMD=SSE4.1" + CMAKE_EXTRA_OPTIONS: "-DGMX_OPENMP=OFF" gromacs:clang-static-analyzer:configure: extends: @@ -329,15 +330,16 @@ gromacs:clang-3.6:release:configure: extends: - .gromacs:base:release:configure - .use-clang:base + - .use-mpi - .rules:nightly-only-for-release image: gromacs/cmake-3.9.6-llvm-3.6-amdopencl-openmpi:2020 variables: COMPILER_MAJOR_VERSION: "3.6" RELEASE_BUILD_DIR: release-builds-clang CMAKE_PRECISION_OPTIONS: "-DGMX_DOUBLE=ON" - CMAKE_MPI_OPTIONS: "-DGMX_OPENMP=OFF" CMAKE_SIMD_OPTIONS: "-DGMX_SIMD=SSE4.1" CMAKE_BUILD_TYPE_OPTIONS: "-DCMAKE_BUILD_TYPE=RelWithAssert" + CMAKE_EXTRA_OPTIONS: "-DGMX_OPENMP=OFF" dependencies: - archive:package - regressiontests:package @@ -543,11 +545,7 @@ gromacs:clang-8:release:build: - ctest -D $CTEST_RUN_MODE --output-on-failure | tee ctestLog.log || true - awk '/The following tests FAILED/,/^Errors while running CTest|^$/' ctestLog.log | tee ctestErrors.log - - if [ $CTEST_RUN_MODE == "ExperimentalTest" ] ; then - xsltproc scripts/CTest2JUnit.xsl Testing/`head -n 1 < Testing/TAG`/Test.xml > JUnitTestResults.xml; - else - touch JUnitTestResults.xml; - fi + - xsltproc scripts/CTest2JUnit.xsl Testing/`head -n 1 < Testing/TAG`/*.xml > JUnitTestResults.xml - if [ -s ctestErrors.log ] ; then echo "Error during running ctest"; exit 1; @@ -587,8 +585,8 @@ gromacs:clang-8:release:build: - cd regressiontests - perl gmxtest.pl $REGRESSIONTEST_PARALLEL $REGRESSIONTEST_TOTAL_RANK_NUMBER -ntomp $REGRESSIONTEST_OMP_RANK_NUMBER -npme $REGRESSIONTEST_PME_RANK_NUMBER $REGRESSIONTEST_DOUBLE $REGRESSIONTEST_MPI_RUN_COMMAND -xml all artifacts: - reports: - junit: regressiontests/gmxtest.xml + paths: + - regressiontests/gmxtest.xml when: always expire_in: 1 week @@ -715,6 +713,7 @@ gromacs:clang-3.6:regressiontest: variables: REGRESSIONTEST_DOUBLE: "-double" REGRESSIONTEST_OMP_RANK_NUMBER: 0 + REGRESSIONTEST_PARALLEL: "-np" needs: - job: gromacs:clang-3.6:build - job: regressiontests:prepare @@ -789,6 +788,7 @@ gromacs:clang-3.6:release:regressiontest: BUILD_DIR: release-builds-clang REGRESSIONTEST_DOUBLE: "-double" REGRESSIONTEST_OMP_RANK_NUMBER: 0 + REGRESSIONTEST_PARALLEL: "-np" needs: - job: gromacs:clang-3.6:release:build - job: regressiontests:package 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 b5f84a86bf..9d27cc96ac 100644 --- a/src/gromacs/utility/tests/enumerationhelpers.cpp +++ b/src/gromacs/utility/tests/enumerationhelpers.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,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 @@ -135,6 +139,21 @@ TEST(EnumerationHelpersTest, EnumerationArrayWorks) EXPECT_EQ(fooStrings[Foo::Fooz], "Fooz"); } +TEST(EnumerationHelpersTest, EnumerationArrayCountIsSafe) +{ + using FooArray = EnumerationArray; + const FooArray fooStrings{ { "Bar", "Baz", "Fooz" } }; + + // Ensures that the assertions in EnumerationArray::operator[] + // would fire if an out-range value (including Count) was used. + EXPECT_LE(fooStrings.size(), size_t(Foo::Count)); +#ifndef NDEBUG + // Tests (where possible) that those assertions do fire in a build + // with debug behavior. + GMX_EXPECT_DEATH_IF_SUPPORTED(fooStrings[Foo::Count], "index out of range"); +#endif +} + //! Helper function void func(ArrayRef a) { 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 1a243802cd..67141294df 100644 --- a/src/testutils/testasserts.h +++ b/src/testutils/testasserts.h @@ -1,7 +1,7 @@ /* * This file is part of the GROMACS molecular simulation package. * - * Copyright (c) 2013,2014,2015,2016,2017,2018,2019, by the GROMACS development team, led by + * Copyright (c) 2013,2014,2015,2016,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. @@ -41,6 +41,7 @@ * - exceptions * - floating-point comparison * - comparison against NULL + * - death tests * * \if internal * \todo @@ -206,6 +207,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