Use MockDBusThreadManagerWithoutGMock for GetUpdateStatusApiTest

Use MockDBusThreadManagerWithoutGMock for GetUpdateStatusApiTest

Adding a queue to emulate the changing status in FakeUpdateEngineClient.

BUG=239333
TEST=trybots
R=bartfab@chromium.org, hashimoto@chromium.org, kalman@chromium.org, nkostylev@chromium.org, satorux@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=200201

[bartfab (slow), 2013-05-14 09:35:06]

You will need a reviewer more familiar with this code. I wrote lines 40-42 of
system_private_apitest.cc only and your change there LGTM but I never touched
any of the other code you are modifying.

https://codereview.chromium.org/14589017/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/system_private/system_private_apitest.cc
(left):

https://codereview.chromium.org/14589017/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/system_private/system_private_apitest.cc:41:
.Times(1)
This used to verify that the method gets called exactly once. Could you add a
corresponding check to the new version?

[hashimoto, 2013-05-14 10:37:21]

https://codereview.chromium.org/14589017/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/system_private/system_private_apitest.cc
(left):

https://codereview.chromium.org/14589017/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/system_private/system_private_apitest.cc:41:
.Times(1)
On 2013/05/14 09:35:06, bartfab wrote:
> This used to verify that the method gets called exactly once. Could you add a
> corresponding check to the new version?

Do we need to check that GetLastStatus() is called to get the empty status?
I think there is no need to add a such kind of check because
GetUpdateStatusApiTest.Progress does enough test for GetLastStatus().

https://codereview.chromium.org/14589017/diff/1/chromeos/dbus/fake_update_eng...
File chromeos/dbus/fake_update_engine_client.h (right):

https://codereview.chromium.org/14589017/diff/1/chromeos/dbus/fake_update_eng...
chromeos/dbus/fake_update_engine_client.h:58: UpdateEngineClient::Status
update_engine_client_status_;
How about renaming this to default_status_ to clarify the role of this variable?
Or, can't we remove this since no one is interested in this variable at all?

[bartfab (slow), 2013-05-14 10:53:14]

https://codereview.chromium.org/14589017/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/system_private/system_private_apitest.cc
(left):

https://codereview.chromium.org/14589017/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/system_private/system_private_apitest.cc:41:
.Times(1)
On 2013/05/14 10:37:21, hashimoto wrote:
> On 2013/05/14 09:35:06, bartfab wrote:
> > This used to verify that the method gets called exactly once. Could you add
a
> > corresponding check to the new version?
> 
> Do we need to check that GetLastStatus() is called to get the empty status?
> I think there is no need to add a such kind of check because
> GetUpdateStatusApiTest.Progress does enough test for GetLastStatus().

Fair enough. Let's leave out the check.

[Haruki Sato, 2013-05-14 13:20:14]

Nikita, could you take a look at the changes in UpdateScreenTest?

Thanks.

https://codereview.chromium.org/14589017/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/system_private/system_private_apitest.cc
(left):

https://codereview.chromium.org/14589017/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/system_private/system_private_apitest.cc:41:
.Times(1)
On 2013/05/14 10:53:14, bartfab wrote:
> On 2013/05/14 10:37:21, hashimoto wrote:
> > On 2013/05/14 09:35:06, bartfab wrote:
> > > This used to verify that the method gets called exactly once. Could you
add
> a
> > > corresponding check to the new version?
> > 
> > Do we need to check that GetLastStatus() is called to get the empty status?
> > I think there is no need to add a such kind of check because
> > GetUpdateStatusApiTest.Progress does enough test for GetLastStatus().
> 
> Fair enough. Let's leave out the check.

Thank you guys for the review!

https://codereview.chromium.org/14589017/diff/1/chromeos/dbus/fake_update_eng...
File chromeos/dbus/fake_update_engine_client.h (right):

https://codereview.chromium.org/14589017/diff/1/chromeos/dbus/fake_update_eng...
chromeos/dbus/fake_update_engine_client.h:58: UpdateEngineClient::Status
update_engine_client_status_;
On 2013/05/14 10:37:21, hashimoto wrote:
> How about renaming this to default_status_ to clarify the role of this
variable?
> Or, can't we remove this since no one is interested in this variable at all?

Good point. It is used for another unittest.
Renamed. Thanks.

[Nikita (slow), 2013-05-14 13:25:04]

lgtm

[hashimoto, 2013-05-14 14:47:54]

lgtm

https://codereview.chromium.org/14589017/diff/7001/chromeos/dbus/fake_update_...
File chromeos/dbus/fake_update_engine_client.h (right):

https://codereview.chromium.org/14589017/diff/7001/chromeos/dbus/fake_update_...
chromeos/dbus/fake_update_engine_client.h:35: // Push UpdateEngineClient::Status
in the queue to test changing status.
nit: s/Push/Pushes/?

[Haruki Sato, 2013-05-15 02:01:08]

Thanks reviewers.

Satoru, could you OWNER-review chromsos/dbus ?

Benjamin, could you OWNER-review chrome/browser/extensions/ ?


Thanks!

https://codereview.chromium.org/14589017/diff/7001/chromeos/dbus/fake_update_...
File chromeos/dbus/fake_update_engine_client.h (right):

https://codereview.chromium.org/14589017/diff/7001/chromeos/dbus/fake_update_...
chromeos/dbus/fake_update_engine_client.h:35: // Push UpdateEngineClient::Status
in the queue to test changing status.
On 2013/05/14 14:47:54, hashimoto wrote:
> nit: s/Push/Pushes/?

Done. Thanks!

[not at google - send to devlin, 2013-05-15 02:22:14]

lgtm

[satorux1, 2013-05-15 04:33:35]

LGTM

[commit-bot: I haz the power, 2013-05-15 04:35:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haruki@chromium.org/14589017/14001

[commit-bot: I haz the power, 2013-05-15 05:48:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haruki@chromium.org/14589017/27001

[Haruki Sato, 2013-05-15 08:42:19]

Committed patchset #4 manually as r200201 (presubmit successful).