codecov / browser-extension

Codecov Browser Extension
http://codecov.io
Apache License 2.0
214 stars 60 forks source link

Show diff coverage in compare view #19

Closed stevepeak closed 8 years ago

stevepeak commented 8 years ago

CC https://twitter.com/zeeg/status/655973292298272768

Concept 1

Show next to entire file coverage

screen shot 2015-10-19 at 7 10 40 am

Concept 2

Instead of showing entire file coverage.

screen shot 2015-10-19 at 7 12 07 am

Have more concepts? Feel free to comment below!

dcramer commented 8 years ago

IMO show both, possibly within the same button?

Coverage 96.77% (100% Diff)

And if you can, extend the TOC (this is the area we really care about).

Basically what we want to do is look at a PR and see "cool, all lines of code are run by robots". We're going to try out the PR comments but in my experience those are way too noisy. Ideally GitHub could provide better hooks for this so you could just manage some state on things, but I can't imagine that happening.

This is primarily an ask due to all of us previously using Phabricator at companies we worked at, which has the best code review flow process of any system I've ever seen.

I'll use this as a reference:

https://secure.phabricator.com/D14295

While they don't seem to have coverage enabled for their own code, the first thing you'd usually see is something in the file's table of contents:

https://www.dropbox.com/s/joux2a0haqtw0l5/Screenshot%202015-10-18%2022.16.23.png?dl=0

Two columns would existing showing diff coverage and total coverage. You primarily focus on "was this change covered by tests" or "how much of it was".

Additionally it does inline coverage in a really nice way. Effectively it keeps the diff view (even though it does side by side), and it attaches indicators in the sidebar instead of overlaying the entire line. You can see an example of that in a screenshot here:

http://cramer.io/2014/05/03/on-pull-requests/

stevepeak commented 8 years ago

Awesome feedback! I do agree inside the button can do the best.

I can also add coverage details to the TOC as suggested, not a problem.

I have not used Phabricator yet...but it does look awesome. What about overlaying coverage data in there as well? Just and idea for now. Thoughts?

The browser extension is open source in this repository and I'm happy to take pull request if you feel inclined to hack on it.