Collect UMA statistics on which chrome://flags lead to chrome restart on ChromeOS.

chrome/browser/about_flags_histogram_unittest.cc

+// Copyright (c) 2011 The Chromium Authors. All rights reserved.

[sky, 2014/08/11 23:45:45]

2014. But is there a reason this is a separate file and not part of the the
existing unittest?

[Alexander Alekseev, 2014/08/12 18:02:25]

This was Ilya's idea:

https://codereview.chromium.org/344883002/diff/380001/chrome/browser/about_fl...

[Ilya Sherman, 2014/08/12 18:05:30]

FWIW, I recommended moving this out to a separate file, as the test uses a
separate test fixture.  If you'd prefer that it be moved back, I'm ok with being
overruled :)

[sky, 2014/08/12 19:57:13]

AFAIK the common pattern is to put them in the same file. Plenty of files use
different fixtures.

[Alexander Alekseev, 2014/08/13 19:59:34]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <stdint.h>
+
+#include "base/files/file_path.h"
+#include "base/memory/scoped_vector.h"
+#include "base/path_service.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/stringprintf.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/values.h"
+#include "chrome/browser/about_flags.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/libxml/chromium/libxml_utils.h"
+
+namespace {
+
+typedef std::map<std::string, uint32_t> SwitchToIdMap;
+
+// This is a helper function to the next one.

[sky, 2014/08/11 23:45:45]

'the next one' makes no sense here.

[Alexander Alekseev, 2014/08/12 18:02:26]

Done.

+// Extracts single enum (with integer values) from histograms.xml.
+// Expects |reader| to point at given enum.
+// Returns map { value => label }.
+// Returns empty map on error.
+std::map<uint32_t, std::string> ParseEnumFromHistogramsXml(
+    const std::string& enum_name,
+    XmlReader* reader) {
+  int entries_index = -1;
+
+  std::map<uint32_t, std::string> result;
+  bool success = true;
+
+  while (true) {
+    const std::string node_name = reader->NodeName();
+    if (node_name == "enum" && reader->IsClosingElement())
+      break;
+
+    if (node_name == "int") {
+      ++entries_index;
+      std::string value_str;
+      std::string label;
+      const bool has_value = reader->NodeAttribute("value", &value_str);
+      const bool has_label = reader->NodeAttribute("label", &label);
+      if (!has_value) {
+        ADD_FAILURE() << "Bad " << enum_name << " enum entry (at index "
+                      << entries_index << ", label='" << label
+                      << "'): No 'value' attribute.";
+        success = false;
+      }
+      if (!has_label) {
+        ADD_FAILURE() << "Bad " << enum_name << " enum entry (at index "
+                      << entries_index << ", value_str='" << value_str
+                      << "'): No 'label' attribute.";
+        success = false;
+      }
+
+      uint32_t value;
+      if (has_value && !base::StringToUint(value_str, &value)) {
+        ADD_FAILURE() << "Bad " << enum_name << " enum entry (at index "
+                      << entries_index << ", label='" << label
+                      << "', value_str='" << value_str
+                      << "'): 'value' attribute is not integer.";
+        success = false;
+      }
+      if (result.count(value)) {
+        ADD_FAILURE() << "Bad " << enum_name << " enum entry (at index "
+                      << entries_index << ", label='" << label
+                      << "', value_str='" << value_str
+                      << "'): duplicate value '" << value_str
+                      << "' found in enum. The previous one has label='"
+                      << result[value] << "'.";
+        success = false;
+      }
+      if (success) {
+        result[value] = label;
+      }
+    }
+    // All enum entries are on the same level, so it is enough to iterate
+    // until possible.
+    reader->Next();
+  }
+  return (success ? result : std::map<uint32_t, std::string>());
+}
+
+// Find and read given enum (with integer values) from histograms.xml.
+// |enum_name| - enum name.
+// |histograms_xml| - must be loaded histograms.xml file.
+//
+// Returns map { value => label } so that:
+//   <int value="9" label="enable-pinch-virtual-viewport"/>
+// becomes:
+//   { 9 => "enable-pinch-virtual-viewport" }
+// Returns empty map on error.
+std::map<uint32_t, std::string> ReadEnumFromHistogramsXml(
+    const std::string& enum_name,
+    XmlReader* histograms_xml) {
+  std::map<uint32_t, std::string> login_custom_flags;
+
+  // Implement simple depth first search.
+  while (true) {
+    const std::string node_name = histograms_xml->NodeName();
+    if (node_name == "enum") {
+      std::string name;
+      if (histograms_xml->NodeAttribute("name", &name) && name == enum_name) {
+        if (!login_custom_flags.empty()) {
+          EXPECT_TRUE(login_custom_flags.empty())
+              << "Duplicate enum '" << enum_name << "' found in histograms.xml";
+          return std::map<uint32_t, std::string>();
+        }
+
+        const bool got_into_enum = histograms_xml->Read();
+        if (got_into_enum) {
+          login_custom_flags =
+              ParseEnumFromHistogramsXml(enum_name, histograms_xml);
+          EXPECT_FALSE(login_custom_flags.empty())
+              << "Bad enum '" << enum_name
+              << "' found in histograms.xml (format error).";
+        } else {
+          EXPECT_TRUE(got_into_enum)
+              << "Bad enum '" << enum_name
+              << "' (looks empty) found in histograms.xml.";
+        }
+        if (login_custom_flags.empty())
+          return std::map<uint32_t, std::string>();
+      }
+    }
+    // Go deeper if possible (stops at the closing tag of the deepest node).
+    if (histograms_xml->Read())
+      continue;
+
+    // Try next node on the same level (skips closing tag).
+    if (histograms_xml->Next())
+      continue;
+
+    // Go up until next node on the same level exists.
+    while (histograms_xml->Depth() && !histograms_xml->SkipToElement()) {
+    }
+
+    // Reached top. histograms.xml consists of the single top level node
+    // 'histogram-configuration', so this is the end.
+    if (!histograms_xml->Depth())
+      break;
+  }
+  EXPECT_FALSE(login_custom_flags.empty())
+      << "Enum '" << enum_name << "' is not found in histograms.xml.";
+  return login_custom_flags;
+}
+
+std::string FilePathStringTypeToString(const base::FilePath::StringType& path) {
+#if defined(OS_WIN)
+  return UTF16ToUTF8(path);
+#else
+  return path;
+#endif
+}
+
+std::set<std::string> GetAllSwitchesForTesting() {
+  std::set<std::string> result;
+
+  size_t num_experiments = 0;
+  const about_flags::Experiment* experiments =
+      about_flags::testing::GetExperiments(&num_experiments);
+
+  for (size_t i = 0; i < num_experiments; ++i) {
+    const about_flags::Experiment& experiment = experiments[i];
+    if (experiment.type == about_flags::Experiment::SINGLE_VALUE) {
+      result.insert(experiment.command_line_switch);
+    } else if (experiment.type == about_flags::Experiment::MULTI_VALUE) {
+      for (int j = 0; j < experiment.num_choices; ++j) {
+        result.insert(experiment.choices[j].command_line_switch);
+      }
+    } else {
+      DCHECK_EQ(experiment.type, about_flags::Experiment::ENABLE_DISABLE_VALUE);
+      result.insert(experiment.command_line_switch);
+      result.insert(experiment.disable_command_line_switch);
+    }
+  }
+  return result;
+}
+
+}  // anonymous namespace
+
+namespace about_flags {
+
+class AboutFlagsHistogramTest : public ::testing::Test {
+ protected:
+  // This is a helper function to check that all IDs in enum LoginCustomFlags in
+  // histograms.xml are unique.
+  void SetSwitchToHistogramIdMapping(const std::string& switch_name,
+                                     const uint32_t switch_histogram_id,
+                                     std::map<std::string, uint32_t>* out_map) {
+    const std::pair<std::map<std::string, uint32_t>::iterator, bool> status =
+        out_map->insert(std::make_pair(switch_name, switch_histogram_id));
+    if (!status.second) {
+      EXPECT_TRUE(status.first->second == switch_histogram_id)
+          << "Duplicate switch '" << switch_name
+          << "' found in enum 'LoginCustomFlags' in histograms.xml.";
+    }
+  }
+
+  // This method generates a hint for the user for what string should be added
+  // to the enum LoginCustomFlags to make in consistent.
+  std::string GetHistogramEnumEntryText(const std::string& switch_name,
+                                        uint32_t value) {
+    return base::StringPrintf(
+        "<int value=\"%u\" label=\"%s\"/>", value, switch_name.c_str());
+  }
+};
+
+TEST_F(AboutFlagsHistogramTest, CheckHistograms) {
+  base::FilePath histograms_xml_file_path;
+  ASSERT_TRUE(
+      PathService::Get(base::DIR_SOURCE_ROOT, &histograms_xml_file_path));
+  histograms_xml_file_path = histograms_xml_file_path.AppendASCII("tools")

[sky, 2014/08/11 23:45:45]

I believe this is going to require updating isolate files for swarming. Also,
does adding this as a dependency for the test work? If not, you'll need to
update the exclusions list for the analyze step. See
https://sites.google.com/a/chromium.org/dev/developers/testing/commit-queue/c...
for details.

[Alexander Alekseev, 2014/08/12 18:02:26]

This CL includes a change to chrome/unit_tests.isolate . Is it enough?
How can I add dependency for AboutFlagsHistogramTest test?
As far as I understand your URL, it says : "If you have added test
dependency and it doesn't work, add your file to exclusions" . But
how do I add the dependency? And how do I check it works?

[sky, 2014/08/12 19:57:13]

If you can express the dependency in the gyp/gypi file and that works as a
dependency, then the analyze step will recognize that if you change
histograms.xml then unit tests must work. I don't know if listing the dependency
as a source will work, but you could try. If it isn't possible, meaning you
can't express the dependency, then add the file to the list of exclusions.

You could check the dependency route by creating a separate patch that adds the
dependency to the appropriate gyp/gypi file, modifies histograms.xml, and modify
the exclusion file by removing the line with ".*gyp[i]?". Now do a try job and
verify unit_tests gets run.

[Ilya Sherman, 2014/08/12 20:07:16]

Oof, I'd really prefer not to have *all* changes to histograms.xml trigger
unit_tests runs.  Is it possible to have a tighter coupling, so that changes to
histograms.xml only trigger this one test to run?  If not, we can use the
sledgehammer approach for now... but if we do, it would be really nice to fix
this in the future.  I routinely submit histograms.xml changes with NOTRY=true,
because it's a huge waste of trybot cycles otherwise.  I'd love to see the
tooling catch up so that I don't need to do this.

[sky, 2014/08/12 22:42:03]

We currently only have the sledgehammer:(

[Alexander Alekseev, 2014/08/13 19:59:34]

It seems that adding histograms.xml to the list of sources works:
https://codereview.chromium.org/470613002/#ps20001
(there is another CL3 with additional changes to exclusions, but it seems it is
enough to add to sources).

+      .AppendASCII("metrics")
+      .AppendASCII("histograms")
+      .AppendASCII("histograms.xml");
+
+  XmlReader histograms_xml;
+  ASSERT_TRUE(histograms_xml.LoadFile(
+      FilePathStringTypeToString(histograms_xml_file_path.value())));
+  std::map<uint32_t, std::string> login_custom_flags =
+      ReadEnumFromHistogramsXml("LoginCustomFlags", &histograms_xml);
+  ASSERT_TRUE(login_custom_flags.size())
+      << "Error reading enum 'LoginCustomFlags' from histograms.xml.";
+
+  // Build reverse map {switch_name => id} from login_custom_flags.
+  SwitchToIdMap histograms_xml_switches_ids;
+
+  EXPECT_TRUE(login_custom_flags.count(kBadSwitchFormatHistogramId))
+      << "Entry for UMA ID of incorrect command-line flag is not found in "
+         "histograms.xml enum LoginCustomFlags. "
+         "Consider adding entry:\n"
+      << "  " << GetHistogramEnumEntryText("BAD_FLAG_FORMAT", 0);
+  // Check that all LoginCustomFlags entries have correct values.
+  for (std::map<uint32_t, std::string>::const_iterator it =
+           login_custom_flags.begin();
+       it != login_custom_flags.end();
+       ++it) {
+    if (it->first == kBadSwitchFormatHistogramId) {
+      // Add eror value with empty name.
+      SetSwitchToHistogramIdMapping(
+          "", it->first, &histograms_xml_switches_ids);
+      continue;
+    }
+    const uint32_t uma_id = GetSwitchUMAId(it->second);
+    EXPECT_EQ(uma_id, it->first)
+        << "histograms.xml enum LoginCustomFlags "
+           "entry '" << it->second << "' has incorrect value=" << it->first
+        << ", but " << uma_id << " is expected. Consider changing entry to:\n"
+        << "  " << GetHistogramEnumEntryText(it->second, uma_id);
+    SetSwitchToHistogramIdMapping(
+        it->second, it->first, &histograms_xml_switches_ids);
+  }
+
+  // Check that all flags in about_flags.cc have entries in login_custom_flags.
+  std::set<std::string> all_switches = GetAllSwitchesForTesting();
+  for (std::set<std::string>::const_iterator it = all_switches.begin();
+       it != all_switches.end();
+       ++it) {
+    // Skip empty placeholders.
+    if (it->empty())
+      continue;
+    const uint32_t uma_id = GetSwitchUMAId(*it);
+    EXPECT_NE(kBadSwitchFormatHistogramId, uma_id)
+        << "Command-line switch '" << *it
+        << "' from about_flags.cc has UMA ID equal to reserved value "
+           "kBadSwitchFormatHistogramId=" << kBadSwitchFormatHistogramId
+        << ". Please modify switch name.";
+    SwitchToIdMap::iterator enum_entry =
+        histograms_xml_switches_ids.lower_bound(*it);
+
+    // Ignore case here when switch ID is incorrect - it has already been
+    // reported in the previous loop.
+    EXPECT_TRUE(enum_entry != histograms_xml_switches_ids.end() &&
+                enum_entry->first == *it)
+        << "histograms.xml enum LoginCustomFlags doesn't contain switch '"
+        << *it << "' (value=" << uma_id
+        << " expected). Consider adding entry:\n"
+        << "  " << GetHistogramEnumEntryText(*it, uma_id);
+  }
+}
+
+}  // namespace about_flags