TaskScheduler [5/9] Task Tracker

base/task_scheduler/task_tracker_unittest.cc

Index: base/task_scheduler/task_tracker_unittest.cc
diff --git a/base/task_scheduler/task_tracker_unittest.cc b/base/task_scheduler/task_tracker_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..a45f15777d3fccde742d8f0582075f87fc8c2037
--- /dev/null
+++ b/base/task_scheduler/task_tracker_unittest.cc
@@ -0,0 +1,379 @@
+// Copyright 2016 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/task_scheduler/task_tracker.h"
+
+#include "base/bind.h"
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/task_scheduler/task.h"
+#include "base/task_scheduler/task_traits.h"
+#include "base/threading/platform_thread.h"
+#include "base/threading/simple_thread.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace base {

[robliao, 2016/03/01 22:29:42]

I think you need a test case where we block shutdown while a
CONTINUE_ON_SHUTDOWN task is running.

[fdoray, 2016/03/02 00:38:41]

Done. PostAndRunBlockedContinueOnShutdownTaskBeforeShutdown

+namespace internal {
+
+namespace {
+
+class ThreadCallingShutdown : public SimpleThread {
+ public:
+  explicit ThreadCallingShutdown(TaskTracker* tracker)
+      : SimpleThread("ThreadCallingShutdown"),
+        tracker_(tracker),
+        has_returned_(false) {}
+
+  // Returns true once the call to Shutdown() has returned.
+  bool has_returned() const { return has_returned_; }
+
+ private:
+  void Run() override {
+    tracker_->Shutdown();
+    has_returned_ = true;
+  }
+
+  TaskTracker* tracker_;
+  bool has_returned_;
+
+  DISALLOW_COPY_AND_ASSIGN(ThreadCallingShutdown);
+};
+
+class TaskSchedulerTaskTrackerTest : public testing::Test {
+ protected:
+  TaskSchedulerTaskTrackerTest() : num_run_tasks_(0) {}
+
+  // Creates a task with |shutdown_behavior|.
+  scoped_ptr<Task> CreateTask(TaskShutdownBehavior shutdown_behavior) {
+    return make_scoped_ptr(new Task(
+        FROM_HERE,
+        Bind(&TaskSchedulerTaskTrackerTest::RunTaskCallback, Unretained(this)),
+        TaskTraits().WithShutdownBehavior(shutdown_behavior)));
+  }
+
+  // Tries to post |task| via |tracker_|. If |tracker_| approves the operation,
+  // returns the posted task. Otherwise, returns nullptr.
+  scoped_ptr<Task> PostTaskViaTracker(scoped_ptr<Task> task) {
+    scoped_ptr<Task> posted_task;
+    tracker_.PostTask(Bind(&TaskSchedulerTaskTrackerTest::PostTaskCallback,
+                           Unretained(this), &posted_task),
+                      std::move(task));
+    return posted_task;

[gab, 2016/03/01 22:18:47]

This feels very weird... how about instead have a fake queue here, e.g. 

std::dequeue task_queue_; accompanied by an AddToQueue() method used in the Bind
here?

And tests can test the top of the queue instead of doing the raw pointer to
returned pointer comparison.

[fdoray, 2016/03/02 00:38:41]

Done. I now use an std::queue.

+  }
+
+  // Calls tracker_->Shutdown() on a new thread. When this returns, the
+  // Shutdown() method has been entered on the new thread, but it hasn't
+  // necessarily returned.
+  void CallShutdownAsync() {
+    DCHECK(!thread_calling_shutdown_.get());
+    thread_calling_shutdown_.reset(new ThreadCallingShutdown(&tracker_));
+    thread_calling_shutdown_->Start();
+    while (!tracker_.is_shutting_down_for_testing() &&
+           !tracker_.shutdown_completed()) {
+      PlatformThread::YieldCurrentThread();
+    }
+  }
+
+  void ExpectShutdownAsyncHasReturned() {
+    DCHECK(thread_calling_shutdown_.get());
+    thread_calling_shutdown_->Join();

[robliao, 2016/03/01 22:29:42]

It would be unexpected to join with the duality between
ExpectShutdownAsyncHas[Not]Returned.
Maybe JoinAndExpectShutdownAsyncHasReturned?

[fdoray, 2016/03/02 00:38:41]

Done.

+    EXPECT_TRUE(thread_calling_shutdown_->has_returned());
+    EXPECT_TRUE(tracker_.shutdown_completed());
+  }
+
+  void ExpectShutdownAsyncHasNotReturned() {
+    DCHECK(thread_calling_shutdown_.get());
+    EXPECT_FALSE(thread_calling_shutdown_->has_returned());
+    EXPECT_FALSE(tracker_.shutdown_completed());
+  }
+
+  TaskTracker tracker_;
+  size_t num_run_tasks_;

[gab, 2016/03/01 22:18:47]

num_tasks_executed_ ?

[fdoray, 2016/03/02 00:38:41]

Done.

+
+ private:
+  void PostTaskCallback(scoped_ptr<Task>* out_task, scoped_ptr<Task> in_task) {
+    *out_task = std::move(in_task);
+  }
+
+  void RunTaskCallback() { ++num_run_tasks_; }
+
+  scoped_ptr<ThreadCallingShutdown> thread_calling_shutdown_;
+
+  DISALLOW_COPY_AND_ASSIGN(TaskSchedulerTaskTrackerTest);
+};
+
+}  // namespace
+
+TEST_F(TaskSchedulerTaskTrackerTest,
+       PostAndRunBeforeShutdown_ContinueOnShutdown) {
+  scoped_ptr<Task> task_to_post(
+      CreateTask(TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN));
+  const Task* task_to_post_raw = task_to_post.get();
+
+  // Post the task.
+  scoped_ptr<Task> posted_task(PostTaskViaTracker(std::move(task_to_post)));
+  ASSERT_EQ(task_to_post_raw, posted_task.get());
+
+  // Run the posted task.
+  tracker_.RunTask(posted_task.get());
+  EXPECT_EQ(1U, num_run_tasks_);

[gab, 2016/03/01 22:18:47]

EXPECT_EQ(0U, ...); as well before RunTask()?

[fdoray, 2016/03/02 00:38:41]

Done.

+
+  // Shutdown() shouldn't block.
+  tracker_.Shutdown();
+}
+
+TEST_F(TaskSchedulerTaskTrackerTest, PostAndRunBeforeShutdown_SkipOnShutdown) {
+  scoped_ptr<Task> task_to_post(
+      CreateTask(TaskShutdownBehavior::SKIP_ON_SHUTDOWN));
+  const Task* task_to_post_raw = task_to_post.get();
+
+  // Post the task.
+  scoped_ptr<Task> posted_task(PostTaskViaTracker(std::move(task_to_post)));
+  ASSERT_EQ(task_to_post_raw, posted_task.get());
+
+  // Run the posted task.
+  tracker_.RunTask(posted_task.get());
+  EXPECT_EQ(1U, num_run_tasks_);
+
+  // Shutdown() shouldn't block.
+  tracker_.Shutdown();
+}
+
+TEST_F(TaskSchedulerTaskTrackerTest, PostAndRunBeforeShutdown_BlockShutdown) {
+  scoped_ptr<Task> task_to_post(
+      CreateTask(TaskShutdownBehavior::BLOCK_SHUTDOWN));
+  const Task* task_to_post_raw = task_to_post.get();
+
+  // Post the task.
+  scoped_ptr<Task> posted_task(PostTaskViaTracker(std::move(task_to_post)));
+  ASSERT_EQ(task_to_post_raw, posted_task.get());
+
+  // Run the posted task.
+  tracker_.RunTask(posted_task.get());
+  EXPECT_EQ(1U, num_run_tasks_);
+
+  // Shutdown() shouldn't block.
+  tracker_.Shutdown();
+}
+
+TEST_F(TaskSchedulerTaskTrackerTest,
+       PostBeforeShutdownRunDuringShutdown_ContinueOnShutdown) {
+  scoped_ptr<Task> task_to_post(
+      CreateTask(TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN));
+  const Task* task_to_post_raw = task_to_post.get();
+
+  // Post the task.
+  scoped_ptr<Task> posted_task(PostTaskViaTracker(std::move(task_to_post)));
+  ASSERT_EQ(task_to_post_raw, posted_task.get());
+
+  // Post a BLOCK_SHUTDOWN task just to block shutdown. Then, call Shutdown()
+  // asynchronously.
+  scoped_ptr<Task> blocking_task(
+      PostTaskViaTracker(CreateTask(TaskShutdownBehavior::BLOCK_SHUTDOWN)));
+  CallShutdownAsync();
+  ExpectShutdownAsyncHasNotReturned();
+
+  // Try to run |posted_task| via |tracker_|. It should not run.
+  tracker_.RunTask(posted_task.get());
+  EXPECT_EQ(0U, num_run_tasks_);
+  ExpectShutdownAsyncHasNotReturned();
+
+  // Unblock shutdown by running the BLOCK_SHUTDOWN task.
+  tracker_.RunTask(blocking_task.get());
+  EXPECT_EQ(1U, num_run_tasks_);
+  ExpectShutdownAsyncHasReturned();
+}
+
+TEST_F(TaskSchedulerTaskTrackerTest,
+       PostBeforeShutdownRunDuringShutdown_SkipOnShutdown) {
+  scoped_ptr<Task> task_to_post(
+      CreateTask(TaskShutdownBehavior::SKIP_ON_SHUTDOWN));
+  const Task* task_to_post_raw = task_to_post.get();
+
+  // Post the task.
+  scoped_ptr<Task> posted_task(PostTaskViaTracker(std::move(task_to_post)));
+  ASSERT_EQ(task_to_post_raw, posted_task.get());
+
+  // Post a BLOCK_SHUTDOWN task just to block shutdown. Then, call Shutdown()
+  // asynchronously.
+  scoped_ptr<Task> blocking_task(
+      PostTaskViaTracker(CreateTask(TaskShutdownBehavior::BLOCK_SHUTDOWN)));
+  CallShutdownAsync();
+  ExpectShutdownAsyncHasNotReturned();
+
+  // Try to run |posted_task| via |tracker_|. It should not run.
+  tracker_.RunTask(posted_task.get());
+  EXPECT_EQ(0U, num_run_tasks_);
+  ExpectShutdownAsyncHasNotReturned();
+
+  // Unblock shutdown by running the BLOCK_SHUTDOWN task.
+  tracker_.RunTask(blocking_task.get());
+  EXPECT_EQ(1U, num_run_tasks_);
+  ExpectShutdownAsyncHasReturned();
+}
+
+TEST_F(TaskSchedulerTaskTrackerTest,
+       PostBeforeShutdownRunDuringShutdown_BlockShutdown) {
+  scoped_ptr<Task> task_to_post(
+      CreateTask(TaskShutdownBehavior::BLOCK_SHUTDOWN));
+  const Task* task_to_post_raw = task_to_post.get();
+
+  // Post the task.
+  scoped_ptr<Task> posted_task(PostTaskViaTracker(std::move(task_to_post)));
+  ASSERT_EQ(task_to_post_raw, posted_task.get());
+
+  // Call Shutdown() asynchronously. It should block before there is a pending
+  // BLOCK_SHUTDOWN task.
+  CallShutdownAsync();
+  ExpectShutdownAsyncHasNotReturned();
+
+  // Run the posted task.
+  tracker_.RunTask(posted_task.get());
+  EXPECT_EQ(1U, num_run_tasks_);
+
+  // The async call to Shutdown() should now return.
+  ExpectShutdownAsyncHasReturned();
+}
+
+TEST_F(TaskSchedulerTaskTrackerTest,
+       PostBeforeShutdownRunAfterShutdown_ContinueOnShutdown) {
+  scoped_ptr<Task> task_to_post(
+      CreateTask(TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN));
+  const Task* task_to_post_raw = task_to_post.get();
+
+  // Post the task.
+  scoped_ptr<Task> posted_task(PostTaskViaTracker(std::move(task_to_post)));
+  ASSERT_EQ(task_to_post_raw, posted_task.get());
+
+  // A call to Shutdown() should return immediately.
+  tracker_.Shutdown();
+
+  // Try to run |posted_task| via |tracker_|. It should not run.
+  tracker_.RunTask(posted_task.get());
+  EXPECT_EQ(0U, num_run_tasks_);
+}
+
+TEST_F(TaskSchedulerTaskTrackerTest,
+       PostBeforeShutdownRunAfterShutdown_SkipOnShutdown) {
+  scoped_ptr<Task> task_to_post(
+      CreateTask(TaskShutdownBehavior::SKIP_ON_SHUTDOWN));
+  const Task* task_to_post_raw = task_to_post.get();
+
+  // Post the task.
+  scoped_ptr<Task> posted_task(PostTaskViaTracker(std::move(task_to_post)));
+  ASSERT_EQ(task_to_post_raw, posted_task.get());
+
+  // A call to Shutdown() should return immediately.
+  tracker_.Shutdown();
+
+  // Try to run |posted_task| via |tracker_|. It should not run.
+  tracker_.RunTask(posted_task.get());
+  EXPECT_EQ(0U, num_run_tasks_);
+}
+
+TEST_F(TaskSchedulerTaskTrackerTest,
+       PostBeforeShutdownRunAfterShutdown_BlockShutdown) {

[robliao, 2016/03/01 22:29:42]

I'm not sure this test should be here as it's not really testing anything. I can
see why it would be here though. Maybe a comment without the test annotation is
sufficient.

[fdoray, 2016/03/02 00:38:41]

Now I test that we get the expected failure with EXPECT_DEBUG_DEATH.

+  // It is not possible to run after shutdown a BLOCK_SHUTDOWN task that has
+  // been posted before shutdown because Shutdown() won't return when there are
+  // pending BLOCK_SHUTDOWN tasks.

[gab, 2016/03/01 22:18:47]

I think your code actually handles this (+ a DCHECK) so it should be test this
with a DEBUG_DEATH test, no?

[fdoray, 2016/03/02 00:38:41]

Done.

+}
+
+TEST_F(TaskSchedulerTaskTrackerTest,
+       PostAndRunDuringShutdown_ContinueOnShutdown) {
+  // Post a BLOCK_SHUTDOWN task just to block shutdown. Then, call Shutdown()
+  // asynchronously.
+  scoped_ptr<Task> blocking_task(
+      PostTaskViaTracker(CreateTask(TaskShutdownBehavior::BLOCK_SHUTDOWN)));
+  CallShutdownAsync();
+  ExpectShutdownAsyncHasNotReturned();
+
+  // Try to post a CONTINUE_ON_SHUTDOWN task via |tracker_|. It should not work.
+  EXPECT_EQ(nullptr, PostTaskViaTracker(
+                         CreateTask(TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN))
+                         .get());
+
+  // Since |tracker_| hasn't allowed the task to be posted, it can't be run.
+
+  // Unblock shutdown by running the BLOCK_SHUTDOWN task.
+  ExpectShutdownAsyncHasNotReturned();
+  tracker_.RunTask(blocking_task.get());
+  EXPECT_EQ(1U, num_run_tasks_);
+  ExpectShutdownAsyncHasReturned();
+}
+
+TEST_F(TaskSchedulerTaskTrackerTest, PostAndRunDuringShutdown_SkipOnShutdown) {
+  // Post a BLOCK_SHUTDOWN task just to block shutdown. Then, call Shutdown()
+  // asynchronously.
+  scoped_ptr<Task> blocking_task(
+      PostTaskViaTracker(CreateTask(TaskShutdownBehavior::BLOCK_SHUTDOWN)));
+  CallShutdownAsync();
+  ExpectShutdownAsyncHasNotReturned();
+
+  // Try to post a SKIP_ON_SHUTDOWN task via |tracker_|. It should not work.
+  EXPECT_EQ(nullptr, PostTaskViaTracker(
+                         CreateTask(TaskShutdownBehavior::SKIP_ON_SHUTDOWN))
+                         .get());
+
+  // Since |tracker_| hasn't allowed the task to be posted, it can't be run.
+
+  // Unblock shutdown by running the BLOCK_SHUTDOWN task.
+  ExpectShutdownAsyncHasNotReturned();
+  tracker_.RunTask(blocking_task.get());
+  EXPECT_EQ(1U, num_run_tasks_);
+  ExpectShutdownAsyncHasReturned();
+}
+
+TEST_F(TaskSchedulerTaskTrackerTest, PostAndRunDuringShutdown_BlockShutdown) {
+  // Post a BLOCK_SHUTDOWN task just to block shutdown. Then, call Shutdown()
+  // asynchronously.
+  scoped_ptr<Task> blocking_task(
+      PostTaskViaTracker(CreateTask(TaskShutdownBehavior::BLOCK_SHUTDOWN)));
+  CallShutdownAsync();
+  ExpectShutdownAsyncHasNotReturned();
+
+  // Post a BLOCK_SHUTDOWN task via |tracker_|.
+  scoped_ptr<Task> task_to_post(
+      CreateTask(TaskShutdownBehavior::BLOCK_SHUTDOWN));
+  const Task* task_to_post_raw = task_to_post.get();
+  scoped_ptr<Task> posted_task(PostTaskViaTracker(std::move(task_to_post)));
+  ASSERT_EQ(task_to_post_raw, posted_task.get());
+
+  // Run |posted_task| via |tracker_|.
+  tracker_.RunTask(posted_task.get());
+  EXPECT_EQ(1U, num_run_tasks_);
+  ExpectShutdownAsyncHasNotReturned();
+
+  // Unblock shutdown by running the BLOCK_SHUTDOWN task posted at the beginning
+  // of the test.
+  tracker_.RunTask(blocking_task.get());
+  EXPECT_EQ(2U, num_run_tasks_);
+  ExpectShutdownAsyncHasReturned();
+}
+
+TEST_F(TaskSchedulerTaskTrackerTest, PostAfterShutdown_ContinueOnShutdown) {
+  // It is not possible to post a task after shutdown.
+  tracker_.Shutdown();
+  EXPECT_EQ(nullptr, PostTaskViaTracker(
+                         CreateTask(TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN))
+                         .get());
+}
+
+TEST_F(TaskSchedulerTaskTrackerTest, PostAfterShutdown_SkipOnShutdown) {
+  // It is not possible to post a task after shutdown.
+  tracker_.Shutdown();
+  EXPECT_EQ(nullptr, PostTaskViaTracker(
+                         CreateTask(TaskShutdownBehavior::SKIP_ON_SHUTDOWN))
+                         .get());
+}
+
+TEST_F(TaskSchedulerTaskTrackerTest, PostAfterShutdown_BlockShutdown) {
+  // It is not possible to post a task after shutdown.
+  tracker_.Shutdown();
+  EXPECT_EQ(nullptr,
+            PostTaskViaTracker(CreateTask(TaskShutdownBehavior::BLOCK_SHUTDOWN))
+                .get());
+}

[gab, 2016/03/01 22:18:47]

To avoid the triad duplication for every test, how about making
TaskSchedulerTaskTrackerTest a parametrized test with
testing::WithParamInterface<TaskShutdownBehavior>.

Then you can do INSTANTIATE_TEST_CASE_P to get each test to run once in each
mode and use GetParam() to figure out which mode you're in right now to drive
the little difference in behavior and expectations for each mode?

[robliao, 2016/03/01 22:36:41]

Duplication for tests should be okay. Given that we're only going to be writing
these once, it's easier to see bugs in the expanded form.

[fdoray, 2016/03/02 00:38:41]

I think it's a good idea to use a typed test to reduce code duplication. It
requires a lot of effort to try to find the differences between the tests for
different shutdown behaviors. With a typed test, the differences become obvious.

[gab, 2016/03/09 21:53:25]

+1 to Francois' argument, readability is improved when the difference is
highlighted + less code duplication == win/win :-)

+
+}  // namespace internal
+}  // namespace base