Make it easier to see which lines are covered and which are not:
Description
Revisions and Commits
| Restricted Differential Revision | |
| Restricted Differential Revision | |
| Restricted Differential Revision | |
| Restricted Differential Revision |
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.
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.