Fail tests fast if Windows registry key override fails
policy_loader_win_unittest uses RegOverridePredefKey to remap registry
keys to a temporary key for the duration of the test. If this remapping
fails, the test should fail fast to avoid accidentally creating
persistent changes in the machine's registry.
This is done by moving the remap step into the test's SetUp() method.
gtest documentation says that the test will not continue if an ASSERT
fails in SetUp()[1][2].
Also, we now delete the temporary registry keys if they already exist.
These are keyed by the process id, so a subsequent test could reuse an
existing temporary key if the previous test (with the same process id)
crashed and failed to clean up.
[1] https://github.com/google/googletest/blob/master/googletest/docs/Primer.md#invoking-the-tests
[2] https://cs.chromium.org/chromium/src/third_party/mesa/src/src/gtest/src/gtest.cc?rcl=ef811c6bd4de74e13e7035ca882cc77f85793fef&l=2159
BUG=721691
Review-Url: https://codereview.chromium.org/2893803005
Cr-Commit-Position: refs/heads/master@{#473174}
diff --git a/components/policy/core/common/configuration_policy_provider_test.cc b/components/policy/core/common/configuration_policy_provider_test.cc
index 7b2efa4..cf9c8e1f 100644
--- a/components/policy/core/common/configuration_policy_provider_test.cc
+++ b/components/policy/core/common/configuration_policy_provider_test.cc
@@ -187,7 +187,7 @@
PolicyTestBase::SetUp();
test_harness_.reset((*GetParam())());
- test_harness_->SetUp();
+ ASSERT_NO_FATAL_FAILURE(test_harness_->SetUp());
const PolicyNamespace chrome_ns(POLICY_DOMAIN_CHROME, "");
Schema chrome_schema = *schema_registry_.schema_map()->GetSchema(chrome_ns);
diff --git a/components/policy/core/common/policy_loader_win_unittest.cc b/components/policy/core/common/policy_loader_win_unittest.cc
index 165f3d7..3efc8c71 100644
--- a/components/policy/core/common/policy_loader_win_unittest.cc
+++ b/components/policy/core/common/policy_loader_win_unittest.cc
@@ -155,8 +155,12 @@
ScopedGroupPolicyRegistrySandbox();
~ScopedGroupPolicyRegistrySandbox();
- private:
+ // Activates the registry keys overrides. This must be called before doing any
+ // writes to registry and the call should be wrapped in
+ // ASSERT_NO_FATAL_FAILURE.
void ActivateOverrides();
+
+ private:
void RemoveOverrides();
// Deletes the sandbox keys.
@@ -295,7 +299,14 @@
DISALLOW_COPY_AND_ASSIGN(PRegTestHarness);
};
-ScopedGroupPolicyRegistrySandbox::ScopedGroupPolicyRegistrySandbox() {
+ScopedGroupPolicyRegistrySandbox::ScopedGroupPolicyRegistrySandbox() {}
+
+ScopedGroupPolicyRegistrySandbox::~ScopedGroupPolicyRegistrySandbox() {
+ RemoveOverrides();
+ DeleteKeys();
+}
+
+void ScopedGroupPolicyRegistrySandbox::ActivateOverrides() {
// Generate a unique registry key for the override for each test. This
// makes sure that tests executing in parallel won't delete each other's
// key, at DeleteKeys().
@@ -304,6 +315,11 @@
std::wstring hklm_key_name = key_name_ + L"\\HKLM";
std::wstring hkcu_key_name = key_name_ + L"\\HKCU";
+ // Delete the registry test keys if they already exist (this could happen if
+ // the process id got recycled and the last test running under the same
+ // process id crashed ).
+ DeleteKeys();
+
// Create the subkeys to hold the overridden HKLM and HKCU
// policy settings.
temp_hklm_hive_key_.Create(HKEY_CURRENT_USER,
@@ -313,19 +329,22 @@
hkcu_key_name.c_str(),
KEY_ALL_ACCESS);
- ActivateOverrides();
-}
+ auto result_override_hklm =
+ RegOverridePredefKey(HKEY_LOCAL_MACHINE, temp_hklm_hive_key_.Handle());
+ auto result_override_hkcu =
+ RegOverridePredefKey(HKEY_CURRENT_USER, temp_hkcu_hive_key_.Handle());
-ScopedGroupPolicyRegistrySandbox::~ScopedGroupPolicyRegistrySandbox() {
- RemoveOverrides();
- DeleteKeys();
-}
+ if (result_override_hklm != ERROR_SUCCESS ||
+ result_override_hkcu != ERROR_SUCCESS) {
+ // We need to remove the overrides first in case one succeeded and one
+ // failed, otherwise deleting the keys fails.
+ RemoveOverrides();
+ DeleteKeys();
-void ScopedGroupPolicyRegistrySandbox::ActivateOverrides() {
- ASSERT_HRESULT_SUCCEEDED(RegOverridePredefKey(HKEY_LOCAL_MACHINE,
- temp_hklm_hive_key_.Handle()));
- ASSERT_HRESULT_SUCCEEDED(RegOverridePredefKey(HKEY_CURRENT_USER,
- temp_hkcu_hive_key_.Handle()));
+ // Assert on the actual results to print the error code in failure case.
+ ASSERT_HRESULT_SUCCEEDED(result_override_hklm);
+ ASSERT_HRESULT_SUCCEEDED(result_override_hkcu);
+ }
}
void ScopedGroupPolicyRegistrySandbox::RemoveOverrides() {
@@ -347,7 +366,12 @@
RegistryTestHarness::~RegistryTestHarness() {}
-void RegistryTestHarness::SetUp() {}
+void RegistryTestHarness::SetUp() {
+ // SetUp is called at gtest SetUp time, and gtest documentation guarantees
+ // that the test will not be executed if SetUp has a fatal failure. This is
+ // important, see crbug.com/721691.
+ ASSERT_NO_FATAL_FAILURE(registry_sandbox_.ActivateOverrides());
+}
ConfigurationPolicyProvider* RegistryTestHarness::CreateProvider(
SchemaRegistry* registry,
@@ -714,6 +738,11 @@
base::win::SetDomainStateForTesting(false);
PolicyTestBase::SetUp();
+ // Activate overrides of registry keys. gtest documentation guarantees
+ // that the test will not be executed if SetUp has a fatal failure. This is
+ // important, see crbug.com/721691.
+ ASSERT_NO_FATAL_FAILURE(registry_sandbox_.ActivateOverrides());
+
ASSERT_TRUE(PathService::Get(base::DIR_SOURCE_ROOT, &test_data_dir_));
test_data_dir_ = test_data_dir_.AppendASCII("chrome")
.AppendASCII("test")
