Closed Bug 1530462 Opened 7 months ago Closed 7 months ago

float-retry-push-image.html makes bogus/flaky positioning assumption

Categories

(Core :: Layout: Floats, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(1 file)

Test float-retry-push-image.html is flaky (and is currently annotated as being extremely fuzzy, except on webrender).

Its expectations are bogus because its shape-outside is a diagonal line that crosses the central point of a rectangle, and it's undefined whether or not the that central pixel is "shaded" and available for floats or not (which creates a positioning ambiguity).

It looks like we can fix this by adjusting the diagonal slightly to be at e.g. 50.01% rather than at 50%. (At least, that fixes the test locally.)

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

Test float-retry-push-image.html is flaky (and is currently annotated as being extremely fuzzy, except on webrender)

Its annotations are a bit inconsistent actually -- we have this "fuzzy everywhere" annotation in the reftest version:

fuzzy(0-255,0-500) == float-retry-push-image.html float-retry-push-ref.html
https://searchfox.org/mozilla-central/rev/b10ae6b7a50d176a813900cbe9dc18c85acd604b/layout/reftests/w3c-css/submitted/shapes1/reftest.list#109

...vs. this one in the WPT reimport version (this is where we have an "except webrender"):

if webrender and (os == "win"): PASS
FAIL
https://searchfox.org/mozilla-central/rev/b10ae6b7a50d176a813900cbe9dc18c85acd604b/testing/web-platform/meta/css/vendor-imports/mozilla/mozilla-central-reftests/shapes1/float-retry-push-image.html.ini#2

Anyway, the webrender exception is itself unreliable, per https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=229865751&repo=mozilla-inbound&lineNumber=20240

Let's just fix the test.

And while we're at it, update the reference case to have "Reference" in
its title, so that the reference case and testcases are easier to distinguish
when viewing them side-by-side in several tabs.

The patch makes the test pass locally on my machine. Let's see if it fixes it elsewhere, too... Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d15321afc6dee0fcc56b0585082874010777421e

Note: the WPT copy of this test (the copy in testing/web-platform) will still be broken for the time being, but it'll become fixed after this makes the roundtrip through upstream WPT and gets reimported back into the tree.

As I understand it, this is the least-bumpy way to fix tests that exist in these two locations in our tree.

Darn, Try shows that we're still flaky on some platforms even with the adjustment. (Possibly due to those platforms having HiDPI displays? not sure. I know Windows uses 125% pixel scaling by default, which I could imagine causing subtle weirdness here.)

New proposal: let's adjust the test to bake in 1px of vertical flexibility, by:

  • making the float 1px taller
  • adding a clip-path to its container, to only paint the region that we expect would be covered in an idealized scenario.

This allows the float to be positioned either at the "ideal" position or 1px upwards from it, and the test will still pass either way.

Attachment #9046423 - Attachment description: Bug 1530462: Adjust reftest 'float-retry-push-image.html' to avoid rounding ambiguity and produce more consistent results. r?bradwerth → Bug 1530462: Adjust reftest 'float-retry-push-image.html' to allow for rounding error on linear-gradient edge. r?bradwerth

Try looks good with the new clipping approach discussed in comment 4:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=155e7aeb41090fe58e0916dcd27716f06e1520ff&selectedJob=230445681

(The Linux x64 QuantumRender R8 oranges there are for an unrelated test, and seem intermittent or like an unrelated regression.)

Brad, does the updated patch look good to you? (I can't request re-review in phabricator, unfortunately.)

Flags: needinfo?(bwerth)

Yes, that's a good fix. I marked the patch as approved.

Flags: needinfo?(bwerth)

Thank you!

Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e6333b7c1331
Adjust reftest 'float-retry-push-image.html' to allow for rounding error on linear-gradient edge. r=bradwerth
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

(In reply to David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it) from comment #10)

WPT pull at https://github.com/web-platform-tests/wpt/pull/15581 ... which has a failure I don't understand.

The key issue appears to be:

ERROR:lint:css/vendor-imports/mozilla/mozilla-central-reftests/shapes1/float-retry-push-image.html:51: Whitespace at EOL (TRAILING WHITESPACE)

Aha, thanks (& sorry about that). I can fix that with a quick push to inbound.

Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d678e501c5a
followup: remove end-of-line whitespace in testcase. (no review, trivial test-only change)

There were 2 failures: the lint failure in https://travis-ci.com/web-platform-tests/wpt/jobs/181034954 is easily understandable; I don't understand the wpt-firefox-nightly-stability failure at https://tools.taskcluster.net/groups/FEO4R2y5SQWQHSd0r4elHA/tasks/aBa71QnzTce7CWoawaeXLg/details I still don't understand.

That one seems to be a harness issue during the "test-verify" testrun process.

The log is here:
https://taskcluster-artifacts.net/aBa71QnzTce7CWoawaeXLg/0/public/logs/live_backing.log

It trips over "NoSuchWindowException: Browsing context has been discarded" while trying to run the test 10 times. I'm guessing it might be intermittent?

TEST-START | /css/vendor-imports/mozilla/mozilla-central-reftests/shapes1/float-retry-push-circle.html
Run 0/10
PID 272 | 1551290483252	Marionette	INFO	Testing http://web-platform.test:8000/css/vendor-imports/mozilla/mozilla-central-reftests/shapes1/float-retry-push-circle.html == http://web-platform.test:8000/css/vendor-imports/mozilla/mozilla-central-reftests/shapes1/float-retry-push-ref.html
PID 272 | JavaScript error: resource:///modules/sessionstore/SessionStore.jsm, line 2383: TypeError: tabbrowser.getTabForBrowser is not a function
PID 272 | JavaScript error: resource:///modules/sessionstore/SessionStore.jsm, line 2383: TypeError: tabbrowser.getTabForBrowser is not a function
TEST-PASS | /css/vendor-imports/mozilla/mozilla-central-reftests/shapes1/float-retry-push-circle.html | took 663ms
TEST-START | /css/vendor-imports/mozilla/mozilla-central-reftests/shapes1/float-retry-push-circle.html
Run 1/10
PID 272 | 1551290483441	Marionette	INFO	No differences allowed
Error running command reset with arguments ():
Traceback (most recent call last):
  File "/home/test/web-platform-tests/tools/wptrunner/wptrunner/testrunner.py", line 100, in run
    rv = commands[command](*args)
  File "/home/test/web-platform-tests/tools/wptrunner/wptrunner/testrunner.py", line 113, in reset
    self.executor.reset()
  File "/home/test/web-platform-tests/tools/wptrunner/wptrunner/executors/executormarionette.py", line 784, in reset
    self.implementation.reset(**self.implementation_kwargs)
  File "/home/test/web-platform-tests/tools/wptrunner/wptrunner/executors/executormarionette.py", line 869, in reset
    self.setup(screenshot)
  File "/home/test/web-platform-tests/tools/wptrunner/wptrunner/executors/executormarionette.py", line 865, in setup
    self.executor.protocol.marionette._send_message("reftest:setup", data)
  File "/home/test/web-platform-tests/_venv/lib/python2.7/site-packages/marionette_driver/decorators.py", line 26, in _
    return func(*args, **kwargs)
  File "/home/test/web-platform-tests/_venv/lib/python2.7/site-packages/marionette_driver/marionette.py", line 741, in _send_message
    self._handle_error(err)
  File "/home/test/web-platform-tests/_venv/lib/python2.7/site-packages/marionette_driver/marionette.py", line 765, in _handle_error
    raise errors.lookup(error)(message, stacktrace=stacktrace)
NoSuchWindowException: Browsing context has been discarded
stacktrace:
	WebDriverError@chrome://marionette/content/error.js:179:5
	NoSuchWindowError@chrome://marionette/content/error.js:411:5
	assert.that/<@chrome://marionette/content/assert.js:417:13
	assert.open@chrome://marionette/content/assert.js:175:25
	setup@chrome://marionette/content/reftest.js:74:33
	GeckoDriver.prototype.setupReftest@chrome://marionette/content/driver.js:3529:17
	Async*despatch@chrome://marionette/content/server.js:289:40
	async*execute@chrome://marionette/content/server.js:262:16
	async*onPacket/<@chrome://marionette/content/server.js:235:20
	async*onPacket@chrome://marionette/content/server.js:236:9
	_onJSONObjectReady/<@chrome://marionette/content/transport.js:492:20

[...snip...]

SUITE-END | took 5s
Closing logging queue
queue closed
## All results ##

### /css/vendor-imports/mozilla/mozilla-central-reftests/shapes1/float-retry-push-circle.html ###
| Subtest |            Results            | Messages |
|---------|-------------------------------|----------|
|         | **PASS: 1/10, MISSING: 9/10** |          |

## Unstable results ##
You need to log in before you can comment on or make changes to this bug.