From 3f0f63c202212759bac1a2b18bee3fdceadfac75 Mon Sep 17 00:00:00 2001 From: Dimitrios Siganos Date: Wed, 9 Nov 2022 15:39:02 +0000 Subject: [PATCH] Unit test fix setup_chains() (#3982) * Minor compilation-time fix to EXPECT_TIMELY EXPECT_TIMELY defined the variable "ec" without taking precautions to restrict its scope. One symptom was that EXPECT_TIMELY could not be used twice in a function. * Fix race condition in setup_chains() in bootstrap_server.cpp * Fix possible race conditions in use of nano::test::confirm() Most of these calls are race conditions because they use the following pattern without allowing for propagation to occur: process() confirm() --- nano/core_test/active_transactions.cpp | 4 ++-- nano/core_test/bootstrap_server.cpp | 4 ++-- nano/core_test/node.cpp | 6 +++--- nano/slow_test/vote_cache.cpp | 4 ++-- nano/test_common/testutil.hpp | 16 +++++++++------- 5 files changed, 18 insertions(+), 16 deletions(-) diff --git a/nano/core_test/active_transactions.cpp b/nano/core_test/active_transactions.cpp index 8ffd9f8a..2a63a424 100644 --- a/nano/core_test/active_transactions.cpp +++ b/nano/core_test/active_transactions.cpp @@ -1433,7 +1433,7 @@ nano::keypair setup_rep (nano::test::system & system, nano::node & node, nano::u .build_shared (); EXPECT_TRUE (nano::test::process (node, { send, open })); - EXPECT_TRUE (nano::test::confirm (node, { send, open })); + EXPECT_TIMELY (5s, nano::test::confirm (node, { send, open })); EXPECT_TIMELY (5s, nano::test::confirmed (node, { send, open })); return key; @@ -1482,7 +1482,7 @@ std::vector> setup_independent_blocks (nano::test:: } // Confirm whole genesis chain at once - EXPECT_TRUE (nano::test::confirm (node, { latest })); + EXPECT_TIMELY (5s, nano::test::confirm (node, { latest })); EXPECT_TIMELY (5s, nano::test::confirmed (node, { latest })); return blocks; diff --git a/nano/core_test/bootstrap_server.cpp b/nano/core_test/bootstrap_server.cpp index 7685066c..3a1032d0 100644 --- a/nano/core_test/bootstrap_server.cpp +++ b/nano/core_test/bootstrap_server.cpp @@ -41,7 +41,7 @@ block_list_t setup_chain (nano::test::system & system, nano::node & node, nano:: EXPECT_TRUE (nano::test::process (node, blocks)); // Confirm whole chain at once - EXPECT_TRUE (nano::test::confirm (node, { blocks.back () })); + EXPECT_TIMELY (5s, nano::test::confirm (node, { blocks.back () })); EXPECT_TIMELY (5s, nano::test::confirmed (node, blocks)); return blocks; @@ -84,7 +84,7 @@ std::vector> setup_chains (nano::test::sy // Ensure blocks are in the ledger and confirmed EXPECT_TRUE (nano::test::process (node, { send, open })); - EXPECT_TRUE (nano::test::confirm (node, { send, open })); + EXPECT_TIMELY (5s, nano::test::confirm (node, { send, open })); EXPECT_TIMELY (5s, nano::test::confirmed (node, { send, open })); auto added_blocks = setup_chain (system, node, key, block_count); diff --git a/nano/core_test/node.cpp b/nano/core_test/node.cpp index d6280494..1e65fa2b 100644 --- a/nano/core_test/node.cpp +++ b/nano/core_test/node.cpp @@ -2573,7 +2573,7 @@ TEST (node, vote_by_hash_bundle) } // Confirming last block will confirm whole chain and allow us to generate votes for those blocks later - ASSERT_TRUE (nano::test::confirm (node, { blocks.back () })); + ASSERT_TIMELY (5s, nano::test::confirm (node, { blocks.back () })); ASSERT_TIMELY (5s, nano::test::confirmed (node, { blocks.back () })); system.wallet (0)->insert_adhoc (nano::dev::genesis_key.prv); @@ -2752,7 +2752,7 @@ TEST (node, epoch_conflict_confirm) ASSERT_TRUE (nano::test::process (node1, { send, send2, open })); // Confirm open block in node1 to allow generating votes - ASSERT_TRUE (nano::test::confirm (node1, { open })); + ASSERT_TIMELY (5s, nano::test::confirm (node1, { open })); ASSERT_TIMELY (5s, nano::test::confirmed (node1, { open })); // Process initial blocks on node0 @@ -2766,7 +2766,7 @@ TEST (node, epoch_conflict_confirm) ASSERT_TIMELY (5s, nano::test::exists (node1, { change, epoch_open })); // Confirm initial blocks in node1 to allow generating votes later - ASSERT_TRUE (nano::test::confirm (node1, { change, epoch_open, send2 })); + ASSERT_TIMELY (5s, nano::test::confirm (node1, { change, epoch_open, send2 })); ASSERT_TIMELY (5s, nano::test::confirmed (node1, { change, epoch_open, send2 })); // Start elections for node0 for conflicting change and epoch_open blocks (those two blocks have the same root) diff --git a/nano/slow_test/vote_cache.cpp b/nano/slow_test/vote_cache.cpp index 1d0b5f73..e2044295 100644 --- a/nano/slow_test/vote_cache.cpp +++ b/nano/slow_test/vote_cache.cpp @@ -38,7 +38,7 @@ nano::keypair setup_rep (nano::test::system & system, nano::node & node, nano::u .build_shared (); EXPECT_TRUE (nano::test::process (node, { send, open })); - EXPECT_TRUE (nano::test::confirm (node, { send, open })); + EXPECT_TIMELY (5s, nano::test::confirm (node, { send, open })); EXPECT_TIMELY (5s, nano::test::confirmed (node, { send, open })); return key; @@ -104,7 +104,7 @@ std::vector> setup_blocks (nano::test::system & sys EXPECT_TRUE (nano::test::process (node, receives)); // Confirm whole genesis chain at once - EXPECT_TRUE (nano::test::confirm (node, { sends.back () })); + EXPECT_TIMELY (5s, nano::test::confirm (node, { sends.back () })); EXPECT_TIMELY (5s, nano::test::confirmed (node, { sends })); std::cout << "setup_blocks done" << std::endl; diff --git a/nano/test_common/testutil.hpp b/nano/test_common/testutil.hpp index ae403c2e..c005cac8 100644 --- a/nano/test_common/testutil.hpp +++ b/nano/test_common/testutil.hpp @@ -56,13 +56,15 @@ } /** Expects that the condition becomes true within the deadline */ -#define EXPECT_TIMELY(time, condition) \ - system.deadline_set (time); \ - std::error_code ec; \ - while (!(condition) && !(ec = system.poll ())) \ - { \ - } \ - EXPECT_NO_ERROR (ec); +#define EXPECT_TIMELY(time, condition) \ + system.deadline_set (time); \ + { \ + std::error_code _ec; \ + while (!(condition) && !(_ec = system.poll ())) \ + { \ + } \ + EXPECT_NO_ERROR (_ec); \ + } /* * Asserts that the `val1 == val2` condition becomes true within the deadline