Combine file stream reader unittests

These are all separate but different unittests that have grown
organically apart from each other, combine into a single typed test
so that future file stream reader unittests can reuse this structure.

The one unfortunate part about doing it this way is that it requires a
lot more `this->` everywhere because of C++ templates.  However,
the alternative of having a test fixture with a bunch of switch
statements for the different types seems strictly worse.

Change-Id: Id56bc28391e1a640748691cb383a19291c47fc51
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2406718
Commit-Queue: Nick Harper <nharper@chromium.org>
Auto-Submit: enne <enne@chromium.org>
Reviewed-by: Nick Harper <nharper@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#807092}

net/base/file_stream_context.cc

@@ -113,6 +113,11 @@ void FileStream::Context::Seek(int64_t offset,
                                Int64CompletionOnceCallback callback) {
   DCHECK(!async_in_progress_);
 
+  if (offset < 0) {
+    std::move(callback).Run(net::ERR_INVALID_ARGUMENT);
+    return;
+  }
+
   bool posted = base::PostTaskAndReplyWithResult(
       task_runner_.get(), FROM_HERE,
       base::BindOnce(&Context::SeekFileImpl, base::Unretained(this), offset),

storage/browser/BUILD.gn

@@ -275,6 +275,8 @@ source_set("unittests") {
     "file_system/copy_or_move_operation_delegate_unittest.cc",
     "file_system/dragged_file_util_unittest.cc",
     "file_system/external_mount_points_unittest.cc",
+    "file_system/file_stream_reader_test.cc",
+    "file_system/file_stream_reader_test.h",
     "file_system/file_stream_test_utils.cc",
     "file_system/file_stream_test_utils.h",
     "file_system/file_system_context_unittest.cc",

storage/browser/file_system/file_stream_reader_test.cc

+// Copyright 2020 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 "storage/browser/file_system/file_stream_reader_test.h"
+
+namespace storage {
+
+const base::StringPiece FileStreamReaderTest::kTestFileName;
+const base::StringPiece FileStreamReaderTest::kTestData;
+
+}  // namespace storage

storage/browser/file_system/file_system_file_stream_reader_unittest.cc

@@ -21,6 +21,8 @@
 #include "net/base/net_errors.h"
 #include "net/base/test_completion_callback.h"
 #include "storage/browser/file_system/external_mount_points.h"
+
+#include "storage/browser/file_system/file_stream_reader_test.h"
 #include "storage/browser/file_system/file_stream_test_utils.h"
 #include "storage/browser/file_system/file_system_context.h"
 #include "storage/browser/file_system/file_system_file_util.h"
@@ -33,58 +35,44 @@
 namespace storage {
 
 namespace {
-
 const char kURLOrigin[] = "http://remote/";
-const char kTestFileName[] = "test.dat";
-const char kTestData[] = "0123456789";
-const int kTestDataSize = base::size(kTestData) - 1;
-
-void NeverCalled(int unused) {
-  ADD_FAILURE();
-}
-
 }  // namespace
 
-class FileSystemFileStreamReaderTest : public testing::Test {
+class FileSystemFileStreamReaderTest : public FileStreamReaderTest {
  public:
   FileSystemFileStreamReaderTest() = default;
 
   void SetUp() override {
-    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+    ASSERT_TRUE(dir_.CreateUniqueTempDir());
 
     file_system_context_ =
-        CreateFileSystemContextForTesting(nullptr, temp_dir_.GetPath());
+        CreateFileSystemContextForTesting(nullptr, dir_.GetPath());
 
-    file_system_context_->OpenFileSystem(url::Origin::Create(GURL(kURLOrigin)),
-                                         kFileSystemTypeTemporary,
-                                         OPEN_FILE_SYSTEM_CREATE_IF_NONEXISTENT,
-                                         base::BindOnce(&OnOpenFileSystem));
+    file_system_context_->OpenFileSystem(
+        url::Origin::Create(GURL(kURLOrigin)), kFileSystemTypeTemporary,
+        OPEN_FILE_SYSTEM_CREATE_IF_NONEXISTENT,
+        base::BindOnce([](const GURL& root_url, const std::string& name,
+                          base::File::Error result) {
+          ASSERT_EQ(base::File::FILE_OK, result);
+        }));
     base::RunLoop().RunUntilIdle();
-
-    WriteFile(kTestFileName, kTestData, kTestDataSize,
-              &test_file_modification_time_);
   }
 
   void TearDown() override { base::RunLoop().RunUntilIdle(); }
 
- protected:
-  FileSystemFileStreamReader* CreateFileReader(
+  std::unique_ptr<FileStreamReader> CreateFileReader(
       const std::string& file_name,
       int64_t initial_offset,
-      const base::Time& expected_modification_time) {
-    return new FileSystemFileStreamReader(
+      const base::Time& expected_modification_time) override {
+    return FileStreamReader::CreateForFileSystemFile(
         file_system_context_.get(), GetFileSystemURL(file_name), initial_offset,
         expected_modification_time);
   }
 
-  base::Time test_file_modification_time() const {
-    return test_file_modification_time_;
-  }
-
   void WriteFile(const std::string& file_name,
                  const char* buf,
-                 int buf_size,
-                 base::Time* modification_time) {
+                 size_t buf_size,
+                 base::Time* modification_time) override {
     FileSystemURL url = GetFileSystemURL(file_name);
 
     ASSERT_EQ(base::File::FILE_OK,
@@ -99,11 +87,17 @@ class FileSystemFileStreamReaderTest : public testing::Test {
       *modification_time = file_info.last_modified;
   }
 
- private:
-  static void OnOpenFileSystem(const GURL& root_url,
-                               const std::string& name,
-                               base::File::Error result) {
-    ASSERT_EQ(base::File::FILE_OK, result);
+  void TouchFile(const std::string& file_name, base::TimeDelta delta) override {
+    FileSystemURL url = GetFileSystemURL(file_name);
+
+    base::File::Info file_info;
+    ASSERT_EQ(base::File::FILE_OK,
+              AsyncFileTestHelper::GetMetadata(file_system_context_.get(), url,
+                                               &file_info));
+    ASSERT_EQ(base::File::FILE_OK,
+              AsyncFileTestHelper::TouchFile(file_system_context_.get(), url,
+                                             file_info.last_accessed,
+                                             file_info.last_modified + delta));
   }
 
   FileSystemURL GetFileSystemURL(const std::string& file_name) {
@@ -112,140 +106,13 @@ class FileSystemFileStreamReaderTest : public testing::Test {
         base::FilePath().AppendASCII(file_name));
   }
 
-  base::test::SingleThreadTaskEnvironment task_environment_{
-      base::test::SingleThreadTaskEnvironment::MainThreadType::IO};
-  base::ScopedTempDir temp_dir_;
+ private:
+  base::ScopedTempDir dir_;
   scoped_refptr<FileSystemContext> file_system_context_;
-  base::Time test_file_modification_time_;
 };
 
-TEST_F(FileSystemFileStreamReaderTest, NonExistent) {
-  const char kFileName[] = "nonexistent";
-  std::unique_ptr<FileSystemFileStreamReader> reader(
-      CreateFileReader(kFileName, 0, base::Time()));
-  int result = 0;
-  std::string data;
-  ReadFromReader(reader.get(), &data, 10, &result);
-  ASSERT_EQ(net::ERR_FILE_NOT_FOUND, result);
-  ASSERT_EQ(0U, data.size());
-}
-
-TEST_F(FileSystemFileStreamReaderTest, Empty) {
-  const char kFileName[] = "empty";
-  WriteFile(kFileName, nullptr, 0, nullptr);
-
-  std::unique_ptr<FileSystemFileStreamReader> reader(
-      CreateFileReader(kFileName, 0, base::Time()));
-  int result = 0;
-  std::string data;
-  ReadFromReader(reader.get(), &data, 10, &result);
-  ASSERT_EQ(net::OK, result);
-  ASSERT_EQ(0U, data.size());
-
-  net::TestInt64CompletionCallback callback;
-  int64_t length_result = reader->GetLength(callback.callback());
-  if (length_result == net::ERR_IO_PENDING)
-    length_result = callback.WaitForResult();
-  ASSERT_EQ(0, length_result);
-}
-
-TEST_F(FileSystemFileStreamReaderTest, GetLengthNormal) {
-  std::unique_ptr<FileSystemFileStreamReader> reader(
-      CreateFileReader(kTestFileName, 0, test_file_modification_time()));
-  net::TestInt64CompletionCallback callback;
-  int64_t result = reader->GetLength(callback.callback());
-  if (result == net::ERR_IO_PENDING)
-    result = callback.WaitForResult();
-  ASSERT_EQ(kTestDataSize, result);
-}
-
-TEST_F(FileSystemFileStreamReaderTest, GetLengthAfterModified) {
-  // Pass a fake expected modifictaion time so that the expectation fails.
-  base::Time fake_expected_modification_time =
-      test_file_modification_time() - base::TimeDelta::FromSeconds(10);
-
-  std::unique_ptr<FileSystemFileStreamReader> reader(
-      CreateFileReader(kTestFileName, 0, fake_expected_modification_time));
-  net::TestInt64CompletionCallback callback1;
-  int64_t result = reader->GetLength(callback1.callback());
-  if (result == net::ERR_IO_PENDING)
-    result = callback1.WaitForResult();
-  ASSERT_EQ(net::ERR_UPLOAD_FILE_CHANGED, result);
-
-  // With nullptr expected modification time this should work.
-  reader.reset(CreateFileReader(kTestFileName, 0, base::Time()));
-  net::TestInt64CompletionCallback callback2;
-  result = reader->GetLength(callback2.callback());
-  if (result == net::ERR_IO_PENDING)
-    result = callback2.WaitForResult();
-  ASSERT_EQ(kTestDataSize, result);
-}
-
-TEST_F(FileSystemFileStreamReaderTest, GetLengthWithOffset) {
-  std::unique_ptr<FileSystemFileStreamReader> reader(
-      CreateFileReader(kTestFileName, 3, base::Time()));
-  net::TestInt64CompletionCallback callback;
-  int64_t result = reader->GetLength(callback.callback());
-  if (result == net::ERR_IO_PENDING)
-    result = callback.WaitForResult();
-  // Initial offset does not affect the result of GetLength.
-  ASSERT_EQ(kTestDataSize, result);
-}
-
-TEST_F(FileSystemFileStreamReaderTest, ReadNormal) {
-  std::unique_ptr<FileSystemFileStreamReader> reader(
-      CreateFileReader(kTestFileName, 0, test_file_modification_time()));
-  int result = 0;
-  std::string data;
-  ReadFromReader(reader.get(), &data, kTestDataSize, &result);
-  ASSERT_EQ(net::OK, result);
-  ASSERT_EQ(kTestData, data);
-}
-
-TEST_F(FileSystemFileStreamReaderTest, ReadAfterModified) {
-  // Pass a fake expected modifictaion time so that the expectation fails.
-  base::Time fake_expected_modification_time =
-      test_file_modification_time() - base::TimeDelta::FromSeconds(10);
-
-  std::unique_ptr<FileSystemFileStreamReader> reader(
-      CreateFileReader(kTestFileName, 0, fake_expected_modification_time));
-  int result = 0;
-  std::string data;
-  ReadFromReader(reader.get(), &data, kTestDataSize, &result);
-  ASSERT_EQ(net::ERR_UPLOAD_FILE_CHANGED, result);
-  ASSERT_EQ(0U, data.size());
-
-  // With nullptr expected modification time this should work.
-  data.clear();
-  reader.reset(CreateFileReader(kTestFileName, 0, base::Time()));
-  ReadFromReader(reader.get(), &data, kTestDataSize, &result);
-  ASSERT_EQ(net::OK, result);
-  ASSERT_EQ(kTestData, data);
-}
-
-TEST_F(FileSystemFileStreamReaderTest, ReadWithOffset) {
-  std::unique_ptr<FileSystemFileStreamReader> reader(
-      CreateFileReader(kTestFileName, 3, base::Time()));
-  int result = 0;
-  std::string data;
-  ReadFromReader(reader.get(), &data, kTestDataSize, &result);
-  ASSERT_EQ(net::OK, result);
-  ASSERT_EQ(&kTestData[3], data);
-}
-
-TEST_F(FileSystemFileStreamReaderTest, DeleteWithUnfinishedRead) {
-  std::unique_ptr<FileSystemFileStreamReader> reader(
-      CreateFileReader(kTestFileName, 0, base::Time()));
-
-  net::TestCompletionCallback callback;
-  scoped_refptr<net::IOBufferWithSize> buf =
-      base::MakeRefCounted<net::IOBufferWithSize>(kTestDataSize);
-  int rv = reader->Read(buf.get(), buf->size(), base::BindOnce(&NeverCalled));
-  ASSERT_TRUE(rv == net::ERR_IO_PENDING || rv >= 0);
-
-  // Delete immediately.
-  // Should not crash; nor should NeverCalled be callback.
-  reader.reset();
-}
+INSTANTIATE_TYPED_TEST_SUITE_P(FileSystem,
+                               FileStreamReaderTypedTest,
+                               FileSystemFileStreamReaderTest);
 
 }  // namespace storage

storage/browser/test/async_file_test_helper.cc

@@ -269,4 +269,18 @@ blink::mojom::QuotaStatusCode AsyncFileTestHelper::GetUsageAndQuota(
   return status;
 }
 
+base::File::Error AsyncFileTestHelper::TouchFile(
+    FileSystemContext* context,
+    const FileSystemURL& url,
+    const base::Time& last_access_time,
+    const base::Time& last_modified_time) {
+  base::File::Error result = base::File::FILE_ERROR_FAILED;
+  base::RunLoop run_loop;
+  context->operation_runner()->TouchFile(
+      url, last_access_time, last_modified_time,
+      AssignAndQuitCallback(&run_loop, &result));
+  run_loop.Run();
+  return result;
+}
+
 }  // namespace storage

storage/browser/test/async_file_test_helper.h

@@ -107,6 +107,14 @@ class AsyncFileTestHelper {
       FileSystemType type,
       int64_t* usage,
       int64_t* quota);
+
+  // Modifies timestamps of a file or directory at |url| with
+  // |last_access_time| and |last_modified_time|. The function DOES NOT
+  // create a file unlike 'touch' command on Linux.
+  static base::File::Error TouchFile(FileSystemContext* context,
+                                     const FileSystemURL& url,
+                                     const base::Time& last_access_time,
+                                     const base::Time& last_modified_time);
 };
 
 }  // namespace storage