Add unit tests for the CLD component installer

chrome/browser/component_updater/test/cld_component_installer_unittest.cc

Index: chrome/browser/component_updater/test/cld_component_installer_unittest.cc
diff --git a/chrome/browser/component_updater/test/cld_component_installer_unittest.cc b/chrome/browser/component_updater/test/cld_component_installer_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..b12eed57b80e7cebcc74794e73661503309a11bc
--- /dev/null
+++ b/chrome/browser/component_updater/test/cld_component_installer_unittest.cc
@@ -0,0 +1,105 @@
+// Copyright 2014 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 <vector>
+
+#include "base/file_util.h"
+#include "base/files/file_path.h"
+#include "base/files/scoped_temp_dir.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/run_loop.h"
+#include "base/strings/string_util.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/values.h"
+#include "base/version.h"
+#include "chrome/browser/component_updater/cld_component_installer.h"
+#include "chrome/common/chrome_constants.h"
+#include "chrome/common/chrome_paths.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "testing/platform_test.h"
+
+namespace component_updater {
+
+class CldComponentInstallerTest : public PlatformTest {
+ public:
+  virtual void SetUp() {
+    PlatformTest::SetUp();
+    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());

[Sorin Jianu, 2014/05/14 10:46:19]

Is the temp_dir_ cleaned up somewhere?

[Andrew Hayden (chromium.org), 2014/05/14 16:17:29]

The destructor of scoped_temp_dir does perform a recursive wipe of the
directory; no cleanup is necessary. Added a comment to this effect.

+  }
+
+  base::ScopedTempDir temp_dir_;
+  component_updater::CldComponentInstallerTraits traits;
+};
+
+TEST_F(CldComponentInstallerTest, SetLatestCldDataFile) {
+  traits.SetLatestCldDataFile(base::FilePath()); // static state, yuck.

[Sorin Jianu, 2014/05/14 10:46:19]

Please make the comment more specific if there is any concern regarding the
behavior of the test.

[Andrew Hayden (chromium.org), 2014/05/14 16:17:29]

Done and moved into SetUp.

+  ASSERT_FALSE(component_updater::GetLatestCldDataFile(
+      static_cast<base::FilePath*>(NULL)));

[Sorin Jianu, 2014/05/14 10:46:19]

static cast is not needed here most likely. Does the code compile without it?

I think calling GetLatestCldDataFile with a null argument should not be allowed.

The caller has no idea whether there  is a CLD data file available or not.
Calling the function with a null argument may or may not crash when the argument
is dereferenced. Therefore, the caller of this function must always pass a valid
file path argument. I did not see this when I had done the initial code review.

What to do? Most likely, handle the null argument case in the implementation of
GetLatestCldDataFile.

[Andrew Hayden (chromium.org), 2014/05/14 16:17:29]

I chatted with Marcus about this and we considered a few options. Ultimately we
decided that the semantics for FilePath.empty() would probably be fine here, so
I've ditched the pointer completely and we now just return the cached FilePath
by value. Seems reasonable, let me know if you object. Code updated in the
translate helper as well.

+  const base::FilePath expected(FILE_PATH_LITERAL("test/foo.test"));
+  traits.SetLatestCldDataFile(expected);
+
+  base::FilePath result;
+  ASSERT_TRUE(component_updater::GetLatestCldDataFile(&result));
+  ASSERT_EQ(expected, result);
+}
+
+TEST_F(CldComponentInstallerTest, VerifyInstallation) {
+  ASSERT_FALSE(traits.VerifyInstallation(temp_dir_.path()));
+  const base::FilePath data_file_dir = temp_dir_.path()
+      .Append(FILE_PATH_LITERAL("_platform_specific"))
+      .Append(FILE_PATH_LITERAL("all"));
+  ASSERT_TRUE(base::CreateDirectory(data_file_dir));

[Sorin Jianu, 2014/05/14 10:46:19]

Is data_file_dir cleaned up?

In general, are these file resources cleaned up throughout the tests?

[Andrew Hayden (chromium.org), 2014/05/14 16:17:29]

Yes, see earlier comment about the temp dir. I'll add a comment here to make it
clear.

+  const base::FilePath data_file = data_file_dir
+      .Append(chrome::kCLDDataFilename);
+  const char test_data[] = "fake cld2 data file content here :)";

[Sorin Jianu, 2014/05/14 10:46:19]

we can use a std::string and don't bother with the length.

[Andrew Hayden (chromium.org), 2014/05/14 16:17:29]

Done.

+  const size_t length = arraysize(test_data) - 1;
+  ASSERT_EQ(static_cast<int>(length),
+            base::WriteFile(data_file, test_data, length));
+  ASSERT_TRUE(traits.VerifyInstallation(temp_dir_.path()));
+}
+
+TEST_F(CldComponentInstallerTest, OnCustomInstall) {
+  const base::DictionaryValue manifest;
+  const base::FilePath install_dir;
+  // Sanity: shouldn't crash.
+  ASSERT_TRUE(traits.OnCustomInstall(manifest, install_dir));
+}
+
+TEST_F(CldComponentInstallerTest, GetInstalledPath) {
+  const base::FilePath base_dir;
+  const base::FilePath result = traits.GetInstalledPath(base_dir);
+  ASSERT_TRUE(EndsWith(result.value(), chrome::kCLDDataFilename, true));
+}
+
+TEST_F(CldComponentInstallerTest, GetBaseDirectory) {
+  const base::FilePath result = traits.GetBaseDirectory();
+  ASSERT_FALSE(result.empty());
+}
+
+TEST_F(CldComponentInstallerTest, GetHash) {
+  std::vector<uint8> hash;
+  traits.GetHash(&hash);
+  ASSERT_EQ(static_cast<size_t>(32), hash.size());
+}
+
+TEST_F(CldComponentInstallerTest, GetName) {
+  ASSERT_FALSE(traits.GetName().empty());
+}
+
+TEST_F(CldComponentInstallerTest, ComponentReady) {
+  traits.SetLatestCldDataFile(base::FilePath()); // static state, yuck.

[Sorin Jianu, 2014/05/14 10:46:19]

What is it being tested at lines 91-93?

[Andrew Hayden (chromium.org), 2014/05/14 16:17:29]

I needed to reset the latest cld file information here as well as up above, to
ensure that this test was isolated from the test case for SetLatestCldDataFile.

I originally wanted to put the reset of the path into SetUp, but the
FRIEND_TEST* macros don't handle SetUp. I eventually decided that I'd rather
have the ugly of the "friend class" in the .h file than repeat this logic over
and over, so I've gone ahead and put the friend class decl in and now this is
part of each test implicitly.

+  ASSERT_FALSE(component_updater::GetLatestCldDataFile(
+      static_cast<base::FilePath*>(NULL)));
+  scoped_ptr<base::DictionaryValue> manifest;
+  const base::FilePath install_dir(FILE_PATH_LITERAL("/foo"));
+  const base::Version version("1.2.3.4");
+  traits.ComponentReady(version, install_dir, manifest.Pass());
+  base::FilePath result;
+  ASSERT_TRUE(component_updater::GetLatestCldDataFile(&result));
+  ASSERT_TRUE(StartsWith(base::ASCIIToUTF16(result.value()),
+                         base::ASCIIToUTF16(install_dir.value()), true));
+  ASSERT_TRUE(EndsWith(result.value(), chrome::kCLDDataFilename, true));
+}
+
+} // namespace component_updater