Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Tests] Include branch coverage info in coverage test #10511

Closed
wants to merge 2 commits into from

Conversation

achow101
Copy link
Member

@achow101 achow101 commented Jun 2, 2017

This PR adds an option to configure to include branch coverage statistics be gathered during make cov. It also disables logging when coverage is enabled so that the logging branches are not included in the coverage stats. This has a side effect of causing -Wunused-variable warnings during compile time when coverage is enabled.

@sipa
Copy link
Member

sipa commented Jun 2, 2017

Here is a possible hack to force a variable list of arguments to be treated as 'used' for the purpose of -Wunused-variable:

static inline void MarkUsed() {}
template<typename T, typename... Args> static inline void MarkUsed(const T& t, const Args&... args)
{
    (void)t;
    MarkUsed(args...);
}

#ifdef USE_COVERAGE
#define LogPrintf(...) do { MarkUsed(__VA_ARGS__); } while(0)
#define LogPrint(category, ...) do { MarkUsed(__VA_ARGS__); } while(0)
#else

@achow101
Copy link
Member Author

achow101 commented Jun 5, 2017

Used @sipa's hack to shut up the warnings.

@gmaxwell
Copy link
Contributor

gmaxwell commented Jun 6, 2017

Cool. Concept ACK.

Added an option to configure to allow for branch coverage statistics gathering.

Disabled logprint macro when coverage testing is on so that unnecessary branches are not analyzed.
with open(tracefile, 'r') as f:
with open(outfile, 'w') as wf:
for line in f:
if pattern in line:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps instead use line.startswith("SF:" + pattern) instead? That would prevent matching on variables that accidentally match the pattern name.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

in_remove = True
if not in_remove:
wf.write(line)
if 'end_of_record' in line:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps use line == 'end_of_record' instead? Same reason as above.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@achow101 achow101 force-pushed the lcov branch 2 times, most recently from 8687849 to 405b86a Compare June 9, 2017 19:25
@sipa
Copy link
Member

sipa commented Jun 12, 2017

utACK 405b86a

Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

@@ -0,0 +1,24 @@
#!/usr/bin/env python3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

explicit license is missing:

# Copyright (c) 2017 Bitcoin Core Developers
# Distributed under the MIT software license, see the accompanying
# file COPYING or http://www.opensource.org/licenses/mit-license.php.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

with open(outfile, 'w') as wf:
for line in f:
if line.startswith("SF:") and pattern in line:
in_remove = True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why you need the var in_remove. Couldn't you write wf.write(line);continue; here?

Copy link
Member Author

@achow101 achow101 Jun 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The format of the file is

SF:<path to file>
<bunch of lines>
end_of_record
<repeat>

In order to know that we are in a <bunch of lines> section that should be removed (because the SF:... line matches our removal pattern, we need to have a variable to remember that we shouldn't be writing these lines to the output file. That is what in_remove is for.

@maflcko maflcko requested a review from theuni June 13, 2017 11:30
Instead of using lcov -r (which is extremely slow), first use a python script to perform bulk cleanup of the /usr/include/* coverage. Then use lcov -a to remove the duplicate entries. This has the same effect of lcov -r but runs significantly faster
@theuni
Copy link
Member

theuni commented Jun 13, 2017

The lcov stuff was added ages ago, when lcov's --no-external was too new to rely on. I think it'd be ok now to require >=1.10 and forego all of the filtering.

@maflcko
Copy link
Member

maflcko commented Jun 19, 2017

@achow101 Are you still working on this?

@achow101
Copy link
Member Author

@MarcoFalke Yes

@achow101
Copy link
Member Author

Would it be okay with everyone if I just merged #10565 into this PR? It all falls under "improvements to coverage data".

@sipa
Copy link
Member

sipa commented Jun 20, 2017

@achow101 Have you tried the approach @theuni suggested above (using --no-external, and removing all filtering)?

@achow101
Copy link
Member Author

@sipa I am thinking of doing that and combining #10565 into this PR. If I remove filtering here, it would just be added back in by #10565.

@maflcko
Copy link
Member

maflcko commented Jun 22, 2017

Needs rebase.

@achow101
Copy link
Member Author

Closing this as #10565 contains this PR (I totally forgot) except for the copyright header I added last commit :/

@achow101 achow101 closed this Jun 22, 2017
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants