Add support for persistent file access in apps.

apps/saved_files_service_unittest.cc

+// Copyright 2013 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 <algorithm>
+
+#include "apps/saved_files_service.h"
+#include "base/files/file_path.h"
+#include "base/string_number_conversions.h"
+#include "base/test/values_test_util.h"
+#include "base/values.h"
+#include "chrome/browser/extensions/extension_prefs.h"
+#include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/extensions/extension_system.h"
+#include "chrome/browser/extensions/test_extension_environment.h"
+#include "chrome/common/extensions/extension.h"
+#include "chrome/test/base/testing_profile.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+#if !defined(OS_ANDROID)
+
+#define TRACE_CALL(expression) \
+  do {                         \
+    SCOPED_TRACE(#expression); \
+    expression;                \
+  } while (0)
+
+using apps::SavedFileEntry;
+using apps::SavedFilesService;
+
+namespace {
+
+std::string GenerateId(int i) {
+  return base::IntToString(i) + ":filename.ext";
+}
+
+}  // namespace
+
+class SavedFilesServiceUnitTest : public testing::Test {

[Matt Giuca, 2013/05/22 08:26:14]

Sam: These are really great tests. Very comprehensive and no redundancy (except
the confusing last one).

+ protected:
+  virtual void SetUp() OVERRIDE {
+    testing::Test::SetUp();
+    extension_ = env_.MakeExtension(*base::test::ParseJson(

[Matt Giuca, 2013/05/22 08:26:14]

Can you expand this into a proper multi-line string.

[Sam McNally, 2013/05/23 03:47:28]

Done.

+        "{\"app\": {\"background\": {\"scripts\": [\"background.js\"]}},"
+        "\"permissions\": [{\"fileSystem\": [\"retainFiles\"]}]}"));
+    service_ = SavedFilesService::Get(env_.profile());
+    path_ = base::FilePath(FILE_PATH_LITERAL("filename.ext"));
+  }
+
+  virtual void TearDown() OVERRIDE {
+    SavedFilesService::ClearMaxSequenceNumberForTest();
+    SavedFilesService::ClearLruSizeForTest();
+    testing::Test::TearDown();
+  }
+
+  void CheckSavedFileEntry(const std::string& id, int sequence_number) {

[Matt Giuca, 2013/05/22 08:26:14]

Document.

[Matt Giuca, 2013/05/22 08:26:14]

It seems like you call this with GenerateId(...) every single time.

Could you just take the int, and call GenerateId inside this function? It would
make the tests easier to read.

[Sam McNally, 2013/05/23 03:47:28]

Done.

[Sam McNally, 2013/05/23 03:47:28]

Done.

+    SCOPED_TRACE(id);
+    SavedFileEntry entry;
+    EXPECT_TRUE(service_->IsRetained(extension_->id(), id));
+    ASSERT_TRUE(service_->GetFileEntry(extension_->id(), id, &entry));
+    EXPECT_EQ(id, entry.id);
+    EXPECT_EQ(path_, entry.path);
+    EXPECT_TRUE(entry.writable);
+    EXPECT_EQ(sequence_number, entry.sequence_number);
+  }
+
+  void CheckSavedFileEntries(int start, int end) {

[Matt Giuca, 2013/05/22 08:26:14]

Document.

[Sam McNally, 2013/05/23 03:47:28]

Done.

+    SavedFileEntry entry;
+    for (int i = start; i < end; i++) {

[Matt Giuca, 2013/05/22 08:26:14]

++i

+      CheckSavedFileEntry(GenerateId(i), i + 1);
+    }
+  }
+  extensions::TestExtensionEnvironment env_;
+  const extensions::Extension* extension_;
+  SavedFilesService* service_;
+  base::FilePath path_;
+};
+
+TEST_F(SavedFilesServiceUnitTest, RetainTwoFilesTest) {

[Matt Giuca, 2013/05/22 08:26:14]

Nit: Maybe add some blank lines to space this out a bit.

[Sam McNally, 2013/05/23 03:47:28]

Done.

+  service_->RetainFileUntilAppSuspend(
+      extension_->id(), GenerateId(1), path_, true);
+  service_->RetainFileUntilAppSuspend(
+      extension_->id(), GenerateId(2), path_, true);
+  service_->RetainFileUntilAppSuspend(
+      extension_->id(), GenerateId(3), path_, true);

[Matt Giuca, 2013/05/22 08:26:14]

Please add a line explaining what each of these sections is testing. For example
// Test that all entries have the empty sequence number.

[Sam McNally, 2013/05/23 03:47:28]

Done.

+  TRACE_CALL(CheckSavedFileEntry(GenerateId(1), 0));
+  TRACE_CALL(CheckSavedFileEntry(GenerateId(2), 0));
+  TRACE_CALL(CheckSavedFileEntry(GenerateId(3), 0));
+  service_->MoveEntryToBackOfQueue(extension_->id(), GenerateId(1));
+  TRACE_CALL(CheckSavedFileEntry(GenerateId(1), 1));

[Matt Giuca, 2013/05/22 08:26:14]

// Test that entry #1 has a valid sequence number.
etc

[Sam McNally, 2013/05/23 03:47:28]

Done.

+  TRACE_CALL(CheckSavedFileEntry(GenerateId(2), 0));
+  service_->MoveEntryToBackOfQueue(extension_->id(), GenerateId(1));
+  TRACE_CALL(CheckSavedFileEntry(GenerateId(1), 1));
+  TRACE_CALL(CheckSavedFileEntry(GenerateId(2), 0));
+  service_->MoveEntryToBackOfQueue(extension_->id(), GenerateId(2));
+  TRACE_CALL(CheckSavedFileEntry(GenerateId(1), 1));
+  TRACE_CALL(CheckSavedFileEntry(GenerateId(2), 2));
+  service_->MoveEntryToBackOfQueue(extension_->id(), GenerateId(2));
+  TRACE_CALL(CheckSavedFileEntry(GenerateId(1), 1));
+  TRACE_CALL(CheckSavedFileEntry(GenerateId(2), 2));
+  service_->MoveEntryToBackOfQueue(extension_->id(), GenerateId(1));
+  TRACE_CALL(CheckSavedFileEntry(GenerateId(1), 3));
+  TRACE_CALL(CheckSavedFileEntry(GenerateId(2), 2));
+  TRACE_CALL(CheckSavedFileEntry(GenerateId(3), 0));
+  EXPECT_FALSE(service_->IsRetained(extension_->id(), "another id"));

[Matt Giuca, 2013/05/22 08:26:14]

Why don't you test IsRetained on one of the actually retained files?
You should also test that IsRetained returns false after calling
RetainFileUntilAppSuspend but before MoveEntryToBackOfQueue.

[Sam McNally, 2013/05/23 03:47:28]

CheckSavedFileEntry tests that. It's intended to return true for that case.

+  SavedFileEntry entry;
+  EXPECT_FALSE(service_->GetFileEntry(extension_->id(), "another id", &entry));
+  service_->MaybeClearQueue(extension_);

[Matt Giuca, 2013/05/22 08:26:14]

// MaybeClearQueue should have no effect, since the app has the "retainFiles"
permission.

[Sam McNally, 2013/05/23 03:47:28]

Done.

+  TRACE_CALL(CheckSavedFileEntry(GenerateId(1), 3));
+  TRACE_CALL(CheckSavedFileEntry(GenerateId(2), 2));
+  TRACE_CALL(CheckSavedFileEntry(GenerateId(3), 0));
+  service_->Clear(extension_->id());

[Matt Giuca, 2013/05/22 08:26:14]

I don't follow why Clear has no effect here?

[Sam McNally, 2013/05/23 03:47:28]

Done.

+  TRACE_CALL(CheckSavedFileEntry(GenerateId(1), 3));
+  TRACE_CALL(CheckSavedFileEntry(GenerateId(2), 2));
+  EXPECT_FALSE(service_->IsRetained(extension_->id(), GenerateId(3)));
+}
+
+TEST_F(SavedFilesServiceUnitTest, NoRetainFilesPermissionTest) {
+  extension_ = env_.MakeExtension(*base::test::ParseJson(
+      "{\"app\": {\"background\": {\"scripts\": [\"background.js\"]}},"
+      "\"permissions\": [\"fileSystem\"]}"));
+  service_->RetainFileUntilAppSuspend(
+      extension_->id(), GenerateId(1), path_, true);
+  TRACE_CALL(CheckSavedFileEntry(GenerateId(1), 0));
+  SavedFileEntry entry;
+  service_->MoveEntryToBackOfQueue(extension_->id(), GenerateId(1));
+  TRACE_CALL(CheckSavedFileEntry(GenerateId(1), 1));
+  EXPECT_FALSE(service_->IsRetained(extension_->id(), "another id"));
+  EXPECT_FALSE(service_->GetFileEntry(extension_->id(), "another id", &entry));
+  service_->MaybeClearQueue(extension_);

[Matt Giuca, 2013/05/22 08:26:14]

// MaybeClearQueue should clear the queue, since the app does not have the
"retainFiles" permission.

[Sam McNally, 2013/05/23 03:47:28]

Done.

+  std::vector<SavedFileEntry> entries =
+      service_->GetAllFileEntries(extension_->id());
+  EXPECT_TRUE(entries.empty());
+}
+
+TEST_F(SavedFilesServiceUnitTest, EvictionTest) {
+  SavedFilesService::SetLruSizeForTest(10);
+  for (int i = 0; i < 10; i++) {

[Matt Giuca, 2013/05/22 08:26:14]

++i (find and replace throughout this file)

+    service_->RetainFileUntilAppSuspend(
+        extension_->id(), GenerateId(i), path_, true);
+    service_->MoveEntryToBackOfQueue(extension_->id(), GenerateId(i));
+  }
+  service_->RetainFileUntilAppSuspend(

[Matt Giuca, 2013/05/22 08:26:14]

// Expect that entries 0 to 9 are in the LRU.

[Sam McNally, 2013/05/23 03:47:28]

Done.

+      extension_->id(), GenerateId(10), path_, true);
+  TRACE_CALL(CheckSavedFileEntries(0, 10));
+  TRACE_CALL(CheckSavedFileEntry(GenerateId(10), 0));
+  service_->MoveEntryToBackOfQueue(extension_->id(), GenerateId(10));
+
+  TRACE_CALL(CheckSavedFileEntry(GenerateId(0), 0));

[Matt Giuca, 2013/05/22 08:26:14]

// Expect that entries 1 to 10 are in the LRU, and 0 has been evicted.

[Sam McNally, 2013/05/23 03:47:28]

Done.

+  TRACE_CALL(CheckSavedFileEntries(1, 11));
+  service_->Clear(extension_->id());
+  SavedFileEntry entry;
+  EXPECT_FALSE(service_->GetFileEntry(extension_->id(), GenerateId(0), &entry));
+  TRACE_CALL(CheckSavedFileEntries(1, 11));
+
+  service_->MoveEntryToBackOfQueue(extension_->id(), GenerateId(1));

[Matt Giuca, 2013/05/22 08:26:14]

Might it be worth testing this on id 2 instead of 1, to prove that the least
recently used id (1) was not evicted?

[Sam McNally, 2013/05/23 03:47:28]

Done.

+  TRACE_CALL(CheckSavedFileEntry(GenerateId(1), 12));

[Matt Giuca, 2013/05/22 08:26:14]

// Expect that entry 1 is now at the back of the queue, and no further entries
have been evicted.

[Sam McNally, 2013/05/23 03:47:28]

Done.

+  TRACE_CALL(CheckSavedFileEntries(2, 11));
+  service_->Clear(extension_->id());

[Matt Giuca, 2013/05/22 08:26:14]

Again, I don't know why Clear is not expected to have any effect?

[Sam McNally, 2013/05/23 03:47:28]

Done.

+  TRACE_CALL(CheckSavedFileEntry(GenerateId(1), 12));
+  TRACE_CALL(CheckSavedFileEntries(2, 11));
+}
+
+TEST_F(SavedFilesServiceUnitTest, SequenceNumberCompactionTest) {
+  SavedFilesService::SetMaxSequenceNumberForTest(8);

[Matt Giuca, 2013/05/22 08:26:14]

SavedFilesService::SetLruSizeForTest(8);

To satisfy DCHECK(g_max_sequence_number >= g_max_saved_file_entries); which I
suggested in saved_files_service.cc.

[Sam McNally, 2013/05/23 03:47:28]

Done.

+  for (int i = 0; i < 4; i++) {
+    service_->RetainFileUntilAppSuspend(
+        extension_->id(), GenerateId(i), path_, true);
+    service_->MoveEntryToBackOfQueue(extension_->id(), GenerateId(i));
+  }
+  service_->MoveEntryToBackOfQueue(extension_->id(), GenerateId(2));
+  service_->MoveEntryToBackOfQueue(extension_->id(), GenerateId(3));
+  service_->MoveEntryToBackOfQueue(extension_->id(), GenerateId(2));
+  TRACE_CALL(CheckSavedFileEntry(GenerateId(0), 1));

[Matt Giuca, 2013/05/22 08:26:14]

// The sequence numbers should be sparse, as they have not gone over the limit.

[Sam McNally, 2013/05/23 03:47:28]

Done.

+  TRACE_CALL(CheckSavedFileEntry(GenerateId(1), 2));
+  TRACE_CALL(CheckSavedFileEntry(GenerateId(2), 7));
+  TRACE_CALL(CheckSavedFileEntry(GenerateId(3), 6));
+  service_->Clear(extension_->id());
+  TRACE_CALL(CheckSavedFileEntry(GenerateId(0), 1));
+  TRACE_CALL(CheckSavedFileEntry(GenerateId(1), 2));
+  TRACE_CALL(CheckSavedFileEntry(GenerateId(2), 7));
+  TRACE_CALL(CheckSavedFileEntry(GenerateId(3), 6));
+
+  service_->MoveEntryToBackOfQueue(extension_->id(), GenerateId(3));

[Matt Giuca, 2013/05/22 08:26:14]

// This should push the sequence number to the limit of 8, and trigger a
sequence number compaction.
// Expect that the sequence numbers are contiguous from 1 to 4.

[Sam McNally, 2013/05/23 03:47:28]

Done.

+  TRACE_CALL(CheckSavedFileEntries(0, 4));
+  service_->Clear(extension_->id());
+  TRACE_CALL(CheckSavedFileEntries(0, 4));
+}
+
+TEST_F(SavedFilesServiceUnitTest, SequenceNumberCompactionFirstAboveOneTest) {

[Matt Giuca, 2013/05/22 08:26:14]

I don't get what this is testing that the previous one did not test. Please
explain in comments or delete this test.

[Sam McNally, 2013/05/23 03:47:28]

Done.

+  SavedFilesService::SetMaxSequenceNumberForTest(10);

[Matt Giuca, 2013/05/22 08:26:14]

SavedFilesService::SetLruSizeForTest(10);

+  for (int i = 0; i < 4; i++) {
+    service_->RetainFileUntilAppSuspend(
+        extension_->id(), GenerateId(i), path_, true);
+    service_->MoveEntryToBackOfQueue(extension_->id(), GenerateId(i));
+  }
+  for (int i = 0; i < 4; i++) {
+    service_->MoveEntryToBackOfQueue(extension_->id(), GenerateId(i));
+  }
+  service_->MoveEntryToBackOfQueue(extension_->id(), GenerateId(2));
+  TRACE_CALL(CheckSavedFileEntry(GenerateId(0), 5));
+  TRACE_CALL(CheckSavedFileEntry(GenerateId(1), 6));
+  TRACE_CALL(CheckSavedFileEntry(GenerateId(2), 9));
+  TRACE_CALL(CheckSavedFileEntry(GenerateId(3), 8));
+  service_->Clear(extension_->id());
+  TRACE_CALL(CheckSavedFileEntry(GenerateId(0), 5));
+  TRACE_CALL(CheckSavedFileEntry(GenerateId(1), 6));
+  TRACE_CALL(CheckSavedFileEntry(GenerateId(2), 9));
+  TRACE_CALL(CheckSavedFileEntry(GenerateId(3), 8));
+
+  service_->MoveEntryToBackOfQueue(extension_->id(), GenerateId(3));
+  TRACE_CALL(CheckSavedFileEntries(0, 4));
+  service_->Clear(extension_->id());
+  TRACE_CALL(CheckSavedFileEntries(0, 4));
+}
+#endif