Bug 101976 – garden-o-matic doesn't know about reftests

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Dirk Pranke]

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.

[Ojan Vafai]

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.

[Dirk Pranke]

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.

[Dirk Pranke]

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.

[Ojan Vafai]

Yeah. I don't disagree with any of that.

[Dirk Pranke]

*** Bug 104713 has been marked as a duplicate of this bug. ***

[Dirk Pranke]

Created attachment 178929 [details]
Patch

[Dirk Pranke]

please give me feedback on what good js style would be for this; I don't know the idioms, but this looks pretty crufty.

[Ojan Vafai]

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.

[Dirk Pranke]

Committed r137407: <http://trac.webkit.org/changeset/137407>

[Dirk Pranke]

Change reverted in http://trac.webkit.org/changeset/137664 ... this totally busted garden-o-matic (the rebaseline button was broken).

[Dirk Pranke]

Created attachment 179371 [details]
Patch

[Dirk Pranke]

Eric or Dimitri, can you take a look in both Adam and Ojan's absence?

[Dirk Pranke]

(whoops, r+'ed the wrong patch by accident).

[Eric Seidel (no email)]

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.

[Dirk Pranke]

Committed r137780: <http://trac.webkit.org/changeset/137780>

[Tony Chang]

*** Bug 90765 has been marked as a duplicate of this bug. ***