Add unit tests for BackgroundApplicationListModel

Add unit tests for BackgroundApplicationListModel

BUG=73178
TEST=unittests

This adds two tests for the basic list-management functionality of the BackgroundApplicationListManager.  The first minimizes testing logic with explicit adding and removing of Background Apps and other extensions.  The second does more extensive testing over a large series of operations that add and remove Background App and other extensions.

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

[The wrong rickcam account, 2011-02-16 18:07:03]

Please take a look.

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

http://codereview.chromium.org/6525056/diff/5001/chrome/browser/background_ap...
File chrome/browser/background_application_list_model_unittest.cc (right):

http://codereview.chromium.org/6525056/diff/5001/chrome/browser/background_ap...
chrome/browser/background_application_list_model_unittest.cc:37: return
FilePath(FILE_PATH_LITERAL("c:\\foo"));
To avoid having platform-dependent code here, can you just use relative paths
like FILE_PATH_LITERAL("foobar_nonexistent") ?

http://codereview.chromium.org/6525056/diff/5001/chrome/browser/background_ap...
chrome/browser/background_application_list_model_unittest.cc:101:
EXPECT_TRUE(extension) << error;
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.

http://codereview.chromium.org/6525056/diff/5001/chrome/browser/background_ap...
chrome/browser/background_application_list_model_unittest.cc:116:
ASSERT_FALSE(model->size());
It's more readable and generates better error output if you do this:
ASSERT_EQ(0, model->size())

http://codereview.chromium.org/6525056/diff/5001/chrome/browser/background_ap...
chrome/browser/background_application_list_model_unittest.cc:125:
ASSERT_TRUE(model->size() == 0);
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

http://codereview.chromium.org/6525056/diff/5001/chrome/browser/background_ap...
chrome/browser/background_application_list_model_unittest.cc:170: typedef
std::map<std::string,scoped_refptr<Extension> > ExtensionTable;
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?

http://codereview.chromium.org/6525056/diff/5001/chrome/browser/background_ap...
chrome/browser/background_application_list_model_unittest.cc:174:
GenerateRandomExtensionName() {
put return type and function name on same line.

http://codereview.chromium.org/6525056/diff/5001/chrome/browser/background_ap...
chrome/browser/background_application_list_model_unittest.cc:175: static const
char VALID[] = "ABCDEFGHIHJKMNOPabcdefghihjkmnop0123456789 ";
Should you put some non-ascii chars in here (unicode?)

http://codereview.chromium.org/6525056/diff/5001/chrome/browser/background_ap...
chrome/browser/background_application_list_model_unittest.cc:202: static const
unsigned int iterations = 500;
nit: I think the style for contstants is of the form: "kIterations"

http://codereview.chromium.org/6525056/diff/5001/chrome/browser/background_ap...
chrome/browser/background_application_list_model_unittest.cc:237:
ASSERT_TRUE(service->extensions()->size() == count);
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]

Please take another look.

http://codereview.chromium.org/6525056/diff/5001/chrome/browser/background_ap...
File chrome/browser/background_application_list_model_unittest.cc (right):

http://codereview.chromium.org/6525056/diff/5001/chrome/browser/background_ap...
chrome/browser/background_application_list_model_unittest.cc:37: return
FilePath(FILE_PATH_LITERAL("c:\\foo"));
On 2011/02/16 18:45:46, Andrew T Wilson wrote:
> To avoid having platform-dependent code here, can you just use relative paths
> like FILE_PATH_LITERAL("foobar_nonexistent") ?

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.

http://codereview.chromium.org/6525056/diff/5001/chrome/browser/background_ap...
chrome/browser/background_application_list_model_unittest.cc:101:
EXPECT_TRUE(extension) << error;
On 2011/02/16 18:45:46, Andrew T Wilson wrote:
> 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.

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.

http://codereview.chromium.org/6525056/diff/5001/chrome/browser/background_ap...
chrome/browser/background_application_list_model_unittest.cc:116:
ASSERT_FALSE(model->size());
On 2011/02/16 18:45:46, Andrew T Wilson wrote:
> It's more readable and generates better error output if you do this:
> ASSERT_EQ(0, model->size())

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

http://codereview.chromium.org/6525056/diff/5001/chrome/browser/background_ap...
chrome/browser/background_application_list_model_unittest.cc:125:
ASSERT_TRUE(model->size() == 0);
On 2011/02/16 18:45:46, Andrew T Wilson wrote:
> 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

Done.

http://codereview.chromium.org/6525056/diff/5001/chrome/browser/background_ap...
chrome/browser/background_application_list_model_unittest.cc:170: typedef
std::map<std::string,scoped_refptr<Extension> > ExtensionTable;
On 2011/02/16 18:45:46, Andrew T Wilson wrote:
> 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?

Done.

http://codereview.chromium.org/6525056/diff/5001/chrome/browser/background_ap...
chrome/browser/background_application_list_model_unittest.cc:174:
GenerateRandomExtensionName() {
On 2011/02/16 18:45:46, Andrew T Wilson wrote:
> put return type and function name on same line.

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.

http://codereview.chromium.org/6525056/diff/5001/chrome/browser/background_ap...
chrome/browser/background_application_list_model_unittest.cc:175: static const
char VALID[] = "ABCDEFGHIHJKMNOPabcdefghihjkmnop0123456789 ";
On 2011/02/16 18:45:46, Andrew T Wilson wrote:
> Should you put some non-ascii chars in here (unicode?)

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.

http://codereview.chromium.org/6525056/diff/5001/chrome/browser/background_ap...
chrome/browser/background_application_list_model_unittest.cc:202: static const
unsigned int iterations = 500;
On 2011/02/16 18:45:46, Andrew T Wilson wrote:
> nit: I think the style for contstants is of the form: "kIterations"

Done.

http://codereview.chromium.org/6525056/diff/5001/chrome/browser/background_ap...
chrome/browser/background_application_list_model_unittest.cc:237:
ASSERT_TRUE(service->extensions()->size() == count);
On 2011/02/16 18:45:46, Andrew T Wilson wrote:
> 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 ...

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.

[Andrew T Wilson (Slow), 2011-02-17 05:56:46]

LGTM if you address my comments below.

http://codereview.chromium.org/6525056/diff/7001/chrome/browser/background_ap...
File chrome/browser/background_application_list_model_unittest.cc (right):

http://codereview.chromium.org/6525056/diff/7001/chrome/browser/background_ap...
chrome/browser/background_application_list_model_unittest.cc:9: #include <set>
Remove duplicate #include <set> line.

http://codereview.chromium.org/6525056/diff/7001/chrome/browser/background_ap...
chrome/browser/background_application_list_model_unittest.cc:172:
GenerateUniqueExtensionName() {
Looks like the return value is still on a separate line?

http://codereview.chromium.org/6525056/diff/7001/chrome/browser/background_ap...
chrome/browser/background_application_list_model_unittest.cc:196: unsigned int
count = 0;
This should either be int or size_t, whichever is more convenient. Likewise for
expected.

http://codereview.chromium.org/6525056/diff/7001/chrome/browser/background_ap...
chrome/browser/background_application_list_model_unittest.cc:199: for (unsigned
int index = 0; index < kIterations; ++index) {
Use int here, not "unsigned int":

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

File this under "style decisions Drew is deeply ambivalent about".

[The wrong rickcam account, 2011-02-17 18:24:01]

Please take another look even though you said LGTM :-)

I made the changes that you listed, plus a couple of similar issues, and made
one unrelated change that I described to you yesterday afternoon.

http://codereview.chromium.org/6525056/diff/7001/chrome/browser/background_ap...
File chrome/browser/background_application_list_model_unittest.cc (right):

http://codereview.chromium.org/6525056/diff/7001/chrome/browser/background_ap...
chrome/browser/background_application_list_model_unittest.cc:9: #include <set>
On 2011/02/17 05:56:47, Andrew T Wilson wrote:
> Remove duplicate #include <set> line.

Done.  A couple of others there were leftovers, so they're gone as well.

http://codereview.chromium.org/6525056/diff/7001/chrome/browser/background_ap...
chrome/browser/background_application_list_model_unittest.cc:172:
GenerateUniqueExtensionName() {
On 2011/02/17 05:56:47, Andrew T Wilson wrote:
> Looks like the return value is still on a separate line?

Done.  For real this time.

http://codereview.chromium.org/6525056/diff/7001/chrome/browser/background_ap...
chrome/browser/background_application_list_model_unittest.cc:194: static const
unsigned int kIterations = 500;
Making this int as well.

http://codereview.chromium.org/6525056/diff/7001/chrome/browser/background_ap...
chrome/browser/background_application_list_model_unittest.cc:196: unsigned int
count = 0;
On 2011/02/17 05:56:47, Andrew T Wilson wrote:
> This should either be int or size_t, whichever is more convenient. Likewise
for
> expected.

Done.

http://codereview.chromium.org/6525056/diff/7001/chrome/browser/background_ap...
chrome/browser/background_application_list_model_unittest.cc:199: for (unsigned
int index = 0; index < kIterations; ++index) {
On 2011/02/17 05:56:47, Andrew T Wilson wrote:
> Use int here, not "unsigned int":
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Intege...
> 
> File this under "style decisions Drew is deeply ambivalent about".

Done.

http://codereview.chromium.org/6525056/diff/7001/chrome/browser/background_ap...
chrome/browser/background_application_list_model_unittest.cc:217: } else {  //
Maybe remove an extension.
I decided to make that minor logic tweak that I mentioned yesterday.  I think
that testing with empty() rather than begin==end is marginally more readable.

http://codereview.chromium.org/6525056/diff/7001/chrome/browser/background_ap...
chrome/browser/background_application_list_model_unittest.cc:229: for (unsigned
int index = 0; index < offset; ++index)
I made this int as well.

[Andrew T Wilson (Slow), 2011-02-17 18:43:14]

LGTM with two nits.

http://codereview.chromium.org/6525056/diff/13001/chrome/browser/background_a...
File chrome/browser/background_application_list_model_unittest.cc (right):

http://codereview.chromium.org/6525056/diff/13001/chrome/browser/background_a...
chrome/browser/background_application_list_model_unittest.cc:169: static size_t
uniqueness = 0;
Actually, I think this should just be an int, since uniqueness doesn't contain
the size of anything.

http://codereview.chromium.org/6525056/diff/13001/chrome/browser/background_a...
chrome/browser/background_application_list_model_unittest.cc:213: } else if
(extensions.empty()) {
nit: It was more readable before when this was just an else { // maybe remove an
extension

Now you have:

if (random() % 2) {
   ... do stuff ...
} else if (extensions.empty()) {
   ... more stuff...
} else {
   ... more ...
}

And it's not clear what extensions.empty() has to do with the first if.

I think it's clearer as:

if (random() % 2) { // Add an extension
   ...
} else { // Maybe remove an extension if there are any.
  if (extensions.empty()) {
    ...asserts...
  } else {
    // Randomly select an extension to remove
  }
}

Anyhow, this is a total nit so feel free to ignore.

[The wrong rickcam account, 2011-02-17 23:50:55]

Moving forward with dcommit now.  Thanks.

http://codereview.chromium.org/6525056/diff/13001/chrome/browser/background_a...
File chrome/browser/background_application_list_model_unittest.cc (right):

http://codereview.chromium.org/6525056/diff/13001/chrome/browser/background_a...
chrome/browser/background_application_list_model_unittest.cc:169: static size_t
uniqueness = 0;
On 2011/02/17 18:43:14, Andrew T Wilson wrote:
> Actually, I think this should just be an int, since uniqueness doesn't contain
> the size of anything.

Done.

http://codereview.chromium.org/6525056/diff/13001/chrome/browser/background_a...
chrome/browser/background_application_list_model_unittest.cc:194:
srandom(RANDOM_SEED);
srand/rand are considered more standard(*) to substituting them in for
srandom/random.

(*)  In particular, MS Windows builds fail with srandom/random even when
including <cstdlib> as directed by relevant man pages.  NOTE: The man pages say
that srandom/random are POSIX, but I found people commenting on the web saying
that srandom/random are missing from various places on MS Windows because they
are not standard.

http://codereview.chromium.org/6525056/diff/13001/chrome/browser/background_a...
chrome/browser/background_application_list_model_unittest.cc:213: } else if
(extensions.empty()) {
On 2011/02/17 18:43:14, Andrew T Wilson wrote:
> nit: It was more readable before when this was just an else { // maybe remove
an
> extension
> 
> Now you have:
> 
> if (random() % 2) {
>    ... do stuff ...
> } else if (extensions.empty()) {
>    ... more stuff...
> } else {
>    ... more ...
> }
> 
> And it's not clear what extensions.empty() has to do with the first if.
> 
> I think it's clearer as:
> 
> if (random() % 2) { // Add an extension
>    ...
> } else { // Maybe remove an extension if there are any.
>   if (extensions.empty()) {
>     ...asserts...
>   } else {
>     // Randomly select an extension to remove
>   }
> }
> 
> Anyhow, this is a total nit so feel free to ignore.

Done.  I'm going with the theory that the reader is the better judge of
readability :-)