test: Fix 'make cov' with clang

hebasto commented at 1:30 pm on August 13, 2020: member

This is a follow up of #19688.

With this PR it is possible to do the following:

0$ ./autogen.sh
1$ ./configure --enable-lcov CC=clang CXX=clang++
2$ make
3$ make cov

Currently, on master (8a85377cd0b60cb00dae4f595d628d1afbd28bd5), make cov fails to Processing src/test/test_bitcoin-util_tests.gcda.

practicalswift commented at 1:43 pm on August 13, 2020: contributor

@hebasto What is the root cause of this issue? :)

hebasto commented at 1:59 pm on August 13, 2020: member

@practicalswift

@hebasto What is the root cause of this issue? :)

~~I don’t know ((~~ See #19709 (comment)

~~I can just describe a pattern on which Boost Test suite and Clang’s llvm-cov are incompatible: BOOST_CHECK() macro argument calls some function more then once.~~

DrahtBot added the label Tests on Aug 13, 2020

laanwj commented at 5:14 pm on August 13, 2020: member

@hebasto What is the root cause of this issue? :)

That’s pretty important to figure out imo, we can’t just randomly perturb the test code to make a tool happy. (also so that people maintaining the code in the future know what to take into account)

fjahr commented at 8:26 pm on August 13, 2020: member

Agree we should know what is going on but since I have had this issue locally I have gone ahead and tested it. So, I am tACK this PR cherry-picked on top of #19688 modulo an explanation of why this failed previously. Thanks for fixing this!

hebasto commented at 10:09 pm on August 13, 2020: member

@practicalswift @laanwj @fjahr

What is the root cause of this issue? :)

Thank you for forcing me to find the root of this issue!

BOOST_TEST - Limitations & workaround:

… compound statements containing any logical composition ||, &&… should be surrounded by parenthesis

To solve this issues some variants of the patch are available:

as suggested by Boost docs:

 0--- a/src/test/util_tests.cpp
 1+++ b/src/test/util_tests.cpp
 2@@ -559,11 +559,10 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream)
 3                 && test_args.m_settings.ro_config[""].count("h")
 4                 && test_args.m_settings.ro_config[""].count("i")
 5                );
 6-    BOOST_CHECK(test_args.m_settings.ro_config["sec1"].count("ccc")
 7-                && test_args.m_settings.ro_config["sec1"].count("h")
 8-                && test_args.m_settings.ro_config["sec2"].count("ccc")
 9-                && test_args.m_settings.ro_config["sec2"].count("iii")
10+    BOOST_CHECK(((test_args.m_settings.ro_config["sec1"].count("ccc")
11+                && test_args.m_settings.ro_config["sec1"].count("h"))
12+                && test_args.m_settings.ro_config["sec2"].count("ccc"))
13+                && test_args.m_settings.ro_config["sec2"].count("iii")
14                );
15 
16     BOOST_CHECK(test_args.IsArgSet("-a")
17                 && test_args.IsArgSet("-b")

using a local variable:

 0--- a/src/test/util_tests.cpp
 1+++ b/src/test/util_tests.cpp
 2@@ -559,11 +559,10 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream)
 3                 && test_args.m_settings.ro_config[""].count("h")
 4                 && test_args.m_settings.ro_config[""].count("i")
 5                );
 6-    BOOST_CHECK(test_args.m_settings.ro_config["sec1"].count("ccc")
 7-                && test_args.m_settings.ro_config["sec1"].count("h")
 8-                && test_args.m_settings.ro_config["sec2"].count("ccc")
 9-                && test_args.m_settings.ro_config["sec2"].count("iii")
10-               );
11+    bool test_value = test_args.m_settings.ro_config["sec1"].count("ccc")
12+                && test_args.m_settings.ro_config["sec1"].count("h")
13+                && test_args.m_settings.ro_config["sec2"].count("ccc")
14+                && test_args.m_settings.ro_config["sec2"].count("iii");
15+    BOOST_CHECK(test_value);
16 
17     BOOST_CHECK(test_args.IsArgSet("-a")
18                 && test_args.IsArgSet("-b")

as suggested in this PR:

 0--- a/src/test/util_tests.cpp
 1+++ b/src/test/util_tests.cpp
 2@@ -559,11 +559,10 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream)
 3                 && test_args.m_settings.ro_config[""].count("h")
 4                 && test_args.m_settings.ro_config[""].count("i")
 5                );
 6-    BOOST_CHECK(test_args.m_settings.ro_config["sec1"].count("ccc")
 7-                && test_args.m_settings.ro_config["sec1"].count("h")
 8-                && test_args.m_settings.ro_config["sec2"].count("ccc")
 9-                && test_args.m_settings.ro_config["sec2"].count("iii")
10-               );
11+    BOOST_CHECK(test_args.m_settings.ro_config["sec1"].count("ccc"));
12+    BOOST_CHECK(test_args.m_settings.ro_config["sec1"].count("h"));
13+    BOOST_CHECK(test_args.m_settings.ro_config["sec2"].count("ccc"));
14+    BOOST_CHECK(test_args.m_settings.ro_config["sec2"].count("iii"));                );
15 
16     BOOST_CHECK(test_args.IsArgSet("-a")
17                 && test_args.IsArgSet("-b")

The last variant has the following benefits:

no error-prone parenthesis paring
no redundant local variables
better logging

Update: but https://www.boost.org/doc/libs/1_73_0/libs/test/doc/html/boost_test/utf_reference/testing_tool_ref/assertion_boost_level.html says that for BOOST_CHECK(predicate); “it’s safe to pass complex expression for validation.”

Crypt-iQ commented at 3:45 am on August 14, 2020: contributor

Tested, code review ACK 568d42b On ubuntu, clang-10

Changes are straightforward and they assert the same thing as they did before so should be fine.

test_bitcoin coverage: https://crypt-iq.github.io/pr19709/test_bitcoin.coverage/ total coverage: https://crypt-iq.github.io/pr19709/total.coverage/

MarcoFalke commented at 5:50 am on August 14, 2020: member

Approach ACK. This will also print nicer error messages in case of failure

in src/test/util_tests.cpp:577 in 568d42bd7f outdated

610+    BOOST_CHECK(test_args.IsArgSet("-h"));
611+    BOOST_CHECK(test_args.IsArgSet("-i"));
612+    BOOST_CHECK(!test_args.IsArgSet("-zzz"));
613+    BOOST_CHECK(!test_args.IsArgSet("-iii"));
614+
615+    BOOST_CHECK(test_args.GetArg("-a", "xxx") == "");

vasild commented at 7:36 am on August 14, 2020:

0    BOOST_CHECK_EQUAL(test_args.GetArg("-a", "xxx"), "");

would produce a better error message, if it fails some day.

Same in all places below where BOOST_CHECK(a == b); can be replaced with BOOST_CHECK_EQUAL(a, b);

hebasto commented at 9:26 am on August 14, 2020:

Thank you! Updated.

vasild approved

vasild commented at 7:47 am on August 14, 2020: member

ACK 568d42bd7

It is always better to have

0assert(a);
1assert(b);

instead of

0assert(a && b);

due to better diagnostics in case of failure.

The patch is good as is, but if you are in a mood for it - take the suggestion below.

test: Fix 'make cov' with clang 35cd2da623

hebasto force-pushed on Aug 14, 2020

hebasto commented at 9:26 am on August 14, 2020: member

Updated 568d42bd7f47851def00b185c6f5f285b6f7f951 -> 35cd2da623e32b975fbc485c3605934e4aa8bdc5 (pr19709.01 -> pr19709.02, diff):

addressed @vasild’s comment:

Same in all places below where BOOST_CHECK(a == b); can be replaced with BOOST_CHECK_EQUAL(a, b);

vasild commented at 9:36 am on August 14, 2020: member

ACK 35cd2da

Crypt-iQ commented at 12:02 pm on August 14, 2020: contributor

ACK 35cd2da

MarcoFalke added the label Refactoring on Aug 14, 2020

MarcoFalke merged this on Aug 14, 2020

MarcoFalke closed this on Aug 14, 2020

hebasto deleted the branch on Aug 14, 2020

DrahtBot locked this on Feb 15, 2022

test: Fix ‘make cov’ with clang #19709