Add unit tests for the CLD component installer

Add unit tests for the CLD component installer

BUG=367239
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271028

[Andrew Hayden (chromium.org), 2014-05-13 11:44:04]

+sorin for component updater work, +jochen for chrome_paths. I've removed the
ifdefs around the path settings. As we get closer to enabling this on potential
platforms, there seems to be no reason to guard such simple functionality with
the ifdef anymore.

[jochen (gone - plz use gerrit), 2014-05-14 08:29:15]

lgtm

[Sorin Jianu, 2014-05-14 10:46:18]

Thank you Andrew!

Please also run "git cl lint" and "git cl format". There are some slight style
issues that I did not point out but lint will. Changes due to "git cl format"
are advisory. We generally accept the changes unless the clang format does not
fit the code style around it.

https://codereview.chromium.org/280753003/diff/20001/chrome/browser/component...
File chrome/browser/component_updater/test/cld_component_installer_unittest.cc
(right):

https://codereview.chromium.org/280753003/diff/20001/chrome/browser/component...
chrome/browser/component_updater/test/cld_component_installer_unittest.cc:28:
ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
Is the temp_dir_ cleaned up somewhere?

https://codereview.chromium.org/280753003/diff/20001/chrome/browser/component...
chrome/browser/component_updater/test/cld_component_installer_unittest.cc:36:
traits.SetLatestCldDataFile(base::FilePath()); // static state, yuck.
Please make the comment more specific if there is any concern regarding the
behavior of the test.

https://codereview.chromium.org/280753003/diff/20001/chrome/browser/component...
chrome/browser/component_updater/test/cld_component_installer_unittest.cc:38:
static_cast<base::FilePath*>(NULL)));
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.

https://codereview.chromium.org/280753003/diff/20001/chrome/browser/component...
chrome/browser/component_updater/test/cld_component_installer_unittest.cc:52:
ASSERT_TRUE(base::CreateDirectory(data_file_dir));
Is data_file_dir cleaned up?

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

https://codereview.chromium.org/280753003/diff/20001/chrome/browser/component...
chrome/browser/component_updater/test/cld_component_installer_unittest.cc:55:
const char test_data[] = "fake cld2 data file content here :)";
we can use a std::string and don't bother with the length.

https://codereview.chromium.org/280753003/diff/20001/chrome/browser/component...
chrome/browser/component_updater/test/cld_component_installer_unittest.cc:91:
traits.SetLatestCldDataFile(base::FilePath()); // static state, yuck.
What is it being tested at lines 91-93?

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

https://codereview.chromium.org/280753003/diff/20001/chrome/browser/component...
File chrome/browser/component_updater/test/cld_component_installer_unittest.cc
(right):

https://codereview.chromium.org/280753003/diff/20001/chrome/browser/component...
chrome/browser/component_updater/test/cld_component_installer_unittest.cc:28:
ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
On 2014/05/14 10:46:19, Sorin Jianu wrote:
> Is the temp_dir_ cleaned up somewhere?
The destructor of scoped_temp_dir does perform a recursive wipe of the
directory; no cleanup is necessary. Added a comment to this effect.

https://codereview.chromium.org/280753003/diff/20001/chrome/browser/component...
chrome/browser/component_updater/test/cld_component_installer_unittest.cc:36:
traits.SetLatestCldDataFile(base::FilePath()); // static state, yuck.
On 2014/05/14 10:46:19, Sorin Jianu wrote:
> Please make the comment more specific if there is any concern regarding the
> behavior of the test.

Done and moved into SetUp.

https://codereview.chromium.org/280753003/diff/20001/chrome/browser/component...
chrome/browser/component_updater/test/cld_component_installer_unittest.cc:38:
static_cast<base::FilePath*>(NULL)));
On 2014/05/14 10:46:19, Sorin Jianu wrote:
> 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.

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.

https://codereview.chromium.org/280753003/diff/20001/chrome/browser/component...
chrome/browser/component_updater/test/cld_component_installer_unittest.cc:52:
ASSERT_TRUE(base::CreateDirectory(data_file_dir));
On 2014/05/14 10:46:19, Sorin Jianu wrote:
> Is data_file_dir cleaned up?
> 
> In general, are these file resources cleaned up throughout the tests?

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

https://codereview.chromium.org/280753003/diff/20001/chrome/browser/component...
chrome/browser/component_updater/test/cld_component_installer_unittest.cc:55:
const char test_data[] = "fake cld2 data file content here :)";
On 2014/05/14 10:46:19, Sorin Jianu wrote:
> we can use a std::string and don't bother with the length.

Done.

https://codereview.chromium.org/280753003/diff/20001/chrome/browser/component...
chrome/browser/component_updater/test/cld_component_installer_unittest.cc:91:
traits.SetLatestCldDataFile(base::FilePath()); // static state, yuck.
On 2014/05/14 10:46:19, Sorin Jianu wrote:
> What is it being tested at lines 91-93?

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.

[Andrew Hayden (chromium.org), 2014-05-14 16:18:32]

sorin@: PTAL, thanks!

[Andrew Hayden (chromium.org), 2014-05-14 17:09:48]

Er, forgot to run lint and format, let me do that.

[Andrew Hayden (chromium.org), 2014-05-14 17:19:07]

OK, linter and formatter are now happy. The formatter makes me cry a little.

[Sorin Jianu, 2014-05-15 16:03:57]

lgtm

Thank you Andrew. 

As I was saying, we take the changes from cl format as advisory. The idea is to
get benefits from the tool (it does find some issues in addition to what lint
finds) without causing tears and pain. 

I usually run cl format with the dry switch or sending the diffs to stdout to
see changes before hand. Committing locally before running cl format helps too
in the case the changes must be reverted.

https://codereview.chromium.org/280753003/diff/60002/chrome/browser/component...
File chrome/browser/component_updater/cld_component_installer.cc (right):

https://codereview.chromium.org/280753003/diff/60002/chrome/browser/component...
chrome/browser/component_updater/cld_component_installer.cc:127: // cld_file is
an empty path by default, meaning "file not available yet",
Excellent!

https://codereview.chromium.org/280753003/diff/60002/chrome/browser/component...
chrome/browser/component_updater/cld_component_installer.cc:128: // so we can
simply return by value (making a copy).
@128 Comment not needed as it is both obvious and idiomatic what the getter
does.

https://codereview.chromium.org/280753003/diff/60002/chrome/browser/component...
File chrome/browser/component_updater/test/cld_component_installer_unittest.cc
(right):

https://codereview.chromium.org/280753003/diff/60002/chrome/browser/component...
chrome/browser/component_updater/test/cld_component_installer_unittest.cc:55: //
All files are created within a ScopedTempDir, which deletes and all
The word and could be removed.

[Andrew Hayden (chromium.org), 2014-05-16 12:03:25]

https://codereview.chromium.org/280753003/diff/60002/chrome/browser/component...
File chrome/browser/component_updater/cld_component_installer.cc (right):

https://codereview.chromium.org/280753003/diff/60002/chrome/browser/component...
chrome/browser/component_updater/cld_component_installer.cc:128: // so we can
simply return by value (making a copy).
On 2014/05/15 16:03:58, Sorin Jianu wrote:
> @128 Comment not needed as it is both obvious and idiomatic what the getter
> does.

Done.

https://codereview.chromium.org/280753003/diff/60002/chrome/browser/component...
File chrome/browser/component_updater/test/cld_component_installer_unittest.cc
(right):

https://codereview.chromium.org/280753003/diff/60002/chrome/browser/component...
chrome/browser/component_updater/test/cld_component_installer_unittest.cc:55: //
All files are created within a ScopedTempDir, which deletes and all
On 2014/05/15 16:03:58, Sorin Jianu wrote:
> The word and could be removed.

Done.

[Andrew Hayden (chromium.org), 2014-05-16 12:48:57]

The CQ bit was checked by andrewhayden@chromium.org

[commit-bot: I haz the power, 2014-05-16 12:49:17]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/andrewhayden@chromium.org/280753003/11...

[commit-bot: I haz the power, 2014-05-16 14:41:08]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-05-16 15:37:27]

Change committed as 271028