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

Adding json_summary changes as one commit #366

Merged
merged 107 commits into from Jul 20, 2020

Conversation

opikalo
Copy link
Contributor

@opikalo opikalo commented Apr 26, 2020

This is just #302 rebased into a single commit, to clean up commit history.

JSON summary output allows pushing basic coverage data for processing using other downstream tools or storing in DB.

EDIT: After some discussion, we are keeping --json-pretty behavior identical for backward compatibility. If you want pretty summaries, you would need --json-pretty-summary

opikalo and others added 30 commits April 2, 2019 17:11
JSON output allows pushing basic coverage data for processing using
other downstream tools or storing in DB. Currently text report has
special formatting for filenames, and XML output does not some key
information (total covered / total lines) to be useful for additional
processing.
As discussed in gcovr#302, refactoring to make report more like GCC gcov
9.1 as referenced in:
https://gcc.gnu.org/onlinedocs/gcc/Invoking-Gcov.html

Also, it's json summary, not an intermediate storage, thus renaming
arguments.
This match drops all relative path files, which are critical for our
build.
The lxml module requires far less memory than minidom.  However,
printing the encoded XML document is a bit tricky due to Python2/3
compatibility.
In gcovr#118, TiloW took a stab at this issue.
While lxml is not included in Python core,
this simplifies the code
* choose Xenial image
* update to Python 3.7
* remove version bound from pypy3.5
This function is used by Cobertura, Sonarqube and Txt.
Html uses a proper relative path
Want a Cobertura, HTML, and Sonarqube report?

     gcovr --html-details report.html --xml cobertura.xml --sonarqube sonar.xml

This should speed up some common use cases because the coverage data doesn't
have to be parsed again for each report.

The --output is now only used as a *default* for the output file name,
and should be consumed exactly once.
By using a dedicated type for filter options it's possible include the
path context for a filter. This enables (relative) filters in config
files to be treated as relative to the location of the config file,
rather than relative to the current working directory.
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
UnicodeDecodeError: 'utf-8' codec can't decode byte
@codecov
Copy link

codecov bot commented Apr 26, 2020

Codecov Report

Merging #366 into master will increase coverage by 0.00%.
The diff coverage is 97.67%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #366   +/-   ##
=======================================
  Coverage   94.95%   94.95%           
=======================================
  Files          19       19           
  Lines        2160     2182   +22     
  Branches      374      377    +3     
=======================================
+ Hits         2051     2072   +21     
  Misses         50       50           
- Partials       59       60    +1     
Impacted Files Coverage Δ
gcovr/configuration.py 96.88% <ø> (ø)
gcovr/__main__.py 90.04% <66.66%> (-0.39%) ⬇️
gcovr/csv_generator.py 85.71% <100.00%> (-4.77%) ⬇️
gcovr/json_generator.py 100.00% <100.00%> (ø)
gcovr/tests/test_gcovr.py 92.02% <100.00%> (ø)
gcovr/utils.py 91.97% <100.00%> (+0.31%) ⬆️

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 1bb7249...bdf8bae. Read the comment docs.

@CezaryGapinski
Copy link
Contributor

Hi @opikalo

Why not just use --json-summary to report this summary in JSON format and use --json-summary-pretty to do this in pretty way and leave --json and --json-pretty as it is? That would be something similar to --print-summary.

Added `--json-summary-pretty` option to keep backwards
compatability. Added branch test for json summary coverage for better
test coverage.
@opikalo
Copy link
Contributor Author

opikalo commented May 2, 2020

@CezaryGapinski: should be good to go.

CHANGELOG.rst Outdated Show resolved Hide resolved
@@ -67,7 +68,7 @@ def print_json_summary_report(covdata, output_file, options):

json_dict = {}
json_dict['current_working_directory'] = options.root
json_dict['gcovr/format_version'] = JSON_FORMAT_VERSION
json_dict['gcovr/summary_format_version'] = JSON_SUMMARY_FORMAT_VERSION
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about change:
json_dict['gcovr/summary_format_version'] = JSON_SUMMARY_FORMAT_VERSION
to:
json_dict['summary_format_version'] = JSON_SUMMARY_FORMAT_VERSION

without gcovr prefix? As it is mentioned in the User Guide gcovr/ was used to differentiate between GCOV keys and gcovr keys. This summary doesn't base on some external format, so there is nothing to differentiate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@CezaryGapinski json is notorious about not telling what this payload is. I would rather keep the gcovr in place instead of ambiguous summary_format_version.


json_bar:
./testcase_bar
$(GCOVR) -d --json coverage_bar.json
$(GCOVR) -d --json -o coverage_bar.json
Copy link
Contributor

Choose a reason for hiding this comment

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

It's turned out in this commit 3aa4a34 output should work even without -o, so maybe it is better to leave at least one test to check correct behaviour for this issue?

Copy link
Member

Choose a reason for hiding this comment

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

I touched that file in the html-rewrite branch (now merged). The different JSON reports in that Makefile now exercise different -o/--json combinations:

# Enable JSON output via the --json-pretty mode.
# However, this option cannot take a filename directly
# so that "-o" is used.
coverage_foo.json:
./testcase_foo
$(GCOVR) -d --json-pretty -o $@
# Just use --json and pass the output filename directly.
coverage_bar.json:
./testcase_bar
$(GCOVR) -d --json $@

@CezaryGapinski
Copy link
Contributor

For me this PR after minor changes will look good, but it must be reviewed by @latk. Perhaps it would be nice to add some description in the User Guide.
@Irfy - do you have something do add or change based on your PR #363?

@opikalo
Copy link
Contributor Author

opikalo commented Jun 1, 2020

@latk - appreciate a review

Copy link
Member

@latk latk left a comment

Choose a reason for hiding this comment

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

Thank you for keeping this PR up to date! I want to merge it, but want to discuss the below points first (fairly minor in the large scheme of things though).

Personal note: I have a lower workload during this week and should be now able to respond more promptly than in the past months.

Comment on lines 84 to 89
if options.show_branch:
total, cover, percent = coverage.branch_coverage()
uncovered_lines = coverage.uncovered_branches_str()
else:
total, cover, percent = coverage.line_coverage()
uncovered_lines = coverage.uncovered_lines_str()
Copy link
Member

Choose a reason for hiding this comment

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

Instead of switching between line and branch coverage, the summary can reasonably show both. Compare #376, as they essentially show the same data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand the idea, and the data is essentially right there; however, currently, the text/tabulated summary does not show branch. Are we departing from that? What should be the behavior of the summary with and without show_branch option?

Copy link
Member

Choose a reason for hiding this comment

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

The text report switches between line/branch coverage for space reasons, not because this is fundamentally useful. For a machine-readable report, text width does not matter. Including this data is effectively zero cost but makes the summary much more useful.

It may be useful to use the same field names as the CSV report (#376) uses for columns.

total_covered = 0

json_dict = {}
json_dict['current_working_directory'] = options.root
Copy link
Member

Choose a reason for hiding this comment

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

Should the cwd be root, or root relative to the output file, or the actual current working directory? Should it be a relative or absolute path? Depends on the exact intention for this field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I placed it thinking that it will allow to differentiated multiple analysis from different trees; should we rename it to be root to be consistent?

Copy link
Member

Choose a reason for hiding this comment

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

One of the problems we have with gcov is that it doesn't make quite clear which paths are relative to what, so I'd like to avoid making similar problems for downstream. I think that relevant designs could include:

  • all file paths are relative to the summary report location, and no root/cwd is needed; or

  • there is a root or cwd field that is relative to the summary report location, and the reported file paths are relative to the root – similar to the HTML <base> element.

    Suggested change
    json_dict['current_working_directory'] = options.root
    json_dict['root'] = os.path.relpath(options.root, output_file)

Both designs have things that are in their favour, but I think that most use cases are more interested in the path relative to root, not relative to the report.

In any case, it should be possible to correctly interpret the report without having to know how gcovr was invoked.

gcovr/tests/oos/Makefile Show resolved Hide resolved
@CezaryGapinski
Copy link
Contributor

@opikalo Can you rebase your branch on the current master? For me --json-summary feature is really cool, and it will be great to merge it. I have noticed something weird in the commits history. It is hard to noticed what has changed :). I have tried to create your changes on my fork, so you can take a look if you like it or arrange it differently and of course remove me as an committer :). I know it is a small change, but it helps to keep a linear history.

Warning: percentages are now in [0.0- 1.0] range instead of 0-100%
Also: 'file' -> 'filename'
Also, 'root' is relative to the report file.
@opikalo
Copy link
Contributor Author

opikalo commented Jul 20, 2020

@latk : I think I addressed all issues.

@opikalo opikalo merged commit b49ce8d into gcovr:master Jul 20, 2020
@Spacetown Spacetown added this to the 4.3 milestone Oct 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants