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.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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?
Sure, or instead of omitting totally, print a message like "No {METRIC} changes found in {N} results analyzed."
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 !
Change the patch according to the various review comments (and some offline discussion with kristof.beyls).