New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 637324 link

Starred by 2 users
Status: Archived
Owner: ----
Closed: Feb 2018
Cc:
grt@chromium.org
█ phajdan.jr@chromium.org
gab@chromium.org
dpranke@chromium.org
chrisha@chromium.org
Components:
EstimatedDays: ----
NextAction: 2018-02-21
OS: Windows
Pri: 3
Type: Bug

Blocking:
issue 721245



Sign in to add a comment

Should all tests use RegistryOverrideManager so that they don't touch the real registry?

Project Member Reported by asvitk...@chromium.org, Aug 12 2016 
Should all tests use RegistryOverrideManager so that they don't touch the real registry?

For context, we recently had a bug where a unit test in a WIP patch that was being run on try bots was actually modifying the real registry values on the test bots, causing permanent global state changes that affected future unrelated test runs (making them hit DCHECKs):  http://crbug.com/635255 

Right now, it's way too easy to accidentally make this happen, if you're not careful. That's because if you use an API that touches the registry, unless you explicitly use a RegistryOverrideManager, it will touch the real registry.

This is bad because tests are thus not hermetic and cause cause various flakyness issues.

Discussing this with gab@, one thing we thought of is whether we could hoist a RegistryOverrideManager instance somewhere in the base testing framework - so all unit tests will have a fake registry set up and never touch the real one.

cc'ing some Windows and test folks for their thoughts.

Comment 1 by grt@chromium.org, Aug 15 2016 

In general, I am supportive of the idea. The problem is that certain Win APIs will confusingly fail when the registry is overridden. Some tests jump through hoops to deal with this (https://cs.chromium.org/chromium/src/rlz/test/rlz_test_helpers.cc?dr=C&q=InitializeRegistryOverridesForTesting&sq=package:chromium&l=100, https://cs.chromium.org/chromium/src/chrome/installer/util/installer_state_unittest.cc?type=cs&q=InstallerStateTest,%5C+InitializeTwice&sq=package:chromium&l=617). It might be a slog to get these working for all tests.

I see three paths forward:
1. Status quo.
2. Try getting one unit test binary (installer_util_unittests or setup_unittests, for example) to run with the registry virtualized. If it works, slowly add test by test until it's possible to run them all.
3. Replace base::win::RegKey with something that we can mock/fake in tests in such a way as to not rely on RegOverridePredefKey.

There may be alternatives.

Comment 2 by brajkumar@chromium.org, Aug 22 2016

Components: Tests

Comment 3 by mpear...@chromium.org, May 8 2017 

Wondering if the last nine months made the right path forward on this bug (see three options in comment #1) clearer.

Is anyone aware of this type of issue (see original report) having occurred again?

Comment 4 by gab@chromium.org, May 16 2017

Blocking: 721245

Comment 5 by mpear...@chromium.org, Feb 13 2018

NextAction: 2018-02-21
asvitkine@, I'm tempted to close this out unless either
- someone is actively interested in fixing it
or
- someone is aware of a problem of this form that's occurred since the massive cleanup last May (see bug 721245)

Comment 6 by asvitk...@chromium.org, Feb 14 2018

Cc: dpranke@chromium.org
+dpranke for thoughts

I still think this is an issue, but I don't see myself finding time to do something here. Dirk, do you think this is something that would fit under the Fix Flaky Tests effort? (Is there such an effort currently?)

Comment 7 by dpranke@chromium.org, Feb 14 2018 

There is no such central effort, currently (there are flakiness efforts, but not yet a "Fix Flaky Tests" effort, per se). Best case is that you'd mark this as a bug that might be contributing to flakiness, and someone might be look at it someday.

However, if we're not seeing active issues as a result of this, I'd be inclined to follow mpearson's advice.

Comment 8 by monor...@bugs.chromium.org, Feb 21 2018 

The NextAction date has arrived: 2018-02-21

Comment 9 by asvitk...@chromium.org, Feb 21 2018

Status: Archived (was: Untriaged)
OK, closing. I still think this makes sense to do, but if no one is currently looking at the problem space and when they do, there might be bigger fish to fry, let's close in the meantime. Can be re-opened or re-filed if this is identified as one of the areas that should be addressed in an eventual de-flake effort.
►

Sign in to add a comment