[blimp] Change to use SHA-256 instead of SHA-1 for unique ids

blimp/common/blob_cache/id_util_unittest.cc

+// Copyright 2016 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 "blimp/common/blob_cache/id_util.h"
+#include "crypto/sha2.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace blimp {
+namespace {
+const char kInput[] = "\xde\xad\xbe\xef\x4b\x1d\xc0\xd3\xff\xfe";

[Wez, 2016/04/23 00:08:52]

kBlobData?

nit: Blank line between namespace and the const char declaration, for
readability, innit.

[nyquist, 2016/04/25 08:11:17]

Done.

+
+// Caution: If these expected outputs ever change, it means that all client-side
+// caches will effectively be invalidated.
+const char kExpected[] =

[Wez, 2016/04/23 00:08:52]

kBlobId and kBlobIdString, say?

[nyquist, 2016/04/25 08:11:17]

Done.

+    "\x31\xda\x00\xaf\x5e\xd6\x64\xd6\x5f\xb1\xe6\x34\x99\xd3\xf5\x12\x21\xc8"
+    "\xf8\x51\x19\xfd\x1d\x17\xaa\x0b\x5e\x85\x10\x4f\x17\x15";
+const char kExpectedFormatted[] =
+    "31da00af5ed664d65fb1e63499d3f51221c8f85119fd1d17aa0b5e85104f1715";
+
+class IdUtilTest : public testing::Test {

[Wez, 2016/04/23 00:08:52]

Why are we overriding testing::Test if we're not actually going to add anything?

[nyquist, 2016/04/25 08:11:17]

Done.

+ public:
+  IdUtilTest() {}
+  ~IdUtilTest() override {}
+};
+
+TEST_F(IdUtilTest, VerifyHashCalculationCreatesExpectedHash) {

[Wez, 2016/04/23 00:08:52]

Suggest calling this just BlobIdIsCorrect or CorrectBlobId or Basic.

Consider also adding a test to verify that different Blobs have different Ids.

[nyquist, 2016/04/25 08:11:17]

Done.

+  BlobId id = CalculateBlobId(kInput, 10);
+  EXPECT_EQ(crypto::kSHA256Length, id.length());
+  for (size_t i = 0; i < crypto::kSHA256Length; i++)
+    EXPECT_EQ(kExpected[i], static_cast<int>(id[i]));
+  EXPECT_EQ(kExpectedFormatted, FormatBlobId(id));
+}
+
+TEST_F(IdUtilTest, VerifyLengthChecker) {

[Wez, 2016/04/23 00:08:52]

This looks like two different tests; one is verifying that different Blobs' Ids
have the correct length, the other that the length-checker is correct - suggest
breaking into separate BlobIdsAreCorrectLength and BlobIdLengthCheck tests.

[nyquist, 2016/04/25 08:11:17]

Done.

+  BlobId id1 = CalculateBlobId(kInput, 10);
+  EXPECT_EQ(crypto::kSHA256Length, id1.length());
+  EXPECT_TRUE(BlobIdHasCorrectLength(id1));
+
+  BlobId id2 = CalculateBlobId(kInput, 8);
+  EXPECT_EQ(crypto::kSHA256Length, id2.length());
+  EXPECT_TRUE(BlobIdHasCorrectLength(id2));
+
+  EXPECT_FALSE(BlobIdHasCorrectLength(BlobId("")));
+  EXPECT_FALSE(BlobIdHasCorrectLength(BlobId("123")));
+  EXPECT_TRUE(
+      BlobIdHasCorrectLength(BlobId("12345678901234567890123456789012")));
+  EXPECT_FALSE(
+      BlobIdHasCorrectLength(BlobId("12345678901234567890123456789012fff")));
+}
+
+}  // namespace
+}  // namespace blimp