Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2612733003:
Machine-local policy files should not affect tests (Closed)
DescriptionMachine-local policy files should not affect tests
Override location of machine-local policy directory for both unit tests
and browser tests. This makes sure that any policy data the machine has
does not affect the test results.
BUG=630614
Review-Url: https://codereview.chromium.org/2612733003
Cr-Commit-Position: refs/heads/master@{#442852}
Committed: https://chromium.googlesource.com/chromium/src/+/6edddec42dae4c5797b642f59580a8c7024c1b6a
Patch Set 1 #Patch Set 2 : browser tests: Don't try to create the policy dir #
Total comments: 6
Patch Set 3 : Improve comment #
Messages
Total messages: 27 (14 generated)
The CQ bit was checked by pmarko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by pmarko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
pmarko@chromium.org changed reviewers: + phajdan.jr@chromium.org
Please review the changes. It has been reported that machine-local policy files on developers' machines cause confusing test errors (see the linked bug). The suggested solution is to override the paths to a known-empty/nonexisting directory.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2612733003/diff/20001/chrome/test/base/test_l... File chrome/test/base/test_launcher_utils.cc (right): https://codereview.chromium.org/2612733003/diff/20001/chrome/test/base/test_l... chrome/test/base/test_launcher_utils.cc:112: base::FilePath policy_files = user_data_dir.AppendASCII("policies"); Why is this in POSIX non-OSX #ifdef? https://codereview.chromium.org/2612733003/diff/20001/chrome/test/base/testin... File chrome/test/base/testing_browser_process.cc (right): https://codereview.chromium.org/2612733003/diff/20001/chrome/test/base/testin... chrome/test/base/testing_browser_process.cc:162: base::FilePath local_policy_path("/tmp/non/existing/directory"); Why is this in POSIX non-OSX #ifdef?
https://codereview.chromium.org/2612733003/diff/20001/chrome/test/base/test_l... File chrome/test/base/test_launcher_utils.cc (right): https://codereview.chromium.org/2612733003/diff/20001/chrome/test/base/test_l... chrome/test/base/test_launcher_utils.cc:112: base::FilePath policy_files = user_data_dir.AppendASCII("policies"); On 2017/01/05 15:48:02, Paweł Hajdan Jr. wrote: > Why is this in POSIX non-OSX #ifdef? That's because chrome_paths.h actually defines DIR_POLICY_FILES only in a #if defined(OS_POSIX) && !defined(OS_MACOSX) block. I could add that information to the comment? https://codereview.chromium.org/2612733003/diff/20001/chrome/test/base/testin... File chrome/test/base/testing_browser_process.cc (right): https://codereview.chromium.org/2612733003/diff/20001/chrome/test/base/testin... chrome/test/base/testing_browser_process.cc:162: base::FilePath local_policy_path("/tmp/non/existing/directory"); On 2017/01/05 15:48:02, Paweł Hajdan Jr. wrote: > Why is this in POSIX non-OSX #ifdef? See Reply on test_launcher_util.cc - if we decide to add the information to the comment, I'd of course add it here too.
On 2017/01/05 16:07:53, pmarko wrote: > https://codereview.chromium.org/2612733003/diff/20001/chrome/test/base/test_l... > File chrome/test/base/test_launcher_utils.cc (right): > > https://codereview.chromium.org/2612733003/diff/20001/chrome/test/base/test_l... > chrome/test/base/test_launcher_utils.cc:112: base::FilePath policy_files = > user_data_dir.AppendASCII("policies"); > On 2017/01/05 15:48:02, Paweł Hajdan Jr. wrote: > > Why is this in POSIX non-OSX #ifdef? > > That's because chrome_paths.h actually defines DIR_POLICY_FILES only in a > #if defined(OS_POSIX) && !defined(OS_MACOSX) > block. > I could add that information to the comment? > > https://codereview.chromium.org/2612733003/diff/20001/chrome/test/base/testin... > File chrome/test/base/testing_browser_process.cc (right): > > https://codereview.chromium.org/2612733003/diff/20001/chrome/test/base/testin... > chrome/test/base/testing_browser_process.cc:162: base::FilePath > local_policy_path("/tmp/non/existing/directory"); > On 2017/01/05 15:48:02, Paweł Hajdan Jr. wrote: > > Why is this in POSIX non-OSX #ifdef? > > See Reply on test_launcher_util.cc - if we decide to add the information to the > comment, I'd of course add it here too. (The background is that machine-wide policies are stored by different mechanisms on Windows/MacOS, e.g. Windows Registry instead of json files in a directory)
LGTM Consider making the /non/existing/directory really unique, or a TODO for that.
On 2017/01/09 20:06:55, Paweł Hajdan Jr. wrote: > LGTM > > Consider making the /non/existing/directory really unique, or a TODO for that. To get a unique path I'd have to actually create a temp directory (e.g. use ScopedTempDir) - should be no problem, just thought in the beginning that it's not really necessary until someone needs it. But I guess consistency would be more important. Or did you mean to do it in a different way? Thanks.
tnagel@chromium.org changed reviewers: + tnagel@chromium.org
https://codereview.chromium.org/2612733003/diff/20001/chrome/test/base/testin... File chrome/test/base/testing_browser_process.cc (right): https://codereview.chromium.org/2612733003/diff/20001/chrome/test/base/testin... chrome/test/base/testing_browser_process.cc:159: // machine-wide poilcies do not affect tests. Nit: typo
On 2017/01/10 13:14:44, pmarko wrote: > On 2017/01/09 20:06:55, Paweł Hajdan Jr. wrote: > > LGTM > > > > Consider making the /non/existing/directory really unique, or a TODO for that. > > To get a unique path I'd have to actually create a temp directory (e.g. use > ScopedTempDir) - should be no problem, just thought in the beginning that it's > not really necessary until someone needs it. But I guess consistency would be > more important. > > Or did you mean to do it in a different way? > Thanks. I saw the code uses PathService::OverrideAndCreateIfNeeded, so it seems the directory might be getting created.
On 2017/01/10 16:27:21, Paweł Hajdan Jr. wrote: > On 2017/01/10 13:14:44, pmarko wrote: > > On 2017/01/09 20:06:55, Paweł Hajdan Jr. wrote: > > > LGTM > > > > > > Consider making the /non/existing/directory really unique, or a TODO for > that. > > > > To get a unique path I'd have to actually create a temp directory (e.g. use > > ScopedTempDir) - should be no problem, just thought in the beginning that it's > > not really necessary until someone needs it. But I guess consistency would be > > more important. > > > > Or did you mean to do it in a different way? > > Thanks. > > I saw the code uses PathService::OverrideAndCreateIfNeeded, so it seems the > directory might be getting created. Ah, you're right, that's confusing. PathService::OverrideAndCreateIfNeeded's 4th parameter is a boolean which decides if the directory should be created, it is passing false there. There's also PathService::Override, but it uses OverrideAndCreateIfNeeded internally with create=true, so that would actually create it. I see two possibilities to improve: (1) I could write a comment that it's not getting created and that's why we're passing false. (2) I could use a temp dir and just pass it into PathService::Override . I have no real preference either way, do you?
I have slight preference towards (1). LGTM with that.
https://codereview.chromium.org/2612733003/diff/20001/chrome/test/base/testin... File chrome/test/base/testing_browser_process.cc (right): https://codereview.chromium.org/2612733003/diff/20001/chrome/test/base/testin... chrome/test/base/testing_browser_process.cc:159: // machine-wide poilcies do not affect tests. On 2017/01/10 13:24:37, Thiemo Nagel (slow) wrote: > Nit: typo Done.
The CQ bit was checked by pmarko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from phajdan.jr@chromium.org Link to the patchset: https://codereview.chromium.org/2612733003/#ps40001 (title: "Improve comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484126039032620,
"parent_rev": "d1f2198a42c85ae73a03d8ad29f11d85f336e9ef", "commit_rev":
"6edddec42dae4c5797b642f59580a8c7024c1b6a"}
Message was sent while issue was closed.
Description was changed from ========== Machine-local policy files should not affect tests Override location of machine-local policy directory for both unit tests and browser tests. This makes sure that any policy data the machine has does not affect the test results. BUG=630614 ========== to ========== Machine-local policy files should not affect tests Override location of machine-local policy directory for both unit tests and browser tests. This makes sure that any policy data the machine has does not affect the test results. BUG=630614 Review-Url: https://codereview.chromium.org/2612733003 Cr-Commit-Position: refs/heads/master@{#442852} Committed: https://chromium.googlesource.com/chromium/src/+/6edddec42dae4c5797b642f59580... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/6edddec42dae4c5797b642f59580... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
