Add unit tests for BackgroundApplicationListModel

chrome/browser/background_application_list_model_unittest.cc

Index: chrome/browser/background_application_list_model_unittest.cc
diff --git a/chrome/browser/background_application_list_model_unittest.cc b/chrome/browser/background_application_list_model_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..96178cd6b3735f4507e6a793514a6743118864eb
--- /dev/null
+++ b/chrome/browser/background_application_list_model_unittest.cc
@@ -0,0 +1,276 @@
+// Copyright (c) 2011 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.
+
+// TODO(rickcam): Bug 73183: Add unit tests for image loading
+
+#include <algorithm>
+#include <cstdlib>
+#include <map>
+#include <set>
+#include <vector>
+
+#include "chrome/browser/background_application_list_model.h"
+
+#include "base/command_line.h"
+#include "base/file_path.h"
+#include "base/file_util.h"
+#include "base/message_loop.h"
+#include "base/scoped_ptr.h"
+#include "base/stl_util-inl.h"
+#include "chrome/browser/browser_thread.h"
+#include "chrome/browser/extensions/extension_service.h"
+#include "chrome/common/extensions/extension.h"
+#include "chrome/common/notification_registrar.h"
+#include "chrome/common/notification_service.h"
+#include "chrome/common/notification_type.h"
+#include "chrome/test/testing_profile.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+// This value is used to seed the PRNG at the beginning of a sequence of
+// operations to produce a repeatable sequence.
+#define RANDOM_SEED (0x33F7A7A7)
+
+// For ExtensionService interface when it requires a path that is not used.
+FilePath bogus_file_path() {
+#if defined(OS_WIN)
+  return FilePath(FILE_PATH_LITERAL("c:\\foo"));

[Andrew T Wilson (Slow), 2011/02/16 18:45:46]

To avoid having platform-dependent code here, can you just use relative paths
like FILE_PATH_LITERAL("foobar_nonexistent") ?

[The wrong rickcam account, 2011/02/16 22:39:20]

It must be absolute to avoid "Check failed: path.IsAbsolute()", however a read
through the FilePath header file led me to use
FILE_PATH_LITERAL("//foobar_nonexistent").  I've eliminated the
platform-dependency, but kept this as a separate function since it's used more
than once and the documentation should be helpful to someone reading the code.

+#elif defined(OS_POSIX)
+  return FilePath(FILE_PATH_LITERAL("/foo"));
+#endif
+}
+
+class BackgroundApplicationListModelTest : public testing::Test {
+ public:
+  BackgroundApplicationListModelTest();
+  ~BackgroundApplicationListModelTest();
+
+  virtual void InitializeEmptyExtensionService();
+
+ protected:
+  scoped_ptr<Profile> profile_;
+  scoped_refptr<ExtensionService> service_;
+  MessageLoop loop_;
+  BrowserThread ui_thread_;
+};
+
+// The message loop may be used in tests which require it to be an IO loop.
+BackgroundApplicationListModelTest::BackgroundApplicationListModelTest()
+    : loop_(MessageLoop::TYPE_IO),
+      ui_thread_(BrowserThread::UI, &loop_) {
+}
+
+BackgroundApplicationListModelTest::~BackgroundApplicationListModelTest() {
+  // Drop reference to ExtensionService and TestingProfile, so that they can be
+  // destroyed while BrowserThreads and MessageLoop are still around.  They
+  // are used in the destruction process.
+  service_ = NULL;
+  profile_.reset(NULL);
+  MessageLoop::current()->RunAllPending();
+}
+
+// This is modeled on a similar routine in ExtensionServiceTestBase.
+void BackgroundApplicationListModelTest::InitializeEmptyExtensionService() {
+  TestingProfile* profile = new TestingProfile();
+  profile_.reset(profile);
+  service_ = profile->CreateExtensionService(
+      CommandLine::ForCurrentProcess(),
+      bogus_file_path());
+  service_->set_extensions_enabled(true);
+  service_->set_show_extensions_prompts(false);
+  service_->OnLoadedInstalledExtensions(); /* Sends EXTENSIONS_READY */
+}
+
+// Returns a barebones test Extension object with the specified |name|.  The
+// returned extension will include background permission iff
+// |background_permission| is true.
+static scoped_refptr<Extension> CreateExtension(const std::string& name,
+                                                bool background_permission) {
+  DictionaryValue manifest;
+  manifest.SetString(extension_manifest_keys::kVersion, "1.0.0.0");
+  manifest.SetString(extension_manifest_keys::kName, name);
+  if (background_permission) {
+    ListValue* permissions = new ListValue();
+    manifest.Set(extension_manifest_keys::kPermissions, permissions);
+    permissions->Append(Value::CreateStringValue("background"));
+  }
+  std::string error;
+  scoped_refptr<Extension> extension = Extension::Create(
+      bogus_file_path().AppendASCII(name), Extension::INVALID, manifest, false,
+      &error);
+  EXPECT_TRUE(extension) << error;

[Andrew T Wilson (Slow), 2011/02/16 18:45:46]

Use an ASSERT here rather than EXPECT since you want to halt the test if you
can't create an extension (I realize that this code was probably copied from
another test).

I usually prefer "ASSERT_TRUE(extension != NULL)" instead of
ASSERT_TRUE(extension), but there are plenty of examples of both idioms so feel
free to go with your own preference.

[The wrong rickcam account, 2011/02/16 22:39:20]

As discussed with you, I'm going to leave the code as is, but adding a note
about my choise of EXPECT_TRUE.  It turns out that all of the ASSERT_* macros
return void and thus produce compilation errors when used in routines that
return a value.

+  return extension;
+}
+
+// With minimal test logic, verifies behavior over an explicit set of
+// extensions, of which some are Background Apps and others are not.
+TEST_F(BackgroundApplicationListModelTest, LoadExplicitExtensions) {
+  InitializeEmptyExtensionService();
+  ExtensionService* service = profile_->GetExtensionService();
+  ASSERT_TRUE(service);
+  ASSERT_TRUE(service->is_ready());
+  ASSERT_TRUE(service->extensions());
+  ASSERT_TRUE(service->extensions()->empty());
+  scoped_ptr<BackgroundApplicationListModel> model(
+      new BackgroundApplicationListModel(profile_.get()));
+  ASSERT_FALSE(model->size());

[Andrew T Wilson (Slow), 2011/02/16 18:45:46]

It's more readable and generates better error output if you do this:
ASSERT_EQ(0, model->size())

[The wrong rickcam account, 2011/02/16 22:39:20]

Done, but with 0U as size() returns an unsigned int.

+
+  scoped_refptr<Extension> ext1 = CreateExtension("alpha", false);
+  scoped_refptr<Extension> ext2 = CreateExtension("bravo", false);
+  scoped_refptr<Extension> ext3 = CreateExtension("charlie", false);
+  scoped_refptr<Extension> bgapp1 = CreateExtension("delta", true);
+  scoped_refptr<Extension> bgapp2 = CreateExtension("echo", true);
+  ASSERT_TRUE(service->extensions());
+  ASSERT_FALSE(service->extensions()->size());
+  ASSERT_TRUE(model->size() == 0);

[Andrew T Wilson (Slow), 2011/02/16 18:45:46]

Use ASSERT_EQ here and elsewhere in the routine where you are either comparing
against a given value or comparing against 0. Pretty much every assert involving
size() in this file should use ASSERT_EQ

[The wrong rickcam account, 2011/02/16 22:39:20]

Done.

+  // Add alternating Extensions and Background Apps
+  ASSERT_FALSE(BackgroundApplicationListModel::IsBackgroundApp(*ext1));
+  service->AddExtension(ext1);
+  ASSERT_TRUE(service->extensions()->size() == 1);
+  ASSERT_TRUE(model->size() == 0);
+  ASSERT_TRUE(BackgroundApplicationListModel::IsBackgroundApp(*bgapp1));
+  service->AddExtension(bgapp1);
+  ASSERT_TRUE(service->extensions()->size() == 2);
+  ASSERT_TRUE(model->size() == 1);
+  ASSERT_FALSE(BackgroundApplicationListModel::IsBackgroundApp(*ext2));
+  service->AddExtension(ext2);
+  ASSERT_TRUE(service->extensions()->size() == 3);
+  ASSERT_TRUE(model->size() == 1);
+  ASSERT_TRUE(BackgroundApplicationListModel::IsBackgroundApp(*bgapp2));
+  service->AddExtension(bgapp2);
+  ASSERT_TRUE(service->extensions()->size() == 4);
+  ASSERT_TRUE(model->size() == 2);
+  ASSERT_FALSE(BackgroundApplicationListModel::IsBackgroundApp(*ext3));
+  service->AddExtension(ext3);
+  ASSERT_TRUE(service->extensions()->size() == 5);
+  ASSERT_TRUE(model->size() == 2);
+  // Remove in FIFO order.
+  ASSERT_FALSE(BackgroundApplicationListModel::IsBackgroundApp(*ext1));
+  service->UninstallExtension(ext1->id(), false);
+  ASSERT_TRUE(service->extensions()->size() == 4);
+  ASSERT_TRUE(model->size() == 2);
+  ASSERT_TRUE(BackgroundApplicationListModel::IsBackgroundApp(*bgapp1));
+  service->UninstallExtension(bgapp1->id(), false);
+  ASSERT_TRUE(service->extensions()->size() == 3);
+  ASSERT_TRUE(model->size() == 1);
+  ASSERT_FALSE(BackgroundApplicationListModel::IsBackgroundApp(*ext2));
+  service->UninstallExtension(ext2->id(), false);
+  ASSERT_TRUE(service->extensions()->size() == 2);
+  ASSERT_TRUE(model->size() == 1);
+  ASSERT_TRUE(BackgroundApplicationListModel::IsBackgroundApp(*bgapp2));
+  service->UninstallExtension(bgapp2->id(), false);
+  ASSERT_TRUE(service->extensions()->size() == 1);
+  ASSERT_TRUE(model->size() == 0);
+  ASSERT_FALSE(BackgroundApplicationListModel::IsBackgroundApp(*ext3));
+  service->UninstallExtension(ext3->id(), false);
+  ASSERT_TRUE(service->extensions()->size() == 0);
+  ASSERT_TRUE(model->size() == 0);
+}
+
+typedef std::map<std::string,scoped_refptr<Extension> > ExtensionTable;

[Andrew T Wilson (Slow), 2011/02/16 18:45:46]

Do you really need a map here? I don't think you actually ever do a lookup by
id, so can you just use a std::set<scoped_refptr<Extension> > instead?

[The wrong rickcam account, 2011/02/16 22:39:20]

Done.

+
+namespace {
+std::string
+GenerateRandomExtensionName() {

[Andrew T Wilson (Slow), 2011/02/16 18:45:46]

put return type and function name on same line.

[The wrong rickcam account, 2011/02/16 22:39:20]

Done.  FWIW, this is leftover from previous style guides (and personal
preference) saying exactly the opposite so that you can search for definitions
of foo as ^foo.

+  static const char VALID[] = "ABCDEFGHIHJKMNOPabcdefghihjkmnop0123456789 ";

[Andrew T Wilson (Slow), 2011/02/16 18:45:46]

Should you put some non-ascii chars in here (unicode?)

[The wrong rickcam account, 2011/02/16 22:39:20]

No, but I need to change this!  Your question made me realize that this is just
not the right thing at all.  I'm not trying to test the possible values of
names, I'm simply trying to avoid conflict.  Given that I've replaced
GenerateRandomExtensionName with GenerateUniqueExtensionName and used a
completely different implementation.

+  static const unsigned int VALID_COUNT = sizeof(VALID) - 1;
+  static const unsigned int MINIMUM_NAME_LENGTH = 10;
+  static const unsigned int MAXIMUM_NAME_LENGTH = 30;
+
+  unsigned int length = MINIMUM_NAME_LENGTH +
+      random() % (MAXIMUM_NAME_LENGTH - MINIMUM_NAME_LENGTH);
+  char* characters = new char[length];
+  for (unsigned int index = 0; index < length; ++index)
+    characters[index] = VALID[random() % VALID_COUNT];
+  return std::string(characters, length);
+}
+}
+
+// Verifies behavior with a pseudo-randomly generated set of actions: Adding and
+// removing extensions, of which some are Background Apps and others are not.
+TEST_F(BackgroundApplicationListModelTest, LoadRandomExtension) {
+  InitializeEmptyExtensionService();
+  ExtensionService* service = profile_->GetExtensionService();
+  ASSERT_TRUE(service);
+  ASSERT_TRUE(service->is_ready());
+  ASSERT_TRUE(service->extensions());
+  ASSERT_TRUE(service->extensions()->empty());
+  scoped_ptr<BackgroundApplicationListModel> model(
+      new BackgroundApplicationListModel(profile_.get()));
+  ASSERT_FALSE(model->size());
+
+  static const unsigned int iterations = 500;

[Andrew T Wilson (Slow), 2011/02/16 18:45:46]

nit: I think the style for contstants is of the form: "kIterations"

[The wrong rickcam account, 2011/02/16 22:39:20]

Done.

+  ExtensionTable extensions;
+  unsigned int count = 0;
+  unsigned int expected = 0;
+  srandom(RANDOM_SEED);
+  for (unsigned int index = 0; index < iterations; ++index) {
+    // Randomly select: Add Extension (25%), Add Bg App (25%), Remove (50%)
+    switch (random() % 4) {
+      case 0: {
+        // Add a non-Background-App extension
+        std::string name = GenerateRandomExtensionName();
+        scoped_refptr<Extension> extension = CreateExtension(name, false);
+        ASSERT_FALSE(
+            BackgroundApplicationListModel::IsBackgroundApp(*extension));
+        std::string id = extension->id();
+        extensions[id] = extension;
+        ++count;
+        ASSERT_TRUE(extensions.size() == count);
+        service->AddExtension(extension);
+        ASSERT_TRUE(service->extensions()->size() == count);
+        ASSERT_TRUE(model->size() == expected);
+        break;
+      }
+      case 1: {
+        // Add a Background App
+        std::string name = GenerateRandomExtensionName();
+        scoped_refptr<Extension> extension = CreateExtension(name, true);
+        ASSERT_TRUE(
+            BackgroundApplicationListModel::IsBackgroundApp(*extension));
+        std::string id = extension->id();
+        extensions[id] = extension;
+        ++expected;
+        ++count;
+        ASSERT_TRUE(extensions.size() == count);
+        service->AddExtension(extension);
+        ASSERT_TRUE(service->extensions()->size() == count);

[Andrew T Wilson (Slow), 2011/02/16 18:45:46]

I suspect you could combine case 0: and case 1: and get rid of this duplicate
code:

bool create_background = false;

switch (random() % 4) {
  case 0:
    create_background = true;
    // FALL_THROUGH
  case 1:
    std::string name = GenerateRandomExtensionName();
    scoped_refptr<Extension> extension = CreateExtension(name,
create_background);
    ASSERT_EQ(create_background, IsBackgroundApp(*extension))
    ... etc ...

[The wrong rickcam account, 2011/02/16 22:39:20]

It got a little funkier than that, but I've redone this code a bit to reduce
duplication.  See what you think of the rework.

+        ASSERT_TRUE(model->size() == expected);
+        break;
+      }
+      case 2:  // Intentional fall through
+      case 3: {
+        // Maybe remove an extension.
+        ExtensionTable::iterator cursor = extensions.begin();
+        if (cursor == extensions.end()) {
+          // Nothing to remove.  Just verify accounting.
+          ASSERT_TRUE(count == 0);
+          ASSERT_TRUE(expected == 0);
+          ASSERT_TRUE(service->extensions()->size() == 0);
+          ASSERT_TRUE(model->size() == 0);
+        } else {
+          // Randomly select an extension
+          if (extensions.size() > 1) {
+            unsigned int offset = random() % (extensions.size() - 1);
+            for (unsigned int index = 0; index < offset; ++index)
+              ++cursor;
+          }
+          scoped_refptr<Extension> extension = cursor->second.get();
+          std::string id = extension->id();
+          if (BackgroundApplicationListModel::IsBackgroundApp(*extension))
+            --expected;
+          extensions.erase(cursor);
+          --count;
+          ASSERT_TRUE(extensions.size() == count);
+          service->UninstallExtension(extension->id(), false);
+          ASSERT_TRUE(service->extensions()->size() == count);
+          ASSERT_TRUE(model->size() == expected);
+        }
+        break;
+      }
+      default:
+         NOTREACHED();
+         break;
+    }
+  }
+}