Fixing and enhancing OrderedSimpleTaskRunner to allow 100% deterministic tests.

cc/test/ordered_simple_task_runner.h

Index: cc/test/ordered_simple_task_runner.h
diff --git a/cc/test/ordered_simple_task_runner.h b/cc/test/ordered_simple_task_runner.h
index 8850a760bf88de7fb7f7df663324271809d3e8c2..38809edec1cabed1fc6cdf0c2c5a01ca62cf505a 100644
--- a/cc/test/ordered_simple_task_runner.h
+++ b/cc/test/ordered_simple_task_runner.h
@@ -5,24 +5,205 @@
 #ifndef CC_TEST_ORDERED_SIMPLE_TASK_RUNNER_H_
 #define CC_TEST_ORDERED_SIMPLE_TASK_RUNNER_H_
 
+#include <limits>
+#include <set>
+#include <vector>
+
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
+#include "base/debug/trace_event.h"
+#include "base/logging.h"
 #include "base/test/test_simple_task_runner.h"
+#include "cc/test/test_now_source.h"
 
 namespace cc {
 
+// Subclass of TestPendingTask which has a unique ID for every task, supports
+// being used inside a std::set and has debug tracing support.
+class TestOrderablePendingTask : public base::TestPendingTask {
+ public:
+  TestOrderablePendingTask();
+  TestOrderablePendingTask(const tracked_objects::Location& location,
+                           const base::Closure& task,
+                           base::TimeTicks post_time,
+                           base::TimeDelta delay,
+                           TestNestability nestability);
+  ~TestOrderablePendingTask();
+
+  // operators needed by std::set and comparision

[Sami, 2014/08/29 13:13:52]

s/comparision/comparison/

[mithro-old, 2014/09/01 06:19:46]

Done.

+  bool operator==(const TestOrderablePendingTask& other) const;
+  bool operator<(const TestOrderablePendingTask& other) const;
+
+  // debug tracing functions
+  scoped_refptr<base::debug::ConvertableToTraceFormat> AsValue() const;
+  void AsValueInto(base::debug::TracedValue* state) const;
+
+ private:
+  static size_t task_id_counter;
+  const size_t task_id_;
+};
+
 // This runs pending tasks based on task's post_time + delay.
 // We should not execute a delayed task sooner than some of the queued tasks
 // which don't have a delay even though it is queued early.
-class OrderedSimpleTaskRunner : public base::TestSimpleTaskRunner {
+class OrderedSimpleTaskRunner : public base::SingleThreadTaskRunner {
  public:
   OrderedSimpleTaskRunner();
+  OrderedSimpleTaskRunner(scoped_refptr<TestNowSource> now_src,
+                          bool advance_now);
+
+  // base::TestSimpleTaskRunner implementation:
+  virtual bool PostDelayedTask(const tracked_objects::Location& from_here,
+                               const base::Closure& task,
+                               base::TimeDelta delay) OVERRIDE;
+  virtual bool PostNonNestableDelayedTask(
+      const tracked_objects::Location& from_here,
+      const base::Closure& task,
+      base::TimeDelta delay) OVERRIDE;
+
+  virtual bool RunsTasksOnCurrentThread() const OVERRIDE;
+
+  // Base class for checks which control how many of the pending tasks the
+  // OrderedSimpleTaskRunner will run.
+  class RunCheck {
+   public:
+    virtual ~RunCheck() {}
+
+    virtual bool Check() = 0;
+
+    // This callback is only valid as long as this object continues to exist.
+    base::Callback<bool(void)> AsCallback() const;

[brianderson, 2014/08/28 22:15:16]

Why is this needed?

[mithro-old, 2014/09/01 06:19:46]

Removed now.

+  };
+
+  // Base class for checks which need access to the next pending task.
+  class RunCheckNeedsNextTask : public RunCheck {
+   public:
+    explicit RunCheckNeedsNextTask(OrderedSimpleTaskRunner* task_runner);
+    virtual ~RunCheckNeedsNextTask() {}
+
+    virtual bool Check() OVERRIDE;

[Sami, 2014/08/29 13:13:51]

It might be worth adding a comment saying when Check() is called (so that
RunNTasks is guaranteed to work). Also document the return value.

[mithro-old, 2014/09/01 06:19:46]

Done.

+
+   protected:
+    virtual bool CheckWithTask(const TestOrderablePendingTask& next_task) = 0;
+
+   private:
+    OrderedSimpleTaskRunner* task_runner_;
+  };
+
+  // Keep running until the given number of tasks have run.
+  // You generally shouldn't use this check as it will cause your tests to fail
+  // when code is changed adding a new task. It is useful as a "timeout" type
+  // solution.
+  class RunNTasks : public RunCheck {
+   public:
+    explicit RunNTasks(size_t number);
+    virtual ~RunNTasks();
+
+    virtual bool Check() OVERRIDE;
+
+   private:
+    size_t tasks_run_;
+    size_t tasks_to_run_;
+  };
 
-  virtual void RunPendingTasks() OVERRIDE;
+  // Keep running until a task which didn't exist initially would run.
+  class RunOnlyExisting : public RunCheckNeedsNextTask {
+   public:
+    explicit RunOnlyExisting(OrderedSimpleTaskRunner* task_runner);
+    virtual ~RunOnlyExisting();
+
+   protected:
+    virtual bool CheckWithTask(
+        const TestOrderablePendingTask& next_task) OVERRIDE;
+
+   private:
+    const std::set<TestOrderablePendingTask> pending_tasks_;
+  };
+
+  // Base class for run checks which compare time
+  class RunCheckNeedsNextTaskTime : public RunCheckNeedsNextTask {
+   public:
+    RunCheckNeedsNextTaskTime(OrderedSimpleTaskRunner* task_runner,
+                              scoped_refptr<TestNowSource> now_src);
+    virtual ~RunCheckNeedsNextTaskTime();
+
+   protected:
+    virtual bool CheckWithTask(
+        const TestOrderablePendingTask& next_task) OVERRIDE;
+    virtual bool CheckWithTaskTime(base::TimeTicks now,
+                                   base::TimeTicks next_task) = 0;
+
+    scoped_refptr<TestNowSource> now_src_;
+  };
+
+  // Stop running tasks when TimeToRun() >= stop_at
+  class RunOnlyBefore : public RunCheckNeedsNextTaskTime {

[brianderson, 2014/08/28 22:15:16]

This class is 4 levels of inheritance deep! My head hurts =P. Please limit it to
1. 

Even though there will be more code duplication, I think it'll help remove a lot
of the boilerplate and indirection of the inheritance.

[Sami, 2014/08/29 13:13:51]

Yeah, I think we could just have one Check() that is given all the parameters
any one of these needs.

[mithro-old, 2014/09/01 06:19:46]

After some thinking, I refactored this code to just use
base::Callback/base::Closure directly rather than a micro-class type system. The
micro classes where only really advantageous for making the "RunNTasks /
RunOnlyExistingTasks" storage explicit.

+   public:
+    RunOnlyBefore(OrderedSimpleTaskRunner* task_runner,
+                  scoped_refptr<TestNowSource> now_src,
+                  base::TimeTicks stop_at);
+    virtual ~RunOnlyBefore();
+
+   protected:
+    virtual bool CheckWithTaskTime(base::TimeTicks now,
+                                   base::TimeTicks next_task) OVERRIDE;
+
+   private:
+    base::TimeTicks stop_at_;
+  };
+
+  // Advance Now() to the next task to run.
+  class RunAdvanceNow : public RunCheckNeedsNextTaskTime {
+   public:
+    RunAdvanceNow(OrderedSimpleTaskRunner* task_runner,
+                  scoped_refptr<TestNowSource> now_src);
+    virtual ~RunAdvanceNow();
+
+   protected:
+    virtual bool CheckWithTaskTime(base::TimeTicks now,
+                                   base::TimeTicks next_task) OVERRIDE;
+  };
+
+  //
+  static const size_t absolute_max_tasks = std::numeric_limits<size_t>::max();
+  void SetTimeout(size_t max_tasks) { max_tasks_ = max_tasks; }

[Sami, 2014/08/29 13:13:51]

Instead of adding this arbitrary limit, should we just rely on the overall test
timing out like we do now?

[mithro-old, 2014/09/01 06:19:46]

This makes our tests fail much faster when things are broken, faster failing
tests are good :)

+  void ClearTimeout() { max_tasks_ = absolute_max_tasks; }
+
+  bool RunPendingTasks();
+  bool RunUntilIdle();
+  bool RunUntilTime(base::TimeTicks time);
+  bool RunForPeriod(base::TimeDelta period);
+
+  bool RunTasksUntil(const RunCheck& check_callback);

[Sami, 2014/08/29 13:13:51]

Do we need all of these entrypoints? How about adding the ones we need initially
and introducing more as we go on? Otherwise I'm not sure we can still call this
a simple task runner :)

[mithro-old, 2014/09/01 06:19:46]

Drastically simplified "RunTaskUntil" function with the base::Callback change.

The convenience functions (RunPendingTasks, RunUntilIdle, RunUntilTime and
RunForPeriod) are all used by the scheduler_unittest.

It would be nice to see RunPendingTasks go away (because its usage leads to
fragile tests and is almost always not what you want) but that requires more
work on the scheduler_unittest side.

[Sami, 2014/09/01 11:35:32]

Is that in a future patch or did I miss it? It feels like we should avoid adding
code that we're not using especially since this patch is already pretty big.
 

+  bool RunTasksUntil(RunCheck* check_callback);
+  bool RunTasksUntil(base::Callback<bool(void)> check_callback);
+
+  bool RunTasksUntil(const std::vector<RunCheck*> check_callbacks);
+  bool RunTasksUntil(
+      const std::vector<base::Callback<bool(void)> > check_callbacks);
+
+  void SetAutoAdvanceNowToPendingTasks(bool advance_now) {
+    advance_now_ = advance_now;
+  }
+  base::TimeDelta DelayToNextPendingTask();
+
+  // base::debug tracing functionality
+  scoped_refptr<base::debug::ConvertableToTraceFormat> AsValue() const;
+  virtual void AsValueInto(base::debug::TracedValue* state) const;
 
  protected:
   virtual ~OrderedSimpleTaskRunner();
 
+  base::ThreadChecker thread_checker_;
+
+  bool advance_now_;
+  scoped_refptr<TestNowSource> now_src_;

[Sami, 2014/08/29 13:13:52]

scoped_ptr?

[mithro-old, 2014/09/01 06:19:46]

See above.

+
+  size_t max_tasks_;
+
+  bool inside_run_tasks_until_;
+  std::set<TestOrderablePendingTask> pending_tasks_;
+
  private:
   DISALLOW_COPY_AND_ASSIGN(OrderedSimpleTaskRunner);
 };