So, if a reftest fails it shows up as an ImageFailure, and will let you happily rebaseline it. However, NRWT will notice that the test is a reftest, log a warning, and ignore the baselines. I believe the information about whether the test is or isn't a reftest is in results.json, so we probably just need to make the UI aware of it. If not, it certainly would be easy to add.
In the examine view, it doesn't show you the rebaseline button for reftest. If it does, it's a regression. On the main page, it's less clear what to do since multiple tests are grouped together. I suppose we shouldn't show the rebaseline button there either? Also, webkit-patch can check if something is a reftest. It really should never rebaseline reftests. Fixing garden-o-matic is nice, but "webkit-patch rebaseline" should also know about reftests.
I definitely get the rebaseline button when looking at the examine view under expected failures, so maybe it's just a regression. As you say, it's really a bug if anything tries to rebaseline a reftest, not just garden-o-matic, but it's most important to fix the UI first, IMO.
i.e., I think garden-o-matic, in addition to *not* showing the rebaseline button, should probably show something indicating that the test is a reftest.
Yeah. I don't disagree with any of that.
*** Bug 104713 has been marked as a duplicate of this bug. ***
Created attachment 178929 [details] Patch
please give me feedback on what good js style would be for this; I don't know the idioms, but this looks pretty crufty.
Comment on attachment 178929 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178929&action=review > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/controllers.js:44 > + if (isAnyReftest(testName, resultsByTest)) { > + statusView.addMessage(id, testName + ' is a ref test, skipping'); > + } else { Nit: single-line if shouldn't have curlies. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/controllers.js:60 > + } else { > + statusView.addFinalMessage(id, 'No tests left to rebaseline!') > + } Nit: single-line else shouldn't have curlies. Non-nit: maybe make this text more explict? 'No non-reftests to rebaseline.' > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/controllers.js:63 > +// FIXME: This is duplicated from ui/results.js :(. ui.results.js is probably the wrong place for it. Maybe it should just be in results.js (i.e. not in the ui directory) as results.isAnyReftest. Then this code and ui.results.js can both call it? I dunno. I don't have the intended architecture of this stuff fresh in my brain, so not sure if that makes sense. This is fine for now w the FIXME.
Committed r137407: <http://trac.webkit.org/changeset/137407>
Change reverted in http://trac.webkit.org/changeset/137664 ... this totally busted garden-o-matic (the rebaseline button was broken).
Created attachment 179371 [details] Patch
Eric or Dimitri, can you take a look in both Adam and Ojan's absence?
(whoops, r+'ed the wrong patch by accident).
Comment on attachment 179371 [details] Patch I'm happy to rs=me, but you probably want someone more qualified to look at this, even if post-facto.
Committed r137780: <http://trac.webkit.org/changeset/137780>
*** Bug 90765 has been marked as a duplicate of this bug. ***