ChromeOS: Per-user time zone: refactor tests first.

chrome/browser/chromeos/scoped_sys_info_chromeos_version_for_testing.cc

Index: chrome/browser/chromeos/scoped_sys_info_chromeos_version_for_testing.cc
diff --git a/chrome/browser/chromeos/scoped_sys_info_chromeos_version_for_testing.cc b/chrome/browser/chromeos/scoped_sys_info_chromeos_version_for_testing.cc
new file mode 100644
index 0000000000000000000000000000000000000000..1851c63f29e61fc56b2a5c21067be03b355b057e
--- /dev/null
+++ b/chrome/browser/chromeos/scoped_sys_info_chromeos_version_for_testing.cc
@@ -0,0 +1,21 @@
+// Copyright 2017 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 "chrome/browser/chromeos/scoped_sys_info_chromeos_version_for_testing.h"
+
+#include "base/sys_info.h"
+
+namespace chromeos {
+
+ScopedSetRunningOnChromeOSForTesting::ScopedSetRunningOnChromeOSForTesting(

[xiyuan, 2017/05/26 19:14:35]

ScopedSetRunningOnChromeOSForTesting and the file name does not match. We should
change one of them to make them consistent.

[Alexander Alekseev, 2017/05/29 17:06:04]

Done.

+    const std::string& lsb_release,
+    const base::Time& lsb_release_time) {
+  base::SysInfo::SetChromeOSVersionInfoForTest(lsb_release, lsb_release_time);

[xiyuan, 2017/05/26 17:14:53]

ScopedSetRunningOnChromeOSForTesting is necessary because
SetChromeOSVersionInfoForTest uses env to pass the lsb data. Since env is shared
for all processes, the test jobs would interfere with each other. Even with this
class, it could still happen.

Can we fix SetChromeOSVersionInfoForTest so that it passes the lsb data directly
to ChromeOSVersionInfo without touching env? Think this way would provide better
test isolation. And we don't need to worry about cleanning up this way since
each test job should be running its own process already.

[Alexander Alekseev, 2017/05/26 18:06:21]

https://en.wikipedia.org/wiki/Environment_variable#Unix :
"Environment variables are local to the process in which they were set. If two
shell processes are spawned and the value of an environment variable is changed
in one, that change will not be seen by the other."

Changing base/ could be tricky, so maybe it's easier to have this class here.

[xiyuan, 2017/05/26 19:14:35]

Oops. I messed it up with inherited env from parent process.

Now I don't understand why we need this class. AFAICT, test launcher would run
test jobs in separate processes (unless the test has PRE_ tests) so that global
variable changes would not affect each other. Since env is the same as global
variable in this perspective, can the test just call
SetChromeOSVersionInfoForTest and be done with it? 

[Alexander Alekseev, 2017/05/29 17:06:04]

I found that this is wrong. If you run unit_tests without any options, it will
split tests into several buckets and run each bucket in a separate process. That
is why some tests (like ProfileAuthDataTest.DoNotTransfer ) always succeed only
on retry.(unit_tests will retry failed tests three times. But it will only retry
failed tests, so a successful test (like  MachineHWIDCheckerTest.TestSwitch)
will break only the first attempt that breaks environment will not affect retry.

Actually I do not need this for my other Cl. I just came across this issue while
working on my CL, and spent some time debugging it. There are other tests that
use SetChromeOSVersionInfoForTest, but this change seems to fix unit_tests.

[xiyuan, 2017/05/30 15:39:39]

Acknowledged.

+}
+
+ScopedSetRunningOnChromeOSForTesting::~ScopedSetRunningOnChromeOSForTesting() {
+  base::SysInfo::SetChromeOSVersionInfoForTest("", base::Time());

[xiyuan, 2017/05/26 19:14:35]

This does not restore to previous values. We should at least add a note in class
document to avoid surprises from future users.

[Alexander Alekseev, 2017/05/29 17:06:03]

Done.

+}
+
+}  // namespace chromeos