Closed Bug 1014838 Opened 6 years ago Closed 6 years ago

Added checks for state correctness to the apzc gtests

Categories

(Core :: Panning and Zooming, defect, minor)

ARM
Gonk (Firefox OS)
defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: nl, Assigned: nl)

References

Details

Attachments

(1 file, 3 obsolete files)

After i've introduced a regression to the apzc logic (bug 1013378) where we had apzc stayed in the touching state after gesture was completed, i've thought about adding tests for apzc state (not only checks for returned statuses and performed pans/zooms as we have now).

For now i think it should smth primitive, e.g. we can check that apzc state is none in the end of each test. Not sure whether it will work for all tests but for most of tests it will i believe.

Please let me know your considerations about it.
Depends on: 1013378
Flags: needinfo?(drs+bugzilla)
Flags: needinfo?(bugmail.mozilla)
Flags: needinfo?(botond)
I think this is a good idea!
Flags: needinfo?(botond)
I'm glad you filed this, because I was going to suggest that we add a test to make sure that the regression in bug 1013378 doesn't happen again.

I'm kind of lukewarm on the idea of testing states since it's an implementation detail. That is, it doesn't matter what the state actually is, as long as everything behaves the way we expect it to. But it does tell us so much.

I'd be in favor of testing states in places where it's harder to guarantee correctness via inputs/outputs. But I don't think we should be sprinkling state checks everywhere in gtest, or using it as a replacement for any other testing.

In that case, the fix for the regression in bug 1013378 would be something we could test this way (among other less robust ways I can think of).
Flags: needinfo?(drs+bugzilla)
(In reply to Doug Sherk (:drs) from comment #2)
> I'm kind of lukewarm on the idea of testing states since it's an
> implementation detail. That is, it doesn't matter what the state actually
> is, as long as everything behaves the way we expect it to. But it does tell
> us so much.
> 
> I'd be in favor of testing states in places where it's harder to guarantee
> correctness via inputs/outputs. But I don't think we should be sprinkling
> state checks everywhere in gtest, or using it as a replacement for any other
> testing.

I think the potential cost of having to update the gtests when an implementation change results in the state in a particular situation being different, is relatively low compared to the potential benefit of catching things like bug 1013378 earlier.

I'm with you on the state checks _adding to_, not _replacing_, other testing.
Assignee: nobody → nicklebedev37
Status: NEW → ASSIGNED
I agree with what Doug said, that I would like to avoid having state checks everywhere. But we can use them in cases where there's no other way to test a particular outcome.
Flags: needinfo?(bugmail.mozilla)
Added a first version of apzc state tests.

These test changes ensure that apzc stays in the NOTHING state after short/medium/long taps and therefore subsequent gestures aren't affected.

I didn't add similar tests for panning/zooming since in the end of such tests apzc may stay in the fling or animated zoom state and it's not obvious hot to handle it properly.

try results: https://tbpl.mozilla.org/?tree=Try&rev=c1fec63ed777.
Attachment #8428377 - Flags: review?(bugmail.mozilla)
Attachment #8428377 - Flags: review?(botond)
Comment on attachment 8428377 [details] [diff] [review]
Add testing of the apzc state correctness after short/medium/long taps.

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

I am OK with this with the suggested comment added.

::: gfx/layers/apz/src/AsyncPanZoomController.h
@@ +69,4 @@
>    typedef uint32_t TouchBehaviorFlags;
>  
>  public:
> +  enum PanZoomState {

I would have preferred keeping PanZoomState non-public in AsyncPanZoomController and exposing it to test code only, but I can't think of an easy way to do that, so this is OK. Let's add a comment though saying that only test code should be accessing this enum, not other clients of APZC.
Attachment #8428377 - Flags: review?(botond) → review+
Comment on attachment 8428377 [details] [diff] [review]
Add testing of the apzc state correctness after short/medium/long taps.

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

Instead of making stuff public, can we make PanZoomState protected and have a public method on TestAsyncPanZoomController that does the assertion internally? Something like:

void AssertStateIsReset() {
  ReentrantMonitorAutoEnter lock(mMonitor);
  ASSERT_EQ(NOTHING, mState);
}

That way the unit test wouldn't be able to query the exact state, but they can check that the state at the end of the test is back to NOTHING.
Comment on attachment 8428377 [details] [diff] [review]
Add testing of the apzc state correctness after short/medium/long taps.

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

Minusing to clear my queue for now, but I could be convinced to turn it into an r+ if you have strong objections against what I suggested.
Attachment #8428377 - Flags: review?(bugmail.mozilla) → review-
Kats, i think you've suggested a good idea.
I think we should make it your way because not sure that in foreseeable future we would have tests with any advanced state checks.

Try results: https://tbpl.mozilla.org/?tree=Try&rev=d3e6e4d98ced.
Attachment #8428377 - Attachment is obsolete: true
Attachment #8431459 - Flags: review?(bugmail.mozilla)
Comment on attachment 8431459 [details] [diff] [review]
Add testing of the apzc state correctness after tap gestures.

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

LGTM. Bugzilla says this is a "WINDOWS PATCH" which presumably means it has Windows line-endings instead of Unix line-endings. Please make sure that whatever lands doesn't introduce windows line endings into these files. Thanks.

::: gfx/layers/apz/src/AsyncPanZoomController.h
@@ +340,5 @@
> +                                 prevented the default actions yet and the allowed touch behavior
> +                                 was not set yet. we still need to abort animations. */
> +    SNAP_BACK,                /* snap-back animation to relieve overscroll */
> +  };
> +  

nit: trailing whitespace
Attachment #8431459 - Flags: review?(bugmail.mozilla) → review+
Carrying over the r+.
Attachment #8431459 - Attachment is obsolete: true
Attachment #8431517 - Flags: review+
Sorry for spam, forgot to change line-ending chars.
Attachment #8431517 - Attachment is obsolete: true
Attachment #8431521 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9905efdd8d21
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.