Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(520)

Side by Side Diff: chrome/browser/about_flags_unittest.cc

Issue 509923003: about_flags::ReportCustomFlags() should use signed 32-bit IDs. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master
Patch Set: Cleanup. Created 6 years, 3 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View unified diff | Download patch
« no previous file with comments | « chrome/browser/about_flags.cc ('k') | tools/metrics/histograms/histograms.xml » ('j') | no next file with comments »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
OLDNEW
1 // Copyright (c) 2011 The Chromium Authors. All rights reserved. 1 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
2 // Use of this source code is governed by a BSD-style license that can be 2 // Use of this source code is governed by a BSD-style license that can be
3 // found in the LICENSE file. 3 // found in the LICENSE file.
4 4
5 #include <stdint.h> 5 #include <stdint.h>
6 6
7 #include "base/files/file_path.h" 7 #include "base/files/file_path.h"
8 #include "base/memory/scoped_vector.h" 8 #include "base/memory/scoped_vector.h"
9 #include "base/path_service.h" 9 #include "base/path_service.h"
10 #include "base/prefs/pref_registry_simple.h" 10 #include "base/prefs/pref_registry_simple.h"
(...skipping 23 matching lines...) Expand all
34 const char kSwitch3[] = "switch3"; 34 const char kSwitch3[] = "switch3";
35 const char kValueForSwitch2[] = "value_for_switch2"; 35 const char kValueForSwitch2[] = "value_for_switch2";
36 36
37 const char kMultiSwitch1[] = "multi_switch1"; 37 const char kMultiSwitch1[] = "multi_switch1";
38 const char kMultiSwitch2[] = "multi_switch2"; 38 const char kMultiSwitch2[] = "multi_switch2";
39 const char kValueForMultiSwitch2[] = "value_for_multi_switch2"; 39 const char kValueForMultiSwitch2[] = "value_for_multi_switch2";
40 40
41 const char kEnableDisableValue1[] = "value1"; 41 const char kEnableDisableValue1[] = "value1";
42 const char kEnableDisableValue2[] = "value2"; 42 const char kEnableDisableValue2[] = "value2";
43 43
44 typedef std::map<std::string, uint32_t> SwitchToIdMap; 44 typedef base::HistogramBase::Sample Sample;
45 typedef std::map<std::string, Sample> SwitchToIdMap;
45 46
46 // This is a helper function to the ReadEnumFromHistogramsXml(). 47 // This is a helper function to the ReadEnumFromHistogramsXml().
47 // Extracts single enum (with integer values) from histograms.xml. 48 // Extracts single enum (with integer values) from histograms.xml.
48 // Expects |reader| to point at given enum. 49 // Expects |reader| to point at given enum.
49 // Returns map { value => label }. 50 // Returns map { value => label }.
50 // Returns empty map on error. 51 // Returns empty map on error.
51 std::map<uint32_t, std::string> ParseEnumFromHistogramsXml( 52 std::map<Sample, std::string> ParseEnumFromHistogramsXml(
52 const std::string& enum_name, 53 const std::string& enum_name,
53 XmlReader* reader) { 54 XmlReader* reader) {
54 int entries_index = -1; 55 int entries_index = -1;
55 56
56 std::map<uint32_t, std::string> result; 57 std::map<Sample, std::string> result;
57 bool success = true; 58 bool success = true;
58 59
59 while (true) { 60 while (true) {
60 const std::string node_name = reader->NodeName(); 61 const std::string node_name = reader->NodeName();
61 if (node_name == "enum" && reader->IsClosingElement()) 62 if (node_name == "enum" && reader->IsClosingElement())
62 break; 63 break;
63 64
64 if (node_name == "int") { 65 if (node_name == "int") {
65 ++entries_index; 66 ++entries_index;
66 std::string value_str; 67 std::string value_str;
67 std::string label; 68 std::string label;
68 const bool has_value = reader->NodeAttribute("value", &value_str); 69 const bool has_value = reader->NodeAttribute("value", &value_str);
69 const bool has_label = reader->NodeAttribute("label", &label); 70 const bool has_label = reader->NodeAttribute("label", &label);
70 if (!has_value) { 71 if (!has_value) {
71 ADD_FAILURE() << "Bad " << enum_name << " enum entry (at index " 72 ADD_FAILURE() << "Bad " << enum_name << " enum entry (at index "
72 << entries_index << ", label='" << label 73 << entries_index << ", label='" << label
73 << "'): No 'value' attribute."; 74 << "'): No 'value' attribute.";
74 success = false; 75 success = false;
75 } 76 }
76 if (!has_label) { 77 if (!has_label) {
77 ADD_FAILURE() << "Bad " << enum_name << " enum entry (at index " 78 ADD_FAILURE() << "Bad " << enum_name << " enum entry (at index "
78 << entries_index << ", value_str='" << value_str 79 << entries_index << ", value_str='" << value_str
79 << "'): No 'label' attribute."; 80 << "'): No 'label' attribute.";
80 success = false; 81 success = false;
81 } 82 }
82 83
83 uint32_t value; 84 Sample value;
84 if (has_value && !base::StringToUint(value_str, &value)) { 85 if (has_value && !base::StringToInt(value_str, &value)) {
85 ADD_FAILURE() << "Bad " << enum_name << " enum entry (at index " 86 ADD_FAILURE() << "Bad " << enum_name << " enum entry (at index "
86 << entries_index << ", label='" << label 87 << entries_index << ", label='" << label
87 << "', value_str='" << value_str 88 << "', value_str='" << value_str
88 << "'): 'value' attribute is not integer."; 89 << "'): 'value' attribute is not integer.";
89 success = false; 90 success = false;
90 } 91 }
91 if (result.count(value)) { 92 if (result.count(value)) {
92 ADD_FAILURE() << "Bad " << enum_name << " enum entry (at index " 93 ADD_FAILURE() << "Bad " << enum_name << " enum entry (at index "
93 << entries_index << ", label='" << label 94 << entries_index << ", label='" << label
94 << "', value_str='" << value_str 95 << "', value_str='" << value_str
95 << "'): duplicate value '" << value_str 96 << "'): duplicate value '" << value_str
96 << "' found in enum. The previous one has label='" 97 << "' found in enum. The previous one has label='"
97 << result[value] << "'."; 98 << result[value] << "'.";
98 success = false; 99 success = false;
99 } 100 }
100 if (success) { 101 if (success) {
101 result[value] = label; 102 result[value] = label;
102 } 103 }
103 } 104 }
104 // All enum entries are on the same level, so it is enough to iterate 105 // All enum entries are on the same level, so it is enough to iterate
105 // until possible. 106 // until possible.
106 reader->Next(); 107 reader->Next();
107 } 108 }
108 return (success ? result : std::map<uint32_t, std::string>()); 109 return (success ? result : std::map<Sample, std::string>());
109 } 110 }
110 111
111 // Find and read given enum (with integer values) from histograms.xml. 112 // Find and read given enum (with integer values) from histograms.xml.
112 // |enum_name| - enum name. 113 // |enum_name| - enum name.
113 // |histograms_xml| - must be loaded histograms.xml file. 114 // |histograms_xml| - must be loaded histograms.xml file.
114 // 115 //
115 // Returns map { value => label } so that: 116 // Returns map { value => label } so that:
116 // <int value="9" label="enable-pinch-virtual-viewport"/> 117 // <int value="9" label="enable-pinch-virtual-viewport"/>
117 // becomes: 118 // becomes:
118 // { 9 => "enable-pinch-virtual-viewport" } 119 // { 9 => "enable-pinch-virtual-viewport" }
119 // Returns empty map on error. 120 // Returns empty map on error.
120 std::map<uint32_t, std::string> ReadEnumFromHistogramsXml( 121 std::map<Sample, std::string> ReadEnumFromHistogramsXml(
121 const std::string& enum_name, 122 const std::string& enum_name,
122 XmlReader* histograms_xml) { 123 XmlReader* histograms_xml) {
123 std::map<uint32_t, std::string> login_custom_flags; 124 std::map<Sample, std::string> login_custom_flags;
124 125
125 // Implement simple depth first search. 126 // Implement simple depth first search.
126 while (true) { 127 while (true) {
127 const std::string node_name = histograms_xml->NodeName(); 128 const std::string node_name = histograms_xml->NodeName();
128 if (node_name == "enum") { 129 if (node_name == "enum") {
129 std::string name; 130 std::string name;
130 if (histograms_xml->NodeAttribute("name", &name) && name == enum_name) { 131 if (histograms_xml->NodeAttribute("name", &name) && name == enum_name) {
131 if (!login_custom_flags.empty()) { 132 if (!login_custom_flags.empty()) {
132 EXPECT_TRUE(login_custom_flags.empty()) 133 EXPECT_TRUE(login_custom_flags.empty())
133 << "Duplicate enum '" << enum_name << "' found in histograms.xml"; 134 << "Duplicate enum '" << enum_name << "' found in histograms.xml";
134 return std::map<uint32_t, std::string>(); 135 return std::map<Sample, std::string>();
135 } 136 }
136 137
137 const bool got_into_enum = histograms_xml->Read(); 138 const bool got_into_enum = histograms_xml->Read();
138 if (got_into_enum) { 139 if (got_into_enum) {
139 login_custom_flags = 140 login_custom_flags =
140 ParseEnumFromHistogramsXml(enum_name, histograms_xml); 141 ParseEnumFromHistogramsXml(enum_name, histograms_xml);
141 EXPECT_FALSE(login_custom_flags.empty()) 142 EXPECT_FALSE(login_custom_flags.empty())
142 << "Bad enum '" << enum_name 143 << "Bad enum '" << enum_name
143 << "' found in histograms.xml (format error)."; 144 << "' found in histograms.xml (format error).";
144 } else { 145 } else {
145 EXPECT_TRUE(got_into_enum) 146 EXPECT_TRUE(got_into_enum)
146 << "Bad enum '" << enum_name 147 << "Bad enum '" << enum_name
147 << "' (looks empty) found in histograms.xml."; 148 << "' (looks empty) found in histograms.xml.";
148 } 149 }
149 if (login_custom_flags.empty()) 150 if (login_custom_flags.empty())
150 return std::map<uint32_t, std::string>(); 151 return std::map<Sample, std::string>();
151 } 152 }
152 } 153 }
153 // Go deeper if possible (stops at the closing tag of the deepest node). 154 // Go deeper if possible (stops at the closing tag of the deepest node).
154 if (histograms_xml->Read()) 155 if (histograms_xml->Read())
155 continue; 156 continue;
156 157
157 // Try next node on the same level (skips closing tag). 158 // Try next node on the same level (skips closing tag).
158 if (histograms_xml->Next()) 159 if (histograms_xml->Next())
159 continue; 160 continue;
160 161
(...skipping 508 matching lines...) Expand 10 before | Expand all | Expand 10 after
669 std::string name = experiments->internal_name; 670 std::string name = experiments->internal_name;
670 EXPECT_EQ(std::string::npos, name.find(testing::kMultiSeparator)) << i; 671 EXPECT_EQ(std::string::npos, name.find(testing::kMultiSeparator)) << i;
671 } 672 }
672 } 673 }
673 674
674 class AboutFlagsHistogramTest : public ::testing::Test { 675 class AboutFlagsHistogramTest : public ::testing::Test {
675 protected: 676 protected:
676 // This is a helper function to check that all IDs in enum LoginCustomFlags in 677 // This is a helper function to check that all IDs in enum LoginCustomFlags in
677 // histograms.xml are unique. 678 // histograms.xml are unique.
678 void SetSwitchToHistogramIdMapping(const std::string& switch_name, 679 void SetSwitchToHistogramIdMapping(const std::string& switch_name,
679 const uint32_t switch_histogram_id, 680 const Sample switch_histogram_id,
680 std::map<std::string, uint32_t>* out_map) { 681 std::map<std::string, Sample>* out_map) {
681 const std::pair<std::map<std::string, uint32_t>::iterator, bool> status = 682 const std::pair<std::map<std::string, Sample>::iterator, bool> status =
682 out_map->insert(std::make_pair(switch_name, switch_histogram_id)); 683 out_map->insert(std::make_pair(switch_name, switch_histogram_id));
683 if (!status.second) { 684 if (!status.second) {
684 EXPECT_TRUE(status.first->second == switch_histogram_id) 685 EXPECT_TRUE(status.first->second == switch_histogram_id)
685 << "Duplicate switch '" << switch_name 686 << "Duplicate switch '" << switch_name
686 << "' found in enum 'LoginCustomFlags' in histograms.xml."; 687 << "' found in enum 'LoginCustomFlags' in histograms.xml.";
687 } 688 }
688 } 689 }
689 690
690 // This method generates a hint for the user for what string should be added 691 // This method generates a hint for the user for what string should be added
691 // to the enum LoginCustomFlags to make in consistent. 692 // to the enum LoginCustomFlags to make in consistent.
692 std::string GetHistogramEnumEntryText(const std::string& switch_name, 693 std::string GetHistogramEnumEntryText(const std::string& switch_name,
693 uint32_t value) { 694 Sample value) {
694 return base::StringPrintf( 695 return base::StringPrintf(
695 "<int value=\"%u\" label=\"%s\"/>", value, switch_name.c_str()); 696 "<int value=\"%d\" label=\"%s\"/>", value, switch_name.c_str());
696 } 697 }
697 }; 698 };
698 699
699 TEST_F(AboutFlagsHistogramTest, CheckHistograms) { 700 TEST_F(AboutFlagsHistogramTest, CheckHistograms) {
700 base::FilePath histograms_xml_file_path; 701 base::FilePath histograms_xml_file_path;
701 ASSERT_TRUE( 702 ASSERT_TRUE(
702 PathService::Get(base::DIR_SOURCE_ROOT, &histograms_xml_file_path)); 703 PathService::Get(base::DIR_SOURCE_ROOT, &histograms_xml_file_path));
703 histograms_xml_file_path = histograms_xml_file_path.AppendASCII("tools") 704 histograms_xml_file_path = histograms_xml_file_path.AppendASCII("tools")
704 .AppendASCII("metrics") 705 .AppendASCII("metrics")
705 .AppendASCII("histograms") 706 .AppendASCII("histograms")
706 .AppendASCII("histograms.xml"); 707 .AppendASCII("histograms.xml");
707 708
708 XmlReader histograms_xml; 709 XmlReader histograms_xml;
709 ASSERT_TRUE(histograms_xml.LoadFile( 710 ASSERT_TRUE(histograms_xml.LoadFile(
710 FilePathStringTypeToString(histograms_xml_file_path.value()))); 711 FilePathStringTypeToString(histograms_xml_file_path.value())));
711 std::map<uint32_t, std::string> login_custom_flags = 712 std::map<Sample, std::string> login_custom_flags =
712 ReadEnumFromHistogramsXml("LoginCustomFlags", &histograms_xml); 713 ReadEnumFromHistogramsXml("LoginCustomFlags", &histograms_xml);
713 ASSERT_TRUE(login_custom_flags.size()) 714 ASSERT_TRUE(login_custom_flags.size())
714 << "Error reading enum 'LoginCustomFlags' from histograms.xml."; 715 << "Error reading enum 'LoginCustomFlags' from histograms.xml.";
715 716
716 // Build reverse map {switch_name => id} from login_custom_flags. 717 // Build reverse map {switch_name => id} from login_custom_flags.
717 SwitchToIdMap histograms_xml_switches_ids; 718 SwitchToIdMap histograms_xml_switches_ids;
718 719
719 EXPECT_TRUE(login_custom_flags.count(kBadSwitchFormatHistogramId)) 720 EXPECT_TRUE(login_custom_flags.count(kBadSwitchFormatHistogramId))
720 << "Entry for UMA ID of incorrect command-line flag is not found in " 721 << "Entry for UMA ID of incorrect command-line flag is not found in "
721 "histograms.xml enum LoginCustomFlags. " 722 "histograms.xml enum LoginCustomFlags. "
722 "Consider adding entry:\n" 723 "Consider adding entry:\n"
723 << " " << GetHistogramEnumEntryText("BAD_FLAG_FORMAT", 0); 724 << " " << GetHistogramEnumEntryText("BAD_FLAG_FORMAT", 0);
724 // Check that all LoginCustomFlags entries have correct values. 725 // Check that all LoginCustomFlags entries have correct values.
725 for (std::map<uint32_t, std::string>::const_iterator it = 726 for (std::map<Sample, std::string>::const_iterator it =
726 login_custom_flags.begin(); 727 login_custom_flags.begin();
727 it != login_custom_flags.end(); 728 it != login_custom_flags.end();
728 ++it) { 729 ++it) {
729 if (it->first == kBadSwitchFormatHistogramId) { 730 if (it->first == kBadSwitchFormatHistogramId) {
730 // Add eror value with empty name. 731 // Add eror value with empty name.
731 SetSwitchToHistogramIdMapping( 732 SetSwitchToHistogramIdMapping(
732 "", it->first, &histograms_xml_switches_ids); 733 "", it->first, &histograms_xml_switches_ids);
733 continue; 734 continue;
734 } 735 }
735 const uint32_t uma_id = GetSwitchUMAId(it->second); 736 const Sample uma_id = GetSwitchUMAId(it->second);
736 EXPECT_EQ(uma_id, it->first) 737 EXPECT_EQ(uma_id, it->first)
737 << "histograms.xml enum LoginCustomFlags " 738 << "histograms.xml enum LoginCustomFlags "
738 "entry '" << it->second << "' has incorrect value=" << it->first 739 "entry '" << it->second << "' has incorrect value=" << it->first
739 << ", but " << uma_id << " is expected. Consider changing entry to:\n" 740 << ", but " << uma_id << " is expected. Consider changing entry to:\n"
740 << " " << GetHistogramEnumEntryText(it->second, uma_id); 741 << " " << GetHistogramEnumEntryText(it->second, uma_id);
741 SetSwitchToHistogramIdMapping( 742 SetSwitchToHistogramIdMapping(
742 it->second, it->first, &histograms_xml_switches_ids); 743 it->second, it->first, &histograms_xml_switches_ids);
743 } 744 }
744 745
745 // Check that all flags in about_flags.cc have entries in login_custom_flags. 746 // Check that all flags in about_flags.cc have entries in login_custom_flags.
746 std::set<std::string> all_switches = GetAllSwitchesForTesting(); 747 std::set<std::string> all_switches = GetAllSwitchesForTesting();
747 for (std::set<std::string>::const_iterator it = all_switches.begin(); 748 for (std::set<std::string>::const_iterator it = all_switches.begin();
748 it != all_switches.end(); 749 it != all_switches.end();
749 ++it) { 750 ++it) {
750 // Skip empty placeholders. 751 // Skip empty placeholders.
751 if (it->empty()) 752 if (it->empty())
752 continue; 753 continue;
753 const uint32_t uma_id = GetSwitchUMAId(*it); 754 const Sample uma_id = GetSwitchUMAId(*it);
754 EXPECT_NE(kBadSwitchFormatHistogramId, uma_id) 755 EXPECT_NE(kBadSwitchFormatHistogramId, uma_id)
755 << "Command-line switch '" << *it 756 << "Command-line switch '" << *it
756 << "' from about_flags.cc has UMA ID equal to reserved value " 757 << "' from about_flags.cc has UMA ID equal to reserved value "
757 "kBadSwitchFormatHistogramId=" << kBadSwitchFormatHistogramId 758 "kBadSwitchFormatHistogramId=" << kBadSwitchFormatHistogramId
758 << ". Please modify switch name."; 759 << ". Please modify switch name.";
759 SwitchToIdMap::iterator enum_entry = 760 SwitchToIdMap::iterator enum_entry =
760 histograms_xml_switches_ids.lower_bound(*it); 761 histograms_xml_switches_ids.lower_bound(*it);
761 762
762 // Ignore case here when switch ID is incorrect - it has already been 763 // Ignore case here when switch ID is incorrect - it has already been
763 // reported in the previous loop. 764 // reported in the previous loop.
764 EXPECT_TRUE(enum_entry != histograms_xml_switches_ids.end() && 765 EXPECT_TRUE(enum_entry != histograms_xml_switches_ids.end() &&
765 enum_entry->first == *it) 766 enum_entry->first == *it)
766 << "histograms.xml enum LoginCustomFlags doesn't contain switch '" 767 << "histograms.xml enum LoginCustomFlags doesn't contain switch '"
767 << *it << "' (value=" << uma_id 768 << *it << "' (value=" << uma_id
768 << " expected). Consider adding entry:\n" 769 << " expected). Consider adding entry:\n"
769 << " " << GetHistogramEnumEntryText(*it, uma_id); 770 << " " << GetHistogramEnumEntryText(*it, uma_id);
770 } 771 }
771 } 772 }
772 773
773 } // namespace about_flags 774 } // namespace about_flags
OLDNEW
« no previous file with comments | « chrome/browser/about_flags.cc ('k') | tools/metrics/histograms/histograms.xml » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698