|
|
DescriptionAdd 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
Patch Set 1 #
Total comments: 9
Patch Set 2 : Addressed Colin's review comments #
Total comments: 13
Patch Set 3 : Add service unittest for WakeLockServiceImpl. #Patch Set 4 : "console_test_launcher" -> "windowed_test_launcher" #
Total comments: 2
Patch Set 5 : Add FakeWakeLockServiceImpl. #
Total comments: 7
Patch Set 6 : Rename Fake* to *ForTesting #Messages
Total messages: 69 (35 generated)
The CQ bit was checked by ke.he@intel.com 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ke.he@intel.com changed reviewers: + blundell@chromium.org
Colin, PTAL. Thanks!
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.
The CQ bit was checked by ke.he@intel.com 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...
ke.he@intel.com changed reviewers: + scottmg@chromium.org
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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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?
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?
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 :).
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.
> 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
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!
Description was changed from ========== Add service unittest for WakeLockServiceImpl. BUG=689410 ========== to ========== Add service unittest for WakeLockServiceImpl. BUG=689410 ==========
scottmg@chromium.org changed reviewers: + hashimoto@chromium.org
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.)
The CQ bit was checked by ke.he@intel.com to run a CQ dry run
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.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
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?
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.
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.
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.
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.
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.
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...
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!
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.
Scottmg@, Thanks very much!
The CQ bit was checked by ke.he@intel.com 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...
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@intel.com changed reviewers: + jbudorick@chromium.org
Jbudorick@, PTAL on "testing/buildbot/gn_isolate_map.pyl". Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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?
//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.
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.
The CQ bit was checked by ke.he@intel.com 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...
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:)
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 ==========
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
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.
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!
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_|.
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?
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 :).
The CQ bit was checked by ke.he@intel.com 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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.
The CQ bit was checked by ke.he@intel.com
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")
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": 100001, "attempt_start_ts": 1496726913982370, "parent_rev": "990c17fa96c3e5d4108cecf602f45ec74627285c", "commit_rev": "4d8a87f26e4b1027bd9281e4006506ab0f983732"}
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/4d8a87f26e4b1027bd9281e40065... |