IntersectionObserver: convert tests to testharness.js

IntersectionObserver: convert tests to testharness.js

No loss of coverage, but a few chromium-specific (hence non-upstreamable
to w3c) tests were split out into separate files.  The
cross-origin-iframe test was moved out of http/tests and now just uses
the 'sandbox' iframe attribute instead.

R=foolip@chromium.org,ojan@chromium.org
BUG=

Review-Url: https://codereview.chromium.org/2560253004
Cr-Commit-Position: refs/heads/master@{#445564}
Committed: https://chromium.googlesource.com/chromium/src/+/a2933a3bfccbc547107f31eb878c86d9e79ed61d

[szager1, 2016-12-10 01:28:58]

Description was changed from

==========
IntersectionObserver: convert tests to testharness.js

A few chromium-specific (hence non-upstreamable to w3c) tests were
split out into separate files.  The cross-origin-iframe test was
moved out of http/tests and now just uses the 'sandbox' iframe
attribute instead.

R=rbyers@chromium.org,ojan@chromium.org
BUG=
==========

to

==========
IntersectionObserver: convert tests to testharness.js

No loss of coverage, but a few chromium-specific (hence non-upstreamable
to w3c) tests were split out into separate files.  The
cross-origin-iframe test was moved out of http/tests and now just uses
the 'sandbox' iframe attribute instead.

R=rbyers@chromium.org,ojan@chromium.org
BUG=
==========

[szager1, 2016-12-10 01:29:18]

The CQ bit was checked by szager@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-10 01:30:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-10 02:33:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-10 02:33:33]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[szager1, 2016-12-13 22:18:39]

szager@chromium.org changed reviewers:
+ foolip@chromium.org

[szager1, 2016-12-13 22:19:08]

foolip@, can I interest you in a code review?  This is in preparation for
upstreaming the IntersectionObserver tests to w3c.

[foolip, 2016-12-14 13:42:17]

Took a close look at the first test and suggested what will amount to be pretty
significant rewrite, so not reviewing further until I know if we're on the same
page about that.

With some shared utility code, I suspect many of these tests could be made much
smaller.

https://codereview.chromium.org/2560253004/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/intersection-observer/client-rect.html
(right):

https://codereview.chromium.org/2560253004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/client-rect.html:10:
setTimeout(f);
Is the spec actually this lax about the timing?

https://codereview.chromium.org/2560253004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/client-rect.html:15:
iframe.onload = function() {
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")

https://codereview.chromium.org/2560253004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/client-rect.html:18:
test(function() { assert_equals(window.innerWidth, 800) }, "Window must be 800
pixels wide.");
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.)

https://codereview.chromium.org/2560253004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/client-rect.html:21: var
target = this.contentDocument.getElementById("target");
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.)

https://codereview.chromium.org/2560253004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/client-rect.html:27:
entries = entries.concat(observer.takeRecords());
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.

https://codereview.chromium.org/2560253004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/client-rect.html:32:
test(function() { assert_equals(entries.length, 1) }, "One notification.");
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]

https://codereview.chromium.org/2560253004/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/intersection-observer/client-rect.html
(right):

https://codereview.chromium.org/2560253004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/client-rect.html:10:
setTimeout(f);
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.

https://codereview.chromium.org/2560253004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/client-rect.html:18:
test(function() { assert_equals(window.innerWidth, 800) }, "Window must be 800
pixels wide.");
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.

https://codereview.chromium.org/2560253004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/client-rect.html:21: var
target = this.contentDocument.getElementById("target");
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.

https://codereview.chromium.org/2560253004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/client-rect.html:27:
entries = entries.concat(observer.takeRecords());
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.

https://codereview.chromium.org/2560253004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/client-rect.html:32:
test(function() { assert_equals(entries.length, 1) }, "One notification.");
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.

[szager1, 2016-12-14 19:57:02]

https://codereview.chromium.org/2560253004/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/intersection-observer/client-rect.html
(right):

https://codereview.chromium.org/2560253004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/client-rect.html:15:
iframe.onload = function() {
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]

https://codereview.chromium.org/2560253004/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/intersection-observer/client-rect.html
(right):

https://codereview.chromium.org/2560253004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/client-rect.html:10:
setTimeout(f);
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"?

https://codereview.chromium.org/2560253004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/client-rect.html:15:
iframe.onload = function() {
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.

https://codereview.chromium.org/2560253004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/client-rect.html:18:
test(function() { assert_equals(window.innerWidth, 800) }, "Window must be 800
pixels wide.");
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.

https://codereview.chromium.org/2560253004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/client-rect.html:27:
entries = entries.concat(observer.takeRecords());
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.

https://codereview.chromium.org/2560253004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/client-rect.html:32:
test(function() { assert_equals(entries.length, 1) }, "One notification.");
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.

[Rick Byers, 2016-12-16 21:43:25]

rbyers@chromium.org changed reviewers:
- rbyers@chromium.org

[szager1, 2016-12-22 19:55:26]

PTAL, I have restructured the tests extensively.

Common code is now in resources/intersection-observer-test-utils.js.

Most tests now interleave calls to async_test(), so that a non-fatal assert
failure will not prevent the rest of the test from running.  This required more
magic than I am entirely comfortable with (see waitForNotification and
runTestCycle), but it seems like the only way.

https://codereview.chromium.org/2560253004/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/intersection-observer/client-rect.html
(right):

https://codereview.chromium.org/2560253004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/client-rect.html:10:
setTimeout(f);
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

https://codereview.chromium.org/2560253004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/client-rect.html:15:
iframe.onload = function() {
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.

https://codereview.chromium.org/2560253004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/client-rect.html:18:
test(function() { assert_equals(window.innerWidth, 800) }, "Window must be 800
pixels wide.");
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.

> 
> 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.

https://codereview.chromium.org/2560253004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/client-rect.html:27:
entries = entries.concat(observer.takeRecords());
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.

https://codereview.chromium.org/2560253004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/client-rect.html:32:
test(function() { assert_equals(entries.length, 1) }, "One notification.");
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

[szager1, 2016-12-22 21:46:43]

The CQ bit was checked by szager@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-22 21:47:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-22 21:55:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-22 21:55:30]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[szager1, 2017-01-03 23:00:02]

New Year's ping.

[Rick Byers, 2017-01-04 14:45:14]

Description was changed from

==========
IntersectionObserver: convert tests to testharness.js

No loss of coverage, but a few chromium-specific (hence non-upstreamable
to w3c) tests were split out into separate files.  The
cross-origin-iframe test was moved out of http/tests and now just uses
the 'sandbox' iframe attribute instead.

R=rbyers@chromium.org,ojan@chromium.org
BUG=
==========

to

==========
IntersectionObserver: convert tests to testharness.js

No loss of coverage, but a few chromium-specific (hence non-upstreamable
to w3c) tests were split out into separate files.  The
cross-origin-iframe test was moved out of http/tests and now just uses
the 'sandbox' iframe attribute instead.

R=foolip@chromium.org,ojan@chromium.org
BUG=
==========

[szager1, 2017-01-11 03:50:24]

szager@chromium.org changed reviewers:
+ rbyers@chromium.org

[szager1, 2017-01-11 03:51:00]

Bueller?

Bueller?

Bueller?

[foolip, 2017-01-11 11:12:02]

On 2017/01/11 03:51:00, szager1 wrote:
> Bueller?
> 
> Bueller?
> 
> Bueller?

Happy new year, and as my nick suggests I am very sorry. Will review this today!

[foolip, 2017-01-11 11:42:33]

Comments on the framework before lunch, more coming.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/intersection-observer/cross-origin-iframe.html
(right):

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/cross-origin-iframe.html:36:
waitForNotification(function() { iframe.contentWindow.postMessage("", "*"); },
I suppose it can be assumed that the callbacks here won't throw exceptions, but
if there's any doubt then wrapping in t.func is a good idea.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/cross-origin-iframe.html:49:
window.addEventListener("message", handleMessage);
t.step_func(handleMessage). With that change, it should work to have
checkJsonEntries just assert things, which would fail the async_test we are in.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/cross-origin-iframe.html:51:
iframe.onload = function() {
Maybe t.step_func here. It is pretty ugly, but does save you from wondering
about whether it's needed or not.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/intersection-observer/resources/intersection-observer-test-utils.js
(right):

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/resources/intersection-observer-test-utils.js:1:
// Here's how waitForNotification works:
Great documentation! :)

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/resources/intersection-observer-test-utils.js:19:
//     - waitForNotification(myTestFunction2)
I guess that this part is optional, and that the order relative to the two
points below isn't important? If so might want to put it as an "optional: ..."
at the end?

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/resources/intersection-observer-test-utils.js:32:
async_test(function(t) {
Can you document that the timing of when runTestCycle is called matters? As long
as it's called before/during the window load event or from a step of another
test it should be fine, but if for any odd reason one were to to setTimeout from
one test to start another, then that second test may or may not run. Such a test
could use setup({ explicit_done: true }) though.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/resources/intersection-observer-test-utils.js:33:
waitForNotification(t.step_func(function() {
Simplification nit: waitForNotification(t.step_func_done(f));

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/resources/intersection-observer-test-utils.js:65:
if (expected.length > 0)
Just skimming the call sites I don't quite see when expected.length is anything
other than 4. Can it be?

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/resources/intersection-observer-test-utils.js:66:
assert_equals(actual.left, expected[0], description + ".left == " +
expected[0]);
testharness.js style nit: the assertion description could simply be `description
+ ".left"` result in a sensible failure message, there should be no reason to
include the expected value in the description, but simply to say what the actual
value is.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/resources/intersection-observer-test-utils.js:76:
assert_equals(entries.length, i+1, String(i+1) + " notification(s).");
Nit: Here the description "number of notifications" or "entries.length" would
result in a sensible failure message.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/resources/intersection-observer-test-utils.js:87:
function coordinatesToClientRectJson(top, right, bottom, left) {
This appears to be unused now.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/resources/intersection-observer-test-utils.js:119:
if (actual.rootBounds == "null")
I've argued against requiring === everywhere on blink-dev, but here it would
actually help my understanding. Can actual.rootBounds be the null literal, or
should only the string "null" work here?

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/resources/intersection-observer-test-utils.js:127:
test(function() {
Shouldn't this already be inside a test? Will comment on
cross-origin-iframe.html

[foolip, 2017-01-11 13:42:54]

lgtm % many nits and suggestions, take as many as you like :)

I think that structure of spawning more async_tests as you go works. Not a
common pattern, but the only other options are repeating the setup phase for
each test or failing on the first error. In practice most of these tests will
fail on the first error, because the async_tests that follow won't be reached.
Maybe there are a few with parallel async_tests where this isn't the case.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/intersection-observer/client-rect.html
(right):

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/client-rect.html:46:
checkEntry(entries, 0, [bcr.left, bcr.right, bcr.top, bcr.bottom]);
See question about checkLastEntry in framework, seeing the call site made me
wonder what the second argument really means.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/intersection-observer/deleted-root.html
(right):

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/deleted-root.html:16:
rootDiv.parentNode.removeChild(rootDiv);
You can use rootDiv.remove() if you like.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/deleted-root.html:25:
assert_throws("INVALID_STATE_ERR", function() {
https://wicg.github.io/IntersectionObserver/#dom-intersectionobserver-observe
doesn't seem to ever throw, can file a spec issue to define this?

If the observable behavior should depend on GC, then be sure to highlight that
for other implementers, as that tends to be controversial and avoided to the
fullest degree possible.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/deleted-root.html:28: },
"IntersectionObserver.observe() with a deleted root did not throw.");
A bit of personal preference here, but the "did not throw" part isn't needed. I
try to follow Simon Pieters' principle of not having to change the test
description if the pass condition is inverted.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/intersection-observer/edge-inclusive-intersection.html
(right):

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/edge-inclusive-intersection.html:48:
assert_equals(entries.length, 0, "entries.length == 0");
" == 0" is superfluous

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/intersection-observer/multiple-targets.html
(right):

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/multiple-targets.html:37:
assert_true(!!target1, "target1 exists.");
There are a lot of these. Would a single test that observer.observe(null) throws
TypeError not suffice to catch typos in the IDs used to find these elements?

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/intersection-observer/observer-attributes.html
(right):

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/observer-attributes.html:10:
test(function() { assert_true(observer.root === null) },
Suggest assert_equals(observer.root, null, "observer.root"), excluding expected
value from the description.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/observer-attributes.html:29:
}, "Test for observer attribute getters.");
"Test for " not needed.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/intersection-observer/observer-exceptions.html
(right):

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/observer-exceptions.html:10:
}, "IntersectionObserver constructor throws RangeError due to threshold=1.1.");
Optional nit: "IntersectionObserver constructor with { threshold: [1.1] }" and
similar would omit the pass condition, but it's pretty harmless here.

If there were even more cases, simplifying to an array of init dicts and
arguments for assert_throws would be attractive.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/intersection-observer/observer-without-js-reference.html
(right):

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/observer-without-js-reference.html:41:
}, "IntersectionObserver that is unreachable in js should still generate
notifications.");
Should something trigger GC for this to fail more reliably if the bug is there?
Wrapping the constructor in a function shouldn't matter here since the instance
isn't assigned to any variable.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/intersection-observer/resources/intersection-observer-test-utils.js
(right):

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/resources/intersection-observer-test-utils.js:76:
assert_equals(entries.length, i+1, String(i+1) + " notification(s).");
On 2017/01/11 11:42:33, foolip_slow_very_sorry wrote:
> Nit: Here the description "number of notifications" or "entries.length" would
> result in a sensible failure message.

Actually, why can checkEntry only be used to check the last entry? Should it be
called checkLastEntry?

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/intersection-observer/resources/observer-in-iframe-subframe.html
(right):

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/resources/observer-in-iframe-subframe.html:53:
runTestCycle(step1, "First rAF.");
I think some other tests say this too, but it's not quite right is it? It's
rAF+timeout+timeout, is there a handy short term for that time?

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/intersection-observer/shadow-content.html
(right):

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/shadow-content.html:27:
assert_true(!!shadowRoot, "Shadow root exists");
It would help readability with fewer of these checks, and here's a redundant
one, because the next line would throw if this fails.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/intersection-observer/timestamp.html
(right):

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/timestamp.html:34:
setTimeout(function() {
Wrap in t.step_func here and below.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/timestamp.html:82: "Time
ranges for top and iframe windows are disjoint.");
I see red » symbols, looks like some tabs here and above?

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/resources/intersection-observer-helper-functions.js
(left):

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/resources/intersection-observer-helper-functions.js:33:
requestAnimationFrame(() => {
FWIW, it would have been fine to keep the arrow functions and most other newish
JavaScript features. As long as the tests run in the most recent version of all
browsers without gratuitous failures, it's OK.

[szager1, 2017-01-23 23:18:05]

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/intersection-observer/cross-origin-iframe.html
(right):

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/cross-origin-iframe.html:36:
waitForNotification(function() { iframe.contentWindow.postMessage("", "*"); },
On 2017/01/11 11:42:33, foolip_slow_very_sorry wrote:
> I suppose it can be assumed that the callbacks here won't throw exceptions,
but
> if there's any doubt then wrapping in t.func is a good idea.

Adding t.step_func to the message handler below will cover this.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/cross-origin-iframe.html:49:
window.addEventListener("message", handleMessage);
On 2017/01/11 11:42:33, foolip_slow_very_sorry wrote:
> t.step_func(handleMessage). With that change, it should work to have
> checkJsonEntries just assert things, which would fail the async_test we are
in.

I'll add t.step_func, but this is a case where failing an assert in
checkJsonEntries() should not cause the async_test to abort (which is why
checkJsonEntries wraps everything in a test()).  So I didn't remove the test()
wrapper in checkJsonEntries.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/cross-origin-iframe.html:51:
iframe.onload = function() {
On 2017/01/11 11:42:33, foolip_slow_very_sorry wrote:
> Maybe t.step_func here. It is pretty ugly, but does save you from wondering
> about whether it's needed or not.

Done.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/intersection-observer/deleted-root.html
(right):

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/deleted-root.html:16:
rootDiv.parentNode.removeChild(rootDiv);
On 2017/01/11 13:42:53, foolip_slow_very_sorry wrote:
> You can use rootDiv.remove() if you like.

Done.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/deleted-root.html:25:
assert_throws("INVALID_STATE_ERR", function() {
On 2017/01/11 13:42:53, foolip_slow_very_sorry wrote:
> https://wicg.github.io/IntersectionObserver/#dom-intersectionobserver-observe
> doesn't seem to ever throw, can file a spec issue to define this?
> 
> If the observable behavior should depend on GC, then be sure to highlight that
> for other implementers, as that tends to be controversial and avoided to the
> fullest degree possible.

Yeah, there was a separate bug filed about the fact that this reveals gc timing.
 I will land another patch first to fix that bug, and I updated this test to
expect the new behavior.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/deleted-root.html:28: },
"IntersectionObserver.observe() with a deleted root did not throw.");
On 2017/01/11 13:42:53, foolip_slow_very_sorry wrote:
> A bit of personal preference here, but the "did not throw" part isn't needed.
I
> try to follow Simon Pieters' principle of not having to change the test
> description if the pass condition is inverted.

Done.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/intersection-observer/edge-inclusive-intersection.html
(right):

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/edge-inclusive-intersection.html:48:
assert_equals(entries.length, 0, "entries.length == 0");
On 2017/01/11 13:42:53, foolip_slow_very_sorry wrote:
> " == 0" is superfluous

Done.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/intersection-observer/multiple-targets.html
(right):

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/multiple-targets.html:37:
assert_true(!!target1, "target1 exists.");
On 2017/01/11 13:42:53, foolip_slow_very_sorry wrote:
> There are a lot of these. Would a single test that observer.observe(null)
throws
> TypeError not suffice to catch typos in the IDs used to find these elements?

I didn't have these asserts originally, but in a previous comment, you suggested
that I assert() anything that could possibly fail.  I'm not sure what you want.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/intersection-observer/observer-attributes.html
(right):

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/observer-attributes.html:10:
test(function() { assert_true(observer.root === null) },
On 2017/01/11 13:42:53, foolip_slow_very_sorry wrote:
> Suggest assert_equals(observer.root, null, "observer.root"), excluding
expected
> value from the description.

Done.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/observer-attributes.html:29:
}, "Test for observer attribute getters.");
On 2017/01/11 13:42:53, foolip_slow_very_sorry wrote:
> "Test for " not needed.

Done.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/intersection-observer/observer-exceptions.html
(right):

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/observer-exceptions.html:10:
}, "IntersectionObserver constructor throws RangeError due to threshold=1.1.");
On 2017/01/11 13:42:53, foolip_slow_very_sorry wrote:
> Optional nit: "IntersectionObserver constructor with { threshold: [1.1] }" and
> similar would omit the pass condition, but it's pretty harmless here.
> 
> If there were even more cases, simplifying to an array of init dicts and
> arguments for assert_throws would be attractive.

Fixed the descriptions.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/intersection-observer/observer-without-js-reference.html
(right):

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/observer-without-js-reference.html:41:
}, "IntersectionObserver that is unreachable in js should still generate
notifications.");
On 2017/01/11 13:42:53, foolip_slow_very_sorry wrote:
> Should something trigger GC for this to fail more reliably if the bug is
there?
> Wrapping the constructor in a function shouldn't matter here since the
instance
> isn't assigned to any variable.

This test is intended to be part of the upstream conformance suite, so it can't
rely on gc.  Wrapping the constructor shouldn't have an effect, but I think it
makes it more obvious what this code is meant to test.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/intersection-observer/resources/intersection-observer-test-utils.js
(right):

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/resources/intersection-observer-test-utils.js:19:
//     - waitForNotification(myTestFunction2)
On 2017/01/11 11:42:33, foolip_slow_very_sorry wrote:
> I guess that this part is optional, and that the order relative to the two
> points below isn't important? If so might want to put it as an "optional: ..."
> at the end?

Done.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/resources/intersection-observer-test-utils.js:32:
async_test(function(t) {
On 2017/01/11 11:42:33, foolip_slow_very_sorry wrote:
> Can you document that the timing of when runTestCycle is called matters? As
long
> as it's called before/during the window load event or from a step of another
> test it should be fine, but if for any odd reason one were to to setTimeout
from
> one test to start another, then that second test may or may not run. Such a
test
> could use setup({ explicit_done: true }) though.

Done.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/resources/intersection-observer-test-utils.js:33:
waitForNotification(t.step_func(function() {
On 2017/01/11 11:42:33, foolip_slow_very_sorry wrote:
> Simplification nit: waitForNotification(t.step_func_done(f));

Done.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/resources/intersection-observer-test-utils.js:65:
if (expected.length > 0)
On 2017/01/11 11:42:33, foolip_slow_very_sorry wrote:
> Just skimming the call sites I don't quite see when expected.length is
anything
> other than 4. Can it be?

Yes, there are cases where it's called with expected.length==0.  I simplified
the length checking, since length will only ever be 0 or 4.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/resources/intersection-observer-test-utils.js:66:
assert_equals(actual.left, expected[0], description + ".left == " +
expected[0]);
On 2017/01/11 11:42:33, foolip_slow_very_sorry wrote:
> testharness.js style nit: the assertion description could simply be
`description
> + ".left"` result in a sensible failure message, there should be no reason to
> include the expected value in the description, but simply to say what the
actual
> value is.

Done.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/resources/intersection-observer-test-utils.js:76:
assert_equals(entries.length, i+1, String(i+1) + " notification(s).");
On 2017/01/11 13:42:53, foolip_slow_very_sorry wrote:
> On 2017/01/11 11:42:33, foolip_slow_very_sorry wrote:
> > Nit: Here the description "number of notifications" or "entries.length"
would
> > result in a sensible failure message.
> 
> Actually, why can checkEntry only be used to check the last entry? Should it
be
> called checkLastEntry?

That's a good idea, changed to checkLastEntry.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/resources/intersection-observer-test-utils.js:87:
function coordinatesToClientRectJson(top, right, bottom, left) {
On 2017/01/11 11:42:33, foolip_slow_very_sorry wrote:
> This appears to be unused now.

It's used in resources/cross-origin-subframe.html

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/resources/intersection-observer-test-utils.js:119:
if (actual.rootBounds == "null")
On 2017/01/11 11:42:33, foolip_slow_very_sorry wrote:
> I've argued against requiring === everywhere on blink-dev, but here it would
> actually help my understanding. Can actual.rootBounds be the null literal, or
> should only the string "null" work here?

This value is encoded by clientRectToJson, which uses a literal "null" string.

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/resources/intersection-observer-test-utils.js:127:
test(function() {
On 2017/01/11 11:42:33, foolip_slow_very_sorry wrote:
> Shouldn't this already be inside a test? Will comment on
> cross-origin-iframe.html

I addressed this in the test, but: this is a case where a failure should not
halt execution of the entire test run.

[szager1, 2017-01-23 23:18:15]

The CQ bit was checked by szager@chromium.org

[szager1, 2017-01-23 23:18:16]

The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2560253004/#ps80001
(title: "Address comments")

[commit-bot: I haz the power, 2017-01-23 23:19:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-24 00:35:31]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485213495371060,
"parent_rev": "b28505b27acc4a6b9b242be8b4bc93d69aa58dbe", "commit_rev":
"a2933a3bfccbc547107f31eb878c86d9e79ed61d"}

[commit-bot: I haz the power, 2017-01-24 00:36:06]

Description was changed from

==========
IntersectionObserver: convert tests to testharness.js

No loss of coverage, but a few chromium-specific (hence non-upstreamable
to w3c) tests were split out into separate files.  The
cross-origin-iframe test was moved out of http/tests and now just uses
the 'sandbox' iframe attribute instead.

R=foolip@chromium.org,ojan@chromium.org
BUG=
==========

to

==========
IntersectionObserver: convert tests to testharness.js

No loss of coverage, but a few chromium-specific (hence non-upstreamable
to w3c) tests were split out into separate files.  The
cross-origin-iframe test was moved out of http/tests and now just uses
the 'sandbox' iframe attribute instead.

R=foolip@chromium.org,ojan@chromium.org
BUG=

Review-Url: https://codereview.chromium.org/2560253004
Cr-Commit-Position: refs/heads/master@{#445564}
Committed:
https://chromium.googlesource.com/chromium/src/+/a2933a3bfccbc547107f31eb878c...
==========

[commit-bot: I haz the power, 2017-01-24 00:36:07]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/a2933a3bfccbc547107f31eb878c...

[foolip, 2017-01-24 10:50:36]

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/intersection-observer/multiple-targets.html
(right):

https://codereview.chromium.org/2560253004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/multiple-targets.html:37:
assert_true(!!target1, "target1 exists.");
On 2017/01/23 23:18:03, szager1 wrote:
> On 2017/01/11 13:42:53, foolip_slow_very_sorry wrote:
> > There are a lot of these. Would a single test that observer.observe(null)
> throws
> > TypeError not suffice to catch typos in the IDs used to find these elements?
> 
> I didn't have these asserts originally, but in a previous comment, you
suggested
> that I assert() anything that could possibly fail.  I'm not sure what you
want.

I'm not sure which comment that was, but in any case I apologize for failing to
communicate.

I might not have this well thought out, it seems I have two ideas that are in
tension. First, it can be good to have some preconditions that prove that
everything is in the state that the rest of the test assumes. The canonical case
would be when checking that an exceptions isn't thrown or an event isn't fired
in some special scenario, as small typos could cause the test to pass for the
wrong reason. Second, it's nice with as little boilerplate as possible, so that
the interesting bits stand out more, so I remove things that would obviously
cause the test to fail anyway.

Here observer.observe(target1) will throw if target1 is not an Element, but
that's really not obvious from the call site. So, OK either way. If there isn't
already a test for observer.observe(null) throwing, that might be a nice
addition though, bugs of that nature are pretty common.