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.

Sign up for GitHub

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

Jump to bottom

Request to Implement the lcov "-t" option. #39

Closed
rgh-sas opened this issue Mar 16, 2020 · 19 comments · Fixed by #40
Closed

Request to Implement the lcov "-t" option. #39

rgh-sas opened this issue Mar 16, 2020 · 19 comments · Fixed by #40

Comments

@rgh-sas
Copy link

rgh-sas commented Mar 16, 2020

The implementation of the "-t" option is likely also related to the previously reported issue of implementing the "-a" option from lcov within fastcov where it would be useful to merge multiple .info files together with fastcov.

The lcov doc states the following for the "-t" option:

-t testname
--test-name testname
Specify test name to be stored in the tracefile.
This name identifies a coverage data set when more than one data set is merged into a combined tracefile (see option -a).
Valid test names can consist of letters, decimal digits and the underscore character ("_").

This is important if you'd like to retain a test name when you merge multiple .info files together using fastcov/lcov. That way when the combined info file gets passed to genhtml, you can use "Show Details" pull down to see the test coverage that each test contributed to a given source file. Very useful info.

Also, thank you very much for writing fastcov! You did a nice job and the result are impressive!
@RPGillespie6
Copy link
Owner

Sure, I never heard back from the guy on the other issue, but it sounds like this is a commonly used feature.

In that case here's what I'm going to do:

  1. I'm going to add support for this to fastcov's native json format first

So you'll be able to do:

$ fastcov -t Test1 ... -o report1.json
$ fastcov -t Test1 ... -o report2.json
$ fastcov -t Test1 ... -o report3.json
$ fastcov -a report1.json report2.json report3.json --lcov -o test1.info
$ fastcov -t Test2 ... -o report1.json
$ fastcov -t Test2 ... -o report1.json
$ fastcov -t Test2 ... -o report1.json
$ fastcov -a report1.json report2.json report3.json --lcov -o test2.info

This is super easy and I could probably do it tomorrow. It's all just simple JSON manipulation.

  1. I'll add support for parsing/combining .info files second. This is slightly more complicated since currently fastcov can only serialize info format, it can't parse it.

However, once I do that, you would be able to use -a in conjunction with .info files as well as .json files.

Does that sound okay?

@rgh-sas
Copy link
Author

rgh-sas commented Mar 17, 2020 via email •
edited
Loading

@RPGillespie6
Copy link
Owner

Yes, -t option would be fully supported.

RPGillespie6 added a commit that referenced this issue Mar 18, 2020
@RPGillespie6
Remove gcov-json output support, fix issues.
6c27bf8 
Description:
- Fix #39
- Fix #32
@RPGillespie6
Copy link
Owner

Ok, I've fully implemented this feature on a branch. Need to implement tests for it to make sure it behaves as expected and to iron out any bugs. Should be merged to master sometime this week.

@rgh-sas
Copy link
Author

rgh-sas commented Mar 18, 2020 via email

@RPGillespie6
Copy link
Owner

That branch is definitely WIP, so don't be surprised if there are bugs :P.

RPGillespie6 added a commit that referenced this issue Mar 20, 2020
@RPGillespie6
Add combine and testname options
eb459ef 
Description:
- Fix #39
- Fix #32
- Remove support for gcov-json output (nobody uses it)
- Fix example build
- Add additional test to make sure example builds
@RPGillespie6 RPGillespie6 mentioned this issue Mar 20, 2020
Merged
@RPGillespie6
Copy link
Owner

Ok, I think this branch has all of the bugs ironed out now. Could you please try it if you have time to make sure it meets your expectations?

RPGillespie6 added a commit that referenced this issue Mar 20, 2020
@RPGillespie6
Add combine and testname options
11e64e1 
Description:
- Fix #39
- Fix #32
- Remove support for gcov-json output (nobody uses it)
- Fix example build
- Add additional test to make sure example builds
@rgh-sas
Copy link
Author

rgh-sas commented Mar 20, 2020 via email

@rgh-sas
Copy link
Author

rgh-sas commented Mar 22, 2020 via email •
edited
Loading

@RPGillespie6
Copy link
Owner

This very helpful feedback. I misunderstood how the combine option should work given report files with different test names. This explains why the lcov generated one has both "TKTESTS" and "WTESTS" but fastcov only has "WTESTS". I will fix this and experiment with genhtml to make sure I understand this feature better.

For a given set of GCDA and GCNO files, would you expect that the resultant .info files to be the same between lcov and fastcov using similar options?

Yes, though be aware that -b is not equivalent to --rc lcov_branch_coverage=1. Instead use -B which tells fastcov to include all branches (whereas -b tries to intelligently remove "noisy" gcc-generated branches such as c++ exception and initializer list branches).

I noticed that a number of the following warnings were produced. Should I be concerned about these?

The warning basically means that fastcov doesn't know how to combine branch coverage for the given line because each file is reporting a different number of branches for the same line. This could be explained if one coverage file was using -b and other was generated using all branches.

@rgh-sas
Copy link
Author

rgh-sas commented Mar 22, 2020 via email

@RPGillespie6
Copy link
Owner

RPGillespie6 commented Mar 28, 2020 •
edited
Loading

Sorry, I haven't had much time to work on this, this week. However, I've found the root problem. I need to keep coverage reports with different test names in separate buckets. Right now I'm combining them all into one report, which destroys the information you want to see when invoking genhtml --show-details. I will push a fix soon.

RPGillespie6 added a commit that referenced this issue Mar 28, 2020
@RPGillespie6
Add combine and testname options
65ecbfb 
Description:
- Fix #39
- Fix #32
- Remove support for gcov-json output (nobody uses it)
- Fix example build
- Add additional test to make sure example builds
@RPGillespie6
Copy link
Owner

Ok, I believe the latest version of the branch is fixed to correctly segregate test names. I squashed the branch so you'll have to do a git reset --hard origin/bpg/add_testname_combine (or similar) to pick up the latest.

With the fix, I ran the fastcov example project twice (example/build.sh), each time with a different test name -t parameter, then combined with fastcov -C example.info example2.info --lcov -o example_combined.info. I can now see the separate coverage info in genhtml:

image

I will probably write a few more tests just to increase my confidence in this feature, but please try it out at your own leisure to see if it meets your needs.

@rgh-sas
Copy link
Author

rgh-sas commented Mar 28, 2020 via email

@rgh-sas
Copy link
Author

rgh-sas commented Mar 29, 2020 via email •
edited
Loading

@rgh-sas
Copy link
Author

rgh-sas commented Mar 29, 2020 via email •
edited
Loading

@RPGillespie6
Copy link
Owner

RPGillespie6 commented Mar 29, 2020 •
edited
Loading

What does it mean that a file got skipped? Bad format or perhaps zero length?

A file is skipped if one of fastcov's filtering options matches it. The following options will cause files to be skipped:

--exclude: (skip files whose absolute path contains any of the listed substrings)
--include: (skip files whose absolute path does not contain any of the listed substrings)
--source-files: (skip any files that are not one of these source files)

Are you using one of these 3 options?

Currently fastcov does not print which files it is skipping because it could make the terminal output very spammy, However, this could be a future CLI option (--verbose or something)

@rgh-sas
Copy link
Author

rgh-sas commented Mar 30, 2020 via email •
edited
Loading

@RPGillespie6
Copy link
Owner

This would be easy to add (just need to sprinkle in some conditional traces). I will take care of it after this issue. I've created a new issue to track this:

#41

RPGillespie6 added a commit that referenced this issue Mar 31, 2020
@RPGillespie6
Add combine and testname options
71969ac 
Description:
- Fix #39
- Fix #32
- Remove support for gcov-json output (nobody uses it)
- Fix example build
- Add additional test to make sure example builds
RPGillespie6 added a commit that referenced this issue Mar 31, 2020
@RPGillespie6
Add combine and testname options
c264133 
Description:
- Fix #39
- Fix #32
- Remove support for gcov-json output (nobody uses it)
- Fix example build
- Add additional test to make sure example builds
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add combine and testname options RPGillespie6/fastcov
2 participants
@RPGillespie6 @rgh-sas