From 0a3f511cfed071c9c4ad512341ae7569383922b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Ho=C5=99e=C5=88ovsk=C3=BD?= Date: Wed, 9 Jun 2021 20:48:58 +0200 Subject: [PATCH] Fix inconsistencies in reporting benchmarking failures With these changes, all these benchmarks ```cpp BENCHMARK("Empty benchmark") {}; BENCHMARK("Throwing benchmark") { throw "just a plain literal, bleh"; }; BENCHMARK("Asserting benchmark") { REQUIRE(1 == 2); }; BENCHMARK("FAIL'd benchmark") { FAIL("This benchmark only fails, nothing else"); }; ``` report the respective failure and mark the outer `TEST_CASE` as failed. Previously, the first two would not fail the `TEST_CASE`, and the latter two would break xml reporter's formatting, because `benchmarkFailed`, `benchmarkEnded` etc would not be be called properly in failure cases. --- src/catch2/benchmark/catch_benchmark.hpp | 9 ++-- .../detail/catch_complete_invoke.hpp | 9 +--- .../internal/catch_benchmark_combined_tu.cpp | 20 --------- tests/CMakeLists.txt | 44 +++++++++++++++++++ .../InternalBenchmark.tests.cpp | 21 +++++++++ 5 files changed, 73 insertions(+), 30 deletions(-) diff --git a/src/catch2/benchmark/catch_benchmark.hpp b/src/catch2/benchmark/catch_benchmark.hpp index de41f0c0..7eb1a018 100644 --- a/src/catch2/benchmark/catch_benchmark.hpp +++ b/src/catch2/benchmark/catch_benchmark.hpp @@ -81,10 +81,13 @@ namespace Catch { auto analysis = Detail::analyse(*cfg, env, samples.begin(), samples.end()); BenchmarkStats> stats{ info, analysis.samples, analysis.mean, analysis.standard_deviation, analysis.outliers, analysis.outlier_variance }; getResultCapture().benchmarkEnded(stats); - + } CATCH_CATCH_ANON (TestFailureException) { + getResultCapture().benchmarkFailed("Benchmark failed due to failed assertion"_sr); } CATCH_CATCH_ALL{ - if (translateActiveException() != Detail::benchmarkErrorMsg) // benchmark errors have been reported, otherwise rethrow. - std::rethrow_exception(std::current_exception()); + getResultCapture().benchmarkFailed(translateActiveException()); + // We let the exception go further up so that the + // test case is marked as failed. + std::rethrow_exception(std::current_exception()); } } diff --git a/src/catch2/benchmark/detail/catch_complete_invoke.hpp b/src/catch2/benchmark/detail/catch_complete_invoke.hpp index 8cb13211..d97e0056 100644 --- a/src/catch2/benchmark/detail/catch_complete_invoke.hpp +++ b/src/catch2/benchmark/detail/catch_complete_invoke.hpp @@ -10,6 +10,7 @@ #ifndef CATCH_COMPLETE_INVOKE_HPP_INCLUDED #define CATCH_COMPLETE_INVOKE_HPP_INCLUDED +#include #include #include #include @@ -51,17 +52,11 @@ namespace Catch { return CompleteInvoker>::invoke(std::forward(fun), std::forward(args)...); } - extern const std::string benchmarkErrorMsg; } // namespace Detail template Detail::CompleteType_t> user_code(Fun&& fun) { - CATCH_TRY{ - return Detail::complete_invoke(std::forward(fun)); - } CATCH_CATCH_ALL{ - getResultCapture().benchmarkFailed(translateActiveException()); - CATCH_RUNTIME_ERROR(Detail::benchmarkErrorMsg); - } + return Detail::complete_invoke(std::forward(fun)); } } // namespace Benchmark } // namespace Catch diff --git a/src/catch2/benchmark/internal/catch_benchmark_combined_tu.cpp b/src/catch2/benchmark/internal/catch_benchmark_combined_tu.cpp index c7234966..ffbae696 100644 --- a/src/catch2/benchmark/internal/catch_benchmark_combined_tu.cpp +++ b/src/catch2/benchmark/internal/catch_benchmark_combined_tu.cpp @@ -47,26 +47,6 @@ namespace Catch { } // namespace Catch -//////////////////////////////////////////////// -// vvv formerly catch_complete_invoke.cpp vvv // -//////////////////////////////////////////////// - -#include - -namespace Catch { - namespace Benchmark { - namespace Detail { - CATCH_INTERNAL_START_WARNINGS_SUPPRESSION - CATCH_INTERNAL_SUPPRESS_GLOBALS_WARNINGS - const std::string benchmarkErrorMsg = "a benchmark failed to run successfully"; - CATCH_INTERNAL_STOP_WARNINGS_SUPPRESSION - } // namespace Detail - } // namespace Benchmark -} // namespace Catch - - - - ///////////////////////////////////////////////// // vvv formerly catch_run_for_at_least.cpp vvv // ///////////////////////////////////////////////// diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index a1e34226..750547c0 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -315,6 +315,50 @@ add_test(NAME CheckConvenienceHeaders ${PYTHON_EXECUTABLE} ${CATCH_DIR}/tools/scripts/checkConvenienceHeaders.py ) + +add_test(NAME "Benchmarking::FailureReporting::OptimizedOut" + COMMAND + $ "Failing benchmarks" -c "empty" -r xml + # This test only makes sense with the optimizer being enabled when + # the tests are being compiled. + CONFIGURATIONS Release +) +set_tests_properties("Benchmarking::FailureReporting::OptimizedOut" + PROPERTIES + PASS_REGULAR_EXPRESSION "could not measure benchmark\, maybe it was optimized away" + FAIL_REGULAR_EXPRESSION "successes=\"1\"" +) + +add_test(NAME "Benchmarking::FailureReporting::ThrowingBenchmark" + COMMAND + $ "Failing benchmarks" -c "throw" -r xml +) +set_tests_properties("Benchmarking::FailureReporting::ThrowingBenchmark" + PROPERTIES + PASS_REGULAR_EXPRESSION " "Failing benchmarks" -c "assert" -r xml +) +set_tests_properties("Benchmarking::FailureReporting::FailedAssertion" + PROPERTIES + PASS_REGULAR_EXPRESSION " "Failing benchmarks" -c "fail" -r xml +) +set_tests_properties("Benchmarking::FailureReporting::FailMacro" + PROPERTIES + PASS_REGULAR_EXPRESSION "This benchmark only fails\, nothing else" + FAIL_REGULAR_EXPRESSION "successes=\"1\"" +) + if (CATCH_USE_VALGRIND) add_test(NAME ValgrindRunTests COMMAND valgrind --leak-check=full --error-exitcode=1 $) add_test(NAME ValgrindListTests COMMAND valgrind --leak-check=full --error-exitcode=1 $ --list-tests --verbosity high) diff --git a/tests/SelfTest/IntrospectiveTests/InternalBenchmark.tests.cpp b/tests/SelfTest/IntrospectiveTests/InternalBenchmark.tests.cpp index 0a3895a8..b2250415 100644 --- a/tests/SelfTest/IntrospectiveTests/InternalBenchmark.tests.cpp +++ b/tests/SelfTest/IntrospectiveTests/InternalBenchmark.tests.cpp @@ -412,3 +412,24 @@ TEST_CASE("run benchmark", "[benchmark][approvals]") { CHECK((end - start).count() == 2867251000); } + +TEST_CASE("Failing benchmarks", "[!benchmark][.approvals]") { + SECTION("empty", "Benchmark that has been optimized away (because it is empty)") { + BENCHMARK("Empty benchmark") {}; + } + SECTION("throw", "Benchmark that throws an exception") { + BENCHMARK("Throwing benchmark") { + throw "just a plain literal, bleh"; + }; + } + SECTION("assert", "Benchmark that asserts inside") { + BENCHMARK("Asserting benchmark") { + REQUIRE(1 == 2); + }; + } + SECTION("fail", "Benchmark that fails inside") { + BENCHMARK("FAIL'd benchmark") { + FAIL("This benchmark only fails, nothing else"); + }; + } +}