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

add "--add-tracefile" like lcov #10

Closed
whart222 opened this issue Sep 2, 2013 · 11 comments

Comments

@whart222
Copy link
Member

@whart222 whart222 commented Sep 2, 2013

I want to merge two gcda result into one.
lcov can use --add-tracefile do the work, how gcovr do it? Thanks

@wichtounet

This comment has been minimized.

Copy link

@wichtounet wichtounet commented Jun 14, 2015

Such a feature would indeed be great. I also have the need for it with several object directories.

@verbitan

This comment has been minimized.

Copy link

@verbitan verbitan commented Nov 23, 2016

This is exactly the feature I'd like as well, so a vote for this issue from me 👍

@pbregener

This comment has been minimized.

Copy link

@pbregener pbregener commented Dec 14, 2016

@whart222 As you opened this issue 3 years ago and now seem to be the maintainer of gcovr - any thoughts on this?

@pbregener

This comment has been minimized.

Copy link

@pbregener pbregener commented Jan 13, 2017

ping @whart222

@latk

This comment has been minimized.

Copy link
Member

@latk latk commented Oct 12, 2018

While working on a new internal data model (191b30c) I realized that merging coverage data is not actually difficult – such merging is already done internally to combine the coverage from multiple object files. All that is missing is to serialize and deserialize that data. In particular:

  • a new generator that dumps the complete coverage data into a file, preferably as JSON.
  • an option to load such a files so that additional coverage can be merged. “Steal” the design of that option from lcov?

This file format should allow some sanity checks, for example that all runs must use the same options.

The file format could be modeled after the upcoming gcov json format (see #282) which would allow for some interesting synergies, but this is not necessary.

If anyone would like to take on this work, please do :) As per the contribution guidelines: if you need help please ask in the Gitter chat, discuss the design on this issue, or open a WIP pull request.

@CezaryGapinski

This comment has been minimized.

Copy link
Contributor

@CezaryGapinski CezaryGapinski commented Aug 16, 2019

Hi!

I am trying to make add-tracefile option with JSON files. I have noticed that something can be wrong with LineCoverage class constructor and update method.
Maybe I missed something, but noncode fileld should be set to True as a default value?
During update when one of the object has noncode field cleared and the other has it set that means the line is still real code, filed shouldn't be set and line should be taken into coverage?

@latk

This comment has been minimized.

Copy link
Member

@latk latk commented Aug 16, 2019

@CezaryGapinski Thank you for looking into this! That's a really good question, and I'm not entirely sure what the proper behaviour should be. Each line can have three states: covered, uncovered, noncode. A line that has no coverage data is implicitly noncode.

Noncode is not the default because when we create LineCoverage, that's usually because there is code on the line. If you find this problematic, it would be better to make that argument non-optional.

When I created this data model I just followed the previous design, which used set operations for convenience. Maybe it would be better to combine the flag with & rather than |, so that the covered/uncovered states dominate over the noncode state. I don't think a conflict could even occur during normal usage, so this shouldn't matter.

Please do what you think is best :) If your changes would break something, the test suite should complain.

@CezaryGapinski

This comment has been minimized.

Copy link
Contributor

@CezaryGapinski CezaryGapinski commented Aug 17, 2019

@latk

If your changes would break something, the test suite should complain.

It complains a lot, because change in LineCoverage changing GcovParser behaviour :).

I have pushed branch to my fork repository to show you an example, where is the problem. You can take a look on my test named "add_coverages" where I have two functions called depended on macro definition set by the compiler command line.

Without changing the noncode filed in LineCoverage class, generating report with merged two coverage results are wrong and shows that foo() and bar() functions were not called. With change in LineCoverage merged results are fine, but it has a bad impact on GcovParser :(.

Maybe I did something stupid in my code, but I stuck with that, so I would be grateful for any suggestions.

BTW: lcov with this example works fine.

@latk

This comment has been minimized.

Copy link
Member

@latk latk commented Aug 17, 2019

@CezaryGapinski Ok the problem is this: when a .line() of a FileCoverage object is accessed, a new LineCoverage object is instantiated if that line doesn't already exist.

(1) In the GcovParser, new lines usually contain coverage data. Making them noncode is wrong.

(2) When merging/updating coverage data, new lines should not affect the coverage. They have to be a “neutral element” regarding update(). I.e. we need to ensure:

new = LineCoverage(old.lineno)
new.update(old)
assert new == old  # doesn't actually have an equals operator

All of this previously worked fine because False is the neutral element for the | operator. You've changed the default to True which is the neutral element for &, but this violates requirement (1) leading to test case failures.

The simplest alternative I see is to stick to the old default, but to let the FileCoverage.line() function take defaults so that the update() method can get a neutral element:

- def line(self, lineno):
+ def line(self, lineno, **defaults):
     try:
         return self.lines[lineno]
     except KeyError:
-        self.lines[lineno] = line_cov = LineCoverage(lineno)
+        self.lines[lineno] = line_cov = LineCoverage(lineno, **defaults)
         return line_cov

 def update(self, other):
     for lineno, line_cov in other.lines.items():
-        self.line(lineno).update(line_cov)
+        self.line(lineno, noncode=True).update(line_cov)

With this change, all tests incl. your JSON tests pass :)

CezaryGapinski added a commit to CezaryGapinski/gcovr that referenced this issue Aug 17, 2019
Change operation on the LineCoverage.noncode from logical operator 'or' to 'and' and let the FileCoverage.line() function take defaults so that the LineCoverage.update() method can get a neutral value for noncode and doesn't change coverage results (incorrect change code line to from 'covered/uncovered' to 'noncode' during merging results).

Intention of this fix is described in gcovr#10
@CezaryGapinski

This comment has been minimized.

Copy link
Contributor

@CezaryGapinski CezaryGapinski commented Aug 17, 2019

@latk Thanks! You are my hero of the day :).
Updated version is on my forked repo.
Do you think that is good enough to make a pull request to add this new feature to gcovr?

@latk

This comment has been minimized.

Copy link
Member

@latk latk commented Aug 17, 2019

Awesome! Yes, please do open a PR. Per my skimming this looks really good, but there are some details about the JSON usage that I'd like to discuss.

I'm really excited about this --add-tracefile feature because this makes it possible to simplify the test suite a lot (separating gcov-level tests from report generator tests)

CezaryGapinski added a commit to CezaryGapinski/gcovr that referenced this issue Aug 19, 2019
Change operation on the LineCoverage.noncode from logical operator 'or' to 'and' and let the FileCoverage.line() function take defaults so that the LineCoverage.update() method can get a neutral value for noncode and doesn't change coverage results (incorrect change code line to from 'covered/uncovered' to 'noncode' during merging results).

Intention of this fix is described in gcovr#10
CezaryGapinski added a commit to CezaryGapinski/gcovr that referenced this issue Aug 21, 2019
Change operation on the LineCoverage.noncode from logical operator 'or' to 'and' and let the FileCoverage.line() function take defaults so that the LineCoverage.update() method can get a neutral value for noncode and doesn't change coverage results (incorrect change code line to from 'covered/uncovered' to 'noncode' during merging results).

Intention of this fix is described in gcovr#10
@latk latk closed this in 67a4e6a Nov 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.