Write additional unit tests (espressomd#4420)

Description of changes:
- improve unit testing of utils, core and script interface
- run MPI-capable unit tests in parallel
- improve separation of concerns
    - significantly narrow inclusion of `MpiCallbacks.hpp`
    - run include what you use
- fix minor regressions in the core

src/core/RuntimeErrorCollector.hpp

@@ -54,17 +54,18 @@ class RuntimeErrorCollector {
              const char *file, int line);
 
   /**
-   * \brief Return the number of all flying messages.
+   * \brief Get the number of all flying messages on all nodes.
    *
    * @return Total number of messages.
    */
   int count() const;
 
   /**
-   * \brief Number of Messages that have at least level level.
+   * \brief Get the number of messages that have at least severity
+   * @p level on this node.
    *
    * @param level Severity filter.
-   * @return Number of Messages that have at least level.
+   * @return Number of messages that match the filter.
    */
   int count(RuntimeError::ErrorLevel level);
 

src/core/constraints/Constraint.hpp

@@ -21,7 +21,8 @@
 
 #include "Observable_stat.hpp"
 #include "Particle.hpp"
-#include "energy.hpp"
+
+#include <utils/Vector.hpp>
 
 namespace Constraints {
 class Constraint {

src/core/forces.hpp

@@ -32,7 +32,6 @@
 
 #include "CellStructure.hpp"
 #include "ParticleRange.hpp"
-#include "actor/Actor.hpp"
 #include "actor/ActorList.hpp"
 
 #include <utils/Vector.hpp>

src/core/interactions.cpp

@@ -20,12 +20,11 @@
  */
 #include "communication.hpp"
 
-#include "TabulatedPotential.hpp"
 #include "bonded_interactions/bonded_interaction_data.hpp"
-#include "bonded_interactions/bonded_tab.hpp"
 #include "collision.hpp"
 #include "electrostatics_magnetostatics/coulomb.hpp"
 #include "electrostatics_magnetostatics/dipole.hpp"
+#include "event.hpp"
 #include "grid.hpp"
 
 #include "serialization/IA_parameters.hpp"

src/core/nonbonded_interactions/nonbonded_interaction_data.hpp

@@ -29,11 +29,8 @@
 
 #include <utils/index.hpp>
 
-#include <MpiCallbacks.hpp>
 #include <algorithm>
 #include <cassert>
-#include <communication.hpp>
-#include <event.hpp>
 #include <string>
 #include <vector>
 

src/core/unit_tests/CMakeLists.txt

@@ -23,7 +23,7 @@ include(unit_test)
 unit_test(NAME RuntimeError_test SRC RuntimeError_test.cpp DEPENDS
           Boost::serialization)
 unit_test(NAME RuntimeErrorCollector_test SRC RuntimeErrorCollector_test.cpp
-          DEPENDS EspressoCore Boost::mpi MPI::MPI_CXX)
+          DEPENDS EspressoCore Boost::mpi MPI::MPI_CXX NUM_PROC 2)
 unit_test(NAME EspressoSystemStandAlone_test SRC
           EspressoSystemStandAlone_test.cpp DEPENDS EspressoCore Boost::mpi
           MPI::MPI_CXX NUM_PROC 2)

src/core/unit_tests/RuntimeErrorCollector_test.cpp

@@ -32,59 +32,43 @@
 #include <boost/mpi.hpp>
 
 #include <algorithm>
-#include <functional>
 #include <vector>
 
-int main(int argc, char **argv) {
-  boost::mpi::environment mpi_env(argc, argv);
-
-  return boost::unit_test::unit_test_main(init_unit_test, argc, argv);
-}
-
-namespace Testing {
-
-void reduce_and_check(const boost::mpi::communicator &comm, bool local_value) {
-  if (comm.rank() == 0) {
-    bool total = true;
-    boost::mpi::reduce(comm, local_value, total, std::logical_and<bool>(), 0);
-    BOOST_CHECK(total);
-  } else {
-    boost::mpi::reduce(comm, local_value, std::logical_and<bool>(), 0);
-  }
-}
-} // namespace Testing
-
 using ErrorHandling::RuntimeError;
 using ErrorHandling::RuntimeErrorCollector;
+using ErrorLevel = ErrorHandling::RuntimeError::ErrorLevel;
 
 BOOST_AUTO_TEST_CASE(count) {
   boost::mpi::communicator world;
 
   RuntimeErrorCollector rec(world);
 
-  Testing::reduce_and_check(world, rec.count() == 0);
+  BOOST_REQUIRE_EQUAL(rec.count(), 0);
+  world.barrier();
 
   /* MPI guarantees that size >= 1 and rank 0 exists. */
-  if (world.rank() == (world.size() - 1)) {
+  auto const rank_of_error = world.size() - 1;
+  if (world.rank() == rank_of_error) {
     rec.error("Test_error", "Test_functions", "Test_file", 42);
   }
-
-  Testing::reduce_and_check(world, rec.count() == 1);
-
+  world.barrier();
+  BOOST_REQUIRE_EQUAL(rec.count(), 1);
+  world.barrier();
   rec.warning("Test_error", "Test_functions", "Test_file", 42);
-
-  Testing::reduce_and_check(world, rec.count() == world.size() + 1);
+  world.barrier();
+  BOOST_REQUIRE_EQUAL(rec.count(), world.size() + 1);
 
   /* There should now be one error and world.size() warnings */
-  Testing::reduce_and_check(world,
-                            rec.count(RuntimeError::ErrorLevel::ERROR) == 1);
-  /* All messages are at least WARNING or higher. */
-  {
-    /* Beware of the execution order */
-    int total = rec.count();
-    Testing::reduce_and_check(
-        world, rec.count(RuntimeError::ErrorLevel::WARNING) == total);
-  }
+  auto const n_local_errors = static_cast<int>(world.rank() == rank_of_error);
+  auto const n_local_warnings = n_local_errors + 1;
+  BOOST_REQUIRE_EQUAL(rec.count(ErrorLevel::ERROR), n_local_errors);
+  BOOST_REQUIRE_EQUAL(rec.count(ErrorLevel::WARNING), n_local_warnings);
+
+  /* Clear list of errors */
+  world.barrier();
+  rec.clear();
+  world.barrier();
+  BOOST_REQUIRE_EQUAL(rec.count(), 0);
 }
 
 /*
@@ -100,10 +84,11 @@ BOOST_AUTO_TEST_CASE(gather) {
 
   rec.error("Test_error", "Test_functions", "Test_file", world.rank());
   rec.warning("Test_error", "Test_functions", "Test_file", world.rank());
+  world.barrier();
 
   if (world.rank() == 0) {
     /* Gathered error messages */
-    auto results = rec.gather();
+    auto const results = rec.gather();
     /* Track how many messages we have seen from which node. */
     std::vector<int> present(world.size());
 
@@ -115,18 +100,24 @@ BOOST_AUTO_TEST_CASE(gather) {
     BOOST_CHECK(std::all_of(present.begin(), present.end(),
                             [](int i) { return i == 2; }));
     /* Count warnings, should be world.rank() many */
-    BOOST_CHECK(std::count_if(
-                    results.begin(), results.end(), [](const RuntimeError &e) {
-                      return e.level() == RuntimeError::ErrorLevel::WARNING;
-                    }) == world.size());
+    BOOST_CHECK(std::count_if(results.begin(), results.end(),
+                              [](const RuntimeError &e) {
+                                return e.level() == ErrorLevel::WARNING;
+                              }) == world.size());
     /* Count errors, should be world.rank() many */
-    BOOST_CHECK(std::count_if(
-                    results.begin(), results.end(), [](const RuntimeError &e) {
-                      return e.level() == RuntimeError::ErrorLevel::ERROR;
-                    }) == world.size());
+    BOOST_CHECK(std::count_if(results.begin(), results.end(),
+                              [](const RuntimeError &e) {
+                                return e.level() == ErrorLevel::ERROR;
+                              }) == world.size());
   } else {
     rec.gather_local();
   }
 
-  Testing::reduce_and_check(world, rec.count() == 0);
+  BOOST_REQUIRE_EQUAL(rec.count(), 0);
+}
+
+int main(int argc, char **argv) {
+  boost::mpi::environment mpi_env(argc, argv);
+
+  return boost::unit_test::unit_test_main(init_unit_test, argc, argv);
 }

src/core/unit_tests/field_coupling_fields_test.cpp

@@ -49,6 +49,7 @@ BOOST_AUTO_TEST_CASE(jacobian_type_test) {
   static_assert(
       is_same<jacobian_type<double, 2>, Utils::Matrix<double, 2, 3>>::value,
       "");
+  BOOST_TEST_PASSPOINT();
 }
 
 BOOST_AUTO_TEST_CASE(constant_scalar_field) {

src/python/espressomd/integrate.pyx

@@ -380,7 +380,7 @@ IF NPT:
             check_type_or_throw_except(
                 self._params["piston"], 1, float, "piston must be a float")
             check_type_or_throw_except(
-                self._params["direction"], 3, int, "direction must be an array-like of 3 bools")
+                self._params["direction"], 3, bool, "direction must be an array-like of 3 bools")
             check_type_or_throw_except(
                 self._params["cubic_box"], 1, int, "cubic_box must be a bool")
 

src/python/espressomd/utils.pyx

@@ -37,21 +37,23 @@ cpdef check_type_or_throw_except(x, n, t, msg):
                 raise ValueError(
                     msg + f" -- {len(x)} values were given but {n} were expected.")
             for i in range(len(x)):
-                if not isinstance(x[i], t):
-                    if not ((t == float and is_valid_type(x[i], int))
-                            or (t == float and issubclass(type(x[i]), np.integer))) \
-                            and not (t == int and issubclass(type(x[i]), np.integer)):
-                        raise ValueError(
-                            msg + f" -- Item {i} was of type {type(x[i]).__name__}")
+                if not (isinstance(x[i], t)
+                        or (t == float and is_valid_type(x[i], int))
+                        or (t == int and is_valid_type(x[i], int))
+                        or (t == bool and (is_valid_type(x[i], bool) or x[i] in (0, 1)))):
+                    raise ValueError(
+                        msg + f" -- Item {i} was of type {type(x[i]).__name__}")
         else:
             # if n>1, but the user passed a single value, also throw exception
             raise ValueError(
                 msg + f" -- A single value was given but {n} were expected.")
     else:
         # N=1 and a single value
         if not isinstance(x, t):
-            if not (t == float and is_valid_type(x, int)) and not (
-                    t == int and issubclass(type(x), np.integer)):
+            if not (isinstance(x, t)
+                    or (t == float and is_valid_type(x, int))
+                    or (t == int and is_valid_type(x, int))
+                    or (t == bool and is_valid_type(x, bool))):
                 raise ValueError(msg + f" -- Got an {type(x).__name__}")
 
 
@@ -275,7 +277,7 @@ def nesting_level(obj):
 
 def is_valid_type(value, t):
     """
-    Extended checks for numpy int and float types.
+    Extended checks for numpy int, float and bool types.
 
     """
     if value is None:
@@ -288,6 +290,8 @@ def is_valid_type(value, t):
                 value, (float, np.float16, np.float32, np.float64, np.float128, np.longdouble))
         return isinstance(
             value, (float, np.float16, np.float32, np.float64, np.longdouble))
+    elif t == bool:
+        return isinstance(value, (bool, np.bool, np.bool_))
     else:
         return isinstance(value, t)
 

src/script_interface/CylindricalTransformationParameters.hpp

@@ -22,11 +22,13 @@
 #ifndef SCRIPT_INTERFACE_CYL_TRANSFORM_PARAMS_HPP
 #define SCRIPT_INTERFACE_CYL_TRANSFORM_PARAMS_HPP
 
-#include <stdexcept>
+#include "script_interface/auto_parameters/AutoParameters.hpp"
+#include "script_interface/get_value.hpp"
 
-#include "script_interface/ScriptInterface.hpp"
+#include <utils/math/cylindrical_transformation_parameters.hpp>
 
-#include "utils/math/cylindrical_transformation_parameters.hpp"
+#include <memory>
+#include <stdexcept>
 
 namespace ScriptInterface {
 

src/script_interface/ScriptInterface.hpp

@@ -24,7 +24,7 @@
 
 #include <type_traits>
 
-#include "ContextManager.hpp"
+#include "Context.hpp"
 #include "ObjectHandle.hpp"
 #include "Variant.hpp"
 #include "auto_parameters/AutoParameters.hpp"

src/script_interface/constraints/ExternalField.hpp

@@ -26,6 +26,7 @@
 #include "fields.hpp"
 
 #include "script_interface/ScriptInterface.hpp"
+#include "script_interface/constraints/Constraint.hpp"
 
 #include "core/constraints/Constraint.hpp"
 #include "core/constraints/ExternalField.hpp"

src/script_interface/constraints/ExternalPotential.hpp

@@ -29,6 +29,7 @@
 #include "fields.hpp"
 
 #include "script_interface/ScriptInterface.hpp"
+#include "script_interface/constraints/Constraint.hpp"
 
 #include <utils/Vector.hpp>
 

src/script_interface/get_value.hpp

@@ -181,6 +181,9 @@ template <> struct get_value_helper<std::unordered_map<int, Variant>, void> {
   }
 };
 
+/** Custom error for a conversion that fails when the value is a nullptr. */
+class bad_get_nullptr : public boost::bad_get {};
+
 /* This allows direct retrieval of a shared_ptr to the object from
  * an ObjectRef variant. If the type is a derived type, the type is
  * also checked.
@@ -193,7 +196,7 @@ struct get_value_helper<
   std::shared_ptr<T> operator()(Variant const &v) const {
     auto so_ptr = boost::get<ObjectRef>(v);
     if (!so_ptr) {
-      throw boost::bad_get{};
+      throw bad_get_nullptr{};
     }
 
     auto t_ptr = std::dynamic_pointer_cast<T>(so_ptr);
@@ -205,6 +208,32 @@ struct get_value_helper<
     throw boost::bad_get{};
   }
 };
+
+/**
+ * @brief Re-throw a @c boost::bad_get exception wrapped in an @ref Exception.
+ * Write a custom error message for invalid conversions due to type mismatch
+ * and due to nullptr values, possibly with context information if the variant
+ * is in a container.
+ * @tparam T     Type which the variant was supposed to convert to
+ * @tparam U     Type of the container the variant is contained in
+ */
+template <typename T, typename U = void>
+inline void handle_bad_get(Variant const &v) {
+  using Utils::demangle;
+  auto const what = "Provided argument of type " + type_label(v);
+  auto const context =
+      (std::is_void<U>::value)
+          ? ""
+          : " (raised during the creation of a " + demangle<U>() + ")";
+  try {
+    throw;
+  } catch (bad_get_nullptr const &) {
+    throw Exception(what + " is a null pointer" + context);
+  } catch (boost::bad_get const &) {
+    throw Exception(what + " is not convertible to " + demangle<T>() + context);
+  }
+}
+
 } // namespace detail
 
 /**
@@ -219,9 +248,9 @@ struct get_value_helper<
 template <typename T> T get_value(Variant const &v) {
   try {
     return detail::get_value_helper<T>{}(v);
-  } catch (const boost::bad_get &) {
-    throw Exception("Provided argument of type " + detail::type_label(v) +
-                    " is not convertible to " + Utils::demangle<T>());
+  } catch (...) {
+    detail::handle_bad_get<T>(v);
+    throw;
   }
 }
 
@@ -233,12 +262,9 @@ std::unordered_map<K, V> get_map(std::unordered_map<K, Variant> const &v) {
     for (; it != v.end(); ++it) {
       ret.insert({it->first, detail::get_value_helper<V>{}(it->second)});
     }
-  } catch (const boost::bad_get &) {
-    throw Exception("Provided map value of type " +
-                    detail::type_label(it->second) + " is not convertible to " +
-                    Utils::demangle<V>() +
-                    " (raised during the creation of a " +
-                    Utils::demangle<std::unordered_map<K, V>>() + ")");
+  } catch (...) {
+    detail::handle_bad_get<V, std::unordered_map<K, V>>(v);
+    throw;
   }
   return ret;
 }