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 feature to merge coverage data in JSON format #326

Merged
merged 8 commits into from Nov 2, 2019

Conversation

CezaryGapinski
Copy link
Contributor

@CezaryGapinski CezaryGapinski commented Aug 17, 2019

This PR implements merging coverage data from files in JSON format.

PR helps to merge results, simillary to other tools like lcov. Files to combine are in JSON format recommended by @latk in #10, created partially by @opikalo in PR #302 and use update methods in coverage data model. The file structure is based on JSON intermediate format implemented in gcov mentioned in #282 with structure and additional keys specific to gcovr.
gcovr always create reports with source files paths relative to root directory, therefore JSON files structure relative to root. It also helps to combine results from different builds on separate computers or different directories with the same source code directory structure.

Copy link
Member

@latk latk left a comment

Thank you so much for working on this. It looks really good and I'll be happy to merge soon, but I have a couple of comments.

  • minor remarks regarding the structure of the JSON munging code and regarding the structure of the JSON format
  • concerns about the command line parameters
  • the test runner has to be updated in order to try JSON test targets. Unfortunately this would require that every test Makefile has a JSON target, but that target can be empty (for now).

Do not worry about the Travis CI test failure: that seems to be an issue with a dependency that is no longer available for Python 3.4. That version had its end of life in March. My original plan was to drop old Python versions in 2020, but I'd rather merge this PR than support ancient Pythons.

gcovr/configuration.py Outdated Show resolved Hide resolved
gcovr/json_generator.py Outdated Show resolved Hide resolved
gcovr/json_generator.py Outdated Show resolved Hide resolved
gcovr/json_generator.py Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
gcovr/__main__.py Show resolved Hide resolved
gcovr/configuration.py Show resolved Hide resolved
gcovr/tests/add_coverages/Makefile Show resolved Hide resolved
gcovr/tests/add_coverages/reference/coverage_bar.json Outdated Show resolved Hide resolved
gcovr/tests/add_coverages/Makefile Show resolved Hide resolved
@codecov

This comment has been minimized.

@codecov
Copy link

@codecov codecov bot commented Aug 19, 2019

Codecov Report

Merging #326 into master will increase coverage by 0.02%.
The diff coverage is 92.3%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #326      +/-   ##
==========================================
+ Coverage   94.89%   94.91%   +0.02%     
==========================================
  Files          16       17       +1     
  Lines        1879     1965      +86     
  Branches      328      343      +15     
==========================================
+ Hits         1783     1865      +82     
- Misses         46       48       +2     
- Partials       50       52       +2
Impacted Files Coverage Δ
gcovr/configuration.py 96.44% <ø> (ø) ⬆️
gcovr/coverage.py 99.11% <100%> (ø) ⬆️
gcovr/__main__.py 89.47% <84.21%> (-1.78%) ⬇️
gcovr/json_generator.py 96% <96%> (ø)
gcovr/gcov.py 87.86% <0%> (+0.88%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f5af01...099477d. Read the comment docs.

Copy link
Member

@latk latk left a comment

Ok, we're nearly there!

  • let's use a gcovr/noncode field instead of using gcov's unexecuted_block field
  • some extra spaces in the JSON reference file cause test failures. This seems to depend on the Python version, so maybe it's best to use a SCRUBBER in the test runner to remove trailing spaces.

gcovr/json_generator.py Outdated Show resolved Hide resolved


def _line_from_json(line, json_line):
line.noncode = json_line['unexecuted_block']
Copy link
Member

@latk latk Aug 19, 2019

Choose a reason for hiding this comment

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

see above comment about the unexecuted_block field.

Copy link
Contributor Author

@CezaryGapinski CezaryGapinski Aug 24, 2019

Choose a reason for hiding this comment

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

Now line.noncode has value from JSON gcovr/noncode field.

gcovr/tests/add_coverages/reference/coverage.json Outdated Show resolved Hide resolved
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
As mentioned in documentation:
If sort_keys is true (default: False), then the output of dictionaries will be sorted by key; this is useful for regression tests to ensure that JSON serializations can be compared on a day-to-day basis.
This can be useful for test JSON files as a text files, to always prevent the same sequence
@CezaryGapinski
Copy link
Contributor Author

@CezaryGapinski CezaryGapinski commented Aug 21, 2019

Third version of PR. Changes:

  • fix in gcov's unexecuted_block field - replaced to gcovr/noncode field
  • added separators and sort_keys fileds to force ordering in dictionaries in JSON (useful during comparison between reference and result file, where they serialized to text representation)

@opikalo
Copy link
Contributor

@opikalo opikalo commented Sep 18, 2019

Anything I can do to help this PR to land?

@CezaryGapinski
Copy link
Contributor Author

@CezaryGapinski CezaryGapinski commented Sep 20, 2019

Anything I can do to help this PR to land?

For me this PR looks ok, maybe I missed something, but I do not have any feedback from @latk what should I improve.
I guess he has to decide if its is good enough to merge it.

latk added a commit to latk/gcovr that referenced this issue Nov 2, 2019
latk
latk approved these changes Nov 2, 2019
@latk latk merged commit 16e97e6 into gcovr:master Nov 2, 2019
1 of 2 checks passed
@latk
Copy link
Member

@latk latk commented Nov 2, 2019

This took a lot longer than expected, but I finally merged this excellent PR. It seems I have become the bottleneck for gcovr development. If anyone would like write access in order to merge changes without having to wait for me, I'd be happy to arrange that.

@CezaryGapinski
Copy link
Contributor Author

@CezaryGapinski CezaryGapinski commented Nov 3, 2019

Hi!
No problem at all. You did a lot of work with this project and you should continue!
Python is not my primary language, but If you want I can help. I just need some time to go back to this project and analyse it more deeply.

@CezaryGapinski CezaryGapinski deleted the json_add_coverages branch Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants