C++ readability review

components/timers/alarm_timer_unittest.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/macros.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "base/threading/platform_thread.h"
 #include "base/time/time.h"
 #include "components/timers/alarm_timer.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 // Most of these tests have been lifted right out of timer_unittest.cc with only
 // cosmetic changes (like replacing calls to MessageLoop::current()->Run() with
 // a RunLoop).  We want the AlarmTimer to be a drop-in replacement for the
 // regular Timer so it should pass the same tests as the Timer class.

[gromer, 2014/12/05 20:13:41]

GUnit has mechanisms for type-parameterized tests, so you can run the same test
code on different types. Could you use that to avoid duplicating all this code
from the base::Timer tests?

[Chirantan Ekbote, 2014/12/31 00:58:29]

Yes, there is a bug open for me to fix this: crbug.com/426647.

 //
 // The only new tests are the .*ConcurrentResetAndTimerFired tests, which test
 // that race conditions that can come up in the AlarmTimer::Delegate are
 // properly handled.
 namespace timers {
+
 namespace {
 // The message loops on which each timer should be tested.
 const base::MessageLoop::Type testing_message_loops[] = {
   base::MessageLoop::TYPE_DEFAULT,
   base::MessageLoop::TYPE_IO,
 #if !defined(OS_IOS)  // iOS does not allow direct running of the UI loop.
   base::MessageLoop::TYPE_UI,
 #endif
 };
 
@@ skipping 378 matching lines @@
 
 
 namespace {
 void TimerRanCallback(bool* did_run) {
   *did_run = true;
 
   base::MessageLoop::current()->PostTask(
       FROM_HERE, base::MessageLoop::QuitWhenIdleClosure());
 }
 
 void RunTest_OneShotTimerConcurrentResetAndTimerFired(

[gromer, 2014/12/05 20:13:41]

Test code tends to work better in the top-level test function than in
subroutines. This function only has one caller, so could you just replace that
function call with the function implementation?

[Chirantan Ekbote, 2014/12/31 00:58:29]

The issue is that the same test needs to run on multiple types of MessageLoops,
which is why this function takes a base::MessageLoop::Type as a parameter. 
Moving it into the top-level function would create unnecessary duplication of
the code.  I suppose this could be turned into a value-parameterized test but
I'm not sure how that would work in combination with also being a
type-parameterized test.

[gromer, 2015/02/10 23:47:10]

I don't understand- this function only has one caller, so where would the code
duplication come from?

[Chirantan Ekbote, 2015/02/12 01:49:58]

That caller calls it multiple times with a different type of message loop each
time.  If we were to move each separate invocation into a top-level function,
the only thing that would change in the code is |message_loop_type|. 

I already have some ideas on how I can turn this whole thing into a
value-parameterized test so this shouldn't be an issue for much longer.

[gromer, 2015/02/17 22:02:08]

It's called multiple times, but inside a loop; there's still only one place in
the _source code_ where it's called, so there would be no code duplication. I'm
just saying put the body of this function inside the loop, instead of the
function call.

[Chirantan Ekbote, 2015/02/25 03:32:56]

I see.  Sorry, I misunderstood what you were saying.  This is done now.

     base::MessageLoop::Type message_loop_type) {
   base::MessageLoop loop(message_loop_type);
 
   timers::AlarmTimer timer(false, false);
   bool did_run = false;
 
   timer.Start(
       FROM_HERE, kTenMilliseconds, base::Bind(&TimerRanCallback, &did_run));
 
   // Sleep twice as long as the timer to ensure that the timer task gets queued.
   base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(20));
 
   // Now reset the timer.  This is attempting to simulate the timer firing and
   // being reset at the same time.  The previously queued task should be

[gromer, 2014/12/05 20:13:41]

Not quite at the same time, right? The test code seems to assume that the reset
is handled before the timer firing.

[Chirantan Ekbote, 2014/12/31 00:58:29]

When the timer fires, it queues a task in the MessageLoop that started it.  That
task will not run until the current function returns.  The situation being
tested is as follows:

- The timer is started on a MessageLoop that is not a MessageLoopForIO.  So the
timer sets up the system-level timer on a separate thread that does have a
MessageLoopForIO.
- The requested time elapses and the MessageLoopForIO posts a task back to the
MessageLoop that started the timer to run the user task.
- At the same time, the original MessageLoop is in the middle of a function that
resets the timer.

In this case we don't actually want the timer to fire and while we can't
recreate the situation we can recreate the end result, which is that a task was
posted on the original message loop and the timer was reset, cancelling that
task.

   // removed.
   timer.Reset();
 
   base::RunLoop().RunUntilIdle();
   EXPECT_FALSE(did_run);
 
   // If the previous check failed, running the message loop again will hang the
   // test so we only do it if the callback has not run yet.
   if (!did_run) {
     base::RunLoop().Run();
@@ skipping 40 matching lines @@
 TEST(AlarmTimerTest, RepeatingTimerConcurrentResetAndTimerFired) {
   for (int i = 0; i < kNumTestingMessageLoops; i++) {
     RunTest_RepeatingTimerConcurrentResetAndTimerFired(
         testing_message_loops[i]);
   }
 }
 
 }  // namespace
 
 }  // namespace timers