third_party/WebKit/LayoutTests/intersection-observer/client-rect.html - Issue 2560253004: IntersectionObserver: convert tests to testharness.js

Side by Side Diff: third_party/WebKit/LayoutTests/intersection-observer/client-rect.html

Issue 2560253004: IntersectionObserver: convert tests to testharness.js (Closed)

Patch Set: rebase Created 4 years ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

« no previous file with comments | « third_party/WebKit/LayoutTests/http/tests/intersection-observer/resources/cross-origin-subframe.html ('k') | third_party/WebKit/LayoutTests/intersection-observer/client-rect-expected.txt » ('j') | no next file with comments »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
1 <!DOCTYPE html>	1 <!DOCTYPE html>

2 <script src="../resources/js-test.js"></script>	2 <script src="../resources/testharness.js"></script>

3 <script src="../resources/intersection-observer-helper-functions.js"></script>	3 <script src="../resources/testharnessreport.js"></script>

4 <iframe id="iframe" srcdoc="<div id='target'><div style='width:1000px;height:100 0px'></div></div>"></iframe>	4 <iframe id="iframe" srcdoc="<div id='target'><div style='width:1000px;height:100 0px'></div></div>"></iframe>

	5

5 <script>	6 <script>

6 description("Ensure that change.boundingClientRect matches change.target.getBoun dingClientRect() for a clipped element with overflow inside an iframe.");	7 function waitForNotification(f) {

7	8 requestAnimationFrame(() => {

8 var iframe = document.getElementById("iframe");	9 setTimeout(() => {

9 var entries = [];	10 setTimeout(f);
	foolip 2016/12/14 13:42:16 Is the spec actually this lax about the timing? 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 Show quoted text On 2016/12/14 13:42:16, foolip wrote: > Is the spec actually this lax about the timing? The sequence of events is this: - js: modify DOM in a way that should cause a notification - js: waitForNotificationm(nextTestRoutine) - requestAnimationFrame(first setTimeout) - beginFrame - run rAF handler - setTimeout(second setTimeout) - style, layout, paint - generate IntersectionObserver notifications and post task to deliver them. - First setTimeout handler runs - setTimeout(second setTimeout) - IntersectionObserver notifications are delivered - Second setTimeoutHandler runs - nextTestRoutine(), which checks for notifications, modifies DOM, waitForNotifications... Because the IntersectionObserver spec details both the timing of when notifications are generated, and also that post task is used to schedule the delivery of the notifications, this should be consistent across browsers. This is, in my opinion, not lax, but actually quite precise. foolip 2016/12/14 22:17:56 The nested setTimeout is what looked suspicious to Show quoted text On 2016/12/14 19:54:11, szager1 wrote: > On 2016/12/14 13:42:16, foolip wrote: > > Is the spec actually this lax about the timing? > > The sequence of events is this: > > - js: modify DOM in a way that should cause a notification > - js: waitForNotificationm(nextTestRoutine) > - requestAnimationFrame(first setTimeout) > - beginFrame > - run rAF handler > - setTimeout(second setTimeout) > - style, layout, paint > - generate IntersectionObserver notifications and post task to deliver them. > - First setTimeout handler runs > - setTimeout(second setTimeout) > - IntersectionObserver notifications are delivered > - Second setTimeoutHandler runs > - nextTestRoutine(), which checks for notifications, modifies DOM, > waitForNotifications... > > Because the IntersectionObserver spec details both the timing of when > notifications are generated, and also that post task is used to schedule the > delivery of the notifications, this should be consistent across browsers. This > is, in my opinion, not lax, but actually quite precise. The nested setTimeout is what looked suspicious to me, as if the spec had said "queue a task to queue task to ..." at animation frame timing. Starting at https://wicg.github.io/IntersectionObserver/#update-intersection-observations... I can only find one "queue a task" in the steps that lead up to actually invoking the callbacks, so if there are any callbacks wouldn't the be guaranteed to have been invoked at requestAnimationFrame(() => setTimeout(/* this time /))? Or does one of the timeouts mean "wait for layout" and not "wait for queued tasks to have run"? szager1* 2016/12/22 19:55:26 I added a lengthy comment about how this works to Show quoted text On 2016/12/14 22:17:56, foolip (OOO til Jan. 2) wrote: > On 2016/12/14 19:54:11, szager1 wrote: > > On 2016/12/14 13:42:16, foolip wrote: > > > Is the spec actually this lax about the timing? > > > > The sequence of events is this: > > > > - js: modify DOM in a way that should cause a notification > > - js: waitForNotificationm(nextTestRoutine) > > - requestAnimationFrame(first setTimeout) > > - beginFrame > > - run rAF handler > > - setTimeout(second setTimeout) > > - style, layout, paint > > - generate IntersectionObserver notifications and post task to deliver them. > > - First setTimeout handler runs > > - setTimeout(second setTimeout) > > - IntersectionObserver notifications are delivered > > - Second setTimeoutHandler runs > > - nextTestRoutine(), which checks for notifications, modifies DOM, > > waitForNotifications... > > > > Because the IntersectionObserver spec details both the timing of when > > notifications are generated, and also that post task is used to schedule the > > delivery of the notifications, this should be consistent across browsers. > This > > is, in my opinion, not lax, but actually quite precise. > > The nested setTimeout is what looked suspicious to me, as if the spec had said > "queue a task to queue task to ..." at animation frame timing. Starting at > https://wicg.github.io/IntersectionObserver/#update-intersection-observations... > I can only find one "queue a task" in the steps that lead up to actually > invoking the callbacks, so if there are any callbacks wouldn't the be guaranteed > to have been invoked at requestAnimationFrame(() => setTimeout(/* this time > */))? > > Or does one of the timeouts mean "wait for layout" and not "wait for queued > tasks to have run"? I added a lengthy comment about how this works to LayoutTests/intersection-observer/resources/intersection-observer-test-utils.js
10 var target;	11 });

	12 });

	13 }

11	14

12 iframe.onload = function() {	15 iframe.onload = function() {
	foolip 2016/12/14 13:42:16 Waiting for the load event before creating the tes Waiting for the load event before creating the tests should typically not be necessary. I would try to write this as a single async test: async_test(t => { iframe.onload = t.step_func(() => { assert_equals(window.innerWidth, 800, "window width"); /* and so on / }); }, "change.boundingClientRect matches change.target.getBoundingClientRect() for a clipped element with overflow inside an iframe") szager1* 2016/12/14 19:57:02 Is this a style nit, or is there good reason to st Show quoted text On 2016/12/14 13:42:16, foolip wrote: > Waiting for the load event before creating the tests should typically not be > necessary. I would try to write this as a single async test: > > async_test(t => { > iframe.onload = t.step_func(() => { > assert_equals(window.innerWidth, 800, "window width"); > /* and so on / > }); > }, "change.boundingClientRect matches change.target.getBoundingClientRect() for > a clipped element with overflow inside an iframe") Is this a style nit, or is there good reason to structure it that way? Just asking before I take the time to change all the tests, since that's a lot of busy work. foolip* 2016/12/14 22:17:57 testharness.js doesn't wait for tests to appear af Show quoted text On 2016/12/14 19:57:02, szager1 wrote: > On 2016/12/14 13:42:16, foolip wrote: > > Waiting for the load event before creating the tests should typically not be > > necessary. I would try to write this as a single async test: > > > > async_test(t => { > > iframe.onload = t.step_func(() => { > > assert_equals(window.innerWidth, 800, "window width"); > > /* and so on / > > }); > > }, "change.boundingClientRect matches change.target.getBoundingClientRect() > for > > a clipped element with overflow inside an iframe") > > Is this a style nit, or is there good reason to structure it that way? Just > asking before I take the time to change all the tests, since that's a lot of > busy work. testharness.js doesn't wait for tests to appear after the window load event if there are already test started, so the thing to worry about is files with multiple tests, if it's possible that some test is created after the window load event. The iframe load event blocks the window load event, so with just a single test per file there's no problem. The other way around one need not wonder about timing or number of tests, which is why I tend to prefer it. Either way is fine for this CL. szager1* 2016/12/22 19:55:26 I followed your suggestion for iframe tests. Show quoted text On 2016/12/14 22:17:57, foolip (OOO til Jan. 2) wrote: > On 2016/12/14 19:57:02, szager1 wrote: > > On 2016/12/14 13:42:16, foolip wrote: > > > Waiting for the load event before creating the tests should typically not be > > > necessary. I would try to write this as a single async test: > > > > > > async_test(t => { > > > iframe.onload = t.step_func(() => { > > > assert_equals(window.innerWidth, 800, "window width"); > > > /* and so on */ > > > }); > > > }, "change.boundingClientRect matches change.target.getBoundingClientRect() > > for > > > a clipped element with overflow inside an iframe") > > > > Is this a style nit, or is there good reason to structure it that way? Just > > asking before I take the time to change all the tests, since that's a lot of > > busy work. > > testharness.js doesn't wait for tests to appear after the window load event if > there are already test started, so the thing to worry about is files with > multiple tests, if it's possible that some test is created after the window load > event. > > The iframe load event blocks the window load event, so with just a single test > per file there's no problem. The other way around one need not wonder about > timing or number of tests, which is why I tend to prefer it. Either way is fine > for this CL. I followed your suggestion for iframe tests.
13 target = iframe.contentDocument.getElementById("target");	16 var t = async_test("Ensure that change.boundingClientRect matches change.targe t.getBoundingClientRect() for a clipped element with overflow inside an iframe." );

14 new IntersectionObserver((changes) => {	17

15 entries.push(...changes);	18 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 This is a bit unusual, to wrap every assertion in a separate test. Are there parts of web-platform-tests where this is commonplace? How about just `assert_equals(window.innerWidth, 800, 'window width')? (I suggested it so obviously I agree, but https://chromium.googlesource.com/chromium/src/+/master/docs/testing/writing_... says "The assertion description conveys the way the actual value was obtained" and that results in sensible failure messages.) szager1 2016/12/14 19:54:11 The problem is that when running an async test, th Show quoted text On 2016/12/14 13:42:16, foolip wrote: > This is a bit unusual, to wrap every assertion in a separate test. Are there > parts of web-platform-tests where this is commonplace? > > How about just `assert_equals(window.innerWidth, 800, 'window width')? > > (I suggested it so obviously I agree, but > https://chromium.googlesource.com/chromium/src/+/master/docs/testing/writing_... > says "The assertion description conveys the way the actual value was obtained" > and that results in sensible failure messages.) The problem is that when running an async test, the first failed assert will cause test execution to halt immediately. So, we only get to see the first failing result. This was the only way I could manage to patch over that behavior. foolip 2016/12/14 22:17:57 Yeah, this can be a bother, that an early failure Show quoted text On 2016/12/14 19:54:11, szager1 wrote: > On 2016/12/14 13:42:16, foolip wrote: > > This is a bit unusual, to wrap every assertion in a separate test. Are there > > parts of web-platform-tests where this is commonplace? > > > > How about just `assert_equals(window.innerWidth, 800, 'window width')? > > > > (I suggested it so obviously I agree, but > > > https://chromium.googlesource.com/chromium/src/+/master/docs/testing/writing_... > > says "The assertion description conveys the way the actual value was obtained" > > and that results in sensible failure messages.) > > The problem is that when running an async test, the first failed assert will > cause test execution to halt immediately. So, we only get to see the first > failing result. This was the only way I could manage to patch over that > behavior. Yeah, this can be a bother, that an early failure obscures later ones. However, all except the first failure will still be quite suspect because some assumption might have been violated. And an async test may time out instead of saying where it failed. The best remedies I've found are to make tests small so that there aren't many interesting ways of failing, and to try reduce overlap, so that if one test would catch a trivial bug, not all other tests need to also fail on that same bug. If one knows for a fact that a certain secondary assertion will fail in some browser, one can also save it until after the important bits, so that the failure doesn't obscure the most important bit. szager1 2016/12/22 19:55:26 As we discussed offline: testharness allows you to Show quoted text On 2016/12/14 22:17:57, foolip (OOO til Jan. 2) wrote: > On 2016/12/14 19:54:11, szager1 wrote: > > On 2016/12/14 13:42:16, foolip wrote: > > > This is a bit unusual, to wrap every assertion in a separate test. Are there > > > parts of web-platform-tests where this is commonplace? > > > > > > How about just `assert_equals(window.innerWidth, 800, 'window width')? > > > > > > (I suggested it so obviously I agree, but > > > > > > https://chromium.googlesource.com/chromium/src/+/master/docs/testing/writing_... > > > says "The assertion description conveys the way the actual value was > obtained" > > > and that results in sensible failure messages.) > > > > The problem is that when running an async test, the first failed assert will > > cause test execution to halt immediately. So, we only get to see the first > > failing result. This was the only way I could manage to patch over that > > behavior. > > Yeah, this can be a bother, that an early failure obscures later ones. However, > all except the first failure will still be quite suspect because some assumption > might have been violated. And an async test may time out instead of saying where > it failed. As we discussed offline: testharness allows you to run a sequence of independent test() statements that don't short circuit the test run, for good reason. The problem here is that when t.done() is called on an async_test -- or if an assert fails while running the test -- and there are no other async_tests in flight, then the test run ends immediately. I have worked around this by interleaving calls to async_test() so that, where appropriate, the test run can continue after a failed assert. Show quoted text > > The best remedies I've found are to make tests small so that there aren't many > interesting ways of failing, and to try reduce overlap, so that if one test > would catch a trivial bug, not all other tests need to also fail on that same > bug. If one knows for a fact that a certain secondary assertion will fail in > some browser, one can also save it until after the important bits, so that the > failure doesn't obscure the most important bit.
16 }).observe(target);	19 test(function() { assert_equals(window.innerHeight, 600) }, "Window must be 60 0 pixels high.");

	20

	21 var target = this.contentDocument.getElementById("target");
	foolip 2016/12/14 13:42:16 Since this sin't associated with any test, if it h Since this sin't associated with any test, if it happens to throw an exception, it'll result in a harness error instead of failing a particular test. Everything that could conceivable fail needs to be inside t.step_func(). (With promise_test that can be avoided for some cases, but not everything is suitable to test with promises.) szager1 2016/12/14 19:54:11 OK, I'll add an assert. Show quoted text On 2016/12/14 13:42:16, foolip wrote: > Since this sin't associated with any test, if it happens to throw an exception, > it'll result in a harness error instead of failing a particular test. Everything > that could conceivable fail needs to be inside t.step_func(). (With promise_test > that can be avoided for some cases, but not everything is suitable to test with > promises.) OK, I'll add an assert.
	22 var entries = [];

	23 var observer = new IntersectionObserver((changes) => {

	24 entries = entries.concat(changes);

	25 });

	26 observer.observe(target);

	27 entries = entries.concat(observer.takeRecords());
	foolip 2016/12/14 13:42:16 Is it possible per spec that there are any records Is it possible per spec that there are any records already? If not assert that there are not or do nothing if some other test has that covered. szager1 2016/12/14 19:54:11 It should not happen that there are already notifi Show quoted text On 2016/12/14 13:42:16, foolip wrote: > Is it possible per spec that there are any records already? If not assert that > there are not or do nothing if some other test has that covered. It should not happen that there are already notifications, and the next line after this one is meant to verify that. Maybe I've misunderstood your comment. foolip 2016/12/14 22:17:56 If it weren't for the fact that the test still con Show quoted text On 2016/12/14 19:54:11, szager1 wrote: > On 2016/12/14 13:42:16, foolip wrote: > > Is it possible per spec that there are any records already? If not assert that > > there are not or do nothing if some other test has that covered. > > It should not happen that there are already notifications, and the next line > after this one is meant to verify that. Maybe I've misunderstood your comment. If it weren't for the fact that the test still continues, I'd say just assert and fail. When it does continue, one wonders if that assert-in-test could fail, and if the concat bit could make some later tests pass but for the wrong reasons. Let's keep that discussion to the other comment though. szager1 2016/12/22 19:55:26 This check is now a hard assert that will cause th Show quoted text On 2016/12/14 22:17:56, foolip (OOO til Jan. 2) wrote: > On 2016/12/14 19:54:11, szager1 wrote: > > On 2016/12/14 13:42:16, foolip wrote: > > > Is it possible per spec that there are any records already? If not assert > that > > > there are not or do nothing if some other test has that covered. > > > > It should not happen that there are already notifications, and the next line > > after this one is meant to verify that. Maybe I've misunderstood your > comment. > > If it weren't for the fact that the test still continues, I'd say just assert > and fail. When it does continue, one wonders if that assert-in-test could fail, > and if the concat bit could make some later tests pass but for the wrong > reasons. Let's keep that discussion to the other comment though. This check is now a hard assert that will cause the test run to halt if it fails.
	28 test(function() { assert_equals(entries.length, 0) }, "No initial notification s.");

17 waitForNotification(step0);	29 waitForNotification(step0);

	30

	31 function step0() {

	32 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 If only one entry is expected, maybe structure the test like this? new IntersectionObserver(t.step_func((entries, observer) => { assert_equals(entries.length, 1, 'number of mutation records') var targetRect = target.getBoundingClientRect(); var entryRect = entries[0].boundingClientRect; assert_rect_equals(entryRect, targetRect, 'entry bounding rect') // something async to fail if there are more entries coming: requestAnimationFrame(t.step_func_done()); }); Where assert_rect_equals is a new helper that you'll be able to use in quite a few places. szager1 2016/12/14 19:54:11 I did define a checkEntry routine in other tests f Show quoted text On 2016/12/14 13:42:16, foolip wrote: > If only one entry is expected, maybe structure the test like this? > > new IntersectionObserver(t.step_func((entries, observer) => { > assert_equals(entries.length, 1, 'number of mutation records') > var targetRect = target.getBoundingClientRect(); > var entryRect = entries[0].boundingClientRect; > assert_rect_equals(entryRect, targetRect, 'entry bounding rect') > // something async to fail if there are more entries coming: > requestAnimationFrame(t.step_func_done()); > }); > > Where assert_rect_equals is a new helper that you'll be able to use in quite a > few places. I did define a checkEntry routine in other tests for this purpose. I avoided sourcing external script files, because the w3c guidance is to make tests self-contained. Maybe I have been overzealous in this regard. foolip 2016/12/14 22:17:56 It's fine to have shared scripts with helpers if i Show quoted text On 2016/12/14 19:54:11, szager1 wrote: > On 2016/12/14 13:42:16, foolip wrote: > > If only one entry is expected, maybe structure the test like this? > > > > new IntersectionObserver(t.step_func((entries, observer) => { > > assert_equals(entries.length, 1, 'number of mutation records') > > var targetRect = target.getBoundingClientRect(); > > var entryRect = entries[0].boundingClientRect; > > assert_rect_equals(entryRect, targetRect, 'entry bounding rect') > > // something async to fail if there are more entries coming: > > requestAnimationFrame(t.step_func_done()); > > }); > > > > Where assert_rect_equals is a new helper that you'll be able to use in quite a > > few places. > > I did define a checkEntry routine in other tests for this purpose. I avoided > sourcing external script files, because the w3c guidance is to make tests > self-contained. Maybe I have been overzealous in this regard. It's fine to have shared scripts with helpers if it makes the tests simpler by sharing code, or at least no WPT reviewer has complained to me about it. szager1 2016/12/22 19:55:26 I moved common code into LayoutTests/intersection- Show quoted text On 2016/12/14 22:17:56, foolip (OOO til Jan. 2) wrote: > On 2016/12/14 19:54:11, szager1 wrote: > > On 2016/12/14 13:42:16, foolip wrote: > > > If only one entry is expected, maybe structure the test like this? > > > > > > new IntersectionObserver(t.step_func((entries, observer) => { > > > assert_equals(entries.length, 1, 'number of mutation records') > > > var targetRect = target.getBoundingClientRect(); > > > var entryRect = entries[0].boundingClientRect; > > > assert_rect_equals(entryRect, targetRect, 'entry bounding rect') > > > // something async to fail if there are more entries coming: > > > requestAnimationFrame(t.step_func_done()); > > > }); > > > > > > Where assert_rect_equals is a new helper that you'll be able to use in quite > a > > > few places. > > > > I did define a checkEntry routine in other tests for this purpose. I avoided > > sourcing external script files, because the w3c guidance is to make tests > > self-contained. Maybe I have been overzealous in this regard. > > It's fine to have shared scripts with helpers if it makes the tests simpler by > sharing code, or at least no WPT reviewer has complained to me about it. I moved common code into LayoutTests/intersection-observer/resources/intersection-observer-test-utils.js
	33 if (entries.length > 0) {

	34 test(function() { assert_equals(entries[0].boundingClientRect.top,

	35 target.getBoundingClientRect().top) },

	36 "entries[0].boundingClientRect.top == target.getBoundingClientRect(). top");

	37 test(function() { assert_equals(entries[0].boundingClientRect.left,

	38 target.getBoundingClientRect().left) },

	39 "entries[0].boundingClientRect.left == target.getBoundingClientRect() .left");

	40 test(function() { assert_equals(entries[0].boundingClientRect.width,

	41 target.getBoundingClientRect().width) },

	42 "entries[0].boundingClientRect.width == target.getBoundingClientRect( ).width");

	43 test(function() { assert_equals(entries[0].boundingClientRect.height,

	44 target.getBoundingClientRect().height) },

	45 "entries[0].boundingClientRect.height == target.getBoundingClientRect ().height");

	46 }

	47 t.done();

	48 }

18 };	49 };

19

20 function step0() {

21 shouldBeEqualToNumber("entries.length", 1);

22 if (entries.length > 0) {

23 shouldBe("entries[0].boundingClientRect.top", "target.getBoundingClientRect( ).top");

24 shouldBe("entries[0].boundingClientRect.left", "target.getBoundingClientRect ().left");

25 shouldBe("entries[0].boundingClientRect.width", "target.getBoundingClientRec t().width");

26 shouldBe("entries[0].boundingClientRect.height", "target.getBoundingClientRe ct().height");

27 }

28 finishJSTest();

29 }

30 </script>	50 </script>

OLD	NEW