Add new API function: feedbackPrivate.readLogSource

chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc

Index: chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc
diff --git a/chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc b/chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..855b0aa89ea46fc16b91b65e96d1ca503515159d
--- /dev/null
+++ b/chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc
@@ -0,0 +1,421 @@
+// Copyright 2017 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 "chrome/browser/extensions/api/feedback_private/feedback_private_api.h"
+
+#include "base/json/json_writer.h"
+#include "base/macros.h"
+#include "base/memory/ptr_util.h"
+#include "base/test/simple_test_tick_clock.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "base/values.h"
+#include "chrome/browser/extensions/api/feedback_private/log_source_resource.h"
+#include "chrome/browser/extensions/api/feedback_private/single_log_source_factory.h"
+#include "chrome/browser/extensions/extension_api_unittest.h"
+#include "extensions/browser/api_test_utils.h"
+#include "extensions/browser/api_unittest.h"
+
+namespace extensions {
+
+namespace {
+
+using api::feedback_private::ReadLogSourceResult;
+using api::feedback_private::ReadLogSourceParams;
+using system_logs::SingleLogSource;
+using system_logs::SystemLogsResponse;
+using SupportedSource = system_logs::SingleLogSource::SupportedSource;
+
+std::unique_ptr<KeyedService> ApiResourceManagerTestFactory(
+    content::BrowserContext* context) {
+  return base::MakeUnique<ApiResourceManager<LogSourceResource>>(context);
+}
+
+// A dummy SingleLogSource that does not require real system logs to be
+// available during testing.
+class TestSingleLogSource : public SingleLogSource {
+ public:
+  explicit TestSingleLogSource(SupportedSource type)
+      : SingleLogSource(type), call_count_(0) {}
+
+  ~TestSingleLogSource() override = default;
+
+  // Fetch() will return a single different string each time, in the following
+  // sequence: "a", "bb", "ccc", until a string of 26 z's. Will never return an
+  // empty result.
+  void Fetch(const system_logs::SysLogsSourceCallback& callback) override {
+    int count_modulus = call_count_ % kNumCharsToIterate;
+    std::string result(count_modulus + 1, kInitialChar + count_modulus);
+    ASSERT_GT(result.size(), 0U);
+    ++call_count_;
+
+    SystemLogsResponse* result_map = new SystemLogsResponse;
+    result_map->emplace("", result);
+
+    // Do not directly pass the result to the callback, because that's not how
+    // log sources actually work. Instead, simulate the asynchronous operation
+    // of a SingleLogSource by invoking the callback separately.
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(callback, base::Owned(result_map)));
+  }
+
+  // Instantiates a new instance of this class. Does not retain ownership. Used
+  // to create a Callback that can be used to override the default behavior of
+  // SingleLogSourceFactory.
+  static std::unique_ptr<SingleLogSource> Create(SupportedSource type) {
+    return base::MakeUnique<TestSingleLogSource>(type);
+  }
+
+ private:
+  // Iterate over the whole lowercase alphabet, starting from 'a'.
+  const int kNumCharsToIterate = 26;
+  const char kInitialChar = 'a';
+
+  // Keep track of how many times Fetch() has been called, in order to determine
+  // its behavior each time.
+  int call_count_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestSingleLogSource);
+};
+
+}  // namespace
+
+class FeedbackApiUnittest : public ExtensionApiUnittest {

[tbarzic, 2017/06/07 03:39:02]

nit: I'd rename this to FeedbackPrivateApiTest

[Simon Que, 2017/06/07 18:25:58]

Done.

+ public:
+  FeedbackApiUnittest()
+      : create_callback_(base::Bind(&TestSingleLogSource::Create)) {}
+  ~FeedbackApiUnittest() override {}
+
+  void SetUp() override {
+    ExtensionApiUnittest::SetUp();
+
+    // The ApiResourceManager used for LogSourceResource is destroyed every time
+    // a unit test finishes, during TearDown(). There is no way to re-create it
+    // normally. The below code forces it to be re-created during SetUp(), so
+    // that there is always a valid ApiResourceManager<LogSourceResource> when
+    // subsequent unit tests are running.
+    ApiResourceManager<LogSourceResource>::GetFactoryInstance()
+        ->SetTestingFactoryAndUse(browser()->profile(),
+                                  ApiResourceManagerTestFactory);
+
+    SingleLogSourceFactory::SetForTesting(&create_callback_);
+  }
+
+  void TearDown() override {
+    SingleLogSourceFactory::SetForTesting(nullptr);
+    LogSourceAccessManager::SetRateLimitingTimeoutForTesting(nullptr);
+
+    FeedbackPrivateAPI::GetFactoryInstance()
+        ->Get(browser()->profile())
+        ->GetLogSourceAccessManager()
+        ->SetTickClockForTesting(nullptr);
+
+    ExtensionApiUnittest::TearDown();
+  }
+
+  // Runs the feedbackPrivate.readLogSource() function. See API function
+  // definition for argument descriptions.
+  //
+  // Note that the second argument of the result is a list of strings, but the
+  // test class TestSingleLogSource always returns a list containing a single
+  // string. To simplify things, the single string result will be returned in
+  // |*result_string|, while the reader ID is returned in |*result_reader_id|.
+  testing::AssertionResult RunReadLogSourceFunction(
+      const ReadLogSourceParams& params,
+      int* result_reader_id,
+      std::string* result_string) {
+    scoped_refptr<FeedbackPrivateReadLogSourceFunction> function =
+        new FeedbackPrivateReadLogSourceFunction;
+
+    std::unique_ptr<base::ListValue> result_list =
+        RunFunctionAndReturnValue(function.get(), ParamsToJSON(params));
+    if (!result_list)
+      return testing::AssertionFailure() << "No result";
+
+    ReadLogSourceResult result;
+    testing::AssertionResult parse_result =
+        ParseReadLogSourceResult(*result_list, &result);
+    if (!parse_result)
+      return parse_result;
+
+    if (result.log_lines.size() != 1) {
+      return testing::AssertionFailure()
+             << "Expected |log_lines| to contain 1 string, actual number: "
+             << result.log_lines.size();
+    }
+
+    *result_reader_id = result.reader_id;
+    *result_string = result.log_lines[0];
+
+    return testing::AssertionSuccess();
+  }
+
+ private:
+  // Converts |params| to a string containing a JSON dictionary within an
+  // argument list.
+  std::string ParamsToJSON(const ReadLogSourceParams& params) {
+    base::ListValue params_value;
+    params_value.Append(params.ToValue());
+    std::string params_json_string;
+    EXPECT_TRUE(base::JSONWriter::Write(params_value, &params_json_string));
+
+    return params_json_string;
+  }
+
+  // Runs |function| with arguments |args| in JSON string format and returns the
+  // result as a base::ListValue.
+  std::unique_ptr<base::ListValue> RunFunctionAndReturnValue(
+      UIThreadExtensionFunction* function,
+      const std::string& args) {
+    function->set_extension(extension());
+    if (!api_test_utils::RunFunction(function, args, browser()->profile()))
+      return nullptr;
+
+    const base::ListValue* result_list = function->GetResultList();
+    if (!result_list)
+      return nullptr;
+
+    return result_list->CreateDeepCopy();
+  }
+
+  // Attempts to interpret the result as a ReadLogSourceResult. Returns failure
+  // if |value| does not conform to the format of a list containing a single
+  // element that is a ReadLogSourceResult object.
+  testing::AssertionResult ParseReadLogSourceResult(
+      const base::ListValue& list_value,
+      ReadLogSourceResult* result) {
+    if (list_value.empty())
+      return testing::AssertionFailure() << "Result is empty.";
+
+    const base::Value* result_value = nullptr;
+    if (!list_value.Get(0, &result_value)) {
+      return testing::AssertionFailure()
+             << "Unable to get first element of result list.";
+    }
+
+    if (!ReadLogSourceResult::Populate(*result_value, result)) {
+      return testing::AssertionFailure()
+             << "Invalid ReadLogSourceResult format: " << *result_value;
+    }
+
+    return testing::AssertionSuccess();
+  }
+
+  // Passed to SingleLogSourceFactory so that the API can create an instance of
+  // TestSingleLogSource for testing.
+  SingleLogSourceFactory::CreateCallback create_callback_;
+
+  // content::TestBrowserThreadBundle browser_thread_bundle_;
+
+  DISALLOW_COPY_AND_ASSIGN(FeedbackApiUnittest);
+};
+
+TEST_F(FeedbackApiUnittest, ReadLogSourceInvalidId) {
+  const base::TimeDelta timeout(base::TimeDelta::FromMilliseconds(0));
+  LogSourceAccessManager::SetRateLimitingTimeoutForTesting(&timeout);
+
+  ReadLogSourceParams params;
+  params.source = api::feedback_private::LOG_SOURCE_MESSAGES;
+  params.incremental = true;
+  params.reader_id.reset(new int(9999));
+
+  int result_reader_id;
+  std::string result_string;
+  EXPECT_FALSE(
+      RunReadLogSourceFunction(params, &result_reader_id, &result_string));
+}
+
+TEST_F(FeedbackApiUnittest, ReadLogSourceNonIncremental) {
+  const base::TimeDelta timeout(base::TimeDelta::FromMilliseconds(0));
+  LogSourceAccessManager::SetRateLimitingTimeoutForTesting(&timeout);
+
+  ReadLogSourceParams params;
+  params.source = api::feedback_private::LOG_SOURCE_MESSAGES;
+  params.incremental = false;
+
+  // Test multiple non-incremental reads.
+  int result_reader_id = -1;
+  std::string result_string;
+  EXPECT_TRUE(
+      RunReadLogSourceFunction(params, &result_reader_id, &result_string));
+  EXPECT_EQ(0, result_reader_id);
+  EXPECT_EQ("a", result_string);
+
+  result_reader_id = -1;
+  result_string.clear();
+  EXPECT_TRUE(
+      RunReadLogSourceFunction(params, &result_reader_id, &result_string));
+  EXPECT_EQ(0, result_reader_id);
+  EXPECT_EQ("a", result_string);
+
+  result_reader_id = -1;
+  result_string.clear();
+  EXPECT_TRUE(
+      RunReadLogSourceFunction(params, &result_reader_id, &result_string));
+  EXPECT_EQ(0, result_reader_id);
+  EXPECT_EQ("a", result_string);
+}
+
+TEST_F(FeedbackApiUnittest, ReadLogSourceIncremental) {
+  const base::TimeDelta timeout(base::TimeDelta::FromMilliseconds(0));
+  LogSourceAccessManager::SetRateLimitingTimeoutForTesting(&timeout);
+
+  ReadLogSourceParams params;
+  params.source = api::feedback_private::LOG_SOURCE_MESSAGES;
+  params.incremental = true;
+
+  int result_reader_id = 0;
+  std::string result_string;
+  EXPECT_TRUE(
+      RunReadLogSourceFunction(params, &result_reader_id, &result_string));
+  EXPECT_EQ(1, result_reader_id);

[tbarzic, 2017/06/07 17:58:38]

reader IDs being incremental seems like implementation detail of the
ApiResourceManager.
I'd consider testing it's > 0 here, and later compare later ID to the one
returned here?

[Simon Que, 2017/06/07 18:25:58]

Done.

+  EXPECT_EQ("a", result_string);
+  params.reader_id.reset(new int(result_reader_id));
+
+  EXPECT_TRUE(
+      RunReadLogSourceFunction(params, &result_reader_id, &result_string));
+  EXPECT_EQ(1, result_reader_id);
+  EXPECT_EQ("bb", result_string);
+
+  EXPECT_TRUE(
+      RunReadLogSourceFunction(params, &result_reader_id, &result_string));
+  EXPECT_EQ(1, result_reader_id);
+  EXPECT_EQ("ccc", result_string);
+
+  // End the incremental read.
+  params.incremental = false;
+  EXPECT_TRUE(
+      RunReadLogSourceFunction(params, &result_reader_id, &result_string));
+  EXPECT_EQ(0, result_reader_id);
+  EXPECT_EQ("dddd", result_string);
+
+  // The log source will no longer be valid if we try to read it.
+  params.incremental = true;
+  EXPECT_FALSE(
+      RunReadLogSourceFunction(params, &result_reader_id, &result_string));
+}
+
+TEST_F(FeedbackApiUnittest, ReadLogSourceMultipleSources) {
+  const base::TimeDelta timeout(base::TimeDelta::FromMilliseconds(0));
+  LogSourceAccessManager::SetRateLimitingTimeoutForTesting(&timeout);
+
+  int result_reader_id = 0;
+  std::string result_string;
+
+  // Attempt to open LOG_SOURCE_MESSAGES twice.
+  ReadLogSourceParams params1a;

[tbarzic, 2017/06/07 17:58:38]

Can you make the param var names more meaningful?
e.g. first_read_params, repeated_first_read_params, second_read_params,
repeated_second_read_params

Or maybe do something like:

int first_reader_id = -1;
{
  ReadLogParams params;
  params.source = ...

  EXPECT_TRUE(RunLogSourceFunction(params, &first_reader_id, result_string));
  EXPECT_GT(first_reader_id, 0);
}

[Simon Que, 2017/06/07 18:25:58]

Done.

+  params1a.source = api::feedback_private::LOG_SOURCE_MESSAGES;
+  params1a.incremental = true;
+  EXPECT_TRUE(
+      RunReadLogSourceFunction(params1a, &result_reader_id, &result_string));
+  EXPECT_EQ(1, result_reader_id);
+  params1a.reader_id.reset(new int(result_reader_id));

[tbarzic, 2017/06/07 17:58:38]

nit: use base::MakeUnique here.

though, I'd prefer to keep this info in a designated variable (e.g.
first_reader_id).

[Simon Que, 2017/06/07 18:25:58]

Done.

This needs to be stored back into the params so that the same log source reader
object can be used in the next call.

+
+  // Cannot perform a second read from the same log source.
+  ReadLogSourceParams params1b;
+  params1b.source = api::feedback_private::LOG_SOURCE_MESSAGES;
+  params1b.incremental = true;
+  EXPECT_FALSE(
+      RunReadLogSourceFunction(params1b, &result_reader_id, &result_string));
+
+  // Attempt to open LOG_SOURCE_UI_LATEST twice.
+  ReadLogSourceParams params2a;
+  params2a.source = api::feedback_private::LOG_SOURCE_UILATEST;
+  params2a.incremental = true;
+  result_reader_id = -1;
+  EXPECT_TRUE(
+      RunReadLogSourceFunction(params2a, &result_reader_id, &result_string));
+  params2a.reader_id.reset(new int(result_reader_id));
+  EXPECT_EQ(2, result_reader_id);
+
+  // Cannot perform a second read from the same log source.
+  ReadLogSourceParams params2b;
+  params2b.source = api::feedback_private::LOG_SOURCE_UILATEST;
+  params2b.incremental = true;
+  EXPECT_FALSE(
+      RunReadLogSourceFunction(params2b, &result_reader_id, &result_string));
+
+  // Close the two open log source readers, and make sure new ones can be
+  // opened.
+  params1a.incremental = false;
+  result_reader_id = -1;
+  EXPECT_TRUE(
+      RunReadLogSourceFunction(params1a, &result_reader_id, &result_string));
+  EXPECT_EQ(0, result_reader_id);
+
+  params2a.incremental = false;
+  result_reader_id = -1;
+  EXPECT_TRUE(
+      RunReadLogSourceFunction(params2a, &result_reader_id, &result_string));
+  EXPECT_EQ(0, result_reader_id);
+
+  EXPECT_TRUE(
+      RunReadLogSourceFunction(params1b, &result_reader_id, &result_string));
+  EXPECT_EQ(3, result_reader_id);
+  EXPECT_TRUE(
+      RunReadLogSourceFunction(params2b, &result_reader_id, &result_string));
+  EXPECT_EQ(4, result_reader_id);
+}
+
+TEST_F(FeedbackApiUnittest, ReadLogSourceWithAccessTimeouts) {
+  const base::TimeDelta timeout(base::TimeDelta::FromMilliseconds(100));
+  LogSourceAccessManager::SetRateLimitingTimeoutForTesting(&timeout);
+
+  base::SimpleTestTickClock* test_clock = new base::SimpleTestTickClock;
+  FeedbackPrivateAPI::GetFactoryInstance()
+      ->Get(browser()->profile())
+      ->GetLogSourceAccessManager()
+      ->SetTickClockForTesting(std::unique_ptr<base::TickClock>(test_clock));
+
+  ReadLogSourceParams params;
+  params.source = api::feedback_private::LOG_SOURCE_MESSAGES;
+  params.incremental = true;
+  int result_reader_id = 0;
+  std::string result_string;
+
+  auto FromMilliseconds = &base::TimeDelta::FromMilliseconds;

[tbarzic, 2017/06/07 17:58:38]

I'd prefer using TimeDelta::FromMilliseconds.
(with using base::TimeDelta at the beginning of the file)

[Simon Que, 2017/06/07 18:25:58]

Done.

+  // |test_clock| must start out at something other than 0, which is interpreted
+  // as an invalid value.
+  test_clock->Advance(FromMilliseconds(100));
+
+  EXPECT_TRUE(
+      RunReadLogSourceFunction(params, &result_reader_id, &result_string));
+  EXPECT_EQ(1, result_reader_id);
+  params.reader_id.reset(new int(result_reader_id));
+
+  // Immediately perform another read. This is not allowed.
+  EXPECT_FALSE(
+      RunReadLogSourceFunction(params, &result_reader_id, &result_string));
+
+  // Advance to t=120, but it will not be allowed.
+  test_clock->Advance(FromMilliseconds(20));
+  EXPECT_FALSE(
+      RunReadLogSourceFunction(params, &result_reader_id, &result_string));
+
+  // Advance to t=150, but still not allowed.
+  test_clock->Advance(FromMilliseconds(30));
+  EXPECT_FALSE(

[tbarzic, 2017/06/07 03:39:02]

it would be nice if the test distinguished between the function returning an
error and the function returning empty logs (this test would pass in both cases)

[Simon Que, 2017/06/07 18:25:58]

Done.

+      RunReadLogSourceFunction(params, &result_reader_id, &result_string));
+
+  // Advance to t=199, but still not allowed.
+  test_clock->Advance(FromMilliseconds(49));
+  EXPECT_FALSE(
+      RunReadLogSourceFunction(params, &result_reader_id, &result_string));
+
+  // Advance to t=210, annd the access is finally allowed.
+  test_clock->Advance(FromMilliseconds(11));
+  EXPECT_TRUE(
+      RunReadLogSourceFunction(params, &result_reader_id, &result_string));
+
+  // Advance to t=309, but it will not be allowed.
+  test_clock->Advance(FromMilliseconds(99));
+  EXPECT_FALSE(
+      RunReadLogSourceFunction(params, &result_reader_id, &result_string));
+
+  // Another read is finally allowed at t=310.
+  test_clock->Advance(FromMilliseconds(1));
+  EXPECT_TRUE(
+      RunReadLogSourceFunction(params, &result_reader_id, &result_string));
+}
+
+}  // namespace extensions