Fix gmock leak issue in AutoplayUmaHelperTest

Fix gmock leak issue in AutoplayUmaHelperTest

AutoplayUmaHelperTest currently uses GMock for mock
AutoplayUmaHelper in order to check if events are correctly
received. However this will make the test flaky since
MockAutoplyUmaHelper is garbage-collected but GMock expect it to
be destroyed when the test finishes.

This CL marks MockAutoplayUmaHelper as AllowLeak which fixes the
issue.

BUG=672831
Committed: https://crrev.com/8906ce8b964bf3431b891bc9eaaca54ad8981f7d
Cr-Commit-Position: refs/heads/master@{#437939}

[Zhiqiang Zhang (Slow), 2016-12-09 16:25:52]

zqzhang@chromium.org changed reviewers:
+ avayvod@chromium.org

[whywhat, 2016-12-09 16:30:24]

Why don't you just use ::testing::Mock::VerifyAndClear() at the end of each test
case and don't reinvent the gMock library? :)

[Zhiqiang Zhang (Slow), 2016-12-09 17:08:05]

PTAL

On 2016/12/09 16:30:24, whywhat wrote:
> Why don't you just use ::testing::Mock::VerifyAndClear() at the end of each
test
> case and don't reinvent the gMock library? :)

Just realized it's a one-line change, since GMock has "AllowLeak"...

The reason I didn't use ::testing::Mock::VerifyAndClear() is that the
MockAutoplayUmaHelper can be destructed at any time after calling
pageHandler().reset(), so getting the pointer is no longer safe after that
point.

PS#2 should already fixed the flakiness.

[whywhat, 2016-12-09 19:46:24]

On 2016/12/09 at 17:08:05, zqzhang wrote:
> PTAL
> 
> On 2016/12/09 16:30:24, whywhat wrote:
> > Why don't you just use ::testing::Mock::VerifyAndClear() at the end of each
test
> > case and don't reinvent the gMock library? :)
> 
> Just realized it's a one-line change, since GMock has "AllowLeak"...
> 
> The reason I didn't use ::testing::Mock::VerifyAndClear() is that the
MockAutoplayUmaHelper can be destructed at any time after calling
pageHandler().reset(), so getting the pointer is no longer safe after that
point.
> 
> PS#2 should already fixed the flakiness.

AllowLeak will not verify the object so the test becomes useless.
If you need to keep the object definitely alive after reset(), you could perhaps
have a Persistent reference to the mock object held by the test fixture maybe?

[Zhiqiang Zhang (Slow), 2016-12-09 21:56:36]

Description was changed from

==========
Avoid using GMock in AutoplayUmaHelperTest

AutoplayUmaHelperTest currently uses GMock for mock AutoplayUmaHelper in
order to check if events are correctly received. However this will make
the test flaky since MockAutoplyUmaHelper is garbage-collected but GMock
expect it to be destroyed when the test finishes.

This CL makes the test implementation not based on GMock which solves
the issue.

BUG=672831
==========

to

==========
Fix gmock leak issue in AutoplayUmaHelperTest

AutoplayUmaHelperTest currently uses GMock for mock
AutoplayUmaHelper in order to check if events are correctly
received. However this will make the test flaky since
MockAutoplyUmaHelper is garbage-collected but GMock expect it to
be destroyed when the test finishes.

This CL marks MockAutoplayUmaHelper as AllowLeak which fixes the
issue.

BUG=672831
==========

[Zhiqiang Zhang (Slow), 2016-12-09 21:56:51]

On 2016/12/09 19:46:24, whywhat wrote:
> On 2016/12/09 at 17:08:05, zqzhang wrote:
> > PTAL
> > 
> > On 2016/12/09 16:30:24, whywhat wrote:
> > > Why don't you just use ::testing::Mock::VerifyAndClear() at the end of
each
> test
> > > case and don't reinvent the gMock library? :)
> > 
> > Just realized it's a one-line change, since GMock has "AllowLeak"...
> > 
> > The reason I didn't use ::testing::Mock::VerifyAndClear() is that the
> MockAutoplayUmaHelper can be destructed at any time after calling
> pageHandler().reset(), so getting the pointer is no longer safe after that
> point.
> > 
> > PS#2 should already fixed the flakiness.
> 
> AllowLeak will not verify the object so the test becomes useless.
> If you need to keep the object definitely alive after reset(), you could
perhaps
> have a Persistent reference to the mock object held by the test fixture maybe?

Done. PTAL :)

[whywhat, 2016-12-09 22:16:42]

lgtm

https://codereview.chromium.org/2562683004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/AutoplayUmaHelperTest.cpp (right):

https://codereview.chromium.org/2562683004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/AutoplayUmaHelperTest.cpp:60:
::testing::Mock::AllowLeak(&element.m_autoplayUmaHelper);
nit: you don't need this anymore - the object will be cleared with
VerifyAndClear and gMock won't care about it being destroyed after the test
fixture AFAIK.

[Zhiqiang Zhang (Slow), 2016-12-12 20:31:24]

https://codereview.chromium.org/2562683004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/AutoplayUmaHelperTest.cpp (right):

https://codereview.chromium.org/2562683004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/AutoplayUmaHelperTest.cpp:60:
::testing::Mock::AllowLeak(&element.m_autoplayUmaHelper);
On 2016/12/09 22:16:42, whywhat wrote:
> nit: you don't need this anymore - the object will be cleared with
> VerifyAndClear and gMock won't care about it being destroyed after the test
> fixture AFAIK.

Done.

[Zhiqiang Zhang (Slow), 2016-12-12 20:31:33]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-12-12 20:31:33]

The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org
Link to the patchset: https://codereview.chromium.org/2562683004/#ps60001
(title: "removed AllowLeak")

[commit-bot: I haz the power, 2016-12-12 20:33:15]

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-12 22:44:41]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481574693268670,
"parent_rev": "eea5ab567bc87cb2af57ce73a03b1c620aa119dd", "commit_rev":
"0a3e59de8857128bf9c6618f0ee5812703e89b21"}

[commit-bot: I haz the power, 2016-12-12 22:45:08]

Description was changed from

==========
Fix gmock leak issue in AutoplayUmaHelperTest

AutoplayUmaHelperTest currently uses GMock for mock
AutoplayUmaHelper in order to check if events are correctly
received. However this will make the test flaky since
MockAutoplyUmaHelper is garbage-collected but GMock expect it to
be destroyed when the test finishes.

This CL marks MockAutoplayUmaHelper as AllowLeak which fixes the
issue.

BUG=672831
==========

to

==========
Fix gmock leak issue in AutoplayUmaHelperTest

AutoplayUmaHelperTest currently uses GMock for mock
AutoplayUmaHelper in order to check if events are correctly
received. However this will make the test flaky since
MockAutoplyUmaHelper is garbage-collected but GMock expect it to
be destroyed when the test finishes.

This CL marks MockAutoplayUmaHelper as AllowLeak which fixes the
issue.

BUG=672831

Review-Url: https://codereview.chromium.org/2562683004
==========

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

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-12 22:48:17]

Description was changed from

==========
Fix gmock leak issue in AutoplayUmaHelperTest

AutoplayUmaHelperTest currently uses GMock for mock
AutoplayUmaHelper in order to check if events are correctly
received. However this will make the test flaky since
MockAutoplyUmaHelper is garbage-collected but GMock expect it to
be destroyed when the test finishes.

This CL marks MockAutoplayUmaHelper as AllowLeak which fixes the
issue.

BUG=672831

Review-Url: https://codereview.chromium.org/2562683004
==========

to

==========
Fix gmock leak issue in AutoplayUmaHelperTest

AutoplayUmaHelperTest currently uses GMock for mock
AutoplayUmaHelper in order to check if events are correctly
received. However this will make the test flaky since
MockAutoplyUmaHelper is garbage-collected but GMock expect it to
be destroyed when the test finishes.

This CL marks MockAutoplayUmaHelper as AllowLeak which fixes the
issue.

BUG=672831

Committed: https://crrev.com/8906ce8b964bf3431b891bc9eaaca54ad8981f7d
Cr-Commit-Position: refs/heads/master@{#437939}
==========

[commit-bot: I haz the power, 2016-12-12 22:48:18]

Patchset 4 (id:??) landed as
https://crrev.com/8906ce8b964bf3431b891bc9eaaca54ad8981f7d
Cr-Commit-Position: refs/heads/master@{#437939}