added md5_unittest

added md5_unittest


BUG=77458
TEST=base_unittests:MD5.*


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=82953

[shinyak (Google), 2011-04-21 23:27:13]

Hi, I have added md5_unittest, so please review it.

[Mark Mentovai, 2011-04-22 03:13:29]

http://codereview.chromium.org/6896006/diff/1/base/base.gyp
File base/base.gyp (right):

http://codereview.chromium.org/6896006/diff/1/base/base.gyp#newcode137
base/base.gyp:137: 'md5_unittest.cc',
Sort! md5 comes after mac.

http://codereview.chromium.org/6896006/diff/1/base/md5_unittest.cc
File base/md5_unittest.cc (right):

http://codereview.chromium.org/6896006/diff/1/base/md5_unittest.cc#newcode1
base/md5_unittest.cc:1: // Copyright (c) 2011 The Chromium Authors. All rights
reserved.
In addition to the test data that you have, I’d like to see a test that
exercises the RFC 1321 MD5 test suite.

http://www.ietf.org/rfc/rfc1321.txt section A.5

http://codereview.chromium.org/6896006/diff/1/base/md5_unittest.cc#newcode11
base/md5_unittest.cc:11: using namespace std;
This is forbidden.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...

> You may not use a using-directive to make all names from a namespace
available.

http://codereview.chromium.org/6896006/diff/1/base/md5_unittest.cc#newcode17
base/md5_unittest.cc:17: MD5Sum(data, strlen(data), &digest);
#include <string.h>.

Alternatively, #include "base/basictypes.h" and use arraysize(data) - 1.

(arraysize(data) would be 1 in this case because "" contains a single NUL
terminator byte.

http://codereview.chromium.org/6896006/diff/1/base/md5_unittest.cc#newcode20
base/md5_unittest.cc:20: string actual = MD5DigestToBase16(digest);
All of your tests run “actual” through MD5DigestToBase16. I’d like to see
testing of just the raw MD5Digest bytes. See sha1_unittest.cc for an example.

You don’t necessarily need to check the MD5Bytes each and every time. I’d be
satisfied with a single test that verified that an MD5Bytes struct is turned
into the expected string after a call to MD5DigestToBase16. You could call this
the test of MD5DigestToBase16 and leave the rest of the tests you’ve written
alone.

http://codereview.chromium.org/6896006/diff/1/base/md5_unittest.cc#newcode25
base/md5_unittest.cc:25: TEST(MD5, OneByteStrnig) {
Typo: OneByteString.

http://codereview.chromium.org/6896006/diff/1/base/md5_unittest.cc#newcode52
base/md5_unittest.cc:52: const int N = 10 * 1024 * 1024 + 1;
N is a bad name for a variable. Also below on line 86.

http://codereview.chromium.org/6896006/diff/1/base/md5_unittest.cc#newcode93
base/md5_unittest.cc:93: int totalLen = 0;
Variables are named like_this and not likeThis.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab...

http://codereview.chromium.org/6896006/diff/1/base/md5_unittest.cc#newcode95
base/md5_unittest.cc:95: int len = 4097; // intentionally not 2^k.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Implem...

> These end-of-line comments should be separated from the code by 2 spaces.

http://codereview.chromium.org/6896006/diff/1/base/md5_unittest.cc#newcode104
base/md5_unittest.cc:104: DCHECK_EQ(totalLen, N);
EXPECT_EQ(N, total_length)?

http://codereview.chromium.org/6896006/diff/1/base/md5_unittest.cc#newcode115
base/md5_unittest.cc:115: }
No test of MD5String?

[shinyak (Google), 2011-04-22 18:35:47]

Hi, I've fixed the problems that you pointed out.
So please review it again. Thanks.

[Mark Mentovai, 2011-04-22 19:32:05]

LG otherwise

http://codereview.chromium.org/6896006/diff/5/base/md5_unittest.cc
File base/md5_unittest.cc (right):

http://codereview.chromium.org/6896006/diff/5/base/md5_unittest.cc#newcode6
base/md5_unittest.cc:6: #include <string.h>
C system headers should precede C++ system headers.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_...

http://codereview.chromium.org/6896006/diff/5/base/md5_unittest.cc#newcode14
base/md5_unittest.cc:14: using namespace std;
I had asked you to fix this. “using namespace” is forbidden. Technically, you
can say “using std::string” in a .cc file, but it’s rare in Chromium code. It’s
most common to just write std::string everywhere you need it.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...

[shinyak (Google), 2011-04-22 19:44:12]

Mark,

Thank you for your quick response, and sorry for ignoring your notice by
mistake.
I've fixed them. So please review it again. Thanks.

[Mark Mentovai, 2011-04-22 19:48:43]

LGTM!

[commit-bot: I haz the power, 2011-04-25 23:54:09]

Change committed as 82953