1325207 - Reftest harness doesn't report fuzzy passes as unexpected when reftest also has fails-if

Status: REOPENED

(Testing :: Reftest, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Say you have a reftest like so:

  fuzzy-if(true,10,10) fails-if(true) == test.html ref.html

If test.html is exactly equal to ref.html, this will result in a TEST-UNEXPECTED-PASS, and cause the reftest job to fail. However, if test.html is a fuzzy match for ref.html (within the 10,10 limits), this will result in a TEST-KNOWN-FAIL. I think the fuzzy match scenario should also result in a TEST-UNEXPECTED-PASS, because without the "fails-if" clause, the job would be passing.

In the code, if a fuzzy match happens, the harness runs the line at [1]. Since |expected| is EXPECTED_FAIL, |equal| becomes false. This results in |test_passed| being false at [2]. The harness then looks up outputs[EXPECTED_FAIL][false] at [3], which produces KNOWN-FAIL [4]. (I want it to produce UNEXPECTED-PASS here instead).

If the fails-if were to be removed, the |expected| would be EXPECTED_FUZZY at [1], so |equal| would be true. That would make |test_passed| true at [2], and outputs[EXPECTED_FUZZY][true] would produce PASS.

[1] http://searchfox.org/mozilla-central/rev/4f5f9f3222f35fa4635ea96a0c963c130854ef19/layout/tools/reftest/reftest.jsm#1691
[2] http://searchfox.org/mozilla-central/rev/4f5f9f3222f35fa4635ea96a0c963c130854ef19/layout/tools/reftest/reftest.jsm#1698
[3] http://searchfox.org/mozilla-central/rev/4f5f9f3222f35fa4635ea96a0c963c130854ef19/layout/tools/reftest/reftest.jsm#1700
[4] http://searchfox.org/mozilla-central/rev/4f5f9f3222f35fa4635ea96a0c963c130854ef19/layout/tools/reftest/reftest.jsm#1571

Comment 1

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=84cf3e3c539222956b35ded4d39024204dafacdd

Comment 2

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

In general, the rule for failure annotations is that the last one that matches wins.  Do we want to make fuzzy-if special?

(See also bug 1252361, which I think is a rather high priority, actually.)

Comment 3

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Also see bug 580786 comment 16 and bug 580786 comment 24.

But, on the main topic, comment 0 really does sound like the expected behavior to me, as documented in http://searchfox.org/mozilla-central/rev/4f5f9f3222f35fa4635ea96a0c963c130854ef19/layout/tools/reftest/README.txt#54 (although somehow when we added failure types for "include" that bit of documentation ended up only covering "include").

Comment 4

[Kartikaya Gupta (email:[e-mail])]

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #2)
> In general, the rule for failure annotations is that the last one that
> matches wins.  Do we want to make fuzzy-if special?

Hm, that's fair. I guess in my mind fuzzy[-if] was already special because when it is encountered it sets the fuzzy_max_delta and fuzzy_max_pixels values, which are not reset even if later a fails[-if] is encountered. So in the example in comment 0 it really shouldn't be doing a fuzzy match at all, since the fails-if should override the fuzzy-if.

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #3)
> But, on the main topic, comment 0 really does sound like the expected
> behavior to me, as documented in
> http://searchfox.org/mozilla-central/rev/
> 4f5f9f3222f35fa4635ea96a0c963c130854ef19/layout/tools/reftest/README.txt#54
> (although somehow when we added failure types for "include" that bit of
> documentation ended up only covering "include").

As defined in that README, yeah I guess this is expected behaviour. If we were to redefine "fails[-if]" as "the opposite of whatever would happen without the fails clause" then comment 0 would make more sense. But it may not be worth doing that.

Closing as WONTFIX. I can just check for these cases manually once in a while on the graphics branch.

Comment 5

[Andrew Halberstadt [:ahal]]

With the defaults feature now landed, I think we'll need to figure out how to solve this. Imagine a manifest that looks like this:

defaults fails-if(webrender)
== foo.html bar.html
fuzzy(0-1,0-10) == foo.html bar2.html

Since the last one that matches wins, we overwrite the fails-if with the fuzzy meaning the second test won't have EXPECTED_FAIL on webrender and the job turns orange.

The same issue will happen with skip/skip-if. See my comments here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1617261#c4

Basically as soon as a developer adds skip/skip-if/fuzzy/fuzzy-if/fails/fails-if to the defaults section of a manifest, they are going to have surprising behaviour on any test that has its own failure annotation.

I have an idea on how to solve it. Will get some patches up soon.

Comment 6

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1325207 - [reftest] Stop treating 'skip/skip-if' as a failure type in the manifests

Comment 7

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1325207 - [reftest] Stop treating fuzzy/fuzzy-if as a failure type in the manifests

Depends on D64457

Comment 8

[Andrew Halberstadt [:ahal]]

These patches are WIP and not ready for review (though feel free to take a peek if you are interested).

Comment 9

[Andrew Halberstadt [:ahal]]

The fuzzy/fuzzy-if patch will need a little extra care (and it doesn't block my other work), so I'll likely land the skip one first and come back to the other once I have time.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/17109740bffa
[reftest] Stop treating 'skip/skip-if' as a failure type in the manifests r=kats

Comment 11

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/17109740bffa

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:ahal, maybe it's time to close this bug?

Comment 13

[Andrew Halberstadt [:ahal]]

I still hope to get back to this sometime in Q4 or later, but I haven't actively looked at this in awhile so unassigning for now.