Summary: | garden-o-matic doesn't know about reftests | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dirk Pranke <dpranke> | ||||||
Component: | Tools / Tests | Assignee: | Dirk Pranke <dpranke> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, adamk, dglazkov, eric, hayato, ojan.autocc, ojan, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Dirk Pranke
2012-11-12 13:38:11 PST
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. *** |