Page MenuHomePhabricator

Improve coverage UI
Closed, InvalidPublic

Description

Make it easier to see which lines are covered and which are not:

https://secure.phabricator.com/P323

Revisions and Commits

Event Timeline

There are a bunch of things actually wrong with the UI:

  • The UI doesn't explain what the column means, or what the colors represent.
  • Files with no coverage information are incorrectly marked entirely as "not executable" if other files have coverage.
  • There is no aggregate information about coverage presented (T841).
  • Similarly, there is no aggregate information about coverage on changed lines, which is the most important thing we could present in this interface.

I think the ability to determine which lines are covered or not isn't actually that important in this interface. Reviewers generally should care only about the numbers we don't currently show (e.g., 0% of your lines have coverage -- maybe write some tests, or 98% of your lines have coverage -- maybe get coverage on those last few lines). Authors can iterate on coverage more effectively with --detailed-coverage, which shows coverage across the entire line.

Basically, Differential's coverage display shouldn't be a surgical tool for analyzing coverage line-by-line. It should be a coarse tool to help reviewers assess whether the tests are effective at covering the changes or not. It's currently awful at this, but not because the blue needs to be changed to a "better" blue.

So let me fix the real problems with the UI and then maybe I'll consider changing the colors.

Thanks! Someone actually liked the colors :) I could make a bootcamp task for selecting the colors in "Display Preferences".

I briefly considered a very sarcastic patch file for this task which just added some kind of jQuery color selector to preferences.

epriestley changed file(s), attached 0: ; detached 0: .Mar 12 2012, 10:33 PM

โ—€ Merged tasks: T841.

epriestley changed file(s), attached 0: ; detached 0: .Mar 12 2012, 10:35 PM

More feedback (bhiller):

By the way, I like this a lot, though right now it feels a bit mysterious how we determine which lines are blue and which are red. It would be cool if there was a way to see what specific test/LOC provided test coverage for another LOC, I think it would help me trust the indicators more.

@bh THANK YOU FOR NOT ASKING ME TO CHANGE THE COLOR OF SOMETHING

I definitely want to do that (and it's actually fairly easy), I need to write a Tooltip class for Javelin first I think though.

epriestley changed file(s), attached 0: ; detached 0: .Mar 13 2012, 12:56 AM
epriestley changed file(s), attached 0: ; detached 0: .Mar 13 2012, 2:39 AM
chad changed the visibility from "All Users" to "Public (No Login Required)".Jul 3 2015, 4:35 AM

Some of this improved in the meantime; some of it we haven't seen more requests for.