Chromium Code Reviews Chromium Code Reviews
Issue 2560253004:
IntersectionObserver: convert tests to testharness.js (Closed)
| Index: third_party/WebKit/LayoutTests/intersection-observer/client-rect.html |
| diff --git a/third_party/WebKit/LayoutTests/intersection-observer/client-rect.html b/third_party/WebKit/LayoutTests/intersection-observer/client-rect.html |
| index c0ae7cbf57bd7498979f7c79602f55931b5a10bd..047e11fbbcce1132680ad9200179a627215dd6d6 100644 |
| --- a/third_party/WebKit/LayoutTests/intersection-observer/client-rect.html |
| +++ b/third_party/WebKit/LayoutTests/intersection-observer/client-rect.html |
| @@ -1,30 +1,50 @@ |
| <!DOCTYPE html> |
| -<script src="../resources/js-test.js"></script> |
| -<script src="../resources/intersection-observer-helper-functions.js"></script> |
| +<script src="../resources/testharness.js"></script> |
| +<script src="../resources/testharnessreport.js"></script> |
| <iframe id="iframe" srcdoc="<div id='target'><div style='width:1000px;height:1000px'></div></div>"></iframe> |
| -<script> |
| -description("Ensure that change.boundingClientRect matches change.target.getBoundingClientRect() for a clipped element with overflow inside an iframe."); |
| -var iframe = document.getElementById("iframe"); |
| -var entries = []; |
| -var target; |
| +<script> |
| +function waitForNotification(f) { |
| + requestAnimationFrame(() => { |
| + setTimeout(() => { |
| + setTimeout(f); |
|
foolip
2016/12/14 13:42:16
Is the spec actually this lax about the timing?
szager1
2016/12/14 19:54:11
The sequence of events is this:
- js: modify DOM
foolip
2016/12/14 22:17:56
The nested setTimeout is what looked suspicious to
szager1
2016/12/22 19:55:26
I added a lengthy comment about how this works to
|
| + }); |
| + }); |
| +} |
| iframe.onload = function() { |
|
foolip
2016/12/14 13:42:16
Waiting for the load event before creating the tes
szager1
2016/12/14 19:57:02
Is this a style nit, or is there good reason to st
foolip
2016/12/14 22:17:57
testharness.js doesn't wait for tests to appear af
szager1
2016/12/22 19:55:26
I followed your suggestion for iframe tests.
|
| - target = iframe.contentDocument.getElementById("target"); |
| - new IntersectionObserver((changes) => { |
| - entries.push(...changes); |
| - }).observe(target); |
| + var t = async_test("Ensure that change.boundingClientRect matches change.target.getBoundingClientRect() for a clipped element with overflow inside an iframe."); |
| + |
| + test(function() { assert_equals(window.innerWidth, 800) }, "Window must be 800 pixels wide."); |
|
foolip
2016/12/14 13:42:16
This is a bit unusual, to wrap every assertion in
szager1
2016/12/14 19:54:11
The problem is that when running an async test, th
foolip
2016/12/14 22:17:57
Yeah, this can be a bother, that an early failure
szager1
2016/12/22 19:55:26
As we discussed offline: testharness allows you to
|
| + test(function() { assert_equals(window.innerHeight, 600) }, "Window must be 600 pixels high."); |
| + |
| + var target = this.contentDocument.getElementById("target"); |
|
foolip
2016/12/14 13:42:16
Since this sin't associated with any test, if it h
szager1
2016/12/14 19:54:11
OK, I'll add an assert.
|
| + var entries = []; |
| + var observer = new IntersectionObserver((changes) => { |
| + entries = entries.concat(changes); |
| + }); |
| + observer.observe(target); |
| + entries = entries.concat(observer.takeRecords()); |
|
foolip
2016/12/14 13:42:16
Is it possible per spec that there are any records
szager1
2016/12/14 19:54:11
It should not happen that there are already notifi
foolip
2016/12/14 22:17:56
If it weren't for the fact that the test still con
szager1
2016/12/22 19:55:26
This check is now a hard assert that will cause th
|
| + test(function() { assert_equals(entries.length, 0) }, "No initial notifications."); |
| waitForNotification(step0); |
| -}; |
| -function step0() { |
| - shouldBeEqualToNumber("entries.length", 1); |
| - if (entries.length > 0) { |
| - shouldBe("entries[0].boundingClientRect.top", "target.getBoundingClientRect().top"); |
| - shouldBe("entries[0].boundingClientRect.left", "target.getBoundingClientRect().left"); |
| - shouldBe("entries[0].boundingClientRect.width", "target.getBoundingClientRect().width"); |
| - shouldBe("entries[0].boundingClientRect.height", "target.getBoundingClientRect().height"); |
| + function step0() { |
| + test(function() { assert_equals(entries.length, 1) }, "One notification."); |
|
foolip
2016/12/14 13:42:16
If only one entry is expected, maybe structure the
szager1
2016/12/14 19:54:11
I did define a checkEntry routine in other tests f
foolip
2016/12/14 22:17:56
It's fine to have shared scripts with helpers if i
szager1
2016/12/22 19:55:26
I moved common code into LayoutTests/intersection-
|
| + if (entries.length > 0) { |
| + test(function() { assert_equals(entries[0].boundingClientRect.top, |
| + target.getBoundingClientRect().top) }, |
| + "entries[0].boundingClientRect.top == target.getBoundingClientRect().top"); |
| + test(function() { assert_equals(entries[0].boundingClientRect.left, |
| + target.getBoundingClientRect().left) }, |
| + "entries[0].boundingClientRect.left == target.getBoundingClientRect().left"); |
| + test(function() { assert_equals(entries[0].boundingClientRect.width, |
| + target.getBoundingClientRect().width) }, |
| + "entries[0].boundingClientRect.width == target.getBoundingClientRect().width"); |
| + test(function() { assert_equals(entries[0].boundingClientRect.height, |
| + target.getBoundingClientRect().height) }, |
| + "entries[0].boundingClientRect.height == target.getBoundingClientRect().height"); |
| + } |
| + t.done(); |
| } |
| - finishJSTest(); |
| -} |
| +}; |
| </script> |
