Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2580003002:
Follow-up fix for leaking mock object in AutoplayUmaHelperTest (Closed)
|
Created:
4 years ago by Zhiqiang Zhang (Slow) Modified:
4 years ago Reviewers:
whywhat CC:
blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFollow-up fix for leaking mock object in AutoplayUmaHelperTest
A previous attempt for fixing AutoplayUmaHelperTest uses
::testing::Mock::VerifyAndClear to clean up the expectations on the
mock. However it does not ignore the mock object to be leaked. This CL
solves the issue by calling ::testing::Mock::AllowLeak in SetUp().
BUG=672831
Committed: https://crrev.com/99ac6d968116db3f8257044d3215fd222f9d7b81
Cr-Commit-Position: refs/heads/master@{#439205}
Patch Set 1 #
Messages
Total messages: 17 (9 generated)
The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
zqzhang@chromium.org changed reviewers: + avayvod@chromium.org
+avayvod@ to review since he reviewed the original CL +CC:mlamouri@ since there's a related test for fullscreen on orientation change.
the object is not leaked, it should be GC-ed, right? how does leaking manifest itself on the bots?
ok, I guess gmock checks for the object to be destroyed anyway :/ I find using gmock in Blink very frustrating due to things like this, perhaps should raise this in a blink-dev thread -- the more people use gmock and write blink unit tests, the worse the general issue is going to be... remote playback tests were broken in a bad way (reported as passed but didn't actually verify expectations) due to the objects not being destroyed in time for the verifications to break the tests. lgtm but this will be the first ever use of AllowLeak in the codebase AFAIK...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/16 16:11:32, whywhat wrote: > ok, I guess gmock checks for the object to be destroyed anyway :/ > > I find using gmock in Blink very frustrating due to things like this, perhaps > should raise this in a blink-dev thread -- the more people use gmock and write > blink unit tests, the worse the general issue is going to be... remote playback > tests were broken in a bad way (reported as passed but didn't actually verify > expectations) due to the objects not being destroyed in time for the > verifications to break the tests. > > lgtm but this will be the first ever use of AllowLeak in the codebase AFAIK... Hmm, found this old thread talking about banning GMock in blink but seems like no action was taken after that discussion. Not sure we should change the tests to not using GMock...
On 2016/12/16 16:51:04, Zhiqiang Zhang wrote: > On 2016/12/16 16:11:32, whywhat wrote: > > ok, I guess gmock checks for the object to be destroyed anyway :/ > > > > I find using gmock in Blink very frustrating due to things like this, perhaps > > should raise this in a blink-dev thread -- the more people use gmock and write > > blink unit tests, the worse the general issue is going to be... remote > playback > > tests were broken in a bad way (reported as passed but didn't actually verify > > expectations) due to the objects not being destroyed in time for the > > verifications to break the tests. > > > > lgtm but this will be the first ever use of AllowLeak in the codebase AFAIK... > > Hmm, found this old thread talking about banning GMock in blink but seems like > no action was taken after that discussion. > Not sure we should change the tests to not using GMock... The old blink-dev thread is: https://groups.google.com/a/chromium.org/d/msg/blink-dev/q9cq3KHZesc/buWLzsOw...
The CQ bit was checked by zqzhang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1481924997132020, "parent_rev":
"c42f299e210c4cfbb75d6705e0133ee31bd6e8dd", "commit_rev":
"946d8f6e6592fc3ea1c7b2481aba4140a2485f14"}
Message was sent while issue was closed.
Description was changed from ========== Follow-up fix for leaking mock object in AutoplayUmaHelperTest A previous attempt for fixing AutoplayUmaHelperTest uses ::testing::Mock::VerifyAndClear to clean up the expectations on the mock. However it does not ignore the mock object to be leaked. This CL solves the issue by calling ::testing::Mock::AllowLeak in SetUp(). BUG=672831 ========== to ========== Follow-up fix for leaking mock object in AutoplayUmaHelperTest A previous attempt for fixing AutoplayUmaHelperTest uses ::testing::Mock::VerifyAndClear to clean up the expectations on the mock. However it does not ignore the mock object to be leaked. This CL solves the issue by calling ::testing::Mock::AllowLeak in SetUp(). BUG=672831 Review-Url: https://codereview.chromium.org/2580003002 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Follow-up fix for leaking mock object in AutoplayUmaHelperTest A previous attempt for fixing AutoplayUmaHelperTest uses ::testing::Mock::VerifyAndClear to clean up the expectations on the mock. However it does not ignore the mock object to be leaked. This CL solves the issue by calling ::testing::Mock::AllowLeak in SetUp(). BUG=672831 Review-Url: https://codereview.chromium.org/2580003002 ========== to ========== Follow-up fix for leaking mock object in AutoplayUmaHelperTest A previous attempt for fixing AutoplayUmaHelperTest uses ::testing::Mock::VerifyAndClear to clean up the expectations on the mock. However it does not ignore the mock object to be leaked. This CL solves the issue by calling ::testing::Mock::AllowLeak in SetUp(). BUG=672831 Committed: https://crrev.com/99ac6d968116db3f8257044d3215fd222f9d7b81 Cr-Commit-Position: refs/heads/master@{#439205} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/99ac6d968116db3f8257044d3215fd222f9d7b81 Cr-Commit-Position: refs/heads/master@{#439205} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
