648751 - Performance results with negative impact on some platforms should not be suppressed

Status: RESOLVED WONTFIX

(addons.mozilla.org Graveyard :: Developer Pages, defect, P4)

Description

[Wladimir Palant]

I think that the following change to AMO's code will primarily hide issues with the measurements: https://github.com/jbalogh/zamboni/commit/80cc09c5db23cfc2cfc5b52ffcd125b640454fb1. A result where the measurement goes into negative is extremely unlikely if the measurement was performed correctly, currently such results are primarily caused by bug 648229. Also, if a fluctuation causes the baseline test to return a higher value this code will cause many add-ons to simply disappear from the map. If you don't want to accept negative numbers then use max(delta, 0), don't throw the result away.

Comment 1

[Jorge Villalobos [:jorgev] (he/him)]

To me it makes more sense not to show a failed test than show an incorrect result. An incorrect result would be more misleading IMO.

My suggestion is WONTFIX.

Comment 2

[Wladimir Palant]

All the more reason not to suppress "strange" results. If you are certain that tests with negative results are always incorrect then it is a reason to rerun the test. At the very least the add-on should be marked with "performance test failed". If you simply suppress these results you will get into a situation again where issues like bug 648229 go unnoticed.

Comment 3

[Jorge Villalobos [:jorgev] (he/him)]

Testing errors should definitely be logged, and I don't have any problems on them being public somehow. However, I disagree that the patch should be backed out, which is what this bug is about.

Comment 4

[Wladimir Palant]

What I asked for is a different handling of suspicious results - in its current form it is perfect for hiding bugs. And it did so at least once already.

Comment 5

[Robert Kaiser]

Could we have AMO annotate the display with a "some tests failed" comment or something like that?