[Findit] Add a sub-pipeline to analyze failures caused by DEPS rolls.

appengine/findit/waterfall/test/build_failure_analysis_test.py

 # Copyright 2015 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 import unittest
 
 from common.diff import ChangeType
 from waterfall import build_failure_analysis
 from waterfall.failure_signal import FailureSignal
 
@@ skipping 55 matching lines @@
         'files': {
             'src/a/b/f1.cc': [],
             'd/e/a2_test.cc': [],
             'b/c/f2.cc': [10, 20],
             'd/e/f3.h': [],
             'x/y/f4.py': [],
             'f5_impl.cc': []
         }
     }
     change_log_json = {
+        'revision': 'rev',
         'touched_files': [
             {
                 'change_type': ChangeType.ADD,
                 'old_path': '/dev/null',
                 'new_path': 'a/b/f1.cc'
             },
             {
                 'change_type': ChangeType.ADD,
                 'old_path': '/dev/null',
                 'new_path': 'd/e/a2.cc'
@@ skipping 18 matching lines @@
                 'old_path': 'h/f5.h',
                 'new_path': '/dev/null'
             },
             {
                 'change_type': ChangeType.RENAME,
                 'old_path': 't/y/x.cc',
                 'new_path': 's/z/x.cc'
             },
         ]
     }
+    deps_info = {}
 
     justification = build_failure_analysis._CheckFiles(
-        FailureSignal.FromDict(failure_signal_json), change_log_json)
+        FailureSignal.FromDict(failure_signal_json), change_log_json, deps_info)
     self.assertIsNotNone(justification)
-    # The score is 14 because:
+    # The score is 15 because:
     # +5 added a/b/f1.cc (same file src/a/b/f1.cc in failure_signal log)
     # +1 added d/e/a2.cc (related file a2_test.cc in failure_signal log)
     # +1 modified b/c/f2.h (related file a/b/c/f2.cc in failure_signal log)
     # +2 modified d/e/f3.h (same file d/e/f3.h in failure_signal log)
     # +5 deleted x/y/f4.py (same file x/y/f4.py in failure_signal log)
     # +1 deleted h/f5.h (related file f5_impl.cc in failure_signal log)
     # +0 renamed t/y/x.cc -> s/z/x.cc (no related file in failure_signal log)
     self.assertEqual(15, justification['score'])
 
   def testCheckFilesAgainstUnrelatedCL(self):
     failure_signal_json = {
         'files': {
             'src/a/b/f.cc': [],
         }
     }
     change_log_json = {
+        'revision': 'rev',
         'touched_files': [
             {
                 'change_type': ChangeType.ADD,
                 'old_path': '/dev/null',
                 'new_path': 'a/d/f1.cc'
             },
         ]
     }
+    deps_info = {}
 
     justification = build_failure_analysis._CheckFiles(
-        FailureSignal.FromDict(failure_signal_json), change_log_json)
+        FailureSignal.FromDict(failure_signal_json), change_log_json, deps_info)
     self.assertIsNone(justification)
 
+  def testCheckFilesAgainstDEPSRoll(self):
+    failure_signal_json = {
+        'files': {
+            'src/third_party/dep1/f.cc': [123],
+        }
+    }
+    change_log_json = {
+        'revision': 'rev',
+        'touched_files': [
+            {
+                'change_type': ChangeType.MODIFY,
+                'old_path': 'DEPS',
+                'new_path': 'DEPS'
+            },
+        ]
+    }
+    deps_info = {
+        'deps_rolls': {
+            'rev': [
+                {
+                    'path': 'src/third_party/dep1/',
+                    'repo_url': 'https://url_dep1',
+                    'old_revision': '7',
+                    'new_revision': '9',
+                },
+                {
+                    'path': 'third_party/dep2',
+                    'repo_url': 'https://url_dep2',
+                    'old_revision': None,
+                    'new_revision': '1',
+                },
+            ]
+        }
+    }
+
+    justification = build_failure_analysis._CheckFiles(
+        FailureSignal.FromDict(failure_signal_json), change_log_json, deps_info)
+    self.assertIsNotNone(justification)
+    # The score is 2 because:
+    # +2 rolled third_party/dep1/ and src/third_party/dep1/f.cc was in log.
+    self.assertEqual(2, justification['score'])
+
   def testAnalyzeSuccessfulBuild(self):
     failure_info = {
         'failed': False,
     }
     result = build_failure_analysis.AnalyzeBuildFailure(
-        failure_info, None, None)
+        failure_info, None, None, None)

[qyearsley, 2015/05/29 23:14:33]

Optional: You can add argument names, even if they're not optional args in the
place where the function is defined:

result = build_failure_analysis.AnalyzeBuildFailure(
    failure_info, change_logs=None, deps_info=None, failure_signals=None)

Pros: Clearer to read here.
Cons: This will break if you change the argument names.

Also, if it still makes sense to call AnalyzeBuildFailure with those other
things passed as None, maybe you should make them optional/named arguments.

[stgao, 2015/05/30 00:21:05]

Done. That's a good idea for readability.
No. Passing None here is for testing purpose only.

     self.assertEqual(0, len(result['failures']))
 
   def testAnalyzeBuildFailure(self):
     failure_info = {
         'failed': True,
         'failed_steps': {
             'a': {
                 'current_failure': 99,
                 'first_failure': 98,
             },
@@ skipping 68 matching lines @@
             'revision': 'r96_1',
             'touched_files': [
                 {
                     'change_type': ChangeType.MODIFY,
                     'old_path': 'a/b/f96_1.cc',
                     'new_path': 'a/b/f96_1.cc'
                 },
             ],
         },
     }
+    deps_info = {}
     failure_signals_json = {
         'a': {
           'files': {
               'src/a/b/f99_2.cc': [],
           },
         },
         'b': {
           'files': {
               'x/y/f99_1.cc': [],
           },
@@ skipping 34 matching lines @@
                         'hints': {
                             'added x/y/f99_1.cc (and it was in log)': 5,
                         },
                     }
                 ],
             }
         ]
     }
 
     analysis_result = build_failure_analysis.AnalyzeBuildFailure(
-        failure_info, change_logs, failure_signals_json)
+        failure_info, change_logs, deps_info, failure_signals_json)
     self.assertEqual(expected_analysis_result, analysis_result)