Closed Bug 1369941 Opened 2 years ago Closed Last year

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)

Categories

(Testing :: Reftest, enhancement, P3)

enhancement

Tracking

(firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

Attachments

(3 files, 1 obsolete file)

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.
Priority: -- → P3
Flags: needinfo?(aschen)
Flags: needinfo?(bmo)
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.
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
Attachment #8998626 - Attachment is obsolete: true
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
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.
Attachment #8998656 - Flags: review+
Pushed by dbaron@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e3a6f64071f2
Replace single integers N in fuzzy() and fuzzy-if() with 0-N ranges. r=dholbert
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12387 for changes under testing/web-platform/tests
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).
Keywords: leave-open
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Running the script again today, against https://hg.mozilla.org/mozilla-central/rev/72442cf86f54 , produced no new changes.
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 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.
Attachment #9001809 - Flags: review+
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?
Flags: needinfo?(dbaron)
(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.)
(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 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.
Attachment #9002978 - Flags: review+
Attachment #9001809 - Attachment description: Bug 1369941 - Remove ability to specify fuzzy()/fuzzy-if() in reftest manifests without ranges. r?dholbert → Bug 1369941 - Remove ability to specify fuzzy()/fuzzy-if() in reftest manifests without ranges. r=dholbert
Attachment #9002978 - Attachment description: Bug 1369941 - Provide more advice about how to write fuzzy() annotations on reftests. r?dholbert → Bug 1369941 - Provide more advice about how to write fuzzy() annotations on reftests. r=dholbert
Flags: needinfo?(dbaron)
Keywords: leave-open
Pushed by dbaron@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5e7484d3b2e3
Remove ability to specify fuzzy()/fuzzy-if() in reftest manifests without ranges.  r=dholbert
Pushed by dbaron@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4fca1f7a874d
Provide more advice about how to write fuzzy() annotations on reftests.  r=dholbert
https://hg.mozilla.org/mozilla-central/rev/5e7484d3b2e3
https://hg.mozilla.org/mozilla-central/rev/4fca1f7a874d
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.