1369941 - go through fuzzy annotations without ranges and give them appropriate ranges (and then change single-value N to mean exactly N rather than 0-N)

Status: RESOLVED FIXED

(Testing :: Reftest, enhancement, P3)

Description

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

As a followup to bug 1252361, we should go through the reftests that are marked fuzzy and either convert the annotations to appropriate ranges, or remove them if they're removable.  (We could probably scrape the data from treeherder logs...)

Once we've done that, we should change the meaning of N to mean exactly N rather than 0-N, for both parameters to fuzzy() and the latter two parameters to fuzzy-if().

This will make it more likely that we'll notice fixes and then correctly annotate those fixes as no longer failing, and thus that we'll catch later regressions.

Comment 1

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

Attachment: fix-1369941.sh: script to edit the source tree

Comment 2

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

This script isn't quite right because the first argument to fuzzy-if() does sometimes have parentheses inside of it, but they're matched parentheses.

Comment 3

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

Attachment: Bug 1369941: Replace single integers N in fuzzy() and fuzzy-if() with 0-N ranges.

This patch was written entirely by the following script:

#!/bin/bash

if [ ! -d "./.hg" ]
then
    echo "Not in a source tree." 1>&2
    exit 1
fi

find . -regex '.*\(ref\|crash\)test.*\.list' | while read FILENAME
do
    echo "Processing ${FILENAME}."
    # The following has four substitutions.
    # The first one replaces the *first* argument to fuzzy() when it
    #   doesn't have a - in it, by replacing it with an explicit 0-N
    #   range.
    # The second one does the same for the *second* argument to fuzzy().
    # The third does the same for the *second* argument to fuzzy-if().
    # The fourth does the same for the *third* argument to fuzzy-if().
    sed -i -e 's/\(fuzzy(\)\([^ ,()-]*\)\(,[^ ,()]*)\)/\10-\2\3/g;s/\(fuzzy([^ ,()]*,\)\([^ ,()-]*\)\()\)/\10-\2\3/g;s/\(fuzzy-if([^ ,()]*,\)\([^ ,()-]*\)\(,[^ ,()]*)\)/\10-\2\3/g;s/\(fuzzy-if([^ ,()]*,[^ ,()]*,\)\([^ ,()-]*\)\()\)/\10-\2\3/g' "${FILENAME}"
done

Comment 4

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8cb334094827a46478e64f4370fe940a3b55ad21&group_state=expanded

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8998656 [details]
Bug 1369941: Replace single integers N in fuzzy() and fuzzy-if() with 0-N ranges.

Daniel Holbert [:dholbert] has approved the revision.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e3a6f64071f2
Replace single integers N in fuzzy() and fuzzy-if() with 0-N ranges. r=dholbert

Comment 7

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12387 for changes under testing/web-platform/tests

Comment 8

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

So I still want to run the script again in a week or two (hopefully I remember), and then I think drop support for non-range values (which I think makes more sense than suddenly changing their meaning to something different).

Comment 9

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 10

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/e3a6f64071f2

Comment 11

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 12

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

Running the script again today, against https://hg.mozilla.org/mozilla-central/rev/72442cf86f54 , produced no new changes.

Comment 13

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=57acfaac8d44b4df0b445d2e7c93d53deaef45c3&group_state=expanded

Comment 14

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

Attachment: Bug 1369941 - Remove ability to specify fuzzy()/fuzzy-if() in reftest manifests without ranges. r=dholbert

I'm removing this no-longer-used feature because it promotes a behavior
(using the default 0 minimum) that means we never see reports of
unexpected passes when the bugs are fixed, and thus aren't protected
from the bugs regressing again.

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 9001809 [details]
Bug 1369941 - Remove ability to specify fuzzy()/fuzzy-if() in reftest manifests without ranges.  r=dholbert

Daniel Holbert [:dholbert] has approved the revision.

Comment 16

[Daniel Holbert [:dholbert]]

If you get a chance, it'd be nice to document your anticipated best-practices somewhere for how people should annotate fuzzy failures in the new world of required min/max range.  (Maybe in the reftest README.txt file?)

Here's my hand-wavy guesses at best practices for test-annotators, across the two most common scenarios (though maybe you've got something else in mind for recommendations):

 (1) For a test that *reliably* fails with a max-difference of D across N pixels: best practice is to use a tight min/max range like "fuzzy-if(condition,D-D,N-N)"

 (2)For a test that *only sometimes* fails, we have to use 0 in at least one of the fuzzy() args so that the non-failing runs aren't flag as unexpected passes. (For this, should we standardize on fuzzy-if(condition,0-D,0-N), with 0 as the minimum for both? We could arbitrarily choose one of the args to have the 0 lower-bound, but that feels arbitrary... maybe we might as well use 0 for both, as long as we've got it in one, to avoid giving the mistaken impression of having extra strictness.)

What do you think?

Comment 17

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

(In reply to Daniel Holbert [:dholbert] from comment #16)
>  (2)For a test that *only sometimes* fails, we have to use 0 in at least one
> of the fuzzy() args so that the non-failing runs aren't flag as unexpected
> passes. (For this, should we standardize on fuzzy-if(condition,0-D,0-N),
> with 0 as the minimum for both? We could arbitrarily choose one of the args
> to have the 0 lower-bound, but that feels arbitrary... maybe we might as
> well use 0 for both, as long as we've got it in one, to avoid giving the
> mistaken impression of having extra strictness.)

I think you have to use 0 for both, given:

                fuzz_exceeded = maxDifference.value > g.urls[0].fuzzyMaxDelta ||
                                differences > g.urls[0].fuzzyMaxPixels;

(I'm still a little worried about some of the other logic around fuzzy, though, after you made me look at it.)

Comment 18

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

(In reply to David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7 from comment #17)
> (I'm still a little worried about some of the other logic around fuzzy,
> though, after you made me look at it.)

Never mind, I think it's ok.

Comment 19

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

Attachment: Bug 1369941 - Provide more advice about how to write fuzzy() annotations on reftests. r=dholbert

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 9002978 [details]
Bug 1369941 - Provide more advice about how to write fuzzy() annotations on reftests.  r=dholbert

Daniel Holbert [:dholbert] has approved the revision.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5e7484d3b2e3
Remove ability to specify fuzzy()/fuzzy-if() in reftest manifests without ranges.  r=dholbert

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4fca1f7a874d
Provide more advice about how to write fuzzy() annotations on reftests.  r=dholbert

Comment 23

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/5e7484d3b2e3
https://hg.mozilla.org/mozilla-central/rev/4fca1f7a874d