Add SingleLogSource to system_logs sources

chrome/browser/chromeos/system_logs/single_log_source_unittest.cc

+// 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/chromeos/system_logs/single_log_source.h"
+
+#include <string>
+
+#include "base/bind.h"
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "base/files/scoped_temp_dir.h"
+#include "base/macros.h"
+#include "base/run_loop.h"
+#include "base/test/scoped_task_scheduler.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace system_logs {
+
+class TestSingleLogSource : public SingleLogSource {

[afakhry, 2017/05/08 19:20:48]

You don't need this class at all, or the mutable_file() accessor in
SingleLogSource. Simply add a 

friend class SingleLogSourceTest; in SingleLogSource.

This is a common pattern in our code base, that test classes are friends with
the classes they test.

[Simon Que, 2017/05/08 19:58:36]

Done.

+ public:
+  TestSingleLogSource() : SingleLogSource(SupportedSource::kMessages) {}
+  ~TestSingleLogSource() override {}
+
+  base::File* mutable_file() { return SingleLogSource::mutable_file(); }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestSingleLogSource);
+};
+
+class SingleLogSourceTest : public ::testing::Test {
+ public:
+  SingleLogSourceTest()
+      : num_callback_calls_(0),
+        callback_(base::Bind(&SingleLogSourceTest::OnFileRead,
+                             base::Unretained(this))) {
+    CHECK(dir_.CreateUniqueTempDir());
+    log_file_path_ = dir_.GetPath().Append("log_file");
+
+    // Create the dummy log file for writing.
+    base::File new_file;
+    new_file.Initialize(log_file_path_,
+                        base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_READ);
+    new_file.Close();
+    CHECK(base::PathExists(log_file_path_));
+
+    // Open the dummy log file for reading from within the log source.
+    source_.mutable_file()->Initialize(
+        log_file_path_, base::File::FLAG_OPEN | base::File::FLAG_READ);
+    CHECK(source_.mutable_file()->IsValid());
+  }
+
+  ~SingleLogSourceTest() override { dir_.Take(); }

[afakhry, 2017/05/08 19:20:48]

Why do you want to make this dir outlive SingleLogSourceTest?

From Take() documentation:
Caller takes ownership of the temporary directory so it won't be destroyed
when this object goes out of scope.

[Simon Que, 2017/05/08 19:58:36]

Done.

+
+ protected:
+  // Writes a string to |log_file_path_|.
+  bool WriteFile(const std::string& input) {
+    return base::WriteFile(log_file_path_, input.data(), input.size());
+  }
+  // Appends a string to |log_file_path_|.
+  bool AppendToFile(const std::string& input) {
+    return base::AppendToFile(log_file_path_, input.data(), input.size());
+  }
+
+  // Callback for fetching logs from |source_|. Overwrites the previous stored
+  // value of |latest_response_|.
+  void OnFileRead(SystemLogsResponse* response) {
+    ++num_callback_calls_;
+    if (response->empty())
+      return;
+
+    // Since |source_| represents a single log source, it should only return a
+    // single string result.
+    EXPECT_EQ(1U, response->size());
+    latest_response_ = std::move(response->begin()->second);
+  }
+
+  // Unit under test.
+  TestSingleLogSource source_;
+
+  // Counts the number of times that |source_| has invoked the callback.
+  int num_callback_calls_;
+
+  // Stores the string response returned from |source_| the last time it invoked
+  // OnFileRead.
+  std::string latest_response_;
+
+  // Callback object for invoking OnFileRead.
+  const SysLogsSourceCallback callback_;
+
+  // Temporary dir for creating a dummy log file.
+  base::ScopedTempDir dir_;
+
+  // Path to the dummy log file in |dir_|.
+  base::FilePath log_file_path_;
+
+  // Used for running scheduled tasks.
+  base::test::ScopedTaskScheduler scheduler_;
+
+  DISALLOW_COPY_AND_ASSIGN(SingleLogSourceTest);
+};
+
+TEST_F(SingleLogSourceTest, EmptyFile) {
+  source_.Fetch(callback_);
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(1, num_callback_calls_);

[afakhry, 2017/05/08 19:20:48]

num_callback_calls_ should not be mutable and exposed to sub class test cases
like this. Please make all members private and expose only those that you need
through const accessors. Then move the functionality that requires mutating the
members to the base class.

[Simon Que, 2017/05/08 19:58:36]

Done.

+  EXPECT_EQ("", latest_response_);
+}
+
+TEST_F(SingleLogSourceTest, SingleRead) {
+  EXPECT_TRUE(AppendToFile("Hello world!\n"));
+  source_.Fetch(callback_);
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(1, num_callback_calls_);
+  EXPECT_EQ("Hello world!\n", latest_response_);
+}
+
+TEST_F(SingleLogSourceTest, IncrementalReads) {
+  EXPECT_TRUE(AppendToFile("Hello world!\n"));
+  source_.Fetch(callback_);
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(1, num_callback_calls_);
+  EXPECT_EQ("Hello world!\n", latest_response_);
+
+  EXPECT_TRUE(AppendToFile("The quick brown fox jumps over the lazy dog\n"));
+  source_.Fetch(callback_);
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(2, num_callback_calls_);
+  EXPECT_EQ("The quick brown fox jumps over the lazy dog\n", latest_response_);
+
+  EXPECT_TRUE(AppendToFile("Some like it hot.\nSome like it cold\n"));
+  source_.Fetch(callback_);
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(3, num_callback_calls_);
+  EXPECT_EQ("Some like it hot.\nSome like it cold\n", latest_response_);
+
+  // As a sanity check, read entire contents of file separately to make sure it
+  // was written incrementally, and hence read incrementally.
+  std::string file_contents;
+  EXPECT_TRUE(base::ReadFileToString(log_file_path_, &file_contents));
+  EXPECT_EQ(
+      "Hello world!\nThe quick brown fox jumps over the lazy dog\n"
+      "Some like it hot.\nSome like it cold\n",
+      file_contents);
+}
+
+TEST_F(SingleLogSourceTest, FileOverwrite) {
+  EXPECT_TRUE(AppendToFile("0123456789\n"));
+  source_.Fetch(callback_);
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(1, num_callback_calls_);
+  EXPECT_EQ("0123456789\n", latest_response_);
+
+  // Overwrite the file.
+  EXPECT_TRUE(WriteFile("abcdefg\n"));

[afakhry, 2017/05/08 19:20:48]

That's just a side effect of  having the overwrite string shorter than the
existing one. So, depending on the needed use cases, you might need to consider
making this smarter.

[Simon Que, 2017/05/08 19:58:36]

I'm not sure that's possible. See my analysis of tail -f, which has a similar
limitation:

https://docs.google.com/document/d/1FyDiP9iOs8INQH5hUjXMmBoQkrQZlRanFRDqr_22C...

I'm not sure if it's worth the trouble to keep track of overwrites. The log
files we are reading should not be overwritten. This test is just to ensure that
the SingleLogSource class is robust enough not to break in the event of an
overwrite.

I'll add a comment.

[afakhry, 2017/05/09 00:39:23]

Acknowledged.

+  source_.Fetch(callback_);
+  base::RunLoop().RunUntilIdle();
+
+  // Should re-read from the beginning.
+  EXPECT_EQ(2, num_callback_calls_);
+  EXPECT_EQ("abcdefg\n", latest_response_);
+
+  // Append to the file to make sure incremental read still works.
+  EXPECT_TRUE(AppendToFile("hijk\n"));
+  source_.Fetch(callback_);
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(3, num_callback_calls_);
+  EXPECT_EQ("hijk\n", latest_response_);
+
+  // Overwrite again, this time with a longer length than the existing file.
+  // Previous contents:
+  //   abcdefg~hijk~     <-- "~" is a single-char representation of newline.
+  // New contents:
+  //   lmnopqrstuvwxyz~  <-- excess text beyond end of prev contents: "yz~"
+  EXPECT_TRUE(WriteFile("lmnopqrstuvwxyz\n"));
+  source_.Fetch(callback_);
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(4, num_callback_calls_);
+  EXPECT_EQ("yz\n", latest_response_);

[afakhry, 2017/05/08 19:20:48]

Similarly, this should be smarter at detecting overwrites depending on the
expected use cases.

+}
+
+TEST_F(SingleLogSourceTest, IncompleteLines) {
+  EXPECT_TRUE(AppendToFile("0123456789"));
+  source_.Fetch(callback_);
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(1, num_callback_calls_);
+  EXPECT_EQ("", latest_response_);
+
+  EXPECT_TRUE(AppendToFile("abcdefg"));
+  source_.Fetch(callback_);
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(2, num_callback_calls_);
+  EXPECT_EQ("", latest_response_);
+
+  EXPECT_TRUE(AppendToFile("hijk\n"));
+  source_.Fetch(callback_);
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(3, num_callback_calls_);
+  // All the previously written text should be read this time.
+  EXPECT_EQ("0123456789abcdefghijk\n", latest_response_);
+
+  EXPECT_TRUE(AppendToFile("Hello world\n"));
+  EXPECT_TRUE(AppendToFile("Goodbye world"));
+  source_.Fetch(callback_);
+  base::RunLoop().RunUntilIdle();
+
+  // Partial whole-line reads are not supported. The last byte of the read must
+  // be a new line.
+  EXPECT_EQ(4, num_callback_calls_);
+  EXPECT_EQ("", latest_response_);
+
+  EXPECT_TRUE(AppendToFile("\n"));
+  source_.Fetch(callback_);
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(5, num_callback_calls_);
+  EXPECT_EQ("Hello world\nGoodbye world\n", latest_response_);
+}
+
+TEST_F(SingleLogSourceTest, Anonymize) {
+  EXPECT_TRUE(AppendToFile("My MAC address is: 11:22:33:44:55:66\n"));
+  source_.Fetch(callback_);
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(1, num_callback_calls_);
+  EXPECT_EQ("My MAC address is: 11:22:33:00:00:01\n", latest_response_);
+
+  // Suppose the write operation is not atomic, and the MAC address is written
+  // across two separate writes.
+  EXPECT_TRUE(AppendToFile("Your MAC address is: AB:88:C"));
+  source_.Fetch(callback_);
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(2, num_callback_calls_);
+  EXPECT_EQ("", latest_response_);
+
+  EXPECT_TRUE(AppendToFile("D:99:EF:77\n"));
+  source_.Fetch(callback_);
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(3, num_callback_calls_);
+  EXPECT_EQ("Your MAC address is: ab:88:cd:00:00:02\n", latest_response_);
+}
+
+}  // namespace system_logs