Cleanup in base/test.

Cleanup in base/test.

This is related to http://codereview.chromium.org/7003028/.

BUG=60476
TEST=None


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

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

[Joao da Silva, 2011-06-10 15:53:55]

@Paweł: This is meant to address your comments on
http://codereview.chromium.org/7003028/, which unfortunately I committed before
getting your approval :-/

I've addressed your original comments and quoted them here again.

@brettw: please review the changes in base/.

Thanks!

http://codereview.chromium.org/7129056/diff/3001/base/test/scoped_locale.cc
File base/test/scoped_locale.cc (right):

http://codereview.chromium.org/7129056/diff/3001/base/test/scoped_locale.cc#n...
base/test/scoped_locale.cc:13: current_locale_ = setlocale(LC_ALL,
locale.c_str());
> Please check the return value. NULL would indicate failure.
We can't do much in the ctor about it, so I've added the current_locale() getter
to allow callers to check if the call succeeded. wdyt?

http://codereview.chromium.org/7129056/diff/3001/base/test/scoped_locale.h
File base/test/scoped_locale.h (right):

http://codereview.chromium.org/7129056/diff/3001/base/test/scoped_locale.h#ne...
base/test/scoped_locale.h:19: explicit ScopedSetLocale(const std::string&
locale);
> nit: Could you change the type of the parameter to const std::string& or maybe
> StringPiece to make it easier to pass strings there without c_str?
Done. I didn't use a StringPiece because is stores a ptr to string.data() and
its size, but setlocale() expects a nul-terminated string.

http://codereview.chromium.org/7129056/diff/3001/base/test/test_util.h
File base/test/test_util.h (left):

http://codereview.chromium.org/7129056/diff/3001/base/test/test_util.h#oldcode41
base/test/test_util.h:41: #endif  // BASE_TEST_TEST_UTIL_H_
> Please don't add yet another _util file. How about naming it scoped_locale.h?
> Also, please split it into .h and .cc file, with most of the implementation in
> the .cc file.
Done, see scoped_locale.[h,cc].

http://codereview.chromium.org/7129056/diff/3001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/7129056/diff/3001/chrome/chrome_tests.gypi#new...
chrome/chrome_tests.gypi:2657: 'common/time_format_browsertest.cc',
> Please handle only running on POSIX in gyp, not in the .cc file.
Done. Please let me know if this should be done differently here.

[Paweł Hajdan Jr., 2011-06-10 17:00:51]

Just minor comments, thanks!

http://codereview.chromium.org/7129056/diff/3001/base/test/scoped_locale.cc
File base/test/scoped_locale.cc (right):

http://codereview.chromium.org/7129056/diff/3001/base/test/scoped_locale.cc#n...
base/test/scoped_locale.cc:7: #include "base/test/scoped_locale.h"
nit: This must always be the first header.

http://codereview.chromium.org/7129056/diff/3001/base/test/scoped_locale.cc#n...
base/test/scoped_locale.cc:13: current_locale_ = setlocale(LC_ALL,
locale.c_str());
On 2011/06/10 15:53:56, joaodasilva wrote:
> > Please check the return value. NULL would indicate failure.
> We can't do much in the ctor about it, so I've added the current_locale()
getter
> to allow callers to check if the call succeeded. wdyt?

I'd prefer using gtest's EXPECT_ macro and getting rid of the getter. Please
also check the return value in the dtor.

http://codereview.chromium.org/7129056/diff/3001/base/test/scoped_locale.h
File base/test/scoped_locale.h (right):

http://codereview.chromium.org/7129056/diff/3001/base/test/scoped_locale.h#ne...
base/test/scoped_locale.h:5: #ifndef BASE_TEST_TEST_UTIL_H_
nit: The #include guards should be updated.

Also, please keep the file name in sync with the class name. I'd suggest
changing the class name to ScopedLocale.

http://codereview.chromium.org/7129056/diff/3001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/7129056/diff/3001/chrome/chrome_tests.gypi#new...
chrome/chrome_tests.gypi:2657: 'common/time_format_browsertest.cc',
On 2011/06/10 15:53:56, joaodasilva wrote:
> > Please handle only running on POSIX in gyp, not in the .cc file.
> Done. Please let me know if this should be done differently here.

I think it would be slightly better to flip the condition, i.e. exclude the test
(sources!) when os_posix == 0.

[brettw, 2011-06-12 14:07:20]

LGTM

[Joao da Silva, 2011-06-14 16:48:44]

@Paweł: reviewed, please have another look.
Thanks!

http://codereview.chromium.org/7129056/diff/3001/base/test/scoped_locale.cc
File base/test/scoped_locale.cc (right):

http://codereview.chromium.org/7129056/diff/3001/base/test/scoped_locale.cc#n...
base/test/scoped_locale.cc:7: #include "base/test/scoped_locale.h"
On 2011/06/10 17:00:51, Paweł Hajdan Jr. wrote:
> nit: This must always be the first header.

Done.

http://codereview.chromium.org/7129056/diff/3001/base/test/scoped_locale.cc#n...
base/test/scoped_locale.cc:13: current_locale_ = setlocale(LC_ALL,
locale.c_str());
On 2011/06/10 17:00:51, Paweł Hajdan Jr. wrote:
> On 2011/06/10 15:53:56, joaodasilva wrote:
> > > Please check the return value. NULL would indicate failure.
> > We can't do much in the ctor about it, so I've added the current_locale()
> getter
> > to allow callers to check if the call succeeded. wdyt?
> 
> I'd prefer using gtest's EXPECT_ macro and getting rid of the getter. Please
> also check the return value in the dtor.
I tried that (see patch set 4), and it worked fine on the mac bots but not on
linux, since the fr_FR locale isn't installed.

To make it work we can either 1) require all bots and developers to install the
locale or 2) make the tests that depend on this optional. I prefer 2 since it
spares all the updates, and the test is still executed on the mac bot (since mac
comes with fr_FR locale). wdyt?

http://codereview.chromium.org/7129056/diff/3001/base/test/scoped_locale.h
File base/test/scoped_locale.h (right):

http://codereview.chromium.org/7129056/diff/3001/base/test/scoped_locale.h#ne...
base/test/scoped_locale.h:5: #ifndef BASE_TEST_TEST_UTIL_H_
On 2011/06/10 17:00:51, Paweł Hajdan Jr. wrote:
> nit: The #include guards should be updated.
> 
> Also, please keep the file name in sync with the class name. I'd suggest
> changing the class name to ScopedLocale.

Done.

http://codereview.chromium.org/7129056/diff/3001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/7129056/diff/3001/chrome/chrome_tests.gypi#new...
chrome/chrome_tests.gypi:2657: 'common/time_format_browsertest.cc',
On 2011/06/10 17:00:51, Paweł Hajdan Jr. wrote:
> On 2011/06/10 15:53:56, joaodasilva wrote:
> > > Please handle only running on POSIX in gyp, not in the .cc file.
> > Done. Please let me know if this should be done differently here.
> 
> I think it would be slightly better to flip the condition, i.e. exclude the
test
> (sources!) when os_posix == 0.

Done.

[Paweł Hajdan Jr., 2011-06-14 20:20:07]

http://codereview.chromium.org/7129056/diff/3001/base/test/scoped_locale.cc
File base/test/scoped_locale.cc (right):

http://codereview.chromium.org/7129056/diff/3001/base/test/scoped_locale.cc#n...
base/test/scoped_locale.cc:13: current_locale_ = setlocale(LC_ALL,
locale.c_str());
On 2011/06/14 16:48:44, joaodasilva wrote:
> On 2011/06/10 17:00:51, Paweł Hajdan Jr. wrote:
> > On 2011/06/10 15:53:56, joaodasilva wrote:
> > > > Please check the return value. NULL would indicate failure.
> > > We can't do much in the ctor about it, so I've added the current_locale()
> > getter
> > > to allow callers to check if the call succeeded. wdyt?
> > 
> > I'd prefer using gtest's EXPECT_ macro and getting rid of the getter. Please
> > also check the return value in the dtor.
> I tried that (see patch set 4), and it worked fine on the mac bots but not on
> linux, since the fr_FR locale isn't installed.
> 
> To make it work we can either 1) require all bots and developers to install
the
> locale or 2) make the tests that depend on this optional. I prefer 2 since it
> spares all the updates, and the test is still executed on the mac bot (since
mac
> comes with fr_FR locale). wdyt?

Can we get the locale installed on the bots? The problem was silent up to this
point, and we shouldn't put things into infrastructure that encourage silent
failures.

[Joao da Silva, 2011-06-15 10:15:13]

Please have a look at the inline comment.

http://codereview.chromium.org/7129056/diff/3001/base/test/scoped_locale.cc
File base/test/scoped_locale.cc (right):

http://codereview.chromium.org/7129056/diff/3001/base/test/scoped_locale.cc#n...
base/test/scoped_locale.cc:13: current_locale_ = setlocale(LC_ALL,
locale.c_str());
On 2011/06/14 20:20:07, Paweł Hajdan Jr. wrote:
> On 2011/06/14 16:48:44, joaodasilva wrote:
> > On 2011/06/10 17:00:51, Paweł Hajdan Jr. wrote:
> > > On 2011/06/10 15:53:56, joaodasilva wrote:
> > > > > Please check the return value. NULL would indicate failure.
> > > > We can't do much in the ctor about it, so I've added the
current_locale()
> > > getter
> > > > to allow callers to check if the call succeeded. wdyt?
> > > 
> > > I'd prefer using gtest's EXPECT_ macro and getting rid of the getter.
Please
> > > also check the return value in the dtor.
> > I tried that (see patch set 4), and it worked fine on the mac bots but not
on
> > linux, since the fr_FR locale isn't installed.
> > 
> > To make it work we can either 1) require all bots and developers to install
> the
> > locale or 2) make the tests that depend on this optional. I prefer 2 since
it
> > spares all the updates, and the test is still executed on the mac bot (since
> mac
> > comes with fr_FR locale). wdyt?
> 
> Can we get the locale installed on the bots? The problem was silent up to this
> point, and we shouldn't put things into infrastructure that encourage silent
> failures.

I'll try to get the locale on the linux bots.

Notice that the test won't fail silently on the infrastructure, since at least
the mac bots will run it.

I'd still keep the code as is, since failing when the locale isn't installed
will make the test fail on lots of developers' machines, and people will likely
waste some time trying to "fix" it.

Let me know how to proceed, thanks!

[Paweł Hajdan Jr., 2011-06-16 10:33:27]

http://codereview.chromium.org/7129056/diff/3001/base/test/scoped_locale.cc
File base/test/scoped_locale.cc (right):

http://codereview.chromium.org/7129056/diff/3001/base/test/scoped_locale.cc#n...
base/test/scoped_locale.cc:13: current_locale_ = setlocale(LC_ALL,
locale.c_str());
On 2011/06/15 10:15:13, joaodasilva wrote:
> I'll try to get the locale on the linux bots.

Thanks, I've noticed the bug - sounds good.

> Notice that the test won't fail silently on the infrastructure, since at least
> the mac bots will run it.

Feel free to commit with error checking and tests enabled just on Mac.

> I'd still keep the code as is, since failing when the locale isn't installed
> will make the test fail on lots of developers' machines, and people will
likely
> waste some time trying to "fix" it.

Can we get the locale installed at dev machines as well? The same point can be
used that if they don't have locale now, the test is effectively a no-op on
those machines, which is similarly confusing.

[Joao da Silva, 2011-06-16 11:12:17]

http://codereview.chromium.org/7129056/diff/3001/base/test/scoped_locale.cc
File base/test/scoped_locale.cc (right):

http://codereview.chromium.org/7129056/diff/3001/base/test/scoped_locale.cc#n...
base/test/scoped_locale.cc:13: current_locale_ = setlocale(LC_ALL,
locale.c_str());
> Feel free to commit with error checking and tests enabled just on Mac.
It's not clear to me what you mean here. Did you mean...
1. ok to commit as is; linux bots will print a warning until the locale is
installed
2. run the test on mac only for now (change os_posix==0 to OS != mac)
Sorry for the confusion. If it's 1, I need the magic word :-)

> Can we get the locale installed at dev machines as well? The same point can be
> used that if they don't have locale now, the test is effectively a no-op on
> those machines, which is similarly confusing.
I just want to save everyone's time by not making it a requirement. If someone
introduces a regression against this test, their try jobs will fail and they can
then install the locales locally. The vast majority who won't deal with this
code doesn't have to bother.

[Paweł Hajdan Jr., 2011-06-16 11:46:19]

http://codereview.chromium.org/7129056/diff/3001/base/test/scoped_locale.cc
File base/test/scoped_locale.cc (right):

http://codereview.chromium.org/7129056/diff/3001/base/test/scoped_locale.cc#n...
base/test/scoped_locale.cc:13: current_locale_ = setlocale(LC_ALL,
locale.c_str());
On 2011/06/16 11:12:18, joaodasilva wrote:
> > Feel free to commit with error checking and tests enabled just on Mac.
> It's not clear to me what you mean here. Did you mean...
> 1. ok to commit as is; linux bots will print a warning until the locale is
> installed
> 2. run the test on mac only for now (change os_posix==0 to OS != mac)
> Sorry for the confusion. If it's 1, I need the magic word :-)

Sorry, I think I meant

3. actually use EXPECT_TRUE to verify setlocale results and get rid of any
accessors added previously. Then enable the tests just on the Mac, because
they'd fail on Linux before locales are installed.

> > Can we get the locale installed at dev machines as well? The same point can
be
> > used that if they don't have locale now, the test is effectively a no-op on
> > those machines, which is similarly confusing.
> I just want to save everyone's time by not making it a requirement. If someone
> introduces a regression against this test, their try jobs will fail and they
can
> then install the locales locally. The vast majority who won't deal with this
> code doesn't have to bother.

I don't think it's right argument. Tests should behave same everywhere, to the
max degree possible. If we allow the dev environment to diverge too much from
the bot environment, the debugging difference cost will outweigh any "savings".

Ah, and for Linux we have build/install-build-deps.sh. All you'd need to do is
to figure out the right package name for *buntu.

[Joao da Silva, 2011-06-16 15:28:23]

Thanks for explaining, I'm now convinced :-)

The test has been marked with FAILS_ except on mac.
Please have another look, also at the build/install-build-deps.sh change.

[Joao da Silva, 2011-06-17 11:46:00]

The infrastructure team has installed the locale on the bots, so the test can be
enabled on all posix platforms.

build/install-build-deps.sh has already been updated on trunk, so I've removed
that change.

Please review again, thanks!

[Paweł Hajdan Jr., 2011-06-17 11:58:57]

LGTM

[commit-bot: I haz the power, 2011-06-20 14:29:14]

Change committed as 89664

[commit-bot: I haz the power, 2011-06-20 17:44:07]

Change committed as 89681