Cleanup in base/test.

base/test/scoped_locale.cc

+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <locale.h>
+
+#include "base/test/scoped_locale.h"

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

nit: This must always be the first header.

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

Done.

+
+namespace base {
+
+ScopedSetLocale::ScopedSetLocale(const std::string& locale) {
+  prev_locale_ = setlocale(LC_ALL, NULL);
+  current_locale_ = setlocale(LC_ALL, locale.c_str());

[Joao da Silva, 2011/06/10 15:53:56]

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?

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

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

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

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?

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

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]

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]

Thanks, I've noticed the bug - sounds good.
Feel free to commit with error checking and tests enabled just on Mac.
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:18]

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 :-)
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]

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.
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.

+}
+
+ScopedSetLocale::~ScopedSetLocale() {
+  setlocale(LC_ALL, prev_locale_);
+}
+
+}  // namespace base