Adding unit tests for google_api_keys functionality.

Adding unit tests for google_api_keys functionality.

TBR=ben@chromium.org
BUG=145584
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157972

[Jói, 2012-09-19 15:08:14]

Hi MAD,

Another API keys change for you to review, this one to add unit tests.

The unit test is a bit unorthodox since I need to test that the preprocessor
overrides actually work, so I'm including a .cc file multiple times in different
namespaces and doing things like undefing the flags used for keys, and the
relevant include header guards, to make this work.  The bottom line is, it works
and is the closest thing to a real-life test I could think of.

The move of the unit test files from google_apis/google_apis.gyp to
chrome_tests.gypi is because it turns out the tests' global objects don't get
linked in if they are in a separate library.  Running these tests from the
'unit_tests' target is temporary anyway, I have an item on my list to move them
to a separate unit test executable.

Cheers,
Jói

[MAD, 2012-09-20 15:39:39]

A few comments/requests/questions...

Cool tricks by the way. ;-)

http://codereview.chromium.org/10941034/diff/1/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/10941034/diff/1/chrome/chrome_tests.gypi#newco...
chrome/chrome_tests.gypi:2629: # We don't need the rest of what we do for the
google_apis
Weird... I find this hard to process when I read this.
Can you rephrase please?

http://codereview.chromium.org/10941034/diff/1/google_apis/google_api_keys_un...
File google_apis/google_api_keys_unittest.cc (right):

http://codereview.chromium.org/10941034/diff/1/google_apis/google_api_keys_un...
google_apis/google_api_keys_unittest.cc:29: std::string variable_name;
Why not use const char* or []?

http://codereview.chromium.org/10941034/diff/1/google_apis/google_api_keys_un...
google_apis/google_api_keys_unittest.cc:37: env_cache_[0].variable_name =
"GOOGLE_API_KEY";
Shouldn't you initialize was_set to false? Otherwise it's value is undefined
between construction and SetUp (though it may not matter too much, it makes me
hitch :-).

http://codereview.chromium.org/10941034/diff/1/google_apis/google_api_keys_un...
google_apis/google_api_keys_unittest.cc:54: cache.value = "";
use .clear() instead?

http://codereview.chromium.org/10941034/diff/1/google_apis/google_api_keys_un...
google_apis/google_api_keys_unittest.cc:74: EnvironmentCache env_cache_[3 + 2 *
google_apis::CLIENT_NUM_ITEMS];
Maybe a comment to explain why 3 + 2 * ...

http://codereview.chromium.org/10941034/diff/1/google_apis/google_apis.gyp
File google_apis/google_apis.gyp (left):

http://codereview.chromium.org/10941034/diff/1/google_apis/google_apis.gyp#ol...
google_apis/google_apis.gyp:91: 'target_name': 'google_apis_unittests',
Please check with chrome-infrastructure to see if they need to update buildbot
scripts or not... I know we used to, but I'm not sure if this has been automated
since then...

[Jói, 2012-09-20 16:25:12]

Thanks MAD!

Cheers,
Jói

http://codereview.chromium.org/10941034/diff/1/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/10941034/diff/1/chrome/chrome_tests.gypi#newco...
chrome/chrome_tests.gypi:2629: # We don't need the rest of what we do for the
google_apis
On 2012/09/20 15:39:39, MAD wrote:
> Weird... I find this hard to process when I read this.
> Can you rephrase please?

This made more sense in the context of google_apis.gyp, where it was in an
earlier version of this patch (that didn't work because none of the unit tests
ran :).  In the context of chrome_tests.gypi it needs no comment except the TODO
comment, so I removed all but that.

http://codereview.chromium.org/10941034/diff/1/google_apis/google_api_keys_un...
File google_apis/google_api_keys_unittest.cc (right):

http://codereview.chromium.org/10941034/diff/1/google_apis/google_api_keys_un...
google_apis/google_api_keys_unittest.cc:29: std::string variable_name;
On 2012/09/20 15:39:39, MAD wrote:
> Why not use const char* or []?

Just to avoid having to worry about buffer overflows, memory management etc. 
But I realize now that I'm having this point at string constants all the time
anyway.  Changed.

http://codereview.chromium.org/10941034/diff/1/google_apis/google_api_keys_un...
google_apis/google_api_keys_unittest.cc:37: env_cache_[0].variable_name =
"GOOGLE_API_KEY";
On 2012/09/20 15:39:39, MAD wrote:
> Shouldn't you initialize was_set to false? Otherwise it's value is undefined
> between construction and SetUp (though it may not matter too much, it makes me
> hitch :-).

Yeah, it doesn't matter in this case, but like you I also prefer to always
initialize (mostly because it helps maintainability), but here it is a bit of
extra work.  Since this stung you too, I've fixed it by giving the
EnvironmentCache struct a default constructor to initialize.  I checked the
style guide and providing a constructor, destructor, and members like Init(),
Reset() and Validate() is OK on a struct, see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Struct...

http://codereview.chromium.org/10941034/diff/1/google_apis/google_api_keys_un...
google_apis/google_api_keys_unittest.cc:54: cache.value = "";
On 2012/09/20 15:39:39, MAD wrote:
> use .clear() instead?

Done.

http://codereview.chromium.org/10941034/diff/1/google_apis/google_api_keys_un...
google_apis/google_api_keys_unittest.cc:74: EnvironmentCache env_cache_[3 + 2 *
google_apis::CLIENT_NUM_ITEMS];
On 2012/09/20 15:39:39, MAD wrote:
> Maybe a comment to explain why 3 + 2 * ...

Done.

http://codereview.chromium.org/10941034/diff/1/google_apis/google_apis.gyp
File google_apis/google_apis.gyp (left):

http://codereview.chromium.org/10941034/diff/1/google_apis/google_apis.gyp#ol...
google_apis/google_apis.gyp:91: 'target_name': 'google_apis_unittests',
On 2012/09/20 15:39:39, MAD wrote:
> Please check with chrome-infrastructure to see if they need to update buildbot
> scripts or not... I know we used to, but I'm not sure if this has been
automated
> since then...

I believe there is still manual set-up, but nothing will be required until I
break these tests into their own separate unit test executable; currently they
are built and they run as part of 'unit_tests'.

[MAD, 2012-09-20 18:07:47]

Excellent! Thanks for the fixes/answers...

Just one more question and then you're good to go!

LGTM!

BYE
MAD...

http://codereview.chromium.org/10941034/diff/6001/google_apis/google_api_keys...
File google_apis/google_api_keys_unittest.cc (right):

http://codereview.chromium.org/10941034/diff/6001/google_apis/google_api_keys...
google_apis/google_api_keys_unittest.cc:41: 
Why two blank lines?

[MAD, 2012-09-20 18:07:47]

Excellent! Thanks for the fixes/answers...

Just one more question and then you're good to go!

LGTM!

BYE
MAD...

http://codereview.chromium.org/10941034/diff/6001/google_apis/google_api_keys...
File google_apis/google_api_keys_unittest.cc (right):

http://codereview.chromium.org/10941034/diff/6001/google_apis/google_api_keys...
google_apis/google_api_keys_unittest.cc:41: 
Why two blank lines?

[Jói, 2012-09-20 18:10:52]

> Why two blank lines?

Thanks for noticing, that was just an accident.  Fixed that, will
upload and flip the CQ bit.

Cheers,
Jói


On Thu, Sep 20, 2012 at 6:07 PM,  <mad@chromium.org> wrote:
> Excellent! Thanks for the fixes/answers...
>
> Just one more question and then you're good to go!
>
> LGTM!
>
> BYE
> MAD...
>
>
>
>
http://codereview.chromium.org/10941034/diff/6001/google_apis/google_api_keys...
> File google_apis/google_api_keys_unittest.cc (right):
>
>
http://codereview.chromium.org/10941034/diff/6001/google_apis/google_api_keys...
> google_apis/google_api_keys_unittest.cc:41:
> Why two blank lines?
>
> http://codereview.chromium.org/10941034/

[commit-bot: I haz the power, 2012-09-20 18:12:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/10941034/6003

[commit-bot: I haz the power, 2012-09-20 18:41:48]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-09-21 09:38:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/10941034/6003

[commit-bot: I haz the power, 2012-09-21 09:56:50]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[Jói, 2012-09-21 11:12:45]

On 2012/09/21 09:56:50, I haz the power (commit-bot) wrote:
> Sorry for I got bad news for ya.
> Compile failed with a clobber build.
> Your code is likely broken or HEAD is junk. Please ensure your
> code is not broken then alert the build sheriffs.
> Look at the try server FAQ for more details.

MAD, it looks like the Win linker barfs on my tricks, and the Android linker
isn't able to decorate names well enough to separate the different namespace
declarations, so I will just limit this test to Mac and Linux, that should be
plenty of coverage for the functionality since there are absolutely no
platform-specific bits.

Cheers,
Jói

[commit-bot: I haz the power, 2012-09-21 11:18:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/10941034/5005

[MAD, 2012-09-21 11:40:01]

On 2012/09/21 11:18:00, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/joi%40chromium.org/10941034/5005

Ok, cool, lgtm!

[commit-bot: I haz the power, 2012-09-21 13:55:48]

Change committed as 157972