Increases the robustness of integration tests (#259)

Updates multiplexer to make requests complete with error_code() rather than error_code(0)
Integration tests now use io_context::run_for to run with a timeout and avoid deadlocks
Tests now use concrete lambdas where generic ones are not required
Tests now use BOOST_TEST with operator== to print values on error
Tests now use anonymous namespaces to detect dead code
Adds run_coroutine_test and removed start from common.hpp
Updates test_reconnect to perform relevant checks
Refactors how test_issue_50 launches its coroutines
Groups tests in CMake as unit or integration
Updates Jamfile tests to contain all unit tests and only unit tests

Author: anarthal

include/boost/redis/impl/multiplexer.ipp

@@ -16,7 +16,7 @@ multiplexer::elem::elem(request const& req, pipeline_adapter_type adapter)
 , adapter_{}
 , remaining_responses_{req.get_expected_responses()}
 , status_{status::waiting}
-, ec_{{}}
+, ec_{}
 , read_size_{0}
 {
    adapter_ = [this, adapter](resp3::node_view const& nd, system::error_code& ec) {

test/CMakeLists.txt

@@ -20,7 +20,7 @@ add_library(boost_redis_tests_common STATIC common.cpp)
 target_compile_features(boost_redis_tests_common PRIVATE cxx_std_17)
 target_link_libraries(boost_redis_tests_common PRIVATE boost_redis_project_options)
 
-macro(make_test TEST_NAME STANDARD)
+macro(make_test TEST_NAME)
   set(EXE_NAME "boost_redis_${TEST_NAME}")
   add_executable(${EXE_NAME} ${TEST_NAME}.cpp)
   target_link_libraries(${EXE_NAME} PRIVATE
@@ -29,31 +29,32 @@ macro(make_test TEST_NAME STANDARD)
     boost_redis_project_options
     Boost::unit_test_framework
   )
-  target_compile_features(${EXE_NAME} PRIVATE cxx_std_${STANDARD})
   add_test(${EXE_NAME} ${EXE_NAME})
 endmacro()
 
-make_test(test_conn_quit 17)
-# TODO: Configure a Redis server with TLS in the CI and reenable this test.
-#make_test(test_conn_tls 17)
-make_test(test_low_level 17)
-make_test(test_conn_exec_retry 17)
-make_test(test_conn_exec_error 17)
-make_test(test_request 17)
-make_test(test_run 17)
-make_test(test_low_level_sync_sans_io 17)
-make_test(test_conn_check_health 17)
+# Unit tests
+make_test(test_low_level)
+make_test(test_request)
+make_test(test_low_level_sync_sans_io)
+make_test(test_any_adapter)
 
-make_test(test_conn_exec 20)
-make_test(test_conn_push 20)
-make_test(test_conn_reconnect 20)
-make_test(test_conn_exec_cancel 20)
-make_test(test_conn_exec_cancel2 20)
-make_test(test_conn_echo_stress 20)
-make_test(test_any_adapter 17)
-make_test(test_conversions 17)
-make_test(test_issue_50 20)
-make_test(test_issue_181 17)
+# Tests that require a real Redis server
+make_test(test_conn_quit)
+# TODO: Configure a Redis server with TLS in the CI and reenable this test.
+#make_test(test_conn_tls)
+make_test(test_conn_exec_retry)
+make_test(test_conn_exec_error)
+make_test(test_run)
+make_test(test_conn_check_health)
+make_test(test_conn_exec)
+make_test(test_conn_push)
+make_test(test_conn_reconnect)
+make_test(test_conn_exec_cancel)
+make_test(test_conn_exec_cancel2)
+make_test(test_conn_echo_stress)
+make_test(test_issue_50)
+make_test(test_issue_181)
+make_test(test_conversions)
 
 # Coverage
 set(

test/Jamfile

@@ -48,10 +48,9 @@ lib redis_test_common
 # B2 runs tests in parallel, and some tests rely on having exclusive
 # access to a Redis server, so we only run the ones that don't require a DB server.
 local tests =
-    test_low_level_sync_sans_io
     test_low_level
     test_request
-    test_run
+    test_low_level_sync_sans_io
     test_any_adapter
 ;
 

test/common.cpp

@@ -5,6 +5,7 @@
 
 #include <cstdlib>
 #include <iostream>
+#include <stdexcept>
 
 namespace net = boost::asio;
 
@@ -55,22 +56,17 @@ boost::redis::config make_test_config()
 }
 
 #ifdef BOOST_ASIO_HAS_CO_AWAIT
-auto start(net::awaitable<void> op) -> int
+void run_coroutine_test(net::awaitable<void> op, std::chrono::steady_clock::duration timeout)
 {
-   try {
-      net::io_context ioc;
-      net::co_spawn(ioc, std::move(op), [](std::exception_ptr p) {
-         if (p)
-            std::rethrow_exception(p);
-      });
-      ioc.run();
-
-      return 0;
-
-   } catch (std::exception const& e) {
-      std::cerr << "start> " << e.what() << std::endl;
-   }
-
-   return 1;
+   net::io_context ioc;
+   bool finished = false;
+   net::co_spawn(ioc, std::move(op), [&finished](std::exception_ptr p) {
+      if (p)
+         std::rethrow_exception(p);
+      finished = true;
+   });
+   ioc.run_for(timeout);
+   if (!finished)
+      throw std::runtime_error("Coroutine test did not finish");
 }
 #endif  // BOOST_ASIO_HAS_CO_AWAIT

test/common.hpp

@@ -8,14 +8,22 @@
 #include <boost/asio/use_awaitable.hpp>
 #include <boost/system/error_code.hpp>
 
+#include <chrono>
 #include <memory>
 
+// The timeout for tests involving communication to a real server.
+// Some tests use a longer timeout by multiplying this value by some
+// integral number.
+inline constexpr std::chrono::seconds test_timeout{30};
+
 #ifdef BOOST_ASIO_HAS_CO_AWAIT
 inline auto redir(boost::system::error_code& ec)
 {
    return boost::asio::redirect_error(boost::asio::use_awaitable, ec);
 }
-auto start(boost::asio::awaitable<void> op) -> int;
+void run_coroutine_test(
+   boost::asio::awaitable<void>,
+   std::chrono::steady_clock::duration timeout = test_timeout);
 #endif  // BOOST_ASIO_HAS_CO_AWAIT
 
 boost::redis::config make_test_config();

test/test_conn_check_health.cpp

@@ -7,7 +7,7 @@
 #include <boost/redis/connection.hpp>
 #include <boost/redis/response.hpp>
 
-#include <boost/system/errc.hpp>
+#include <cstddef>
 #define BOOST_TEST_MODULE check_health
 #include <boost/test/included/unit_test.hpp>
 
@@ -25,9 +25,12 @@ using boost::redis::ignore;
 using boost::redis::operation;
 using boost::redis::generic_response;
 using boost::redis::consume_one;
+using namespace std::chrono_literals;
 
 // TODO: Test cancel(health_check)
 
+namespace {
+
 struct push_callback {
    connection* conn1;
    connection* conn2;
@@ -82,10 +85,13 @@ BOOST_AUTO_TEST_CASE(check_health)
    auto cfg1 = make_test_config();
    cfg1.health_check_id = "conn1";
    cfg1.reconnect_wait_interval = std::chrono::seconds::zero();
-   error_code res1;
-   conn1.async_run(cfg1, {}, [&](auto ec) {
+
+   bool run1_finished = false, run2_finished = false, exec_finished = false;
+
+   conn1.async_run(cfg1, {}, [&](error_code ec) {
+      run1_finished = true;
       std::cout << "async_run 1 completed: " << ec.message() << std::endl;
-      res1 = ec;
+      BOOST_TEST(ec != error_code());
    });
 
    //--------------------------------
@@ -96,32 +102,36 @@ BOOST_AUTO_TEST_CASE(check_health)
 
    auto cfg2 = make_test_config();
    cfg2.health_check_id = "conn2";
-   error_code res2;
-   conn2.async_run(cfg2, {}, [&](auto ec) {
+   conn2.async_run(cfg2, {}, [&](error_code ec) {
+      run2_finished = true;
       std::cout << "async_run 2 completed: " << ec.message() << std::endl;
-      res2 = ec;
+      BOOST_TEST(ec != error_code());
    });
 
    request req2;
    req2.push("MONITOR");
    generic_response resp2;
    conn2.set_receive_response(resp2);
 
-   conn2.async_exec(req2, ignore, [](auto ec, auto) {
+   conn2.async_exec(req2, ignore, [&exec_finished](error_code ec, std::size_t) {
+      exec_finished = true;
       std::cout << "async_exec: " << std::endl;
-      BOOST_TEST(!ec);
+      BOOST_TEST(ec == error_code());
    });
 
    //--------------------------------
 
    push_callback{&conn1, &conn2, &resp2, &req1}();  // Starts reading pushes.
 
-   ioc.run();
+   ioc.run_for(2 * test_timeout);
 
-   BOOST_TEST(!!res1);
-   BOOST_TEST(!!res2);
+   BOOST_TEST(run1_finished);
+   BOOST_TEST(run2_finished);
+   BOOST_TEST(exec_finished);
 
    // Waits before exiting otherwise it might cause subsequent tests
    // to fail.
    std::this_thread::sleep_for(std::chrono::seconds{10});
 }
+
+}  // namespace