test: Fix 'make cov' with clang by hebasto · Pull Request #19709 · bitcoin/bitcoin

hebasto · 2020-08-13T13:30:57Z

This is a follow up of #19688.

With this PR it is possible to do the following:

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

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

practicalswift · 2020-08-13T13:43:49Z

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

hebasto · 2020-08-13T13:59:22Z

@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.~~

laanwj · 2020-08-13T17:14:46Z

@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 · 2020-08-13T20:26:34Z

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 · 2020-08-13T22:09:02Z

@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:

--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -559,11 +559,10 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream)
                 && test_args.m_settings.ro_config[""].count("h")
                 && test_args.m_settings.ro_config[""].count("i")
                );
-    BOOST_CHECK(test_args.m_settings.ro_config["sec1"].count("ccc")
-                && test_args.m_settings.ro_config["sec1"].count("h")
-                && test_args.m_settings.ro_config["sec2"].count("ccc")
-                && test_args.m_settings.ro_config["sec2"].count("iii")
+    BOOST_CHECK(((test_args.m_settings.ro_config["sec1"].count("ccc")
+                && test_args.m_settings.ro_config["sec1"].count("h"))
+                && test_args.m_settings.ro_config["sec2"].count("ccc"))
+                && test_args.m_settings.ro_config["sec2"].count("iii")
                );
 
     BOOST_CHECK(test_args.IsArgSet("-a")
                 && test_args.IsArgSet("-b")

using a local variable:

--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -559,11 +559,10 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream)
                 && test_args.m_settings.ro_config[""].count("h")
                 && test_args.m_settings.ro_config[""].count("i")
                );
-    BOOST_CHECK(test_args.m_settings.ro_config["sec1"].count("ccc")
-                && test_args.m_settings.ro_config["sec1"].count("h")
-                && test_args.m_settings.ro_config["sec2"].count("ccc")
-                && test_args.m_settings.ro_config["sec2"].count("iii")
-               );
+    bool test_value = test_args.m_settings.ro_config["sec1"].count("ccc")
+                && test_args.m_settings.ro_config["sec1"].count("h")
+                && test_args.m_settings.ro_config["sec2"].count("ccc")
+                && test_args.m_settings.ro_config["sec2"].count("iii");
+    BOOST_CHECK(test_value);
 
     BOOST_CHECK(test_args.IsArgSet("-a")
                 && test_args.IsArgSet("-b")

as suggested in this PR:

--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -559,11 +559,10 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream)
                 && test_args.m_settings.ro_config[""].count("h")
                 && test_args.m_settings.ro_config[""].count("i")
                );
-    BOOST_CHECK(test_args.m_settings.ro_config["sec1"].count("ccc")
-                && test_args.m_settings.ro_config["sec1"].count("h")
-                && test_args.m_settings.ro_config["sec2"].count("ccc")
-                && test_args.m_settings.ro_config["sec2"].count("iii")
-               );
+    BOOST_CHECK(test_args.m_settings.ro_config["sec1"].count("ccc"));
+    BOOST_CHECK(test_args.m_settings.ro_config["sec1"].count("h"));
+    BOOST_CHECK(test_args.m_settings.ro_config["sec2"].count("ccc"));
+    BOOST_CHECK(test_args.m_settings.ro_config["sec2"].count("iii"));                );
 
     BOOST_CHECK(test_args.IsArgSet("-a")
                 && 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 · 2020-08-14T03:45:33Z

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/

maflcko

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

vasild

ACK 568d42b

It is always better to have

assert(a);
assert(b);

instead of

assert(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.

vasild · 2020-08-14T07:36:54Z

+    BOOST_CHECK(!test_args.IsArgSet("-zzz"));
+    BOOST_CHECK(!test_args.IsArgSet("-iii"));
+
+    BOOST_CHECK(test_args.GetArg("-a", "xxx") == "");


Suggested change

BOOST_CHECK(test_args.GetArg("-a", "xxx") == "");

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);

Thank you! Updated.

hebasto · 2020-08-14T09:26:07Z

Updated 568d42b -> 35cd2da (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 · 2020-08-14T09:36:04Z

ACK 35cd2da

Crypt-iQ · 2020-08-14T12:02:26Z

ACK 35cd2da

hebasto mentioned this pull request Aug 13, 2020

build: Add support for llvm-cov #19688

Merged

DrahtBot added the Tests label Aug 13, 2020

maflcko reviewed Aug 14, 2020

View reviewed changes

vasild approved these changes Aug 14, 2020

View reviewed changes

test: Fix 'make cov' with clang

35cd2da

hebasto force-pushed the 200813-lcov branch from 568d42b to 35cd2da Compare August 14, 2020 09:22

maflcko added the Refactoring label Aug 14, 2020

maflcko merged commit 1a43cd3 into bitcoin:master Aug 14, 2020

hebasto deleted the 200813-lcov branch August 14, 2020 13:02

ryanofsky mentioned this pull request Sep 28, 2020

common: Disallow calling IsArgSet() on ALLOW_LIST options #17783

Draft

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

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

Uh oh!

Conversation

hebasto commented Aug 13, 2020

Uh oh!

practicalswift commented Aug 13, 2020

Uh oh!

hebasto commented Aug 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Aug 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjahr commented Aug 13, 2020

Uh oh!

hebasto commented Aug 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Crypt-iQ commented Aug 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

vasild Aug 14, 2020

Choose a reason for hiding this comment

Uh oh!

hebasto Aug 14, 2020

Choose a reason for hiding this comment

Uh oh!

hebasto commented Aug 14, 2020

Uh oh!

vasild commented Aug 14, 2020

Uh oh!

Crypt-iQ commented Aug 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

hebasto commented Aug 13, 2020 •

edited

Loading

laanwj commented Aug 13, 2020 •

edited

Loading

hebasto commented Aug 13, 2020 •

edited

Loading

Crypt-iQ commented Aug 14, 2020 •

edited

Loading