[Findit] Adding support for extracting revisions from dict instead of list

appengine/findit/waterfall/test/identify_try_job_culprit_pipeline_test.py

Index: appengine/findit/waterfall/test/identify_try_job_culprit_pipeline_test.py
diff --git a/appengine/findit/waterfall/test/identify_try_job_culprit_pipeline_test.py b/appengine/findit/waterfall/test/identify_try_job_culprit_pipeline_test.py
index 4557aa46735982ce6fdb8ad322e11d8652b2dbe7..9837d72594bb43a027c6b210f21e8467b03ac407 100644
--- a/appengine/findit/waterfall/test/identify_try_job_culprit_pipeline_test.py
+++ b/appengine/findit/waterfall/test/identify_try_job_culprit_pipeline_test.py
@@ -24,96 +24,141 @@ class IdentifyTryJobCulpritPipelineTest(testing.AppengineTestCase):
     mock_change_logs['rev2'] = MockedChangeLog('2', 'url_2')
     return mock_change_logs.get(revision)
 
-  def testIdentifyCulpritForCompileReturnNoneIfNoCompileResult(self):
-    master_name = 'm'
-    builder_name = 'b'
-    build_number = 1
-    try_job_id = '1'
-    compile_result = None
-
-    WfTryJob.Create(master_name, builder_name, build_number).put()
-    pipeline = IdentifyTryJobCulpritPipeline()
-    culprit = pipeline.run(
-        master_name, builder_name, build_number, try_job_id, compile_result)
-    try_job = WfTryJob.Get(master_name, builder_name, build_number)
-
-    self.assertIsNone(culprit)
-    self.assertEqual(wf_analysis_status.ANALYZED, try_job.status)
+  def testGetFailedRevisionFromResultsDict(self):
+    self.assertIsNone(
+        IdentifyTryJobCulpritPipeline._GetFailedRevisionFromResultsDict({}))
+    self.assertEqual(
+        None,
+        IdentifyTryJobCulpritPipeline._GetFailedRevisionFromResultsDict(
+            {'rev1': 'passed'}))
+    self.assertEqual(
+        'rev1',
+        IdentifyTryJobCulpritPipeline._GetFailedRevisionFromResultsDict(
+            {'rev1': 'failed'}))
+    self.assertEqual(
+        'rev2',
+        IdentifyTryJobCulpritPipeline._GetFailedRevisionFromResultsDict(
+            {'rev1': 'passed', 'rev2': 'failed'}))
+
+  def testGetFailedRevisionFromCompileResult(self):
+    self.assertIsNone(
+        IdentifyTryJobCulpritPipeline._GetFailedRevisionFromCompileResult(
+            None))
+    self.assertIsNone(
+        IdentifyTryJobCulpritPipeline._GetFailedRevisionFromCompileResult(
+            {'report': {}}))
+    self.assertIsNone(
+        IdentifyTryJobCulpritPipeline._GetFailedRevisionFromCompileResult(
+            {
+                'report': {
+                    'result': {
+                        'rev1': 'passed'
+                    }
+                }
+            }))
+    self.assertEqual(
+        'rev2',
+        IdentifyTryJobCulpritPipeline._GetFailedRevisionFromCompileResult(
+            {
+                'report': {
+                    'result': {
+                        'rev1': 'passed',
+                        'rev2': 'failed'
+                    }
+                }
+            }))
+    self.assertEqual(
+        'rev2',
+        IdentifyTryJobCulpritPipeline._GetFailedRevisionFromCompileResult(
+            {
+                'report': [
+                    ['rev1', 'passed'],
+                    ['rev2', 'failed']
+                ]
+            }))
+
+  def testGetCulpritFromFailedRevision(self):
+    self.mock(GitRepository, 'GetChangeLog', self._MockGetChangeLog)
+    self.assertIsNone(
+        IdentifyTryJobCulpritPipeline._GetCulpritFromFailedRevision(None))
+    self.assertIsNone(
+        IdentifyTryJobCulpritPipeline._GetCulpritFromFailedRevision(
+            'revision_with_no_change_log'))
+    self.assertEqual(
+        {
+            'revision': 'rev2',
+            'commit_position': '2',
+            'review_url': 'url_2'
+        },
+        IdentifyTryJobCulpritPipeline._GetCulpritFromFailedRevision('rev2'))
 
-  def testIdentifyCulpritForCompileReturnNoneIfNoTryJobResultForCompile(self):
+  def testIdentifyCulpritForCompileTryJobSuccess(self):
     master_name = 'm'
     builder_name = 'b'
     build_number = 1
     try_job_id = '1'
     compile_result = {
-        'result': [],
+        'report': {
+            'result': {
+                'rev1': 'passed',
+                'rev2': 'failed'
+            }
+        },
         'url': 'url',

[stgao, 2016/01/27 01:24:21]

As 'url' is not used and 'try_job_id' is already passed as a separate parameter,
should we delete these two fields?

[lijeffrey, 2016/01/28 02:14:35]

Done.

[chanli, 2016/01/28 21:18:44]

It is fine for the test but the real format have those 2 fields.

         'try_job_id': '1',
     }
-    WfTryJob.Create(master_name, builder_name, build_number).put()
 
-    pipeline = IdentifyTryJobCulpritPipeline()
-    culprit = pipeline.run(
-        master_name, builder_name, build_number, try_job_id, compile_result)
-    try_job = WfTryJob.Get(master_name, builder_name, build_number)
-
-    self.assertIsNone(culprit)
-    self.assertEqual(wf_analysis_status.ANALYZED, try_job.status)
-
-  def testIdentifyCulpritForCompileReturnNoneIfAllPassed(self):
-    master_name = 'm'
-    builder_name = 'b'
-    build_number = 1
-    try_job_id = '1'
-    compile_result = {
-        'result': [
-            ['rev1', 'passed'],
-            ['rev2', 'passed']
-        ],
+    try_job = WfTryJob.Create(master_name, builder_name, build_number)
+    try_job.status = wf_analysis_status.ANALYZING
+    try_job.compile_results = [{
+        'report': {
+            'result': {
+                'rev1': 'passed',
+                'rev2': 'failed'
+            },
+        },
         'url': 'url',
         'try_job_id': '1',
-    }
-    WfTryJob.Create(master_name, builder_name, build_number).put()
+    }]
+    try_job.put()
+
+    self.mock(GitRepository, 'GetChangeLog', self._MockGetChangeLog)
 
     pipeline = IdentifyTryJobCulpritPipeline()
     culprit = pipeline.run(
         master_name, builder_name, build_number, try_job_id, compile_result)
-    try_job = WfTryJob.Get(master_name, builder_name, build_number)
 
-    self.assertIsNone(culprit)
-    self.assertEqual(wf_analysis_status.ANALYZED, try_job.status)
-
-  def testIdentifyCulpritForCompileReturnNoneIfNoChangeLog(self):
-    master_name = 'm'
-    builder_name = 'b'
-    build_number = 1
-    try_job_id = '1'
-    compile_result = {
-        'result': [
-            ['rev1', 'failed']
-        ],
+    expected_compile_result = {
+        'report': {
+            'result': {
+                'rev1': 'passed',
+                'rev2': 'failed'
+            }
+        },
         'url': 'url',
         'try_job_id': '1',
+        'culprit': {
+            'revision': 'rev2',
+            'commit_position': '2',
+            'review_url': 'url_2'
+        }
     }
 
-    self.mock(GitRepository, 'GetChangeLog', self._MockGetChangeLog)
-    WfTryJob.Create(master_name, builder_name, build_number).put()
+    self.assertEqual(expected_compile_result['culprit'], culprit)
 
-    pipeline = IdentifyTryJobCulpritPipeline()
-    culprit = pipeline.run(
-        master_name, builder_name, build_number, try_job_id, compile_result)
     try_job = WfTryJob.Get(master_name, builder_name, build_number)
-
-    self.assertIsNone(culprit)
+    self.assertEqual(expected_compile_result, try_job.compile_results[-1])
     self.assertEqual(wf_analysis_status.ANALYZED, try_job.status)
 
-  def testIdentifyCulpritForCompileTryJobSuccess(self):
+  # TODO(lijeffrey): Deprecate all tests with compile_results whose 'result'
+  # key is a list once it is returned as a dict from the recipe.
+  def testIdentifyCulpritForCompileListTryJobSuccess(self):
     master_name = 'm'
     builder_name = 'b'
     build_number = 1
     try_job_id = '1'
     compile_result = {
-        'result': [
+        'report': [
             ['rev1', 'passed'],
             ['rev2', 'failed']
         ],
@@ -124,7 +169,7 @@ class IdentifyTryJobCulpritPipelineTest(testing.AppengineTestCase):
     try_job = WfTryJob.Create(master_name, builder_name, build_number)
     try_job.status = wf_analysis_status.ANALYZING
     try_job.compile_results = [{
-        'result': [
+        'report': [
             ['rev1', 'passed'],
             ['rev2', 'failed']
         ],
@@ -140,7 +185,7 @@ class IdentifyTryJobCulpritPipelineTest(testing.AppengineTestCase):
         master_name, builder_name, build_number, try_job_id, compile_result)
 
     expected_compile_result = {
-        'result': [
+        'report': [
             ['rev1', 'passed'],
             ['rev2', 'failed']
         ],