Fail on CQ when annotation names don't match between code and XML

- When annotations.xml is modified in a CL, run a full test of traffic
  annotations to catch more errors.

- When there are duplicate hash codes (e.g. after renaming in
  annotations.xml), fail even if error-resilient mode is on.

These 2 things combined means the buildbot will fail on CQ instead of
only failing on the waterfall.

Bug: 876855
Change-Id: Ib4ada288f4aa29943d1413bd81c4ec7c22735fc6
Reviewed-on: https://chromium-review.googlesource.com/c/1291813
Commit-Queue: Nicolas Ouellet-Payeur <nicolaso@chromium.org>
Reviewed-by: Ramin Halavati <rhalavati@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601926}

tools/traffic_annotation/auditor/auditor_result.cc

@@ -175,6 +175,12 @@ std::string AuditorResult::ToText() const {
                                 details_[0].c_str(), details_[1].c_str(),
                                 file_path_.c_str());
 
+    case AuditorResult::Type::ERROR_DEPRECATED_WITH_OS:
+      return base::StringPrintf(
+          "Annotation '%s' has a deprecation date and at least one active OS "
+          "at %s.",
+          details_[0].c_str(), file_path_.c_str());
+
     default:
       return std::string();
   }

tools/traffic_annotation/auditor/auditor_result.h

@@ -46,6 +46,8 @@ class AuditorResult {
     ERROR_ANNOTATIONS_XML_UPDATE,  // Annotations XML requires update.
     ERROR_TEST_ANNOTATION,         // Annotation for tests is used.
     ERROR_INVALID_OS,              // Invalid 'os_list' in annotations.xml
+    ERROR_DEPRECATED_WITH_OS,      // Marked deprecated, but 'os_list' is not
+                                   // empty in annotations.xml.
   };
 
   static const int kNoCodeLineSpecified;

tools/traffic_annotation/auditor/traffic_annotation_auditor.cc

@@ -761,32 +761,17 @@ void TrafficAnnotationAuditor::AddMissingAnnotations(
   }
 }
 
-void TrafficAnnotationAuditor::CheckArchivedAnnotationsHaveValidOsList() {
-  const auto& supported_platforms = exporter_.GetAllSupportedPlatforms();
-  for (const auto& pair : exporter_.GetArchivedAnnotations()) {
-    for (const auto& os : pair.second.os_list) {
-      if (!base::ContainsValue(supported_platforms, os)) {
-        AuditorResult error(
-            AuditorResult::Type::ERROR_INVALID_OS, std::string(),
-            TrafficAnnotationExporter::kAnnotationsXmlPath.MaybeAsASCII(),
-            AuditorResult::kNoCodeLineSpecified);
-        error.AddDetail(os);
-        error.AddDetail(pair.first);
-        errors_.push_back(std::move(error));
-      }
-    }
-  }
-}
-
 bool TrafficAnnotationAuditor::RunAllChecks(
     const std::vector<std::string>& path_filters,
     bool report_xml_updates) {
-  std::set<int> deprecated_ids;
-
-  if (!exporter_.GetDeprecatedHashCodes(&deprecated_ids)) {
+  if (exporter_.GetArchivedAnnotations().empty() &&
+      !exporter_.LoadAnnotationsXML()) {
     return false;
   }
 
+  std::set<int> deprecated_ids;
+  exporter_.GetDeprecatedHashCodes(&deprecated_ids);
+
   if (path_filters.size())
     AddMissingAnnotations(path_filters);
 
@@ -800,15 +785,11 @@ bool TrafficAnnotationAuditor::RunAllChecks(
   if (errors_.empty())
     CheckAnnotationsContents();
 
-  CheckArchivedAnnotationsHaveValidOsList();
-
   CheckAllRequiredFunctionsAreAnnotated();
 
   if (errors_.empty()) {
-    if (!exporter_.UpdateAnnotations(extracted_annotations_,
-                                     GetReservedIDsMap())) {
-      return false;
-    }
+    exporter_.UpdateAnnotations(extracted_annotations_, GetReservedIDsMap(),
+                                &errors_);
   }
 
   // If |report_xml_updates| is true, check annotations.xml whether or not it is

tools/traffic_annotation/auditor/traffic_annotation_auditor.h

@@ -98,9 +98,6 @@ class TrafficAnnotationAuditor {
   // Checks if a call instance can stay not annotated.
   bool CheckIfCallCanBeUnannotated(const CallInstance& call);
 
-  // Checks if all 'os_list' entries in annotations.xml are supported platforms.
-  void CheckArchivedAnnotationsHaveValidOsList();
-
   // Performs all checks on extracted annotations and calls. The input path
   // filters are passed so that the data for files that were not tested would be
   // read from annotations.xml. If |report_xml_updates| is set and

tools/traffic_annotation/auditor/traffic_annotation_auditor_unittest.cc

@@ -909,7 +909,8 @@ TEST_F(TrafficAnnotationAuditorTest, AnnotationsXML) {
   TrafficAnnotationExporter exporter(source_path());
 
   EXPECT_TRUE(exporter.LoadAnnotationsXML());
-  EXPECT_TRUE(exporter.CheckArchivedAnnotations());
+  exporter.CheckArchivedAnnotations(errors());
+  EXPECT_TRUE(errors()->empty());
 }
 
 // Tests if 'annotations.xml' is read and has at least one item.

tools/traffic_annotation/auditor/traffic_annotation_exporter.cc

@@ -26,6 +26,12 @@ const char* kXmlComment =
     "\nRefer to README.md for content description and update process.\n"
     "-->\n\n";
 
+const base::FilePath kAnnotationsXmlPath =
+    base::FilePath(FILE_PATH_LITERAL("tools"))
+        .Append(FILE_PATH_LITERAL("traffic_annotation"))
+        .Append(FILE_PATH_LITERAL("summary"))
+        .Append(FILE_PATH_LITERAL("annotations.xml"));
+
 // Extracts annotation id from a line of XML. Expects to have the line in the
 // following format: <... id="..." .../>
 // TODO(rhalavati): Use real XML parsing.
@@ -56,12 +62,6 @@ void ExtractXMLItems(const std::string& serialized_xml,
 
 }  // namespace
 
-const base::FilePath TrafficAnnotationExporter::kAnnotationsXmlPath =
-    base::FilePath(FILE_PATH_LITERAL("tools"))
-        .Append(FILE_PATH_LITERAL("traffic_annotation"))
-        .Append(FILE_PATH_LITERAL("summary"))
-        .Append(FILE_PATH_LITERAL("annotations.xml"));
-
 TrafficAnnotationExporter::ArchivedAnnotation::ArchivedAnnotation()
     : type(AnnotationInstance::Type::ANNOTATION_COMPLETE),
       unique_id_hash_code(-1),
@@ -166,11 +166,12 @@ bool TrafficAnnotationExporter::LoadAnnotationsXML() {
   return all_ok;
 }
 
-bool TrafficAnnotationExporter::UpdateAnnotations(
+void TrafficAnnotationExporter::UpdateAnnotations(
     const std::vector<AnnotationInstance>& annotations,
-    const std::map<int, std::string>& reserved_ids) {
-  if (archive_.empty() && !LoadAnnotationsXML())
-    return false;
+    const std::map<int, std::string>& reserved_ids,
+    std::vector<AuditorResult>* errors) {
+  CHECK(!archive_.empty());
+  DCHECK(errors);
 
   std::set<int> current_platform_hashcodes;
 
@@ -270,7 +271,7 @@ bool TrafficAnnotationExporter::UpdateAnnotations(
     }
   }
 
-  return CheckArchivedAnnotations();
+  CheckArchivedAnnotations(errors);
 }
 
 std::string TrafficAnnotationExporter::GenerateSerializedXML() const {
@@ -346,41 +347,56 @@ bool TrafficAnnotationExporter::SaveAnnotationsXML() const {
                          xml_content.c_str(), xml_content.length()) != -1;
 }
 
-bool TrafficAnnotationExporter::GetDeprecatedHashCodes(
+void TrafficAnnotationExporter::GetDeprecatedHashCodes(
     std::set<int>* hash_codes) {
-  if (archive_.empty() && !LoadAnnotationsXML())
-    return false;
+  CHECK(!archive_.empty());
 
   hash_codes->clear();
   for (const auto& item : archive_) {
     if (!item.second.deprecation_date.empty())
       hash_codes->insert(item.second.unique_id_hash_code);
   }
-  return true;
 }
 
-bool TrafficAnnotationExporter::CheckArchivedAnnotations() {
+void TrafficAnnotationExporter::CheckArchivedAnnotations(
+    std::vector<AuditorResult>* errors) {
+  DCHECK(errors);
   // Check for annotation hash code duplications.
-  std::set<int> used_codes;
+  std::map<int, std::string> used_codes;
   for (auto& item : archive_) {
     if (base::ContainsKey(used_codes, item.second.unique_id_hash_code)) {
-      LOG(ERROR) << "Unique id hash code " << item.second.unique_id_hash_code
-                 << " is used more than once.";
-      return false;
+      AuditorResult error(AuditorResult::Type::ERROR_HASH_CODE_COLLISION);
+      error.AddDetail(used_codes[item.second.unique_id_hash_code]);
+      error.AddDetail(item.first);
+      errors->push_back(std::move(error));
     } else {
-      used_codes.insert(item.second.unique_id_hash_code);
+      used_codes[item.second.unique_id_hash_code] = item.first;
     }
   }
 
   // Check for coexistence of OS(es) and deprecation date.
   for (auto& item : archive_) {
     if (!item.second.deprecation_date.empty() && !item.second.os_list.empty()) {
-      LOG(ERROR) << "Annotation " << item.first
-                 << " has a deprecation date and at least one active OS.";
-      return false;
+      errors->push_back(
+          AuditorResult(AuditorResult::Type::ERROR_DEPRECATED_WITH_OS,
+                        item.first, kAnnotationsXmlPath.MaybeAsASCII(),
+                        AuditorResult::kNoCodeLineSpecified));
+    }
+  }
+
+  // Check that listed OSes are valid.
+  for (const auto& pair : archive_) {
+    for (const auto& os : pair.second.os_list) {
+      if (!base::ContainsValue(all_supported_platforms_, os)) {
+        AuditorResult error(AuditorResult::Type::ERROR_INVALID_OS,
+                            std::string(), kAnnotationsXmlPath.MaybeAsASCII(),
+                            AuditorResult::kNoCodeLineSpecified);
+        error.AddDetail(os);
+        error.AddDetail(pair.first);
+        errors->push_back(std::move(error));
+      }
     }
   }
-  return true;
 }
 
 unsigned TrafficAnnotationExporter::GetXMLItemsCountForTesting() {

tools/traffic_annotation/auditor/traffic_annotation_exporter.h

@@ -34,8 +34,6 @@ class TrafficAnnotationExporter {
     std::string file_path;
   };
 
-  static const base::FilePath kAnnotationsXmlPath;
-
   TrafficAnnotationExporter(const base::FilePath& source_path);
   ~TrafficAnnotationExporter();
   TrafficAnnotationExporter(const TrafficAnnotationExporter&) = delete;
@@ -44,10 +42,12 @@ class TrafficAnnotationExporter {
   // Loads annotations from annotations.xml file into |archive_|.
   bool LoadAnnotationsXML();
 
-  // Updates |archive_| with current set of extracted annotations and
-  // reserved ids. Sets the |modified_| flag if any item is updated.
-  bool UpdateAnnotations(const std::vector<AnnotationInstance>& annotations,
-                         const std::map<int, std::string>& reserved_ids);
+  // Updates |archive_| with current set of extracted annotations and reserved
+  // ids. Sets the |modified_| flag if any item is updated. Appends errors to
+  // |errors|.
+  void UpdateAnnotations(const std::vector<AnnotationInstance>& annotations,
+                         const std::map<int, std::string>& reserved_ids,
+                         std::vector<AuditorResult>* errors);
 
   // Saves |archive_| into annotations.xml.
   bool SaveAnnotationsXML() const;
@@ -55,24 +55,20 @@ class TrafficAnnotationExporter {
   // Returns the required updates for annotations.xml.
   std::string GetRequiredUpdates();
 
-  // Produces the list of deprecated hash codes. Returns false if
-  // annotations.xml is not loaded and cannot be loaded.
-  bool GetDeprecatedHashCodes(std::set<int>* hash_codes);
+  // Produces the list of deprecated hash codes. Requires annotations.xml to be
+  // loaded.
+  void GetDeprecatedHashCodes(std::set<int>* hash_codes);
 
   bool modified() const { return modified_; }
 
   // Runs tests on content of |archive_|.
-  bool CheckArchivedAnnotations();
+  void CheckArchivedAnnotations(std::vector<AuditorResult>* errors);
 
   const std::map<std::string, ArchivedAnnotation>& GetArchivedAnnotations()
       const {
     return archive_;
   }
 
-  const std::vector<std::string>& GetAllSupportedPlatforms() const {
-    return all_supported_platforms_;
-  }
-
   // Checks if the current platform is in the os list of archived annotation.
   bool MatchesCurrentPlatform(const ArchivedAnnotation& annotation) const {
     return base::ContainsValue(annotation.os_list, current_platform_);

tools/traffic_annotation/scripts/check_annotations.py

@@ -25,6 +25,7 @@ CHANGELIST_SIZE_TO_TRIGGER_FULL_TEST = 100
 
 class NetworkTrafficAnnotationChecker():
   EXTENSIONS = ['.cc', '.mm',]
+  ANNOTATIONS_FILE = 'annotations.xml'
 
   def __init__(self, build_path=None):
     """Initializes a NetworkTrafficAnnotationChecker object.
@@ -36,11 +37,47 @@ class NetworkTrafficAnnotationChecker():
     """
     self.tools = NetworkTrafficAnnotationTools(build_path)
 
+  def IsAnnotationsFile(self, file_path):
+    """Returns true if the given file is the annotations file."""
+    return os.path.basename(file_path) == self.ANNOTATIONS_FILE
+
   def ShouldCheckFile(self, file_path):
     """Returns true if the input file has an extension relevant to network
     traffic annotations."""
     return os.path.splitext(file_path)[1] in self.EXTENSIONS
 
+  def GetFilePaths(self, complete_run, limit):
+    if complete_run:
+      return []
+
+    # Get list of modified files. If failed, silently ignore as the test is
+    # run in error resilient mode.
+    file_paths = self.tools.GetModifiedFiles() or []
+
+    annotations_file_changed = any(
+        self.IsAnnotationsFile(file_path) for file_path in file_paths)
+
+    # If the annotations file has changed, trigger a full test to avoid
+    # missing a case where the annotations file has changed, but not the
+    # corresponding file, causing a mismatch that is not detected by just
+    # checking the changed .cc and .mm files.
+    if annotations_file_changed:
+      return []
+
+    file_paths = [
+        file_path for file_path in file_paths if self.ShouldCheckFile(
+            file_path)]
+    if not file_paths:
+      return None
+
+    # If the number of changed files in the CL exceeds a threshold, trigger
+    # full test to avoid sending very long list of arguments and possible
+    # failure in argument buffers.
+    if len(file_paths) > CHANGELIST_SIZE_TO_TRIGGER_FULL_TEST:
+      file_paths = []
+
+    return file_paths
+
   def CheckFiles(self, complete_run, limit):
     """Passes all given files to traffic_annotation_auditor to be checked for
     possible violations of network traffic annotation rules.
@@ -59,22 +96,9 @@ class NetworkTrafficAnnotationChecker():
             "are required to do it.")
       return 0
 
-    if complete_run:
-      file_paths = []
-    else:
-      # Get list of modified files. If failed, silently ignore as the test is
-      # run in error resilient mode.
-      file_paths = self.tools.GetModifiedFiles() or []
-      file_paths = [
-          file_path for file_path in file_paths if self.ShouldCheckFile(
-              file_path)]
-      if not file_paths:
-        return 0
-      # If the number of changed files in the CL exceeds a threshold, trigger
-      # full test to avoid sending very long list of arguments and possible
-      # failure in argument buffers.
-      if len(file_paths) > CHANGELIST_SIZE_TO_TRIGGER_FULL_TEST:
-        file_paths = []
+    file_paths = self.GetFilePaths(complete_run, limit)
+    if file_paths is None:
+      return 0
 
     args = ["--test-only", "--limit=%i" % limit, "--error-resilient"] + \
            file_paths