1169695 - Make the handling of delayed tasks in the gtests more representative of production use

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Right now in the APZ gtests we have a lot of code that manually manages the "delayed task queue", by calling things like DestroyOldestTask(), RunNextDelayedTask(), etc.. This is bad for a number of reasons: (1) it makes tests complicated to write, (2) it makes the code less modular because changing which tasks are dispatched when will break a slew of tests, and (3) the delayed tasks are run in the order they are created rather than respecting the delays they are dispatched with.

I have a set of patches to clean this up.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Make MockContentControllerDelayed the owner of the test timestamp

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Advance the time when sampling transforms

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 3 - Store the run-at times of delayed tasks

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 4 - Sort tasks by run-at time and make sure they run when advancing time

Comment 5

[Kartikaya Gupta (email:[e-mail])]

I still need to audit most of the tests to make sure they don't do any more manual time handling (which they shouldn't since I removed all those functions) but also to see if I can remove any unnecessary manual-advancing of time.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 4 - Sort tasks by run-at time and make sure they run when advancing time

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 5 - Remove as much manual spinning of the task queue as possible

Comment 8

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8152332cada

Comment 9

[Botond Ballo [:botond]]

Comment on attachment 8612968 [details] [diff] [review]
Part 1 - Make MockContentControllerDelayed the owner of the test timestamp

Review of attachment 8612968 [details] [diff] [review]:
-----------------------------------------------------------------

This is a nice follow-up to bug 1163845, thanks!

The passing around of MockContentControllerDelayed* is a bit "meh", and I suggested a way around it, but we can of course leave that for another time.

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +82,5 @@
> +  const TimeStamp& Time() {
> +    return mTime;
> +  }
> +
> +  void AdvanceBy(int aMillis) {

I would prefer either leaving this overload out, or including "Ms" (or "Millis" or "Milliseconds") in the name.

@@ +155,4 @@
>                               GestureBehavior aBehavior = DEFAULT_GESTURES)
>      : AsyncPanZoomController(aLayersId, aTreeManager, aTreeManager->GetInputQueue(), aMcc, aBehavior)
>      , mWaitForMainThread(false)
> +    , mcc(static_cast<MockContentControllerDelayed*>(aMcc))

Change the type of aMcc instead.

@@ +242,4 @@
>  };
>  
>  AsyncPanZoomController*
>  TestAPZCTreeManager::MakeAPZCInstance(uint64_t aLayersId, GeckoContentController* aController)

(That will entail changing aController's type here, too.)

@@ +322,5 @@
>      const TimeDuration increment = TimeDuration::FromMilliseconds(1);
>      ParentLayerPoint pointOut;
>      ViewTransform viewTransformOut;
> +    mcc->AdvanceBy(increment);
> +    apzc->SampleContentTransformForFrame(mcc->Time(), &viewTransformOut, pointOut);

Since this version of SampleContentTransformForFrame is defined in TestAsyncPanZoomController, which knows its mcc, can we omit the first parameter and automatically use mcc->Time()?

@@ +458,5 @@
>    return aTarget->ReceiveInputEvent(mti, nullptr, nullptr);
>  }
>  
>  template<class InputReceiver> static void
> +Tap(const nsRefPtr<InputReceiver>& aTarget, int aX, int aY, MockContentControllerDelayed* aMcc,

At some point, it might make sense to give APZCBasicTester and APZCTreeManagerTester a common base class, and make some of the static functions in this file, like Tap() and Pan(), methods of it. (The mcc would then be a member of this base class, and thus wouldn't have to be passed around.)

Comment 10

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #9)
> @@ +322,5 @@
> >      const TimeDuration increment = TimeDuration::FromMilliseconds(1);
> >      ParentLayerPoint pointOut;
> >      ViewTransform viewTransformOut;
> > +    mcc->AdvanceBy(increment);
> > +    apzc->SampleContentTransformForFrame(mcc->Time(), &viewTransformOut, pointOut);
> 
> Since this version of SampleContentTransformForFrame is defined in
> TestAsyncPanZoomController, which knows its mcc, can we omit the first
> parameter and automatically use mcc->Time()?

Heh, I see you did just that in Part 2 :)

Comment 11

[Botond Ballo [:botond]]

Comment on attachment 8613069 [details] [diff] [review]
Part 4 - Sort tasks by run-at time and make sure they run when advancing time

Review of attachment 8613069 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +1501,5 @@
>  
> +  check.Call("pre-tap");
> +  TapAndCheckStatus(apzc, 10, 10, mcc, TimeDuration::FromMilliseconds(100));
> +  check.Call("post-tap");
> +  while (mcc->RunThroughDelayedTasks());

Is this still testing what we want it to? My reading of the original test is that it wanted to make sure the HandleSingleTap task is posted *after* the other tasks.

@@ +1522,5 @@
>  
> +  check.Call("pre-tap");
> +  TapAndCheckStatus(apzc, 10, 10, mcc, TimeDuration::FromMilliseconds(400));
> +  check.Call("post-tap");
> +  while (mcc->RunThroughDelayedTasks());

Same here.

@@ -1552,5 @@
>        EXPECT_CALL(check, Call("postHandleSingleTap"));
>      }
>  
> -    // There is a longpress event scheduled on a timeout
> -    mcc->CheckHasDelayedTask();

Aren't we weakening the test by removing these checks?

Comment 12

[Botond Ballo [:botond]]

Comment on attachment 8613070 [details] [diff] [review]
Part 5 - Remove as much manual spinning of the task queue as possible

Review of attachment 8613070 [details] [diff] [review]:
-----------------------------------------------------------------

In the places where we remove "while (mcc->RunThroughDelayedTasks());" lines that precede "apzc->AssertStateIsReset();" lines, is it the case that the tasks were already run prior to that due to time advancing, or do they run in TearDown() and we don't rely on them to clear state?

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ -901,5 @@
>    apzc->ContentReceivedInputBlock(blockId, true);
>  
> -  // Run all pending tasks (this should include at least the
> -  // prevent-default timer).
> -  EXPECT_LE(1, mcc->RunThroughDelayedTasks());

Doesn't the removal of the EXPECT_LE(1) weaken the test?

@@ +1309,3 @@
>    // Sample the overscroll animation once, to get it to initialize
> +  // the first overscroll sample. Note that this advances the time and runs
> +  // the HandleFlingOverscroll task scheduled in the previous call.

Clearer: "This advances the time and runs the HandleFlingOverscroll task scheduled in the previous call, which starts an overscroll animation. It then samples the overscroll animation once, to get it to initialize the first overscroll sample."

@@ +1398,5 @@
>      // Deliver a tap to abort the fling. Ensure that we get a HandleSingleTap
>      // call out of it if and only if the fling is slow.
>      EXPECT_CALL(*mcc, HandleSingleTap(_, 0, apzc->GetGuid())).Times(tapCallsExpected);
>      Tap(apzc, 10, 10, mcc, 0);
> +    mcc->AdvanceBy(500);

Why is this better? If the intention is to allow enough time to elapse for scheduled tasks to run, isn't RunThroughDelayedTasks() clearer and safer than a hardcoded duration?

Comment 13

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #9)
> > +  void AdvanceBy(int aMillis) {
> 
> I would prefer either leaving this overload out, or including "Ms" (or
> "Millis" or "Milliseconds") in the name.

Renamed to AdvanceByMillis

> > +    , mcc(static_cast<MockContentControllerDelayed*>(aMcc))
> 
> Change the type of aMcc instead.

Done

> >  AsyncPanZoomController*
> >  TestAPZCTreeManager::MakeAPZCInstance(uint64_t aLayersId, GeckoContentController* aController)
> 
> (That will entail changing aController's type here, too.)

This is an override, so I can't change the type. I moved the cast from the TestAsyncPanZoomController constructor into this function though.

> > +    mcc->AdvanceBy(increment);
> > +    apzc->SampleContentTransformForFrame(mcc->Time(), &viewTransformOut, pointOut);
> 
> Since this version of SampleContentTransformForFrame is defined in
> TestAsyncPanZoomController, which knows its mcc, can we omit the first
> parameter and automatically use mcc->Time()?

Done in part 2, as you noticed.

> >  template<class InputReceiver> static void
> > +Tap(const nsRefPtr<InputReceiver>& aTarget, int aX, int aY, MockContentControllerDelayed* aMcc,
> 
> At some point, it might make sense to give APZCBasicTester and
> APZCTreeManagerTester a common base class, and make some of the static
> functions in this file, like Tap() and Pan(), methods of it. (The mcc would
> then be a member of this base class, and thus wouldn't have to be passed
> around.)

That's a good idea, I can spin off another bug for that.

(In reply to Botond Ballo [:botond] from comment #11)
> > +  check.Call("post-tap");
> > +  while (mcc->RunThroughDelayedTasks());
> 
> Is this still testing what we want it to? My reading of the original test is
> that it wanted to make sure the HandleSingleTap task is posted *after* the
> other tasks.

It's not that we want to make sure HandleSingleTap is posted after the other tasks, is that we want the single-tap task to run after the ReceiveInputEvent calls are done (in other words, the content gets touchdown, touchup, click rather than touchdown, click, touchup). The timing of the other tasks is not really relevant to the goal of the test. (It occurs to me that the comment in the test is wrong, it should say touchup instead of touchdown. I'll fix that).

> > +  while (mcc->RunThroughDelayedTasks());
> 
> Same here.

Ditto.

> @@ -1552,5 @@
> >        EXPECT_CALL(check, Call("postHandleSingleTap"));
> >      }
> >  
> > -    // There is a longpress event scheduled on a timeout
> > -    mcc->CheckHasDelayedTask();
> 
> Aren't we weakening the test by removing these checks?

I think of it more as removing stuff that's not relevant to the test. Forcing the test to know exactly which events are fired and when (without being able to actually assert to that effect) is brittle and makes it harder to change the code. (This is basically point (2) in comment 0).

(In reply to Botond Ballo [:botond] from comment #12)
> In the places where we remove "while (mcc->RunThroughDelayedTasks());" lines
> that precede "apzc->AssertStateIsReset();" lines, is it the case that the
> tasks were already run prior to that due to time advancing, or do they run
> in TearDown() and we don't rely on them to clear state?

A bit of both, I think.

> > -  EXPECT_LE(1, mcc->RunThroughDelayedTasks());
> 
> Doesn't the removal of the EXPECT_LE(1) weaken the test?

As before, I don't think this check is really necessary because it's an implementation detail. As long as the pinch doesn't actually happen the test should pass.

> @@ +1309,3 @@
> >    // Sample the overscroll animation once, to get it to initialize
> > +  // the first overscroll sample. Note that this advances the time and runs
> > +  // the HandleFlingOverscroll task scheduled in the previous call.
> 
> Clearer: "This advances the time and runs the HandleFlingOverscroll task
> scheduled in the previous call, which starts an overscroll animation. It
> then samples the overscroll animation once, to get it to initialize the
> first overscroll sample."

Updated.

> >      Tap(apzc, 10, 10, mcc, 0);
> > +    mcc->AdvanceBy(500);
> 
> Why is this better? If the intention is to allow enough time to elapse for
> scheduled tasks to run, isn't RunThroughDelayedTasks() clearer and safer
> than a hardcoded duration?

Yeah, good point. I changed this back to a RunThroughDelayedTasks. Same for the AdvanceBy(1000) a few lines below.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Make MockContentControllerDelayed the owner of the test timestamp (v2)

Rebased, carrying r+

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Advance the time when sampling transforms (v2)

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 3 - Store the run-at times of delayed tasks (v2)

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 4 - Sort tasks by run-at time and make sure they run when advancing time (v2)

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 5 - Remove as much manual spinning of the task queue as possible (v2)

Comment 19

[Botond Ballo [:botond]]

Comment on attachment 8613534 [details] [diff] [review]
Part 4 - Sort tasks by run-at time and make sure they run when advancing time (v2)

Review of attachment 8613534 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +142,1 @@
>    nsTArray<std::pair<Task*, TimeStamp>> mTaskQueue;

Please add a comment saying that this array is sorted by timestamp.

Comment 20

[Botond Ballo [:botond]]

Comment on attachment 8613535 [details] [diff] [review]
Part 5 - Remove as much manual spinning of the task queue as possible (v2)

Review of attachment 8613535 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +1310,5 @@
>    // Sample the overscroll animation once, to get it to initialize
> +  // the first overscroll sample. This advances the time and runs the
> +  // HandleFlingOverscroll task scheduled in the previous call, which starts
> +  // an overscroll animation. It then samples the overscroll animation once,
> +  // to get it to initialize the first overscroll sample.

I had in mind omitting the sentence before "This advances the time...".

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5cda3c37343c
https://hg.mozilla.org/integration/mozilla-inbound/rev/eec921ec4516
https://hg.mozilla.org/integration/mozilla-inbound/rev/bae2b32f253a
https://hg.mozilla.org/integration/mozilla-inbound/rev/c862681c40a3
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc758e8f18e3

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5cda3c37343c
https://hg.mozilla.org/mozilla-central/rev/eec921ec4516
https://hg.mozilla.org/mozilla-central/rev/bae2b32f253a
https://hg.mozilla.org/mozilla-central/rev/c862681c40a3
https://hg.mozilla.org/mozilla-central/rev/cc758e8f18e3