Merge pull request #5572 from kidder/remove_assertion_error_test_macros

Remove ASSERTION_TEST and ERROR_TEST macros

cmake/SpectreParseTests.py

@@ -130,11 +130,7 @@ def parse_source_file(file_name):
                 test_timeout = timeout
 
         # Parse the test attributes
-        should_have_output_regex = (
-            "ERROR_TEST()" in test_body_first_line
-            or "ASSERTION_TEST()" in test_body_first_line
-            or "OUTPUT_TEST()" in test_body_first_line
-        )
+        should_have_output_regex = "OUTPUT_TEST()" in test_body_first_line
         output_regex = ""
 
         all_attributes_by_name = re.findall(
@@ -157,8 +153,7 @@ def parse_source_file(file_name):
                     print(
                         "\nERROR: The test '%s' in the file '%s' has the "
                         "attribute OutputRegEx, but does not contain the "
-                        "macro ERROR_TEST(), ASSERTION_TEST(), or "
-                        "OUTPUT_TEST() as its first line.\n"
+                        "macro OUTPUT_TEST() as its first line.\n"
                         % (test_name, file_name)
                     )
                     exit(1)
@@ -167,7 +162,7 @@ def parse_source_file(file_name):
         if should_have_output_regex and output_regex == "":
             print(
                 "\nERROR: The test '%s' in the file '%s' was marked as an "
-                "ERROR_TEST(), ASSERTION_TEST(), or OUTPUT_TEST(), but "
+                "OUTPUT_TEST(), but "
                 "failed to produce a parsable OutputRegex attribute! "
                 "The syntax is // [[OutputRegex, <regular_expression>]] as "
                 "a comment before the SPECTRE_TEST_CASE.\n"

docs/DevGuide/WritingTests.md

@@ -228,38 +228,26 @@ tolerance.
 
 #### Testing Failure Cases {#testing_failure_cases}
 
-Adding the "attribute" `// [[OutputRegex, Regular expression to
-match]]` before the `SPECTRE_TEST_CASE` macro will force ctest to only
-pass the particular test if the regular expression is found in the
-output of the test. This can be used to test error handling. When
-testing `ASSERT`s you must mark the `SPECTRE_TEST_CASE` as
-`[[noreturn]]`, add the macro `ASSERTION_TEST();` to the beginning of
-the test, and also have the test call `ERROR("Failed to trigger ASSERT
-in an assertion test");` at the end of the test body.  The test body
+`ASSERT`s and `ERROR`s can be tested with the `CHECK_THROWS_WITH`
+macro.  This macro takes two arguments: the first is either an
+expression or a lambda that is expected to trigger an exception (which
+now are thrown by `ASSERT` and `ERROR` (Note: You may need to add `()`
+wrapping the lambda in order for it to compile.); the second is a
+Catch Matcher (see [Catch2](https://github.com/catchorg/Catch2) for
+complete documentation), usually a
+`Catch::Matchers::ContainsSubstring()` macro that matches a substring
+of the error message of the thrown exception.
+
+When testing `ASSERT`s the `CHECK_THROWS_WITH`
 should be enclosed between `#%ifdef SPECTRE_DEBUG` and an `#%endif`
-
 If the `#%ifdef SPECTRE_DEBUG` block is omitted then compilers will
 correctly flag the code as being unreachable which results in
 warnings.
 
-You can also test `ERROR`s inside your code. These tests need to have
-the `OutputRegex`, and also call `ERROR_TEST();` at the
-beginning. They do not need the `#%ifdef SPECTRE_DEBUG` block, they
-can just call have the code that triggers an `ERROR`.
-
-We are currently transforming these failure cases to use the
-`CHECK_THROWS_WITH` macro. This macro takes two arguments: the first
-is either an expression or a lambda that is expected to trigger an
-exception (which now are thrown by `ASSERT` and `ERROR` (Note: You may
-need to add `()` wrapping the lambda in order for it to compile.); the
-second is a Catch Matcher (see
-[Catch2](https://github.com/catchorg/Catch2) for complete
-documentation), usually a `Catch::Matchers::ContainsSubstring()` macro
-that matches a substring of the error message of the thrown exception.
-
-Note that a `OutputRegex` can also be specified in a test that is
-supposed to succeed with output that matches the regular expression.
-In this case, the first line of the test should call the macro
+Adding the "attribute" `// [[OutputRegex, Regular expression to match]]`
+before the `SPECTRE_TEST_CASE` macro will force ctest to only pass the
+particular test if the regular expression is found in the output of the
+test.  In this case, the first line of the test should call the macro
 `OUTPUT_TEST();`.
 
 ### Testing Actions

tests/Unit/Framework/TestingFramework.hpp

@@ -291,56 +291,6 @@ struct check_matrix_approx {
 }
 /// \endcond
 
-/*!
- * \ingroup TestingFrameworkGroup
- * \brief Mark a test as checking a call to ERROR
- *
- * \details
- * In order to properly handle aborting with Catch versions newer than 1.6.1
- * we must install a signal handler after Catch does, which means inside the
- * SPECTRE_TEST_CASE itself. The ERROR_TEST() macro should be the first line in
- * the SPECTRE_TEST_CASE.
- *
- * \warning This macro is deprecated.  See \ref testing_failure_cases
- * "the DevGuide" for the modern way to do this.
- */
-#define ERROR_TEST()                                      \
-  do {                                                    \
-    std::signal(SIGABRT, spectre_testing_signal_handler); \
-  } while (false)
-
-/*!
- * \ingroup TestingFrameworkGroup
- * \brief Mark a test to be checking an ASSERT
- *
- * \details
- * Testing error handling is just as important as testing functionality. Tests
- * that are supposed to exit with an error must be annotated with the attribute
- * \code
- * // [[OutputRegex, The regex that should be found in the output]]
- * \endcode
- * Note that the regex only needs to be a sub-expression of the error message,
- * that is, there are implicit wildcards before and after the string.
- *
- * In order to test ASSERT's properly the test must also fail for release
- * builds. This is done by adding this macro at the beginning for the test.
- *
- * \warning This macro is deprecated.  See \ref testing_failure_cases
- * "the DevGuide" for the modern way to do this.
- */
-#ifdef SPECTRE_DEBUG
-#define ASSERTION_TEST() \
-  do {                   \
-    ERROR_TEST();        \
-  } while (false)
-#else
-#define ASSERTION_TEST()                                   \
-  do {                                                     \
-    ERROR_TEST();                                          \
-    sys::abort("### No ASSERT tests in release mode ###"); \
-  } while (false)
-#endif
-
 /*!
  * \ingroup TestingFrameworkGroup
  * \brief Mark a test as checking the output with a regular expression

tests/Unit/Framework/Tests/Test_TestingFramework.cpp

@@ -36,6 +36,6 @@ SPECTRE_TEST_CASE("Unit.TestingFramework.Approx", "[Unit]") {
 
 // [[OutputRegex, I failed]]
 [[noreturn]] SPECTRE_TEST_CASE("Unit.TestingFramework.Abort", "[Unit]") {
-  ERROR_TEST();
+  OUTPUT_TEST();
   sys::abort("I failed");
 }

tests/Unit/Helpers/DataStructures/VectorImplTestHelper.hpp

@@ -643,10 +643,10 @@ enum RefSizeErrorTestKind { Copy, ExpressionAssign, Move };
 /// \brief Test that assigning to a non-owning `VectorType` of the wrong size
 /// appropriately generates an error.
 ///
-/// \details a calling function should be an `ASSERTION_TEST()` and check for
-/// the string "Must copy/move/assign into same size".
-/// Three types of tests are provided and one must be provided as the first
-/// function argument:
+/// \details a calling function should be in a `CHECK_THROWS_WITH()` inside a
+/// `#ifdef SPECTRE_DEBUG` block, and check for the string "Must
+/// copy/move/assign into same size". Three types of tests are provided and one
+/// must be provided as the first function argument:
 /// - `RefSizeErrorTestKind::Copy`: Checks that copy-assigning to a non-owning
 /// `VectorType` from a `VectorType` with the wrong size generates an error.
 /// - `RefSizeErrorTestKind::ExpressionAssign`: Checks that assigning to a

tests/Unit/Parallel/Test_AlgorithmCore.cpp

@@ -848,13 +848,15 @@ extern "C" void CkRegisterMainModule() {
 }
 // [charm_main_example]
 
-// [[OutputRegex, DistributedObject has been constructed with a nullptr]]
 SPECTRE_TEST_CASE("Unit.Parallel.Algorithm.NullptrConstructError",
                   "[Parallel][Unit]") {
-  ERROR_TEST();
-  Parallel::DistributedObject<NoOpsComponent<TestMetavariables>,
-                              tmpl::list<Parallel::PhaseActions<
-                                  Parallel::Phase::Initialization,
-                                  tmpl::list<add_remove_test::initialize>>>>{
-      nullptr};
+  CHECK_THROWS_WITH(
+      (Parallel::DistributedObject<
+          NoOpsComponent<TestMetavariables>,
+          tmpl::list<
+              Parallel::PhaseActions<Parallel::Phase::Initialization,
+                                     tmpl::list<add_remove_test::initialize>>>>{
+          nullptr}),
+      Catch::Matchers::ContainsSubstring(
+          "DistributedObject has been constructed with a nullptr"));
 }

tests/Unit/Utilities/ErrorHandling/Test_SegfaultHandler.cpp

@@ -7,10 +7,14 @@
 
 #include "Utilities/ErrorHandling/SegfaultHandler.hpp"
 
+
 // [[OutputRegex, Segmentation fault!]]
 SPECTRE_TEST_CASE("Unit.ErrorHandling.SegfaultHandler",
                   "[ErrorHandling][Unit]") {
-  ERROR_TEST();
+  // Tried to make this not an OUTPUT_TEST, but it fails on all CI compilers
+  // despite passing on my desktop...
+  OUTPUT_TEST();
   enable_segfault_handler();
-  std::raise(SIGSEGV);
+  CHECK_THROWS_WITH(std::raise(SIGSEGV),
+                    Catch::Matchers::ContainsSubstring("Segmentation fault!"));
 }