1542002 - Mark list-style-021.xht as fuzzy rather than disabled

Status: RESOLVED FIXED

(Core :: Layout, task, P3)

Description

[Daniel Holbert [:dholbert]]

The Web Platform Test list-style-021.xht has a single occasionally fuzzy pixel (visually imperceptible), as discussed in bug 1383451 comment 81 and bug 1383451 comment 89.

We disabled the test in that bug, because we had no other way at the time of stemming the intermittent-failure-spam.

However, now we have support for fuzzy annotations for WPT tests (bug 1478472) so we should convert the disabled annotation to a fuzzy annotation.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: Bug 1542002: Mark list-style-021.xht as fuzzy rather than disabled. r?jgraham

Comment 2

[Daniel Holbert [:dholbert]]

The attached one-line patch doesn't actually work yet, though I don't understand why.

It causes us to report a test failure, with the reftest screenshots for the two files being identical. (If I remove the "fuzzy" line entirely, then the test is reported as passing.) I'm probably misunderstanding something about the syntax, but I'm not sure what.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: output from "./mach wpt [path-to-test]" with patch applied (test is reported as failing, though test/ref screenshots are identical)

Comment 4

[Daniel Holbert [:dholbert]]

Relevant snippet from the log:

INFO Allowed 0-1 pixels different, maximum difference per channel 8-8
INFO Found 0 pixels different, maximum difference per channel 0
TEST_END: FAIL, expected PASS - Testing http://web-platform.test:8000/css/CSS2/lists/list-style-021.xht == http://web-platform.test:8000/css/CSS2/lists/list-style-021-ref.xht

TEST-UNEXPECTED-FAIL | /css/CSS2/lists/list-style-021.xht | Testing http://web-platform.test:8000/css/CSS2/lists/list-style-021.xht == http://web-platform.test:8000/css/CSS2/lists/list-style-021-ref.xht
REFTEST IMAGE 1 (TEST): data:image/png;[...]
REFTEST IMAGE 2 (REFERENCE): data:image/png;[...same data URI as above...]

Right now my fuzzy annotation has maxDifference=8;totalPixels=0-1. If I change it to have maxDifference=0-8, then the test starts being reported as passing. James, should I make that change? (i.e. should I use a 0-8 range for maxDifference)

I might be misremembering, but I think the "regular" reftest harness would treat my current annotation (without 0 included in the maxDifference range) as meaning: "1 pixel is allowed to differ, but it's also fine if it doesn't differ. If it does differ, then you should expect its maxDifference to be exactly 8". Apparently that is not how WPT is interpreting my annotation though, because it treats no-pixels-differing as a failure, despite the totalPixels=0-1 in my syntax.

Comment 5

[James Graham [:jgraham]]

Yeah, at the moment there isn't any special logic to handle 0 pixels different as meaning the maxDifference can be ignored. Maybe there should be.

Comment 6

[Daniel Holbert [:dholbert]]

Can you clarify what syntax I should be using here, then? (Should I use ranges that include 0 for both values? That seems to work, but it feels quite verbose. Is there a more compact way of expressing the condition "sometimes 1 pixel differs, with a difference of 8"?)

Comment 7

[James Graham [:jgraham]]

No, you currently have to use both ranges. I can look at special casing where total pixels is allowed to be 0 to allow any range for maxDifference (I thought what I had matched gecko reftests post dbaron's recent changes but I guess not).

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to James Graham [:jgraham] from comment #7)

No, you currently have to use both ranges.

OK, thanks.

I can look at special casing where total pixels is allowed to be 0 to allow any range for maxDifference (I thought what I had matched gecko reftests post dbaron's recent changes but I guess not).

Intuitively that makes sense to me; otherwise 0 isn't a useful value to include in a pixel value range, really. (unless you specify both ranges as I'll be doing here)

Basically, if 0 is in either range, it intuitively seems like an exact match should be allowed. Having said that, I haven't used the current incarnation of gecko fuzzy annotations very much, so it's possible that your implementation does match the gecko reftest harness's semantics and that those just don't make sense to me. :)

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #8)

Basically, if 0 is in either range, it intuitively seems like an exact match should be allowed.

Aha, I see you've now implemented that in bug 1542329. Thanks!

Comment 10

[Daniel Holbert [:dholbert]]

(ab)using needinfo just in case you missed the review-requested-phabmail -- see https://phabricator.services.mozilla.com/D26242 . Thanks!

Comment 11

[James Graham [:jgraham]]

I think I had processed the review mail when it wasn't actionable and missed the followup, or something. Anyway reviewed now.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/642d92a8a58e
Mark list-style-021.xht as fuzzy rather than disabled. r=jgraham

Comment 13

[Daniel Holbert [:dholbert]]

Thanks! Yeah, I think I recall that the "requested --> not-requested --> requested-again" phabricator-mail flow isn't sufficiently clear about the final next-action state. :)

Anyway: landed now.

Comment 14

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/642d92a8a58e