Make it impossible to have non-matching policy_test_cases.json entries.

There were a few cases where due to mismatch between policy defintions
and entries in the policy_tests_cases.json file. The tests have failed
when Chrome was uprevved. This CL aims at making this a little harder
by enforcing a proper match between policy lifetime and their tests.

BUG: 791125
TEST: browser_tests.exe --gtest_filter=PolicyPrefsTest.PolicyToPrefsMapping
Change-Id: I75fdef47081c27184c05ded7cd2bc6a3e355ac79
Reviewed-on: https://chromium-review.googlesource.com/808184
Commit-Queue: Julian Pastarmov <pastarmovj@chromium.org>
Reviewed-by: Lutz Justen <ljusten@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522083}

chrome/browser/policy/policy_prefs_browsertest.cc

@@ -59,6 +59,10 @@ namespace policy {
 
 namespace {
 
+// The name of the template example in policy_test_cases.json that does not need
+// to be parsed.
+const char kTemplateSampleTest[] = "-- Template --";
+
 const char kCrosSettingsPrefix[] = "cros.";
 
 std::string GetPolicyName(const std::string& policy_name_decorated) {
@@ -250,15 +254,10 @@ class PolicyTestCases {
       ADD_FAILURE() << "Error parsing policy_test_cases.json: " << error_string;
       return;
     }
-    Schema chrome_schema = Schema::Wrap(GetChromeSchemaData());
-    if (!chrome_schema.valid()) {
-      ADD_FAILURE();
-      return;
-    }
     for (base::DictionaryValue::Iterator it(*dict); !it.IsAtEnd();
          it.Advance()) {
       const std::string policy_name = GetPolicyName(it.key());
-      if (!chrome_schema.GetKnownProperty(policy_name).valid())
+      if (policy_name == kTemplateSampleTest)
         continue;
       PolicyTestCase* policy_test_case = GetPolicyTestCase(dict, it.key());
       if (policy_test_case)
@@ -300,6 +299,7 @@ class PolicyTestCases {
     policy_test_dict->GetBoolean("can_be_recommended", &can_be_recommended);
     std::string indicator_selector;
     policy_test_dict->GetString("indicator_selector", &indicator_selector);
+
     PolicyTestCase* policy_test_case = new PolicyTestCase(name,
                                                           is_official_only,
                                                           can_be_recommended,
@@ -312,6 +312,7 @@ class PolicyTestCases {
           policy_test_case->AddSupportedOs(os);
       }
     }
+
     const base::DictionaryValue* policy = NULL;
     if (policy_test_dict->GetDictionary("test_policy", &policy))
       policy_test_case->SetTestPolicy(*policy);
@@ -322,8 +323,8 @@ class PolicyTestCases {
         std::string pref;
         if (!pref_mappings->GetDictionary(i, &pref_mapping_dict) ||
             !pref_mapping_dict->GetString("pref", &pref)) {
-          ADD_FAILURE() << "Malformed pref_mappings entry in "
-                        << "policy_test_cases.json.";
+          ADD_FAILURE() << "Malformed pref_mappings entry for " << name
+                        << " in policy_test_cases.json.";
           continue;
         }
         bool is_local_state = false;
@@ -351,8 +352,8 @@ class PolicyTestCases {
             const base::DictionaryValue* policy = NULL;
             if (!indicator_tests->GetDictionary(i, &indicator_test_dict) ||
                 !indicator_test_dict->GetDictionary("policy", &policy)) {
-              ADD_FAILURE() << "Malformed indicator_tests entry in "
-                            << "policy_test_cases.json.";
+              ADD_FAILURE() << "Malformed indicator_tests entry for " << name
+                            << " in policy_test_cases.json.";
               continue;
             }
             std::string value;
@@ -389,8 +390,26 @@ IN_PROC_BROWSER_TEST_F(PolicyPrefsTestCoverageTest, AllPoliciesHaveATestCase) {
   PolicyTestCases policy_test_cases;
   for (Schema::Iterator it = chrome_schema.GetPropertiesIterator();
        !it.IsAtEnd(); it.Advance()) {
-    EXPECT_TRUE(base::ContainsKey(policy_test_cases.map(), it.key()))
-        << "Missing policy test case for: " << it.key();
+    auto policy = policy_test_cases.map().find(it.key());
+    if (policy == policy_test_cases.map().end()) {
+      ADD_FAILURE() << "Missing policy test case for: " << it.key();
+    } else {
+      bool has_test_case_for_this_os = false;
+      for (PolicyTestCases::PolicyTestCaseVector::const_iterator test_case =
+               policy->second.begin();
+           test_case != policy->second.end() && !has_test_case_for_this_os;
+           ++test_case) {
+        has_test_case_for_this_os |= (*test_case)->IsSupported();
+      }
+      // This can only be a warning as many policies are not really testable
+      // this way and only present as a single line in the file.
+      // Although they could at least contain the "os" and "test_policy" fields.
+      // See http://crbug.com/791125.
+      LOG_IF(WARNING, !has_test_case_for_this_os)
+          << "Policy " << policy->first
+          << " is marked as supported on this OS in policy_templates.json but "
+          << "have a test for this platform in policy_test_cases.json.";
+    }
   }
 }
 
@@ -439,6 +458,8 @@ class PolicyPrefsTest : public InProcessBrowserTest {
 // Verifies that policies make their corresponding preferences become managed,
 // and that the user can't override that setting.
 IN_PROC_BROWSER_TEST_F(PolicyPrefsTest, PolicyToPrefsMapping) {
+  Schema chrome_schema = Schema::Wrap(GetChromeSchemaData());
+  ASSERT_TRUE(chrome_schema.valid());
   PrefService* local_state = g_browser_process->local_state();
   PrefService* user_prefs = browser()->profile()->GetPrefs();
 
@@ -451,6 +472,24 @@ IN_PROC_BROWSER_TEST_F(PolicyPrefsTest, PolicyToPrefsMapping) {
          test_case != policy->second.end();
          ++test_case) {
       const auto& pref_mappings = (*test_case)->pref_mappings();
+      if (!chrome_schema.GetKnownProperty(policy->first).valid()) {
+        // If the policy is supported on this platform according to the test it
+        // should be known otherwise we signal this as a failure.
+        // =====================================================================
+        // !NOTE! If you see this assertion after changing Chrome's VERSION most
+        // probably the mentioned policy was deprecated and deleted. Verify this
+        // in policy_templates.json and remove the corresponding test entry
+        // in policy_test_cases.json. Don't completely delete it from there just
+        // replace it's definition with a single "note" value stating its
+        // deprecation date (see other examples present in the file already).
+        // =====================================================================
+        EXPECT_FALSE((*test_case)->IsSupported())
+            << "Policy " << policy->first
+            << " is marked as supported on this OS but does not exist in the "
+            << "Chrome policy schema.";
+        continue;
+      }
+
       if (!(*test_case)->IsSupported() || pref_mappings.empty())
         continue;
 

chrome/test/data/policy/policy_test_cases.json

@@ -31,7 +31,7 @@
   },
 
   "UnsafelyTreatInsecureOriginAsSecure": {
-    "os": ["win", "linux", "max", "chromeos", "android"],
+    "os": ["win", "linux", "mac"],
     "test_policy": { "UnsafelyTreatInsecureOriginAsSecure": ["http://example.com/"] },
     "pref_mappings": [ { "pref": "unsafely_treat_insecure_origin_as_secure" } ]
   },
@@ -126,16 +126,7 @@
   },
 
   "DnsPrefetchingEnabled": {
-    "os": ["win", "linux", "mac", "chromeos"],
-    "can_be_recommended": true,
-    "test_policy": { "DnsPrefetchingEnabled": false },
-    "pref_mappings": [
-      { "pref": "net.network_prediction_options",
-        "indicator_tests": [
-          { "policy": { "DnsPrefetchingEnabled": false } }
-        ]
-      }
-    ]
+    "note": "This policy has been removed. See https://bugs.chromium.org/p/chromium/issues/detail?id=624095"
   },
 
   "NetworkPredictionOptions": {
@@ -152,11 +143,7 @@
   },
 
   "DisableSpdy": {
-    "os": ["win", "linux", "mac", "chromeos"],
-    "test_policy": { "DisableSpdy": true },
-    "pref_mappings": [
-      { "pref": "spdy.disabled" }
-    ]
+    "note": "This policy has been removed. See https://bugs.chromium.org/p/chromium/issues/detail?id=624095"
   },
 
   "QuicAllowed": {
@@ -540,9 +527,7 @@
   },
 
   "DisablePluginFinder": {
-    "note": "This policy is not in use anymore.",
-    "os": ["win", "linux", "mac", "chromeos"],
-    "test_policy": { "DisablePluginFinder": true }
+    "note": "This policy is not in use anymore since Chrome 65."
   },
 
   "SyncDisabled": {
@@ -562,8 +547,7 @@
   },
 
   "EnableDeprecatedWebBasedSignin": {
-    "os": ["win", "linux", "mac"],
-    "test_policy": { "EnableDeprecatedWebBasedSignin": false }
+    "note": "This policy has been removed in Chrome 42."
   },
 
   "UserDataDir": {
@@ -724,6 +708,7 @@
           { "policy": { "ProxySettings": { "ProxyMode": "direct" } } }
         ]
       }
+
     ]
   },
 
@@ -732,13 +717,7 @@
   },
 
   "DisableSSLRecordSplitting": {
-    "os": ["win", "linux", "mac", "chromeos"],
-    "test_policy": { "DisableSSLRecordSplitting": true },
-    "pref_mappings": [
-      { "pref": "ssl.ssl_record_splitting.disabled",
-        "local_state": true
-      }
-    ]
+    "note": "This policy is retired, see https://bugs.chromium.org/p/chromium/issues/detail?id=485097#c17."
   },
 
   "EnableOnlineRevocationChecks": {
@@ -832,7 +811,7 @@
   },
 
   "GSSAPILibraryName": {
-    "os": ["mac", "linux"],
+    "os": ["linux"],
     "test_policy": { "GSSAPILibraryName": "libwhatever.so" },
     "pref_mappings": [
       { "pref": "auth.gssapi_library_name",
@@ -1113,16 +1092,7 @@
   },
 
   "DefaultSearchProviderInstantURL": {
-    "os": ["win", "linux", "mac", "chromeos"],
-    "test_policy": {
-      "DefaultSearchProviderEnabled": true,
-      "DefaultSearchProviderSearchURL": "http://www.google.com/?q={searchTerms}",
-      "DefaultSearchProviderKeyword": "google",
-      "DefaultSearchProviderInstantURL": "http://www.google.com/instant?q={searchTerms}"
-    },
-    "pref_mappings": [
-      { "pref": "default_search_provider_data.template_url_data" }
-    ]
+    "note": "Deprecated since Chrome 63. See https://crbug.com/476079"
   },
 
   "DefaultSearchProviderNewTabURL": {
@@ -1178,16 +1148,7 @@
   },
 
   "DefaultSearchProviderSearchTermsReplacementKey": {
-    "os": ["win", "linux", "mac", "chromeos"],
-    "test_policy": {
-      "DefaultSearchProviderEnabled": true,
-      "DefaultSearchProviderSearchURL": "http://www.google.com/?q={searchTerms}",
-      "DefaultSearchProviderKeyword": "google",
-      "DefaultSearchProviderSearchTermsReplacementKey": "espv"
-    },
-    "pref_mappings": [
-      { "pref": "default_search_provider_data.template_url_data" }
-    ]
+    "note": "Deprecated since Chrome 63. See https://crbug.com/476079"
   },
 
   "DefaultSearchProviderImageURL": {
@@ -1230,16 +1191,7 @@
   },
 
   "DefaultSearchProviderInstantURLPostParams": {
-    "os": ["win", "linux", "mac", "chromeos"],
-    "test_policy": {
-      "DefaultSearchProviderEnabled": true,
-      "DefaultSearchProviderSearchURL": "http://www.google.com/?q={searchTerms}",
-      "DefaultSearchProviderKeyword": "google",
-      "DefaultSearchProviderInstantURLPostParams": ""
-    },
-    "pref_mappings": [
-      { "pref": "default_search_provider_data.template_url_data" }
-    ]
+    "note": "Deprecated since Chrome 63. See https://crbug.com/476079"
   },
 
   "DefaultSearchProviderImageURLPostParams": {
@@ -1784,11 +1736,7 @@
   },
 
   "AlwaysAuthorizePlugins": {
-    "os": ["win", "linux", "mac", "chromeos"],
-    "test_policy": { "AlwaysAuthorizePlugins": true },
-    "pref_mappings": [
-      { "pref": "plugins.always_authorize" }
-    ]
+    "note": "This policy has been removed since Chrome 65."
   },
 
   "RunAllFlashInAllowMode": {
@@ -1925,7 +1873,7 @@
   },
 
   "MaxConnectionsPerProxy": {
-    "os": ["win", "linux", "mac", "chromeos"],
+    "os": ["win", "linux", "mac"],
     "test_policy": { "MaxConnectionsPerProxy": 16 },
     "pref_mappings": [
       { "pref": "net.max_connections_per_proxy",
@@ -2095,7 +2043,7 @@
   },
 
   "HideWebStoreIcon": {
-    "os": ["win", "linux", "mac", "chromeos"],
+    "os": ["win", "linux", "mac"],
     "test_policy": { "HideWebStoreIcon": true },
     "pref_mappings": [
       { "pref": "hide_web_store_icon" }
@@ -2165,7 +2113,7 @@
   },
 
   "ForceBrowserSignin": {
-    "os": ["win", "linux", "mac"],
+    "os": ["win"],
     "test_policy": { "ForceBrowserSignin": false },
     "pref_mappings": [
       { "pref": "profile.force_browser_signin",
@@ -3313,7 +3261,7 @@
   },
 
   "BrowserNetworkTimeQueriesEnabled": {
-    "os": ["win", "linux", "mac", "chromeos"],
+    "os": ["win", "linux", "mac"],
     "test_policy": { "BrowserNetworkTimeQueriesEnabled": true },
     "pref_mappings": [
       { "pref": "network_time.network_time_queries_enabled",
@@ -3327,8 +3275,7 @@
   },
 
   "DeviceEcryptfsMigrationStrategy": {
-    "note": "This policy is deprecated.",
-    "os": ["chromeos"]
+    "note": "This policy is deprecated and removed since Chrome 61."
   },
 
   "NoteTakingAppsLockScreenWhitelist": {