Add service unittest for WakeLockServiceImpl.

Add service unittest for WakeLockServiceImpl.

To avoid let service unittest depend on "xvfb", we use the
FakeWakeLockServiceImpl which doesn't create the real
PowerSaveBlocker instance during testing.

BUG=689410
Review-Url: https://codereview.chromium.org/2883903002
Cr-Commit-Position: refs/heads/master@{#477200}
Committed: https://chromium.googlesource.com/chromium/src/+/4d8a87f26e4b1027bd9281e4006506ab0f983732

[ke.he, 2017-05-15 09:33:35]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-15 09:33:54]

Dry run: 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, 2017-05-15 10:32:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-15 10:32:06]

Dry run: This issue passed the CQ dry run.

[ke.he, 2017-05-15 10:45:11]

ke.he@intel.com changed reviewers:
+ blundell@chromium.org

[ke.he, 2017-05-15 10:45:12]

Colin, PTAL. Thanks!

[blundell, 2017-05-16 13:37:55]

This is fantastic, thanks! Exactly along the lines of what I was hoping for.

LGTM

https://codereview.chromium.org/2883903002/diff/1/device/power_save_blocker/p...
File device/power_save_blocker/power_save_blocker_x11.cc (right):

https://codereview.chromium.org/2883903002/diff/1/device/power_save_blocker/p...
device/power_save_blocker/power_save_blocker_x11.cc:444: // In service_unittests
context, XDisplay is not well initialized. passing a
Did you check whether it's possible to initialize XDisplay instead?

https://codereview.chromium.org/2883903002/diff/1/services/device/wake_lock/w...
File services/device/wake_lock/wake_lock_service_impl_unittest.cc (right):

https://codereview.chromium.org/2883903002/diff/1/services/device/wake_lock/w...
services/device/wake_lock/wake_lock_service_impl_unittest.cc:17: void
OnHasWakeLock(bool* out, base::Closure quit_closure, bool has_wakelock) {
nit: I would just make this a method of WakeLockServiceImplTest and have a
|has_wakelock_| ivar.

https://codereview.chromium.org/2883903002/diff/1/services/device/wake_lock/w...
services/device/wake_lock/wake_lock_service_impl_unittest.cc:156:
TEST_F(WakeLockServiceImplTest, MixedTest) {
nit: All of these tests could use 1 line of documentation on what they're
testing.

https://codereview.chromium.org/2883903002/diff/1/services/device/wake_lock/w...
services/device/wake_lock/wake_lock_service_impl_unittest.cc:168:
wake_lock_1->RequestWakeLock();
This could use a line of documentation on what the output state after this block
is:

// Execute a series of calls that should result in |wake_lock_1| and
|wake_lock_3| having outstanding wake lock requests.

[ke.he, 2017-05-17 07:18:15]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 07:19:07]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ke.he, 2017-05-17 07:19:53]

ke.he@intel.com changed reviewers:
+ scottmg@chromium.org

[ke.he, 2017-05-17 07:19:54]

Thanks Colin!
Scottmg@, PTAL on power_save_blocker_x11.cc, Thanks!

https://codereview.chromium.org/2883903002/diff/1/device/power_save_blocker/p...
File device/power_save_blocker/power_save_blocker_x11.cc (right):

https://codereview.chromium.org/2883903002/diff/1/device/power_save_blocker/p...
device/power_save_blocker/power_save_blocker_x11.cc:444: // In service_unittests
context, XDisplay is not well initialized. passing a
On 2017/05/16 13:37:55, blundell wrote:
> Did you check whether it's possible to initialize XDisplay instead?

Yes, I followed how the browser_main_runner.cc initializes the x11, calling
gfx::InitialzieThreadedX11(), and the XDisplay can be initialized successfully.
But again powerSaveBlocker crashes on some place related to D-BUS. So, still
complex.

I feel that this unittest is for the WakeLockServiceImpl and its mojo
interfaces, so I stopped digging on why the PowerSaveBlocker doesn't work on
DeviceServiceTestBase. Maybe in future it blocks something, I will continue to
fix it then. Do you think it is OK?

https://codereview.chromium.org/2883903002/diff/1/services/device/wake_lock/w...
File services/device/wake_lock/wake_lock_service_impl_unittest.cc (right):

https://codereview.chromium.org/2883903002/diff/1/services/device/wake_lock/w...
services/device/wake_lock/wake_lock_service_impl_unittest.cc:17: void
OnHasWakeLock(bool* out, base::Closure quit_closure, bool has_wakelock) {
On 2017/05/16 13:37:55, blundell wrote:
> nit: I would just make this a method of WakeLockServiceImplTest and have a
> |has_wakelock_| ivar.

Done.

https://codereview.chromium.org/2883903002/diff/1/services/device/wake_lock/w...
services/device/wake_lock/wake_lock_service_impl_unittest.cc:156:
TEST_F(WakeLockServiceImplTest, MixedTest) {
On 2017/05/16 13:37:55, blundell wrote:
> nit: All of these tests could use 1 line of documentation on what they're
> testing.

Done.

https://codereview.chromium.org/2883903002/diff/1/services/device/wake_lock/w...
services/device/wake_lock/wake_lock_service_impl_unittest.cc:168:
wake_lock_1->RequestWakeLock();
On 2017/05/16 13:37:55, blundell wrote:
> This could use a line of documentation on what the output state after this
block
> is:
> 
> // Execute a series of calls that should result in |wake_lock_1| and
> |wake_lock_3| having outstanding wake lock requests.

Done.

[commit-bot: I haz the power, 2017-05-17 08:33:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-17 08:33:15]

Dry run: This issue passed the CQ dry run.

[blundell, 2017-05-17 09:01:42]

https://codereview.chromium.org/2883903002/diff/1/device/power_save_blocker/p...
File device/power_save_blocker/power_save_blocker_x11.cc (right):

https://codereview.chromium.org/2883903002/diff/1/device/power_save_blocker/p...
device/power_save_blocker/power_save_blocker_x11.cc:444: // In service_unittests
context, XDisplay is not well initialized. passing a
On 2017/05/17 07:19:54, ke.he wrote:
> On 2017/05/16 13:37:55, blundell wrote:
> > Did you check whether it's possible to initialize XDisplay instead?
> 
> Yes, I followed how the browser_main_runner.cc initializes the x11, calling
> gfx::InitialzieThreadedX11(), and the XDisplay can be initialized
successfully.
> But again powerSaveBlocker crashes on some place related to D-BUS. So, still
> complex.
> 
> I feel that this unittest is for the WakeLockServiceImpl and its mojo
> interfaces, so I stopped digging on why the PowerSaveBlocker doesn't work on
> DeviceServiceTestBase. Maybe in future it blocks something, I will continue to
> fix it then. Do you think it is OK?

Short-circuiting out early in the test isn't concerning; my concern is only that
now we're adding a path for silent failure in production code that wasn't there
before. I think maybe it would be a better to add an OS_LINUX-specific
DisableXSSForTest() method to PowerSaveBlocker?

[ke.he, 2017-05-17 10:20:35]

On 2017/05/17 09:01:42, blundell wrote:
>
https://codereview.chromium.org/2883903002/diff/1/device/power_save_blocker/p...
> File device/power_save_blocker/power_save_blocker_x11.cc (right):
> 
>
https://codereview.chromium.org/2883903002/diff/1/device/power_save_blocker/p...
> device/power_save_blocker/power_save_blocker_x11.cc:444: // In
service_unittests
> context, XDisplay is not well initialized. passing a
> On 2017/05/17 07:19:54, ke.he wrote:
> > On 2017/05/16 13:37:55, blundell wrote:
> > > Did you check whether it's possible to initialize XDisplay instead?
> > 
> > Yes, I followed how the browser_main_runner.cc initializes the x11, calling
> > gfx::InitialzieThreadedX11(), and the XDisplay can be initialized
> successfully.
> > But again powerSaveBlocker crashes on some place related to D-BUS. So, still
> > complex.
> > 
> > I feel that this unittest is for the WakeLockServiceImpl and its mojo
> > interfaces, so I stopped digging on why the PowerSaveBlocker doesn't work on
> > DeviceServiceTestBase. Maybe in future it blocks something, I will continue
to
> > fix it then. Do you think it is OK?
> 
> Short-circuiting out early in the test isn't concerning; my concern is only
that
> now we're adding a path for silent failure in production code that wasn't
there
> before. I think maybe it would be a better to add an OS_LINUX-specific
> DisableXSSForTest() method to PowerSaveBlocker?

1)
From the man page I see "XScreenSaverQueryExtension returns True if the
XScreenSaver extension is available on the given display".  So I slightly feel
that we don't change the logic of production code to make it return false if the
"given dispaly" is nullptr?

2)
About adding DisableXSSForTest(), do you mean to add as below?

class PowerSaveBlocker::Delegate{
...
static DisableXSSForTest(bool flag) {
  disable_xss_for_test_ = flag;
}

bool PowerSaveBlocker::Delegate::XSSAvailable() {
  if (disable_xss_for_test_)
     return false;

  ...original code...
}
}

Then our unittest, we set PowerSaveBlocker::Delegate::DisableXSSForTest(true) in
our setup(). right?

[blundell, 2017-05-17 15:22:04]

On 2017/05/17 10:20:35, ke.he wrote:
> On 2017/05/17 09:01:42, blundell wrote:
> >
>
https://codereview.chromium.org/2883903002/diff/1/device/power_save_blocker/p...
> > File device/power_save_blocker/power_save_blocker_x11.cc (right):
> > 
> >
>
https://codereview.chromium.org/2883903002/diff/1/device/power_save_blocker/p...
> > device/power_save_blocker/power_save_blocker_x11.cc:444: // In
> service_unittests
> > context, XDisplay is not well initialized. passing a
> > On 2017/05/17 07:19:54, ke.he wrote:
> > > On 2017/05/16 13:37:55, blundell wrote:
> > > > Did you check whether it's possible to initialize XDisplay instead?
> > > 
> > > Yes, I followed how the browser_main_runner.cc initializes the x11,
calling
> > > gfx::InitialzieThreadedX11(), and the XDisplay can be initialized
> > successfully.
> > > But again powerSaveBlocker crashes on some place related to D-BUS. So,
still
> > > complex.
> > > 
> > > I feel that this unittest is for the WakeLockServiceImpl and its mojo
> > > interfaces, so I stopped digging on why the PowerSaveBlocker doesn't work
on
> > > DeviceServiceTestBase. Maybe in future it blocks something, I will
continue
> to
> > > fix it then. Do you think it is OK?
> > 
> > Short-circuiting out early in the test isn't concerning; my concern is only
> that
> > now we're adding a path for silent failure in production code that wasn't
> there
> > before. I think maybe it would be a better to add an OS_LINUX-specific
> > DisableXSSForTest() method to PowerSaveBlocker?
> 
> 1)
> From the man page I see "XScreenSaverQueryExtension returns True if the
> XScreenSaver extension is available on the given display".  So I slightly feel
> that we don't change the logic of production code to make it return false if
the
> "given dispaly" is nullptr?
> 
> 2)
> About adding DisableXSSForTest(), do you mean to add as below?
> 
> class PowerSaveBlocker::Delegate{
> ...
> static DisableXSSForTest(bool flag) {
>   disable_xss_for_test_ = flag;
> }
> 
> bool PowerSaveBlocker::Delegate::XSSAvailable() {
>   if (disable_xss_for_test_)
>      return false;
> 
>   ...original code...
> }
> }
> 
> Then our unittest, we set PowerSaveBlocker::Delegate::DisableXSSForTest(true)
in
> our setup(). right?

Yes, I think that you're probably right that it's fine to leave the code as-is
(and also, yes, your change was what I had intended, only I think you might need
to put it on PowerSaveBlocker itself as I'm not sure you can get to the delegate
directly from consumer code). I'll let Scott make the final call :).

[scottmg, 2017-05-17 16:00:33]

https://codereview.chromium.org/2883903002/diff/20001/device/power_save_block...
File device/power_save_blocker/power_save_blocker_x11.cc (right):

https://codereview.chromium.org/2883903002/diff/20001/device/power_save_block...
device/power_save_blocker/power_save_blocker_x11.cc:445: // nullptr to
XScreenSaverQueryExtension() will cause crash.
nit; if we keep this, a more natural phrasing:

  // In service_unittests, XDisplay is not initialized. Passing a nullptr to
  // XScreenSaverQueryExtension() will cause a crash.

https://codereview.chromium.org/2883903002/diff/20001/device/power_save_block...
device/power_save_blocker/power_save_blocker_x11.cc:447: return false;
Could you recheck this? At the top of WakeLockServiceImplTest::SetUp(), I added

DCHECK(gfx::InitializeThreadedX11());

(plus BUILD and #include) and all the tests seem to pass locally for me.

I ran as `python testing/xvfb.py out/Debug/service_unittests
--gtest_filter=WakeLockServiceImplTest.*`.

https://codereview.chromium.org/2883903002/diff/20001/device/power_save_block...
device/power_save_blocker/power_save_blocker_x11.cc:448: int dummy;
nit; Add a blank line before int dummy;.

https://codereview.chromium.org/2883903002/diff/20001/services/device/wake_lo...
File services/device/wake_lock/wake_lock_service_impl_unittest.cc (right):

https://codereview.chromium.org/2883903002/diff/20001/services/device/wake_lo...
services/device/wake_lock/wake_lock_service_impl_unittest.cc:1: // Copyright (c)
2017 The Chromium Authors. All rights reserved.
ni;t no (c).

https://codereview.chromium.org/2883903002/diff/20001/services/device/wake_lo...
services/device/wake_lock/wake_lock_service_impl_unittest.cc:83: // Send
multiple requests, which should be coallesced as one request.
"coalesced"

https://codereview.chromium.org/2883903002/diff/20001/services/device/wake_lo...
services/device/wake_lock/wake_lock_service_impl_unittest.cc:96: //
WakeLockProvider connection broken doesn't effect WakeLockService.
effect -> affect.

https://codereview.chromium.org/2883903002/diff/20001/services/device/wake_lo...
services/device/wake_lock/wake_lock_service_impl_unittest.cc:111: // One
WakeLockService instance can serves multiple clients at same time.
serves -> serve.

[ke.he, 2017-05-18 06:09:11]

> Could you recheck this? At the top of WakeLockServiceImplTest::SetUp(), I
added
> 
> DCHECK(gfx::InitializeThreadedX11());
> 
> (plus BUILD and #include) and all the tests seem to pass locally for me.
> 
> I ran as ``.

Hi, Scottmg@, Thanks!
The tests also passed for me by running "python testing/xvfb.py
out/Debug/service_unittests --gtest_filter=WakeLockServiceImplTest.*" 

I never used this "xvfb.py".   I run ./out/release_1/service_unittests
--gtest_filter=WakeLockServiceImplTest.*, then it still goes crash(see crash
callstack below).
xvfb means "X virtual framebuffer"?  So my question is when to use it and when
not?  Should we use it in this case?

By the way I use the release build, the args.gn is
      is_component_build = true
      is_debug = false
      dcheck_always_on = true


---------crash callstack by run the ./out/release_1/service_unittests:
Note: Google Test filter = WakeLockServiceImplTest.MixedTest
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from WakeLockServiceImplTest
[ RUN      ] WakeLockServiceImplTest.MixedTest
Received signal 11 SEGV_MAPERR 000000000168
#0 0x7f73674e9ee7 base::debug::StackTrace::StackTrace()
#1 0x7f73674e9a5f base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7f7367672390 <unknown>
#3 0x7f73674f0a87 base::FileDescriptorWatcher::Controller::Controller()
#4 0x7f73674f1060 base::FileDescriptorWatcher::WatchReadable()
#5 0x7f736438ef3c dbus::(anonymous namespace)::Watch::StartWatching()
#6 0x7f736438c7ef dbus::Bus::OnAddWatchThunk()
#7 0x7f735d8f8d92 _dbus_watch_list_set_functions
#8 0x7f735d8df06f dbus_connection_set_watch_functions
#9 0x7f736438c681 dbus::Bus::SetUpAsyncOperations()
#10 0x7f73643a5ef2 dbus::ObjectProxy::StartAsyncMethodCall()
#11 0x7f73643a9b1c
_ZN4base8internal13FunctorTraitsIMN4dbus11ObjectProxyEFviP11DBusMessageNS_8CallbackIFvPNS2_8ResponseEELNS0_8CopyModeE1ELNS0_10RepeatModeE1EEENS6_IFvPNS2_13ErrorResponseEELSA_1ELSB_1EEENS_9TimeTicksEEvE6InvokeIRK13scoped_refptrIS3_EJRKiRKS5_RKSC_RKSG_RKSH_EEEvSJ_OT_DpOT0_
#12 0x7f73643a9a1d
_ZN4base8internal7InvokerINS0_9BindStateIMN4dbus11ObjectProxyEFviP11DBusMessageNS_8CallbackIFvPNS3_8ResponseEELNS0_8CopyModeE1ELNS0_10RepeatModeE1EEENS7_IFvPNS3_13ErrorResponseEELSB_1ELSC_1EEENS_9TimeTicksEEJ13scoped_refptrIS4_EiS6_SD_SH_SI_EEEFvvEE3RunEPNS0_13BindStateBaseE
#13 0x7f73674d5211
_ZNO4base8CallbackIFvvELNS_8internal8CopyModeE0ELNS2_10RepeatModeE0EE3RunEv
#14 0x7f73674ea803 base::debug::TaskAnnotator::RunTask()
#15 0x7f736751bf0d base::MessageLoop::RunTask()
#16 0x7f736751c2fc base::MessageLoop::DeferOrRunPendingTask()
#17 0x7f736751c6c6 base::MessageLoop::DoWork()
#18 0x7f736751df29 base::MessagePumpDefault::Run()
#19 0x7f736751bc75 base::MessageLoop::RunHandler()
#20 0x7f73675511bc base::RunLoop::Run()
#21 0x7f736758f1bc base::Thread::Run()
#22 0x7f736758f6d8 base::Thread::ThreadMain()
#23 0x7f7367586e3c base::(anonymous namespace)::ThreadFunc()
#24 0x7f73676686ba start_thread
#25 0x7f736332682d clone

[ke.he, 2017-05-18 06:22:35]

I think we should make sure the executable service_unittest pass both with and
without the xvfb.py.
So I prefer to keep the modifications(if !display then return;) in
power_save_blocker_x11.cc. WDYT? Thanks!

[scottmg, 2017-05-18 15:33:12]

Description was changed from

==========
Add service unittest for WakeLockServiceImpl.

BUG=689410
==========

to

==========
Add service unittest for WakeLockServiceImpl.

BUG=689410
==========

[scottmg, 2017-05-18 15:33:12]

scottmg@chromium.org changed reviewers:
+ hashimoto@chromium.org

[scottmg, 2017-05-18 15:35:09]

On 2017/05/18 06:22:35, ke.he wrote:
> I think we should make sure the executable service_unittest pass both with and
> without the xvfb.py.
> So I prefer to keep the modifications(if !display then return;) in
> power_save_blocker_x11.cc. WDYT? Thanks!

OK, I will have to defer to +hashimoto as I don't really know much about
Linux/CrOS/X and whether it's better to try to get this initialized. (Sorry for
all the back and forth.)

[ke.he, 2017-05-19 03:46:30]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

[ke.he, 2017-05-19 03:46:51]

Thanks!

https://codereview.chromium.org/2883903002/diff/20001/device/power_save_block...
File device/power_save_blocker/power_save_blocker_x11.cc (right):

https://codereview.chromium.org/2883903002/diff/20001/device/power_save_block...
device/power_save_blocker/power_save_blocker_x11.cc:445: // nullptr to
XScreenSaverQueryExtension() will cause crash.
On 2017/05/17 16:00:32, scottmg wrote:
> nit; if we keep this, a more natural phrasing:
> 
>   // In service_unittests, XDisplay is not initialized. Passing a nullptr to
>   // XScreenSaverQueryExtension() will cause a crash.

Done.

https://codereview.chromium.org/2883903002/diff/20001/device/power_save_block...
device/power_save_blocker/power_save_blocker_x11.cc:448: int dummy;
On 2017/05/17 16:00:32, scottmg wrote:
> nit; Add a blank line before int dummy;.

Done.

https://codereview.chromium.org/2883903002/diff/20001/services/device/wake_lo...
File services/device/wake_lock/wake_lock_service_impl_unittest.cc (right):

https://codereview.chromium.org/2883903002/diff/20001/services/device/wake_lo...
services/device/wake_lock/wake_lock_service_impl_unittest.cc:1: // Copyright (c)
2017 The Chromium Authors. All rights reserved.
On 2017/05/17 16:00:33, scottmg wrote:
> ni;t no (c).

Done.

https://codereview.chromium.org/2883903002/diff/20001/services/device/wake_lo...
services/device/wake_lock/wake_lock_service_impl_unittest.cc:83: // Send
multiple requests, which should be coallesced as one request.
On 2017/05/17 16:00:32, scottmg wrote:
> "coalesced"

Done.

https://codereview.chromium.org/2883903002/diff/20001/services/device/wake_lo...
services/device/wake_lock/wake_lock_service_impl_unittest.cc:96: //
WakeLockProvider connection broken doesn't effect WakeLockService.
On 2017/05/17 16:00:33, scottmg wrote:
> effect -> affect.

Done.

https://codereview.chromium.org/2883903002/diff/20001/services/device/wake_lo...
services/device/wake_lock/wake_lock_service_impl_unittest.cc:111: // One
WakeLockService instance can serves multiple clients at same time.
On 2017/05/17 16:00:32, scottmg wrote:
> serves -> serve.

Done.

[commit-bot: I haz the power, 2017-05-19 03:47:44]

Dry run: 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, 2017-05-19 05:44:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-19 05:44:25]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[hashimoto, 2017-05-19 10:49:10]

On 2017/05/18 15:35:09, scottmg wrote:
> On 2017/05/18 06:22:35, ke.he wrote:
> > I think we should make sure the executable service_unittest pass both with
and
> > without the xvfb.py.
> > So I prefer to keep the modifications(if !display then return;) in
> > power_save_blocker_x11.cc. WDYT? Thanks!
> 
> OK, I will have to defer to +hashimoto as I don't really know much about
> Linux/CrOS/X and whether it's better to try to get this initialized. (Sorry
for
> all the back and forth.)

Sorry, I'm not familiar with Linux impl, but seems some tests are actually using
GetXDisplay().
https://cs.chromium.org/search/?q=GetXDisplay+file:test.cc$&type=cs

Probably, we should add something to the test's config to make it usable?

[blundell, 2017-05-22 10:05:40]

I don't understand why this test is passing on all the bots: it fails for me
locally if I run the test with the patch as-is (with the stack trace that Ke He
reported). Ke He, can you verify that the test is running on a Linux trybot, and
if so, replicate the commandline that it's using to run the test and see if that
commandline fails locally for you? Whether it does or doesn't, that would give
us a further point for investigation.

[ke.he, 2017-05-22 10:49:16]

I can see the "service_unittests" did be executed in "linux_chromium_rel_ng"
trybot, but cannot find the commandline it uses.  I'll re-trigger it and see
then.

[ke.he, 2017-05-23 08:33:25]

On 2017/05/22 10:49:16, ke.he wrote:
> I can see the "service_unittests" did be executed in "linux_chromium_rel_ng"
> trybot, but cannot find the commandline it uses.  I'll re-trigger it and see
> then.

Colin, Sorry I still cannot find the commandline in the log of
linux_chromium_rel_ng.  Only a python script "python -u
/b/c/b/linux/src/tools/swarming_client/swarming.py trigger --swarming
https://chromium-swarm.appspot.com ......" found int the log, which I don't
think can be run in my local workstation.

I don't know why it passes within the script "xvfb.py", which might be helpful
information.

[blundell, 2017-05-23 08:39:10]

On 2017/05/23 08:33:25, ke.he wrote:
> On 2017/05/22 10:49:16, ke.he wrote:
> > I can see the "service_unittests" did be executed in "linux_chromium_rel_ng"
> > trybot, but cannot find the commandline it uses.  I'll re-trigger it and see
> > then.
> 
> Colin, Sorry I still cannot find the commandline in the log of
> linux_chromium_rel_ng.  Only a python script "python -u
> /b/c/b/linux/src/tools/swarming_client/swarming.py trigger --swarming
> https://chromium-swarm.appspot.com ......" found int the log, which I don't
> think can be run in my local workstation.
> 
> I don't know why it passes within the script "xvfb.py", which might be helpful
> information.

Hi,

I think you might be looking at the trigger step, which is a different step than
the one of actually running the tests (it's confusing). Here's an example log
from the tests running:

https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.linux...

The command line is there.

[ke.he, 2017-05-23 11:06:55]

On 2017/05/23 08:39:10, blundell wrote:
> On 2017/05/23 08:33:25, ke.he wrote:
> > On 2017/05/22 10:49:16, ke.he wrote:
> > > I can see the "service_unittests" did be executed in
"linux_chromium_rel_ng"
> > > trybot, but cannot find the commandline it uses.  I'll re-trigger it and
see
> > > then.
> > 
> > Colin, Sorry I still cannot find the commandline in the log of
> > linux_chromium_rel_ng.  Only a python script "python -u
> > /b/c/b/linux/src/tools/swarming_client/swarming.py trigger --swarming
> > https://chromium-swarm.appspot.com ......" found int the log, which I don't
> > think can be run in my local workstation.
> > 
> > I don't know why it passes within the script "xvfb.py", which might be
helpful
> > information.
> 
> Hi,
> 
> I think you might be looking at the trigger step, which is a different step
than
> the one of actually running the tests (it's confusing). Here's an example log
> from the tests running:
> 
>
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.linux...
> 
> The command line is there.

In my Ubuntu 16.04, with is_component_build = true, is_debug=false, I tried the
trybot commandline: "./service_unittests --brave-new-test-launcher
--test-launcher-bot-mode
--test-launcher-summary-output=/b/s/w/ioHnJ0Vd/output.json"
Still crashed at the some place DBUS related.  Crash callstack is output into
"output.json" specified by "--test-launcher-summary-output".

By the way the --[option] meanings:

1) --brave-new-test-launcher:
"The bots are now running with --brave-new-test-launcher, the new C++ test
launcher that is intended to be more solid and provide more detailed report from
each test run that can be analyzed later. It is also faster by running tests in
parallel."

2) Strange here, we are in unittest not browser_tests. while there is "xvfb"
again in comments below:

browser_tests also has lots of options which you can see via --help.  Most
useful are:
--test-launcher-bot-mode (used by the bots and is the recommended way to run
browser_tests)
For example, on linux use the following:
xvfb-run -s "-screen 0 1024x768x24" ./out/Release/browser_tests
--test-launcher-bot-mode

Also tried "xvfb-run ./out/*/service_unittests --test-launcher-bot-mode" in my
workstation, All successful.

[scottmg, 2017-05-23 21:18:14]

On 2017/05/23 11:06:55, ke.he wrote:
> On 2017/05/23 08:39:10, blundell wrote:
> > On 2017/05/23 08:33:25, ke.he wrote:
> > > On 2017/05/22 10:49:16, ke.he wrote:
> > > > I can see the "service_unittests" did be executed in
> "linux_chromium_rel_ng"
> > > > trybot, but cannot find the commandline it uses.  I'll re-trigger it and
> see
> > > > then.
> > > 
> > > Colin, Sorry I still cannot find the commandline in the log of
> > > linux_chromium_rel_ng.  Only a python script "python -u
> > > /b/c/b/linux/src/tools/swarming_client/swarming.py trigger --swarming
> > > https://chromium-swarm.appspot.com ......" found int the log, which I
don't
> > > think can be run in my local workstation.
> > > 
> > > I don't know why it passes within the script "xvfb.py", which might be
> helpful
> > > information.
> > 
> > Hi,
> > 
> > I think you might be looking at the trigger step, which is a different step
> than
> > the one of actually running the tests (it's confusing). Here's an example
log
> > from the tests running:
> > 
> >
>
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.linux...
> > 
> > The command line is there.
> 
> In my Ubuntu 16.04, with is_component_build = true, is_debug=false, I tried
the
> trybot commandline: "./service_unittests --brave-new-test-launcher
> --test-launcher-bot-mode
> --test-launcher-summary-output=/b/s/w/ioHnJ0Vd/output.json"
> Still crashed at the some place DBUS related.  Crash callstack is output into
> "output.json" specified by "--test-launcher-summary-output".
> 
> By the way the --[option] meanings:
> 
> 1) --brave-new-test-launcher:
> "The bots are now running with --brave-new-test-launcher, the new C++ test
> launcher that is intended to be more solid and provide more detailed report
from
> each test run that can be analyzed later. It is also faster by running tests
in
> parallel."
> 
> 2) Strange here, we are in unittest not browser_tests. while there is "xvfb"
> again in comments below:
> 
> browser_tests also has lots of options which you can see via --help.  Most
> useful are:
> --test-launcher-bot-mode (used by the bots and is the recommended way to run
> browser_tests)
> For example, on linux use the following:
> xvfb-run -s "-screen 0 1024x768x24" ./out/Release/browser_tests
> --test-launcher-bot-mode
> 
> Also tried "xvfb-run ./out/*/service_unittests --test-launcher-bot-mode" in my
> workstation, All successful.

It doesn't entirely solve the problem, but I (eventually! :) found the buildbot
flag that controls whether xvfb is used. You can see the setting in this
file/CL:
https://codereview.chromium.org/2901123002/diff/20001/testing/buildbot/gn_iso...

[ke.he, 2017-05-24 03:43:30]

As hashimoto mentioned, there is other unittest use the XDisplay too. One
example is //ui/events/events_unittests.

I run "./out/*/events_unittests" directly, then I found lots of crashes.
I run "xvfb-run ./out/rele/events_unittests", all test cases pass.

And as scottmg found, it is trybot's responsibility to set the xvfb environment
(in gn_isolate_map.pyl) , "windowed_test_launcher or console_test_launcher".
By the way, don't know why the "--test-launcher-bot-mode and
--brave-new-test-launcher" could have the same effect as
"windowed_test_launcher".

So I think it is "work as expected" that "windowed_test_launcher" type unittests
crash when running without "xvfb". So if it is, scottmg's CL does solve the
problem entirely.
Or if we think we should keep the service_unittests as "console_test_launcher"
(to avoid it goes crash without "xvfb" especially in local workstation), we have
to change power_save_blocker_x11.cc

WDYT?  Thanks all!

[scottmg, 2017-05-24 03:46:14]

On 2017/05/24 03:43:30, ke.he wrote:
> As hashimoto mentioned, there is other unittest use the XDisplay too. One
> example is //ui/events/events_unittests.
> 
> I run "./out/*/events_unittests" directly, then I found lots of crashes.
> I run "xvfb-run ./out/rele/events_unittests", all test cases pass.
> 
> And as scottmg found, it is trybot's responsibility to set the xvfb
environment
> (in gn_isolate_map.pyl) , "windowed_test_launcher or console_test_launcher".
> By the way, don't know why the "--test-launcher-bot-mode and
> --brave-new-test-launcher" could have the same effect as
> "windowed_test_launcher".
> 
> So I think it is "work as expected" that "windowed_test_launcher" type
unittests
> crash when running without "xvfb". So if it is, scottmg's CL does solve the
> problem entirely.
> Or if we think we should keep the service_unittests as "console_test_launcher"
> (to avoid it goes crash without "xvfb" especially in local workstation), we
have
> to change power_save_blocker_x11.cc
> 
> WDYT?  Thanks all!

I think you should change gn_isolate_map.pyl to windowed_test_launcher, revert
the change to device/power_save_blocker/power_save_blocker_x11.cc and with that,
lgtm.

[ke.he, 2017-05-24 04:00:40]

Scottmg@, Thanks very much!

[ke.he, 2017-05-24 04:14:49]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 04:15:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ke.he, 2017-05-24 04:16:03]

Description was changed from

==========
Add service unittest for WakeLockServiceImpl.

BUG=689410
==========

to

==========
Add service unittest for WakeLockServiceImpl.

Also change the service_unittests from "console_test_launcher" to
"windowed_test_launcher", which means "xvfb" is needed when running.

Commandline on linux:
   xvfb-run ./out/*/service_unittests
or
   python testing/xvfb.py out/*/service_unittests

BUG=689410
==========

[ke.he, 2017-05-24 05:30:33]

ke.he@intel.com changed reviewers:
+ jbudorick@chromium.org

[ke.he, 2017-05-24 05:30:34]

Jbudorick@, PTAL on "testing/buildbot/gn_isolate_map.pyl".
Thanks.

[commit-bot: I haz the power, 2017-05-24 06:09:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-24 06:09:55]

Dry run: This issue passed the CQ dry run.

[blundell, 2017-05-24 13:03:19]

lgtm with question

https://codereview.chromium.org/2883903002/diff/60001/testing/buildbot/gn_iso...
File testing/buildbot/gn_isolate_map.pyl (right):

https://codereview.chromium.org/2883903002/diff/60001/testing/buildbot/gn_iso...
testing/buildbot/gn_isolate_map.pyl:870: "type": "windowed_test_launcher",
What does this change mean wrt running tests manually at the command line?

[jbudorick, 2017-05-24 13:11:09]

//testing/buildbot/ lgtm

https://codereview.chromium.org/2883903002/diff/60001/testing/buildbot/gn_iso...
File testing/buildbot/gn_isolate_map.pyl (right):

https://codereview.chromium.org/2883903002/diff/60001/testing/buildbot/gn_iso...
testing/buildbot/gn_isolate_map.pyl:870: "type": "windowed_test_launcher",
On 2017/05/24 13:03:19, blundell wrote:
> What does this change mean wrt running tests manually at the command line?

Mentioned in the CL description.

[blundell, 2017-05-24 13:15:29]

On 2017/05/24 13:11:09, jbudorick wrote:
> //testing/buildbot/ lgtm
> 
>
https://codereview.chromium.org/2883903002/diff/60001/testing/buildbot/gn_iso...
> File testing/buildbot/gn_isolate_map.pyl (right):
> 
>
https://codereview.chromium.org/2883903002/diff/60001/testing/buildbot/gn_iso...
> testing/buildbot/gn_isolate_map.pyl:870: "type": "windowed_test_launcher",
> On 2017/05/24 13:03:19, blundell wrote:
> > What does this change mean wrt running tests manually at the command line?
> 
> Mentioned in the CL description.

Ah OK, thanks. That's a somewhat unfortunate change to push on to devs :(. Sorry
to bog this review down further, but Ke He, would it be easy to add a test-only
method to WakeLockServiceImpl that just has it cut out from actually creating
the PowerSaveBlocker (or alternatively have a fake impl that it creates, or
something like that)? That seems like it wouldn't alter the spirit of these
tests at all, and it would avoid pushing this complexity to devs.

[ke.he, 2017-05-25 09:17:00]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-25 09:17:14]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ke.he, 2017-05-25 09:20:57]

Hi, Colin,
Yeah I totally agree we should go with mock the WakeLockServiceImpl instead of
adding "xvfb" dependency on service_unittests.

CL is updated, PTAL:)

[ke.he, 2017-05-25 09:21:55]

Description was changed from

==========
Add service unittest for WakeLockServiceImpl.

Also change the service_unittests from "console_test_launcher" to
"windowed_test_launcher", which means "xvfb" is needed when running.

Commandline on linux:
   xvfb-run ./out/*/service_unittests
or
   python testing/xvfb.py out/*/service_unittests

BUG=689410
==========

to

==========
Add service unittest for WakeLockServiceImpl.

To avoid let service unittest depend on "xvfb", we use the
FakeWakeLockServiceImpl which doesn't create the real
PowerSaveBlocker instance during testing.

BUG=689410
==========

[commit-bot: I haz the power, 2017-05-25 10:32:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-25 10:32:59]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[blundell, 2017-05-29 13:17:09]

Thanks, this is along the lines of what I was thinking :).

https://codereview.chromium.org/2883903002/diff/80001/device/wake_lock/fake_w...
File device/wake_lock/fake_wake_lock_service_impl.h (right):

https://codereview.chromium.org/2883903002/diff/80001/device/wake_lock/fake_w...
device/wake_lock/fake_wake_lock_service_impl.h:21: class FakeWakeLockServiceImpl
: public WakeLockServiceImpl {
nit: I'd prefer having WakeLockServiceImpl create the PowerSaveBlocker by a
factory function, and then having the test just define a do-nothing
PowerSaveBlocker and inject construction of that as the factory function.

[ke.he, 2017-05-31 11:38:15]

https://codereview.chromium.org/2883903002/diff/80001/device/wake_lock/fake_w...
File device/wake_lock/fake_wake_lock_service_impl.h (right):

https://codereview.chromium.org/2883903002/diff/80001/device/wake_lock/fake_w...
device/wake_lock/fake_wake_lock_service_impl.h:21: class FakeWakeLockServiceImpl
: public WakeLockServiceImpl {
On 2017/05/29 13:17:08, blundell wrote:
> nit: I'd prefer having WakeLockServiceImpl create the PowerSaveBlocker by a
> factory function, and then having the test just define a do-nothing
> PowerSaveBlocker and inject construction of that as the factory function.

Colin, Sorry I'm not sure If I understanding you correctly or not. 
Do you mean
1) we don't need the FakeWakeLockServiceImpl. And,
2) Add a WakeLockServiceImpl::SetPowerSaveBlockerFactory(), then in production
code we set the factory as WakeLockServiceImpl::CreateWakeLock() while in
testing code set it as CreateDoNothingWakeLock(), right?  In testing code, the
SetPowerSaveBlockerFactory() is still called in WakeLockProvider, after the
WakeLockServiceImpl instance is created, right?  

The type of WakeLockServiceImpl::wake_lock_ is
"std::unique_ptr<PowerSaveBlocker>", so wake_lock_ points to either a real
PowerSaveBlocker instance or nullptr. Then the question is: in the
CreateDoNothingWakeLock(), how can we operate on wake_lock_ to simulate the
PowerSaveBlocker? That's why finally I choose a simpler way, just to create the
FakeWakeLockServiceImpl in which I add a new flag "has_fake_wake_lock_".

Please help give some comments, thanks very much!

[blundell, 2017-06-01 11:35:58]

LGTM. Thanks for the explanation.

https://codereview.chromium.org/2883903002/diff/80001/device/wake_lock/fake_w...
File device/wake_lock/fake_wake_lock_service_impl.cc (right):

https://codereview.chromium.org/2883903002/diff/80001/device/wake_lock/fake_w...
device/wake_lock/fake_wake_lock_service_impl.cc:11:
FakeWakeLockServiceImpl::FakeWakeLockServiceImpl(
nit: Let's call this WakeLockServiceImplForTesting and put it directly in the
unittest.

https://codereview.chromium.org/2883903002/diff/80001/device/wake_lock/fake_w...
device/wake_lock/fake_wake_lock_service_impl.cc:26: has_fake_wake_lock_(false)
{}
nit: Let's just call this |has_wake_lock_|.

[ke.he, 2017-06-04 10:33:45]

https://codereview.chromium.org/2883903002/diff/80001/device/wake_lock/wake_l...
File device/wake_lock/wake_lock_provider.cc (right):

https://codereview.chromium.org/2883903002/diff/80001/device/wake_lock/wake_l...
device/wake_lock/wake_lock_provider.cc:52: new FakeWakeLockServiceImpl(
If we move the "WakeLockServiceImplForTesting" into services/device/wake_lock/,
we'll introduce a dependency on //services/ here.

In future we'll move the whole //device/wake_lock into //services/. How about
just leave it in //device/ for now?

[blundell, 2017-06-05 11:42:14]

On 2017/06/04 10:33:45, ke.he wrote:
>
https://codereview.chromium.org/2883903002/diff/80001/device/wake_lock/wake_l...
> File device/wake_lock/wake_lock_provider.cc (right):
> 
>
https://codereview.chromium.org/2883903002/diff/80001/device/wake_lock/wake_l...
> device/wake_lock/wake_lock_provider.cc:52: new FakeWakeLockServiceImpl(
> If we move the "WakeLockServiceImplForTesting" into
services/device/wake_lock/,
> we'll introduce a dependency on //services/ here.
> 
> In future we'll move the whole //device/wake_lock into //services/. How about
> just leave it in //device/ for now?

OK, makes sense. Let's call it WakeLockServiceImplForTesting though...calling it
FakeWakeLockServiceImpl is confusing considering that it's the thing being
tested :).

[ke.he, 2017-06-06 02:39:27]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-06 02:40:25]

Dry run: 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, 2017-06-06 04:48:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-06 04:48:29]

Dry run: This issue passed the CQ dry run.

[ke.he, 2017-06-06 05:25:57]

Thanks!

https://codereview.chromium.org/2883903002/diff/80001/device/wake_lock/fake_w...
File device/wake_lock/fake_wake_lock_service_impl.cc (right):

https://codereview.chromium.org/2883903002/diff/80001/device/wake_lock/fake_w...
device/wake_lock/fake_wake_lock_service_impl.cc:11:
FakeWakeLockServiceImpl::FakeWakeLockServiceImpl(
On 2017/06/01 11:35:58, blundell wrote:
> nit: Let's call this WakeLockServiceImplForTesting and put it directly in the
> unittest.

Done.

https://codereview.chromium.org/2883903002/diff/80001/device/wake_lock/fake_w...
device/wake_lock/fake_wake_lock_service_impl.cc:26: has_fake_wake_lock_(false)
{}
On 2017/06/01 11:35:58, blundell wrote:
> nit: Let's just call this |has_wake_lock_|.

Done.

[ke.he, 2017-06-06 05:28:33]

The CQ bit was checked by ke.he@intel.com

[ke.he, 2017-06-06 05:28:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
scottmg@chromium.org, jbudorick@chromium.org, blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/2883903002/#ps100001
(title: "Rename Fake* to *ForTesting")

[commit-bot: I haz the power, 2017-06-06 05:28:50]

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, 2017-06-06 05:32:35]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1496726913982370,
"parent_rev": "990c17fa96c3e5d4108cecf602f45ec74627285c", "commit_rev":
"4d8a87f26e4b1027bd9281e4006506ab0f983732"}

[commit-bot: I haz the power, 2017-06-06 05:32:59]

Description was changed from

==========
Add service unittest for WakeLockServiceImpl.

To avoid let service unittest depend on "xvfb", we use the
FakeWakeLockServiceImpl which doesn't create the real
PowerSaveBlocker instance during testing.

BUG=689410
==========

to

==========
Add service unittest for WakeLockServiceImpl.

To avoid let service unittest depend on "xvfb", we use the
FakeWakeLockServiceImpl which doesn't create the real
PowerSaveBlocker instance during testing.

BUG=689410

Review-Url: https://codereview.chromium.org/2883903002
Cr-Commit-Position: refs/heads/master@{#477200}
Committed:
https://chromium.googlesource.com/chromium/src/+/4d8a87f26e4b1027bd9281e40065...
==========

[commit-bot: I haz the power, 2017-06-06 05:33:01]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/4d8a87f26e4b1027bd9281e40065...