-
-
Notifications
You must be signed in to change notification settings - Fork 416
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
Json report #825
Json report #825
Conversation
tests/test_json.py
Outdated
cov.json_report(a, outfile=output_path) | ||
with open(output_path) as result_file: | ||
parsed_result = json.loads(result_file.read()) | ||
del(parsed_result['meta']['timestamp']) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Thanks for this. I haven't had time to look at it yet, but will get to it soon! |
@nedbat no worries! I'm in no rush. I would love the feedback whenever you have time. However, I am not blocked as I still need to make time to finish up the tests and such. |
f6e3564
to
dd47d7e
Compare
Codecov Report
@@ Coverage Diff @@
## master #825 +/- ##
==========================================
+ Coverage 92.33% 92.34% +0.01%
==========================================
Files 83 85 +2
Lines 11201 11302 +101
Branches 1141 1146 +5
==========================================
+ Hits 10342 10437 +95
- Misses 723 724 +1
- Partials 136 141 +5
Continue to review full report at Codecov.
|
howdy! Just checking in to say im still alive. I updated my local master and then rebased this branch to keep it from getting stale. Im thinking ill have some time this weekend to beef up the tests a bit and look at the other todos. |
f7020a4
to
52216fe
Compare
Ok, think I got decent tests on this the config file is tested so the only thing this needs (I think) is review/confirmation on the schema and then I need to write some docs. |
5757ce8
to
ed97e33
Compare
updated from master |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Matt, thanks so much for taking this on! My comments run the gamut, I hope you don't mind some pedantry on the English :)
tests/test_json.py
Outdated
from coverage.misc import isolate_module | ||
from tests.coveragetest import UsingModulesMixin, CoverageTest | ||
|
||
os = isolate_module(os) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for this in our own test files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed
coverage/cmdline.py
Outdated
) | ||
json_pretty_print = optparse.make_option( | ||
'', '--pretty-print', action='store_true', | ||
help="print the json formatted for human readers", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Capital P.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed
coverage/cmdline.py
Outdated
Opts.contexts, | ||
] + GLOBAL_ARGS, | ||
usage="[options] [modules]", | ||
description="Generate an JSON report of coverage results." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"an JSON"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed
coverage/cmdline.py
Outdated
@@ -755,6 +788,7 @@ def unglob_args(args): | |||
report Report coverage stats on modules. | |||
run Run a Python program and measure code execution. | |||
xml Create an XML report of coverage results. | |||
json Create a JSON report of coverage results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should go between "html" and "report" (alphabetically)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed
coverage/control.py
Outdated
omit=None, include=None, contexts=None, pretty_print=None, | ||
show_contexts=None | ||
): | ||
"""Generate an JSON report of coverage results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"an JSON"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed
tests/test_json.py
Outdated
output_path = os.path.join(self.temp_dir, "a.json") | ||
cov.json_report(a, outfile=output_path) | ||
with open(output_path) as result_file: | ||
parsed_result = json.loads(result_file.read()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
json.load
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed
coverage/jsonreport.py
Outdated
self.data.set_query_contexts(self.config.report_contexts) | ||
meta = { | ||
"version": __version__, | ||
"timestamp": str(int(time.time())), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would ISO8601 be more friendly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in my head I was thinking time.time()
would be easier for machine parsing. However its not like ISO8601 would be hard... and it has the benefit of being much more human readable. So sure thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually looking at this it may have been me doing what xml was doing. Changed to isoformat
coverage/jsonreport.py
Outdated
reported_file.update({ | ||
'arcs_missing': analysis.missing_branch_arcs(), | ||
'arcs_executed': analysis.arcs_executed(), | ||
'arcs_unpredicted': analysis.arcs_unpredicted(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This value isn't usually in reports (I think?). I'd rather leave it out that confuse people at first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the references to arcs and just stuck with the branch percentages in totals
coverage/jsonreport.py
Outdated
'arcs_missing': analysis.missing_branch_arcs(), | ||
'arcs_executed': analysis.arcs_executed(), | ||
'arcs_unpredicted': analysis.arcs_unpredicted(), | ||
'missing_branch_arcs': analysis.missing_branch_arcs(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've reported this twice (also as 'arcs_missing'). I wonder if we should avoid "arcs" in this report, since it's nerdy and internal. Reports tend to talk about "branches".
Also, this value is a dict, while "arcs_executed" is a list of pairs. Let's make them uniform, and both be a list of pairs.
coverage/jsonreport.py
Outdated
'missing_lines': nums.n_missing, | ||
} | ||
reported_file = { | ||
'executed_lines': list(sorted(analysis.statements)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
analysis.statements
is not the executed lines, it's the executable lines. And, awkwardly, it looks like Analysis computed "executed" but does not make it available...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Analysis also has excluded
, which could be in the report.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Not at all. If anything I should be a tad bit embarrassed for "an json" :-P Looks like a mixture of small things and at least one or two big errors on my part to correct. I collected all the feedback. Left a few comments and will start working though this. Thanks! |
3fa1fce
to
adc857a
Compare
Handled a lot of the smaller feedback. Still have more to look at though |
This piece of feedback is still outstanding. We don't need the summary block but I find when looking at the full reports its nice to have these "summary stats" grouped together. Separating these properties from what I have perceived as the more raw data. So I see it as a tradeoff between making the report a bit easier on the human eye while making it mildly more annoying for the computer to parse since you have to go down one more level to access these properties. You see it when just scrolling though a non trivial report ^^ diff cover with contexts enabled. |
I think all other feedback has been addressed and I updated the readme to reflect the updated state of the code |
26ed1da
to
22a1a64
Compare
updated pr from latest master |
tests/test_config.py
Outdated
@@ -332,6 +332,10 @@ class ConfigFileTest(UsingModulesMixin, CoverageTest): | |||
hello = world | |||
; comments still work. | |||
names = Jane/John/Jenny | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lint reports trailing space here. There's a .editorconfig file in this repo, if you enable it in your editor, it will handle these things automatically.
On the structure of the JSON: OK, files will have a "summary" key. This is very close to merging! You'll need to squash the commits. I can add docs, unless you want to take a pass at it. I wonder if people will want us to explain the JSON structure? I hope not.. :) There's also CHANGES.rst and CONTRIBUTORS.txt, which I can tweak up if you like. |
c82c6da
to
6740c2d
Compare
added myself to contributors and added a line to changes. Squashed everything down to one commit. I can take a stab at the docs. |
c60a8d3
to
436eff6
Compare
are outputted with whitespace formatted for human consumption (True) or for | ||
minimum file size (False) | ||
|
||
``json_show_contexts`` (boolean, default false): should the json report include |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I stole the language for this line from the HTML section. I worry its a tad awkward because "on each line" is not quite right in the json context. Its not like the HTML report where every line literally has something I can hover over and see the contexts.
Its more like "Include data in the report showing the contexts recorded for each line"
Ok @nedbat I took a stab at the docs. One place I considered making an update but left alone was the FAQ there is a question that references the XML and HTML reports
I could change that to 'HTML, JSON, or XML' reports if you want but I did not think it added much. the doc changes are in this commit 436eff6 |
@Bachmann1234 this looks great. I am ready to merge it, and will make some minor edits after that. Squash the commits, and let's get this to master-town! :) |
436eff6
to
d22c847
Compare
@nedbat Squashed down to one commit and ready to go |
I have a watch on the repo so ill keep an eye out for any issues around this (or if something else catches my eye). Im sure something will come up. Also you know how to get ahold of me :-) |
The goal of this pr is to eventually lead to closing #720
Some example reports from a non trivial project
without Contexts: https://gist.github.com/Bachmann1234/418311192ebff1faca1e3fc54453602c
with Contexts: https://gist.github.com/Bachmann1234/bb799bdc5978a81a1e26daeb1881b950
command help:
simple report:
TODO: