TaskScheduler [13] TaskSchedulerImpl

base/task_scheduler/task_scheduler_impl_unittest.cc

+// 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_scheduler_impl.h"
+
+#include <stddef.h>
+
+#include <vector>
+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/macros.h"
+#include "base/task_scheduler/task_traits.h"
+#include "base/task_scheduler/test_task_factory.h"
+#include "base/threading/platform_thread.h"
+#include "base/threading/thread.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace base {
+namespace internal {
+
+namespace {
+
+const size_t kNumTasksPerTest = 150;
+
+struct TaskSchedulerImplTestParams {
+  TaskSchedulerImplTestParams(const TaskTraits& traits,
+                              ExecutionMode execution_mode)
+      : traits(traits), execution_mode(execution_mode) {}
+
+  const TaskTraits traits;
+  const ExecutionMode execution_mode;
+};
+
+class TaskSchedulerImplTest
+    : public testing::TestWithParam<TaskSchedulerImplTestParams> {
+ protected:
+  TaskSchedulerImplTest() = default;
+
+  void SetUp() override {
+    scheduler_ = TaskSchedulerImpl::Create();
+    EXPECT_TRUE(scheduler_);
+  }
+
+  void TearDown() override { scheduler_->JoinForTesting(); }
+
+  std::unique_ptr<TaskSchedulerImpl> scheduler_;

[gab, 2016/04/27 19:15:28]

If we get rid of Create() this can just be a plain member (no ptr) and we can
remove SetUp() :-)

[fdoray, 2016/04/28 18:36:32]

Done.

+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TaskSchedulerImplTest);
+};
+
+std::vector<TaskSchedulerImplTestParams> GetTaskSchedulerImplTestParams() {

[gab, 2016/04/27 19:15:29]

// Returns a vector with a TaskSchedulerImplTestParams for each valid
combination of {ExecutionMode, TaskPriority, WithFileIO()}.

[fdoray, 2016/04/28 18:36:32]

Done.

+  std::vector<TaskSchedulerImplTestParams> params;
+
+  // TODO(fdoray): Add ExecutionMode::SINGLE_THREADED once it's supported.

[gab, 2016/04/27 19:15:29]

Isn't this based on top of the CL that makes it supported?

[fdoray, 2016/04/28 18:36:32]

It is now :)

+  ExecutionMode execution_modes[] = {ExecutionMode::PARALLEL,
+                                     ExecutionMode::SEQUENCED};
+
+  for (size_t execution_mode_index = 0;

[gab, 2016/04/27 19:15:28]

for (ExecutionMode execution_mode : execution_modes)

[fdoray, 2016/04/28 18:36:32]

Done.

+       execution_mode_index < arraysize(execution_modes);
+       ++execution_mode_index) {
+    for (size_t priority_index = static_cast<size_t>(TaskPriority::LOWEST);
+         priority_index <= static_cast<size_t>(TaskPriority::HIGHEST);
+         ++priority_index) {
+      const TaskPriority priority = static_cast<TaskPriority>(priority_index);
+      params.push_back(
+          TaskSchedulerImplTestParams(TaskTraits().WithPriority(priority),
+                                      execution_modes[execution_mode_index]));
+      params.push_back(TaskSchedulerImplTestParams(
+          TaskTraits().WithPriority(priority).WithFileIO(),
+          execution_modes[execution_mode_index]));
+    }
+  }
+
+  return params;
+}
+
+void ExpectThreadPriority(ThreadPriority priority) {
+  EXPECT_EQ(priority, PlatformThread::GetCurrentThreadPriority());
+}
+
+}  // namespace
+
+TEST_P(TaskSchedulerImplTest, PostTasks) {
+  test::TestTaskFactory factory(
+      scheduler_->CreateTaskRunnerWithTraits(GetParam().traits,
+                                             GetParam().execution_mode),
+      GetParam().execution_mode);
+  EXPECT_FALSE(factory.task_runner()->RunsTasksOnCurrentThread());
+
+  for (size_t i = 0; i < kNumTasksPerTest; ++i) {
+    factory.PostTask(
+        test::TestTaskFactory::PostNestedTask::NO,
+        Bind(&ExpectThreadPriority,

[gab, 2016/04/27 19:15:29]

Also add AssertIOAllowed expectations (to verify WithFileIO()) in the
verification task (ExecutionMode will be verified by TestTaskFactory already
after the thread-handles change so that's okay -- probably worth adding a
comment to that effect).

(requires dependency on your incoming CL but that's okay IMO)

[fdoray, 2016/04/28 18:36:32]

Done.

+             GetParam().traits.priority() == TaskPriority::BACKGROUND
+                 ? ThreadPriority::BACKGROUND
+                 : ThreadPriority::NORMAL));
+  }
+
+  factory.WaitForAllTasksToRun();
+}
+

[gab, 2016/04/27 19:15:28]

One of the key part of TaskSchedulerImpl (well I guess SchedulerThreadPool too
but we didn't test that on it either IIRC) is to be able to post tasks of all
combinations at once and have them land where they belong.

I think it's good to test each combination in isolation, but I'd also like to
see a test that spams tasks from all combinations in parallel and ensures they
all end up where they belong (via the enhanced assert described in my previous
comment).

[fdoray, 2016/04/28 18:36:32]

Done.

+INSTANTIATE_TEST_CASE_P(TaskTraitsExecutionModeCombinations,
+                        TaskSchedulerImplTest,
+                        ::testing::ValuesIn(GetTaskSchedulerImplTestParams()));

[gab, 2016/04/27 19:15:28]

Whatttt!!!?!?! This doesn't have to be statically generated?! I didn't know that
:-)! Is gtest_filter happy with this from command-line (can you name a specific
failing combination to deterministically try the same one again -- I guess so
since the index is deterministic but still that's a very awesome trick, I'd
previously resolved to try to define all values here inline with sometimes
complex combinations of testing::Combine)?

[fdoray, 2016/04/28 18:36:32]

The name of a test is
TaskTraitsExecutionModeCombinations/TaskSchedulerImplTest.PostTasks/#, where #
is an index in the vector returned by GetTaskSchedulerImplTestParams() (indexes
don't change between runs). 

[gab, 2016/04/28 19:29:02]

Cool :-), I guess if one of them was flaky the flakiness dashboard would have a
hard time keeping track when this test evolves and indices change, but that's
probably not an issue for this one in practice ;-).

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