Restore locales after tests

Restore locales after tests

Locales should be restored after tests and scoping should be used. Added restores and one restore changed to scoped version.

Committed: https://crrev.com/3be75bf76ac061129ec2a9e887d5d50cfd016a56
Cr-Commit-Position: refs/heads/master@{#344453}

[olli.syrjala, 2015-05-29 12:06:05]

olli.syrjala@intel.com changed reviewers:
+ jshin@chromium.org

[olli.syrjala, 2015-05-29 12:44:33]

Could you review

[Olli Raula, 2015-06-12 08:02:29]

Hi, could you review,

I changed my name.

[Olli Raula, 2015-07-09 12:20:50]

Ping, could you review this?

[Olli Raula, 2015-08-17 07:53:04]

olli.raula@intel.com changed reviewers:
+ mark@chromium.org

[Olli Raula, 2015-08-17 07:53:39]

Mark: can you review as Jungshik is not awailable

[Mark Mentovai, 2015-08-17 14:17:27]

These should use scopers so that an early return (such as via an ASSERT_*()
macro) also allow the default locale to be restored.

[Olli Raula, 2015-08-18 13:00:17]

Thanks for review! Issues fixed. Fixed also same issue in
case_conversion_unittest.cc

[Mark Mentovai, 2015-08-18 13:18:14]

https://codereview.chromium.org/1162603004/diff/40001/base/i18n/number_format...
File base/i18n/number_formatting_unittest.cc (right):

https://codereview.chromium.org/1162603004/diff/40001/base/i18n/number_format...
base/i18n/number_formatting_unittest.cc:16: class ScopedSetICUDefaultLocale {
You shouldn’t have to write this class in every file that needs it. Put it
someplace common and let the various tests depend on it. icu_test_util.{cc,h}
sounds like it’d be right.

[Olli Raula, 2015-08-18 14:10:44]

Thanks for comments, lets do that way then. 

Is there some reason why not all of those are in namespace i18n?

[Mark Mentovai, 2015-08-18 14:13:19]

Probably no good reason. We’re not always consistent about how we namespace
tests.

My preference would be namespace base { namespace i18n { namespace { } } }.
Using base::i18n as the namespace keeps you from having to type base::i18n:: or
i18n:: everywhere, but that’s not always such a big deal.

I don’t see any new files in your change, or any gyp/gn changes.

[Olli Raula, 2015-08-18 14:29:29]

Ah yeh, now it is there :)

[Mark Mentovai, 2015-08-18 14:40:08]

https://codereview.chromium.org/1162603004/diff/80001/base/i18n/icu_test_util.h
File base/i18n/icu_test_util.h (right):

https://codereview.chromium.org/1162603004/diff/80001/base/i18n/icu_test_util...
base/i18n/icu_test_util.h:5: #ifndef ICU_TEST_UTIL_H_
Full paths: BASE_ICU_TEST_UTIL_H_.

https://codereview.chromium.org/1162603004/diff/80001/base/i18n/icu_test_util...
base/i18n/icu_test_util.h:14: std::string default_locale;
Please follow the style guide: public: before private:, this should be named
default_locale_ with a trailing underscore, and the class should be
DISALLOW_COPY_AND_ASSIGN().

https://google-styleguide.googlecode.com/svn/trunk/cppguide.html

https://codereview.chromium.org/1162603004/diff/80001/base/i18n/icu_test_util...
base/i18n/icu_test_util.h:17: ScopedSetICUDefaultLocale(std::string locale)
Use proper indentation per the style guide.

https://codereview.chromium.org/1162603004/diff/80001/base/i18n/icu_test_util...
base/i18n/icu_test_util.h:17: ScopedSetICUDefaultLocale(std::string locale)
Since every caller passes uloc_getDefault(), the constructor shouldn’t take any
arguments, but should store uloc_getDefault() into default_locale_.

(#include uloc.h for uloc_getDefault(), too.)

https://codereview.chromium.org/1162603004/diff/80001/base/i18n/icu_test_util...
base/i18n/icu_test_util.h:18: : default_locale(locale) {}
You should split this up into a .h and a .cc. They should both be listed in the
.gyp and .gn file for the test target. This isn’t critical enough to need to be
inlined, and putting the definitions in a .cc that’s in the test target will
prevent anyone from accidentally trying to use this test_util in non-test code.

https://codereview.chromium.org/1162603004/diff/80001/base/i18n/icu_test_util...
base/i18n/icu_test_util.h:28: #endif
Blank line before, and make this

#endif  // BASE_ICU_TEST_UTIL_H_

[Olli Raula, 2015-08-19 12:41:03]

Fixed, (also removed one space from authors).

[Mark Mentovai, 2015-08-19 13:37:16]

LG with these changes.

https://codereview.chromium.org/1162603004/diff/140001/base/i18n/case_convers...
File base/i18n/case_conversion_unittest.cc (right):

https://codereview.chromium.org/1162603004/diff/140001/base/i18n/case_convers...
base/i18n/case_conversion_unittest.cc:62: ScopedSetICUDefaultLocale
restore_object;
Seeing ScopedSetICUDefaultLocale followed by SetICUDefaultLocale looks kind of
weird. The first one is more setting up for a restore, so let’s name it
ScopedRestoreICUDefaultLocale. And the object name, restore_object, is kind of
funny. Everything’s an object, but we don’t normally put “object” in the name.
How about restore_locale?

https://codereview.chromium.org/1162603004/diff/140001/base/test/icu_test_uti...
File base/test/icu_test_util.cc (right):

https://codereview.chromium.org/1162603004/diff/140001/base/test/icu_test_uti...
base/test/icu_test_util.cc:5: #include "base/i18n/rtl.h"
#include icu_test_util.h first, because it’s associated with this file. Then,
leave a blank line and have the other two #includes.

https://codereview.chromium.org/1162603004/diff/140001/base/test/icu_test_uti...
base/test/icu_test_util.cc:12: default_locale_ = uloc_getDefault();
Use initializer format instead for constructors when possible:

ScopedSetICUDefaultLocale::ScopedSetICUDefaultLocale()
    : default_locale_(uloc_getDefault()) {
}

https://codereview.chromium.org/1162603004/diff/140001/base/test/icu_test_uti...
base/test/icu_test_util.cc:18: }  // namespace base
Blank line before this.

https://codereview.chromium.org/1162603004/diff/140001/base/test/icu_test_util.h
File base/test/icu_test_util.h (right):

https://codereview.chromium.org/1162603004/diff/140001/base/test/icu_test_uti...
base/test/icu_test_util.h:8: #include <string>
Blank line after this, separating the different types of #includes.

https://codereview.chromium.org/1162603004/diff/140001/base/test/icu_test_uti...
base/test/icu_test_util.h:11: namespace base {
If you’re going to be in this directory (base/test), it’s prudent to use

namespace base {
namespace test {

[Olli Raula, 2015-08-19 14:19:02]

Thanks for long list :), fixed

[Mark Mentovai, 2015-08-19 14:20:56]

LGTM

https://codereview.chromium.org/1162603004/diff/160001/base/test/icu_test_uti...
File base/test/icu_test_util.cc (right):

https://codereview.chromium.org/1162603004/diff/160001/base/test/icu_test_uti...
base/test/icu_test_util.cc:11: namespace test {
This too.

https://codereview.chromium.org/1162603004/diff/160001/base/test/icu_test_util.h
File base/test/icu_test_util.h (right):

https://codereview.chromium.org/1162603004/diff/160001/base/test/icu_test_uti...
base/test/icu_test_util.h:13: namespace test {
Blank line after this.

[Olli Raula, 2015-08-20 07:05:17]

The CQ bit was checked by olli.raula@intel.com

[Olli Raula, 2015-08-20 07:05:18]

The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org
Link to the patchset: https://codereview.chromium.org/1162603004/#ps180001
(title: "Added spaces")

[commit-bot: I haz the power, 2015-08-20 07:05:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1162603004/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1162603004/180001

[commit-bot: I haz the power, 2015-08-20 07:37:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-20 07:37:53]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Olli Raula, 2015-08-20 07:48:46]

The CQ bit was checked by olli.raula@intel.com

[commit-bot: I haz the power, 2015-08-20 07:48:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1162603004/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1162603004/180001

[commit-bot: I haz the power, 2015-08-20 08:48:52]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-08-20 08:49:24]

Patchset 10 (id:??) landed as
https://crrev.com/3be75bf76ac061129ec2a9e887d5d50cfd016a56
Cr-Commit-Position: refs/heads/master@{#344453}