Fix DrMemory test for NativeDesktopMediaListTest.UpdateThumbnail

Fix DrMemory test for NativeDesktopMediaListTest.UpdateThumbnail

BUG=612590
Committed: https://crrev.com/a37f94d721559520b7aa7ef72b0055e92948041d
Cr-Commit-Position: refs/heads/master@{#398729}

[GeorgeZ, 2016-06-07 22:55:36]

gyzhou@chromium.org changed reviewers:
+ sergeyu@chromium.org

[GeorgeZ, 2016-06-07 22:55:36]

Sergey,

Could you take a look of this CL.

Thanks,

George

[Sergey Ulanov, 2016-06-07 23:42:31]

So if I understand correctly the problem is that the aura windows may be updated
after they after created and the tests doesn't expect it. Maybe just add an
expectation for this?

https://codereview.chromium.org/2045983004/diff/1/chrome/browser/media/native...
File chrome/browser/media/native_desktop_media_list_unittest.cc (right):

https://codereview.chromium.org/2045983004/diff/1/chrome/browser/media/native...
chrome/browser/media/native_desktop_media_list_unittest.cc:316:
EXPECT_CALL(observer_, OnSourceThumbnailChanged(model_.get(), i));
For aura Windows add .Times(testing::AtLeast(1)) to ignore any extra
OnSourceThumbnailChanged() calls?

[GeorgeZ, 2016-06-07 23:50:58]

On 2016/06/07 23:42:31, Sergey Ulanov wrote:
> So if I understand correctly the problem is that the aura windows may be
updated
> after they after created and the tests doesn't expect it. Maybe just add an
> expectation for this?
> 
>
https://codereview.chromium.org/2045983004/diff/1/chrome/browser/media/native...
> File chrome/browser/media/native_desktop_media_list_unittest.cc (right):
> 
>
https://codereview.chromium.org/2045983004/diff/1/chrome/browser/media/native...
> chrome/browser/media/native_desktop_media_list_unittest.cc:316:
> EXPECT_CALL(observer_, OnSourceThumbnailChanged(model_.get(), i));
> For aura Windows add .Times(testing::AtLeast(1)) to ignore any extra
> OnSourceThumbnailChanged() calls?

Sergey,

The fake aura window and its thumbnail is not going to update most of time.
However, it may be updated occasionally. 
We have no control on whether and when it is going to update. Therefore, it is
safe to not check thumbnaill 
update when fake aura window is in there.

[GeorgeZ, 2016-06-08 00:02:47]

On 2016/06/07 23:50:58, GeorgeZ wrote:
> On 2016/06/07 23:42:31, Sergey Ulanov wrote:
> > So if I understand correctly the problem is that the aura windows may be
> updated
> > after they after created and the tests doesn't expect it. Maybe just add an
> > expectation for this?
> > 
> >
>
https://codereview.chromium.org/2045983004/diff/1/chrome/browser/media/native...
> > File chrome/browser/media/native_desktop_media_list_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/2045983004/diff/1/chrome/browser/media/native...
> > chrome/browser/media/native_desktop_media_list_unittest.cc:316:
> > EXPECT_CALL(observer_, OnSourceThumbnailChanged(model_.get(), i));
> > For aura Windows add .Times(testing::AtLeast(1)) to ignore any extra
> > OnSourceThumbnailChanged() calls?
> 
> Sergey,
> 
> The fake aura window and its thumbnail is not going to update most of time.
> However, it may be updated occasionally. 
> We have no control on whether and when it is going to update. Therefore, it is
> safe to not check thumbnaill 
> update when fake aura window is in there.

Sergey,

We haven't seen the aura window update in all other try-bolts and only get a
report in Drmemory for at least a month.

If we want to expect a very rare happened event, is there any code example that
you can point out.

Thanks,

George

[Sergey Ulanov, 2016-06-08 00:04:55]

On 2016/06/07 23:50:58, GeorgeZ wrote:
> Sergey,
> 
> The fake aura window and its thumbnail is not going to update most of time.
> However, it may be updated occasionally. 
> We have no control on whether and when it is going to update. Therefore, it is
> safe to not check thumbnaill 
> update when fake aura window is in there.

I do not understand why you can't just ignore that OnSourceThumbnailChanged(2)
call for the aura window.
Normally OnSourceThumbnailChanged() is called in the following order:

1. OnSourceThumbnailChanged(1)  // initial content for the fake window
2. OnSourceThumbnailChanged(2)  // initial content for the aura window
3. OnSourceThumbnailChanged(1)  // updated fake window

In some cases we get this:
1. OnSourceThumbnailChanged(1)  // initial content for the fake window
2. OnSourceThumbnailChanged(2)  // initial content for the aura window
3. OnSourceThumbnailChanged(2)  // aura window is updated, not expected, test
fails
4. OnSourceThumbnailChanged(1)  // updated fake window

If you add Times(AtLeast(1)) then the (3) will be ignored and the test would
pass in that scenario.

[Sergey Ulanov, 2016-06-08 00:08:03]

On 2016/06/08 00:02:47, GeorgeZ wrote:
> On 2016/06/07 23:50:58, GeorgeZ wrote:
> > On 2016/06/07 23:42:31, Sergey Ulanov wrote:
> > > So if I understand correctly the problem is that the aura windows may be
> > updated
> > > after they after created and the tests doesn't expect it. Maybe just add
an
> > > expectation for this?
> > > 
> > >
> >
>
https://codereview.chromium.org/2045983004/diff/1/chrome/browser/media/native...
> > > File chrome/browser/media/native_desktop_media_list_unittest.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2045983004/diff/1/chrome/browser/media/native...
> > > chrome/browser/media/native_desktop_media_list_unittest.cc:316:
> > > EXPECT_CALL(observer_, OnSourceThumbnailChanged(model_.get(), i));
> > > For aura Windows add .Times(testing::AtLeast(1)) to ignore any extra
> > > OnSourceThumbnailChanged() calls?
> > 
> > Sergey,
> > 
> > The fake aura window and its thumbnail is not going to update most of time.
> > However, it may be updated occasionally. 
> > We have no control on whether and when it is going to update. Therefore, it
is
> > safe to not check thumbnaill 
> > update when fake aura window is in there.
> 
> Sergey,
> 
> We haven't seen the aura window update in all other try-bolts and only get a
> report in Drmemory for at least a month.

This makes sense. Normally the aura window doesn't get updated by the time the
test finishes. DrMemory runs test very slowly and so it's more likely that the
aura window gets updated before the test finishes.

> 
> If we want to expect a very rare happened event, is there any code example
that
> you can point out.

Adding Times(AtLeast(1)) when calling OnSourceThumbnailChanged() should give us
what we need here.

> 
> Thanks,
> 
> George

[GeorgeZ, 2016-06-08 00:26:27]

On 2016/06/08 00:08:03, Sergey Ulanov wrote:
> On 2016/06/08 00:02:47, GeorgeZ wrote:
> > On 2016/06/07 23:50:58, GeorgeZ wrote:
> > > On 2016/06/07 23:42:31, Sergey Ulanov wrote:
> > > > So if I understand correctly the problem is that the aura windows may be
> > > updated
> > > > after they after created and the tests doesn't expect it. Maybe just add
> an
> > > > expectation for this?
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2045983004/diff/1/chrome/browser/media/native...
> > > > File chrome/browser/media/native_desktop_media_list_unittest.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2045983004/diff/1/chrome/browser/media/native...
> > > > chrome/browser/media/native_desktop_media_list_unittest.cc:316:
> > > > EXPECT_CALL(observer_, OnSourceThumbnailChanged(model_.get(), i));
> > > > For aura Windows add .Times(testing::AtLeast(1)) to ignore any extra
> > > > OnSourceThumbnailChanged() calls?
> > > 
> > > Sergey,
> > > 
> > > The fake aura window and its thumbnail is not going to update most of
time.
> > > However, it may be updated occasionally. 
> > > We have no control on whether and when it is going to update. Therefore,
it
> is
> > > safe to not check thumbnaill 
> > > update when fake aura window is in there.
> > 
> > Sergey,
> > 
> > We haven't seen the aura window update in all other try-bolts and only get a
> > report in Drmemory for at least a month.
> 
> This makes sense. Normally the aura window doesn't get updated by the time the
> test finishes. DrMemory runs test very slowly and so it's more likely that the
> aura window gets updated before the test finishes.
> 
> > 
> > If we want to expect a very rare happened event, is there any code example
> that
> > you can point out.
> 
> Adding Times(AtLeast(1)) when calling OnSourceThumbnailChanged() should give
us
> what we need here.
> 
> > 
> > Thanks,
> > 
> > George

Brilliant idea.

George

[GeorgeZ, 2016-06-08 03:02:24]

Sergey,

Please see my response to your comment. I may not totally catch your point here.
If I am wrong, just let me know.

Thanks,

George

https://codereview.chromium.org/2045983004/diff/1/chrome/browser/media/native...
File chrome/browser/media/native_desktop_media_list_unittest.cc (right):

https://codereview.chromium.org/2045983004/diff/1/chrome/browser/media/native...
chrome/browser/media/native_desktop_media_list_unittest.cc:316:
EXPECT_CALL(observer_, OnSourceThumbnailChanged(model_.get(), i));
On 2016/06/07 23:42:30, Sergey Ulanov wrote:
> For aura Windows add .Times(testing::AtLeast(1)) to ignore any extra
> OnSourceThumbnailChanged() calls?

This part checks the first refresh. The OnSourceThumbnailChanged() will be
called for each window once and just once. testing::InSequence is also declared.
Then the message loop will be killed. There is no extra thumbnail refresh is
expected after that. Therefore .Times(testing::AtLeast(1)) may not be necessary.

[Sergey Ulanov, 2016-06-08 19:56:39]

https://codereview.chromium.org/2045983004/diff/1/chrome/browser/media/native...
File chrome/browser/media/native_desktop_media_list_unittest.cc (right):

https://codereview.chromium.org/2045983004/diff/1/chrome/browser/media/native...
chrome/browser/media/native_desktop_media_list_unittest.cc:316:
EXPECT_CALL(observer_, OnSourceThumbnailChanged(model_.get(), i));
On 2016/06/08 03:02:24, GeorgeZ wrote:
> On 2016/06/07 23:42:30, Sergey Ulanov wrote:
> > For aura Windows add .Times(testing::AtLeast(1)) to ignore any extra
> > OnSourceThumbnailChanged() calls?
> 
> This part checks the first refresh. The OnSourceThumbnailChanged() will be
> called for each window once and just once. testing::InSequence is also
declared.
> Then the message loop will be killed. There is no extra thumbnail refresh is
> expected after that. Therefore .Times(testing::AtLeast(1)) may not be
necessary.

Yeah, Times(AtLeast(1)) would not work here because of the sequence and also
because there is VerifyAndClearExpectations() below. See my next comment.

https://codereview.chromium.org/2045983004/diff/1/chrome/browser/media/native...
chrome/browser/media/native_desktop_media_list_unittest.cc:346: }
Add the expectations here? something like this:

for (size_t i = source_count - aura_window_count; i < source_count; ++i)
  EXPECT_CALL(observer_, OnSourceThumbnailChanged(model_.get(), i))
     .Times(testing::Any());
}

[GeorgeZ, 2016-06-08 21:27:42]

Sergey,

Please take a look of my response and let me know WDYT.

Thanks,

George

https://codereview.chromium.org/2045983004/diff/1/chrome/browser/media/native...
File chrome/browser/media/native_desktop_media_list_unittest.cc (right):

https://codereview.chromium.org/2045983004/diff/1/chrome/browser/media/native...
chrome/browser/media/native_desktop_media_list_unittest.cc:346: }
On 2016/06/08 19:56:38, Sergey Ulanov wrote:
> Add the expectations here? something like this:
> 
> for (size_t i = source_count - aura_window_count; i < source_count; ++i)
>   EXPECT_CALL(observer_, OnSourceThumbnailChanged(model_.get(), i))
>      .Times(testing::Any());
> }

This will lead to TEST_F(NativeDesktopMediaListTest, AddNativeWindow) failed.

AddNativeWindow leads to OnSourceThumbnailChanged(model_.get(), 3));

Here is the snapshot of error report:
EXPECT_CALL(observer_, OnSourceThumbnailChanged(model_.get(), index))...

  Expected arg #1: is equal to 2
           Actual: 3
         Expected: to be called any number of times
           Actual: never called - satisfied and active
[  FAILED  ] NativeDesktopMediaListTest.AddNativeWindow (144 ms)


The number of windows and possible their indices may change int the unit tests.
Other than the first refresh, OnSourceThumbnailchanged() will not be checked for
unit tests that have aura windows. Therefore it is safe to not add
.Times(testing::AnyNumber()). If .Times(testing::AnyNumber()) does need to be
added, I may need add it to each TEST_F unit test.

[Sergey Ulanov, 2016-06-08 21:40:16]

https://codereview.chromium.org/2045983004/diff/1/chrome/browser/media/native...
File chrome/browser/media/native_desktop_media_list_unittest.cc (right):

https://codereview.chromium.org/2045983004/diff/1/chrome/browser/media/native...
chrome/browser/media/native_desktop_media_list_unittest.cc:346: }
On 2016/06/08 21:27:41, GeorgeZ wrote:
> On 2016/06/08 19:56:38, Sergey Ulanov wrote:
> > Add the expectations here? something like this:
> > 
> > for (size_t i = source_count - aura_window_count; i < source_count; ++i)
> >   EXPECT_CALL(observer_, OnSourceThumbnailChanged(model_.get(), i))
> >      .Times(testing::Any());
> > }
> 
> This will lead to TEST_F(NativeDesktopMediaListTest, AddNativeWindow) failed.
> 
> AddNativeWindow leads to OnSourceThumbnailChanged(model_.get(), 3));
> 
> Here is the snapshot of error report:
> EXPECT_CALL(observer_, OnSourceThumbnailChanged(model_.get(), index))...
> 
>   Expected arg #1: is equal to 2
>            Actual: 3
>          Expected: to be called any number of times
>            Actual: never called - satisfied and active
> [  FAILED  ] NativeDesktopMediaListTest.AddNativeWindow (144 ms)
> 
> 
> The number of windows and possible their indices may change int the unit
tests.
> Other than the first refresh, OnSourceThumbnailchanged() will not be checked
for
> unit tests that have aura windows. Therefore it is safe to not add
> .Times(testing::AnyNumber()). If .Times(testing::AnyNumber()) does need to be
> added, I may need add it to each TEST_F unit test.
> 

Just add it for the UpdateThumbnail test? That's the only test that sets
OnSourceThumbnailchanged() expectation

[GeorgeZ, 2016-06-08 22:15:31]

Patchset #2 (id:20001) has been deleted

[GeorgeZ, 2016-06-08 22:16:40]

Sergey,

A patch has uploaded based on your comment.

Thanks,

https://codereview.chromium.org/2045983004/diff/1/chrome/browser/media/native...
File chrome/browser/media/native_desktop_media_list_unittest.cc (right):

https://codereview.chromium.org/2045983004/diff/1/chrome/browser/media/native...
chrome/browser/media/native_desktop_media_list_unittest.cc:346: }
On 2016/06/08 21:40:16, Sergey Ulanov wrote:
> On 2016/06/08 21:27:41, GeorgeZ wrote:
> > On 2016/06/08 19:56:38, Sergey Ulanov wrote:
> > > Add the expectations here? something like this:
> > > 
> > > for (size_t i = source_count - aura_window_count; i < source_count; ++i)
> > >   EXPECT_CALL(observer_, OnSourceThumbnailChanged(model_.get(), i))
> > >      .Times(testing::Any());
> > > }
> > 
> > This will lead to TEST_F(NativeDesktopMediaListTest, AddNativeWindow)
failed.
> > 
> > AddNativeWindow leads to OnSourceThumbnailChanged(model_.get(), 3));
> > 
> > Here is the snapshot of error report:
> > EXPECT_CALL(observer_, OnSourceThumbnailChanged(model_.get(), index))...
> > 
> >   Expected arg #1: is equal to 2
> >            Actual: 3
> >          Expected: to be called any number of times
> >            Actual: never called - satisfied and active
> > [  FAILED  ] NativeDesktopMediaListTest.AddNativeWindow (144 ms)
> > 
> > 
> > The number of windows and possible their indices may change int the unit
> tests.
> > Other than the first refresh, OnSourceThumbnailchanged() will not be checked
> for
> > unit tests that have aura windows. Therefore it is safe to not add
> > .Times(testing::AnyNumber()). If .Times(testing::AnyNumber()) does need to
be
> > added, I may need add it to each TEST_F unit test.
> > 
> 
> Just add it for the UpdateThumbnail test? That's the only test that sets
> OnSourceThumbnailchanged() expectation

Done.

[Sergey Ulanov, 2016-06-08 22:18:04]

lgtm

https://codereview.chromium.org/2045983004/diff/40001/chrome/browser/media/na...
File chrome/browser/media/native_desktop_media_list_unittest.cc (right):

https://codereview.chromium.org/2045983004/diff/40001/chrome/browser/media/na...
chrome/browser/media/native_desktop_media_list_unittest.cc:484: // Aura windows'
thumbnails may unpredictably change overtime.
nit: s/overtime/over time/

[GeorgeZ, 2016-06-08 23:14:39]

The CQ bit was checked by gyzhou@chromium.org

[GeorgeZ, 2016-06-08 23:14:39]

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

[commit-bot: I haz the power, 2016-06-08 23:15:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2045983004/60001

[commit-bot: I haz the power, 2016-06-08 23:20:31]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-06-08 23:22:25]

Description was changed from

==========
Fix DrMemory test for NativeDesktopMediaListTest.UpdateThumbnail

BUG=612590
==========

to

==========
Fix DrMemory test for NativeDesktopMediaListTest.UpdateThumbnail

BUG=612590

Committed: https://crrev.com/a37f94d721559520b7aa7ef72b0055e92948041d
Cr-Commit-Position: refs/heads/master@{#398729}
==========

[commit-bot: I haz the power, 2016-06-08 23:22:26]

Patchset 3 (id:??) landed as
https://crrev.com/a37f94d721559520b7aa7ef72b0055e92948041d
Cr-Commit-Position: refs/heads/master@{#398729}