This is an archive of the discontinued LLVM Phabricator instance.

Daily report: only display non empty result tables to avoid cluttering the UI.
ClosedPublic

Authored by aadg on Feb 15 2017, 8:14 AM.

Details

Summary

This is a useability fix only. Empty tables are only taking space and
this is distracting from the essential overview the daily report page is
supposed to provide. However, this could be considered controversial as
some could prefer to keep the empty tables, thus this commit review
solliciting other users' feedback to check if there is agreement.

Diff Detail

Repository
rL LLVM

Event Timeline

aadg created this revision.Feb 15 2017, 8:14 AM
kristof.beyls edited edge metadata.Feb 15 2017, 8:42 AM

I think that there are 2 main reasons why a table could be empty for a particular metric on the daily report page: either the test run just didn't collect the metric, or it did collect the metric, but there were no significant deltas that need to be reported.
I'd say that the ideal behaviour would be to not show an empty table when the test run didn't collect the metric; but to show the empty table when test results were collected, but there were no significant deltas.

I'm assuming that implementing that would be a bit more work though.

FWIW, if you think it's already an improvement, I'd agree with removing the table header of an empty table, but I think I'd still would like to keep the h3 heading to indicate that the daily report analyzed that metric, and there is simply nothing interesting to report. I think that can be done by just moving the line '{%- if field_results -%}' one line down from where you've put it now?

cmatthews edited edge metadata.Feb 15 2017, 9:51 AM

Sure, or instead of omitting totally, print a message like "No {METRIC} changes found in {N} results analyzed."

aadg added a comment.Feb 15 2017, 10:16 AM

So far, we have never disambiguated "no change worth reporting today for this metric" from "nothing was ever reported for this metric", so there is no regression there. That's where I would like to get eventually, but this would require much more work to channel the info. I will go for the "No {METRIC} changes found in {N} results analyzed.".

Thanks for the comments !

aadg updated this revision to Diff 88741.Feb 16 2017, 8:30 AM

Change the patch according to the various review comments (and some offline discussion with kristof.beyls).

cmatthews accepted this revision.Feb 16 2017, 8:46 AM
This revision is now accepted and ready to land.Feb 16 2017, 8:46 AM
This revision was automatically updated to reflect the committed changes.