[Findit] Reorganizing findit_for_*.py

appengine/findit/crash/test/crash_pipeline_test.py

 # Copyright 2016 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 copy
 import json
 
 from google.appengine.api import app_identity
 
 from common.pipeline_wrapper import pipeline_handlers
 from crash import crash_pipeline
 from crash import findit_for_chromecrash
+from crash.crash_report import CrashReport
+from crash.findit_for_chromecrash import FinditForFracas
+from crash.stacktrace import Stacktrace
 from crash.test.crash_testcase import CrashTestCase
+from crash.type_enums import CrashClient
 from model import analysis_status
 from model.crash.fracas_crash_analysis import FracasCrashAnalysis
 
 
+# TODO(wrengr): Our use of this highly suggests that |crash_identifiers|
+# don't actually need to be passed around everywhere. The only parts that
+# matter are the |channel| and the |process_type|, which can be passed
+# separately.
+def CrashIdentifiers(report):
+  return {
+      'chrome_version': report.crashed_version,
+      'signature': report.signature,
+      'channel': 'canary',
+      'platform': report.platform,
+      'process_type': 'browser'
+    }
+
+DUMMY_REPORT1 = CrashReport(
+    crashed_version = '1',
+    signature = 'signature',
+    platform = 'win',
+    stacktrace = Stacktrace(),
+    regression_range = None)
+
+DUMMY_CRASH_IDENTIFIERS1 = CrashIdentifiers(DUMMY_REPORT1)
+
 class CrashPipelineTest(CrashTestCase):
   app_module = pipeline_handlers._APP
 
   def testNoAnalysisIfLastOneIsNotFailed(self):
-    chrome_version = '1'
-    signature = 'signature'
-    platform = 'win'
-    crash_identifiers = {
-      'chrome_version': chrome_version,
-      'signature': signature,
-      'channel': 'canary',
-      'platform': platform,
-      'process_type': 'browser',
-    }
     for status in (analysis_status.PENDING, analysis_status.RUNNING,
                    analysis_status.COMPLETED, analysis_status.SKIPPED):
-      analysis = FracasCrashAnalysis.Create(crash_identifiers)
+      analysis = FracasCrashAnalysis.Create(DUMMY_CRASH_IDENTIFIERS1)
       analysis.status = status
       analysis.put()
       self.assertFalse(crash_pipeline._NeedsNewAnalysis(
-          crash_identifiers, chrome_version, signature, 'fracas',
-          platform, None, {'channel': 'canary'}))
+          FinditForFracas(), DUMMY_CRASH_IDENTIFIERS1, DUMMY_REPORT1))
 
   def testAnalysisNeededIfLastOneFailed(self):
-    chrome_version = '1'
-    signature = 'signature'
-    platform = 'win'
-    crash_identifiers = {
-      'chrome_version': chrome_version,
-      'signature': signature,
-      'channel': 'canary',
-      'platform': platform,
-      'process_type': 'browser',
-    }
-    analysis = FracasCrashAnalysis.Create(crash_identifiers)
+    analysis = FracasCrashAnalysis.Create(DUMMY_CRASH_IDENTIFIERS1)
     analysis.status = analysis_status.ERROR
     analysis.put()
     self.assertTrue(crash_pipeline._NeedsNewAnalysis(
-          crash_identifiers, chrome_version, signature, 'fracas',
-          platform, None, {'channel': 'canary'}))
+        FinditForFracas(), DUMMY_CRASH_IDENTIFIERS1, DUMMY_REPORT1))
 
   def testAnalysisNeededIfNoAnalysisYet(self):
-    chrome_version = '1'
-    signature = 'signature'
-    platform = 'win'
-    crash_identifiers = {
-      'chrome_version': chrome_version,
-      'signature': signature,
-      'channel': 'canary',
-      'platform': platform,
-      'process_type': 'browser',
-    }
     self.assertTrue(crash_pipeline._NeedsNewAnalysis(
-          crash_identifiers, chrome_version, signature, 'fracas',
-          platform, None, {'channel': 'canary'}))
+        FinditForFracas(), DUMMY_CRASH_IDENTIFIERS1, DUMMY_REPORT1))
 
   def testUnsupportedChannelOrPlatformSkipped(self):
+    channel = None
     self.assertFalse(
-        crash_pipeline.ScheduleNewAnalysisForCrash(
-            {}, None, None, 'fracas', 'win',
-            None, {'channel': 'unsupported_channel',
-                   'historical_metadata': None}))
+        crash_pipeline.ScheduleNewAnalysisForCrash(FinditForFracas(), {},
+            CrashReport(None, None, 'win', Stacktrace(), None), channel))
     self.assertFalse(
-        crash_pipeline.ScheduleNewAnalysisForCrash(
-            {}, None, None, 'fracas', 'unsupported_platform',
-            None, {'channel': 'unsupported_channel',
-                   'historical_metadata': None}))
+        crash_pipeline.ScheduleNewAnalysisForCrash(FinditForFracas(), {},
+            CrashReport(None, None, 'unsupported_platform', Stacktrace(), None),
+            channel))
 
   def testBlackListSignatureSipped(self):
+    channel = None
+    signature = 'Blacklist marker signature'
     self.assertFalse(
-        crash_pipeline.ScheduleNewAnalysisForCrash(
-            {}, None, 'Blacklist marker signature', 'fracas', 'win',
-            None, {'channel': 'canary',
-                   'historical_metadata': None}))
+        crash_pipeline.ScheduleNewAnalysisForCrash(FinditForFracas(), {},
+            CrashReport(None, signature, 'win', Stacktrace(), None), channel))
 
   def testPlatformRename(self):
-    def _MockNeedsNewAnalysis(*args):
-      self.assertEqual(args,
-                       ({}, None, 'signature', 'fracas', 'unix', None,
-                        {'channel': 'canary'}))
+    channel = 'canary'
+    def _MockNeedsNewAnalysis(findit_client, crash_identifiers, report):
+
+      self.assertEqual(
+          (findit_client.client_id, crash_identifiers, report),
+          (CrashClient.FRACAS, {},
+           CrashReport(None, 'signature', 'unix', Stacktrace(), None)))
       return False
 
     self.mock(crash_pipeline, '_NeedsNewAnalysis', _MockNeedsNewAnalysis)
 
-    crash_pipeline.ScheduleNewAnalysisForCrash(
-        {}, None, 'signature', 'fracas', 'linux',
-        None, {'channel': 'canary'})
+    crash_pipeline.ScheduleNewAnalysisForCrash(FinditForFracas(), {},
+        CrashReport(None, 'signature', 'linux', Stacktrace(), None), channel)
 
   def testNoAnalysisNeeded(self):
-    chrome_version = '1'
-    signature = 'signature'
-    platform = 'win'
-    channel = 'canary'
-    crash_identifiers = {
-      'chrome_version': chrome_version,
-      'signature': signature,
-      'channel': channel,
-      'platform': platform,
-      'process_type': 'browser',
-    }
-    analysis = FracasCrashAnalysis.Create(crash_identifiers)
+    analysis = FracasCrashAnalysis.Create(DUMMY_CRASH_IDENTIFIERS1)
     analysis.status = analysis_status.COMPLETED
     analysis.put()
-
     self.assertFalse(
         crash_pipeline.ScheduleNewAnalysisForCrash(
-            crash_identifiers, chrome_version, signature, 'fracas',
-            platform, None, {'channel': channel,
-                             'historical_metadata': None}))
+            FinditForFracas(), DUMMY_CRASH_IDENTIFIERS1, DUMMY_REPORT1,
+            analysis.channel))
 
   def _TestRunningAnalysisForResult(self, analysis_result, analysis_tags):
     pubsub_publish_requests = []
     def Mocked_PublishMessagesToTopic(messages_data, topic):
       pubsub_publish_requests.append((messages_data, topic))
     self.mock(crash_pipeline.pubsub_util, 'PublishMessagesToTopic',
               Mocked_PublishMessagesToTopic)
 
     analyzed_crashes = []
-    class Mocked_FinditForChromeCrash(object):
-      def __init__(self, *_):
-        pass
+    class _MockFinditForFracas(FinditForFracas):
       def FindCulprit(self, *args):
         analyzed_crashes.append(args)
+        # TODO(wrengr): this needs to return a Culprit object
         return analysis_result, analysis_tags
-    self.mock(findit_for_chromecrash, 'FinditForChromeCrash',
-        Mocked_FinditForChromeCrash)
 
-    chrome_version = '1'
-    signature = 'signature'
-    platform = 'win'
-    channel = 'canary'
-    crash_identifiers = {
-      'chrome_version': chrome_version,
-      'signature': signature,
-      'channel': channel,
-      'platform': platform,
-      'process_type': 'browser',
-    }
     stack_trace = 'frame1\nframe2\nframe3'
     chrome_version = '50.2500.0.1'
     historical_metadata = None
 
     mock_host = 'https://host.com'
     self.mock(app_identity, 'get_default_version_hostname', lambda: mock_host)
 
+    channel = 'canary'
     self.assertTrue(
         crash_pipeline.ScheduleNewAnalysisForCrash(
-            crash_identifiers, chrome_version, signature, 'fracas',
-            platform, stack_trace,
-            {'channel': channel, 'historical_metadata': historical_metadata}))
+            _MockFinditForFracas(),
+            DUMMY_CRASH_IDENTIFIERS1,
+            DUMMY_REPORT1._replace(stacktrace=stack_trace), channel))
 
+    # TODO(katesonia): this method call is crashing with an AppError
+    # 500. How to fix it?
     self.execute_queued_tasks()
 
     self.assertEqual(1, len(pubsub_publish_requests))
 
     processed_analysis_result = copy.deepcopy(analysis_result)
     processed_analysis_result['feedback_url'] = (
         mock_host + '/crash/fracas-result-feedback?'
         'key=agx0ZXN0YmVkLXRlc3RyQQsSE0ZyYWNhc0NyYXNoQW5hbHlzaXMiKGU2ZWIyNj'
         'A2OTBlYTAyMjVjNWNjYTM3ZTNjYTlmYWExOGVmYjVlM2UM')
 
     if 'suspected_cls' in processed_analysis_result:
       for cl in processed_analysis_result['suspected_cls']:
         cl['confidence'] = round(cl['confidence'], 2)
         cl.pop('reason', None)
 
     expected_messages_data = [json.dumps({
-            'crash_identifiers': crash_identifiers,
+            'crash_identifiers': DUMMY_CRASH_IDENTIFIERS1,
             'client_id': 'fracas',
             'result': processed_analysis_result,
         }, sort_keys=True)]
     self.assertEqual(expected_messages_data, pubsub_publish_requests[0][0])
 
     self.assertEqual(1, len(analyzed_crashes))
     self.assertEqual(
-        (signature, platform, stack_trace, chrome_version, None),
+        (DUMMY_REPORT1.signature, DUMMY_REPORT1.platform, stack_trace, chrome_version, None),
         analyzed_crashes[0])
 
-    analysis = FracasCrashAnalysis.Get(crash_identifiers)
+    analysis = FracasCrashAnalysis.Get(DUMMY_CRASH_IDENTIFIERS1)
     self.assertEqual(analysis_result, analysis.result)
     return analysis
 
 
   def testRunningAnalysis(self):

[stgao, 2016/10/12 15:46:58]

Hopefully https://codereview.chromium.org/2414523002/ could help on testing the
pipeline logic here.

For failed assertion on JSON output result, if it is too long a diff, you may
add below to show the whole diff:

def setUp(self):
  super(self, CLASS_NAME_HERE).setUp()
  self.maxDiff = None

[wrengr, 2016/10/12 21:14:51]

Um, that's this CL. Did you have another one in mind?

[stgao, 2016/10/13 06:03:17]

Sorry, I don't understand this. Would you mind elaborating a bit more?

For the CL I provided here, I made a change to support mocking sub-pipeline for
testing the containing pipeline. And there is an example there.
I guess we could reuse the same approach here too.

[wrengr, 2016/10/18 23:13:54]

My/this CL is 2414523002, so when I follow the link you gave it just links back
to the current CL. I'm not sure which CL you had in mind with that mocking
change; perhaps you mis-pasted the link?

     analysis_result = {
         'found': True,
         'suspected_cls': [],
         'other_data': 'data',
     }
     analysis_tags = {
         'found_suspects': True,
         'has_regression_range': True,
         'solution': 'core',
         'unsupported_tag': '',
@@ skipping 39 matching lines @@
         'unsupported_tag': '',
     }
 
     analysis = self._TestRunningAnalysisForResult(
         analysis_result, analysis_tags)
     self.assertTrue(analysis.has_regression_range)
     self.assertTrue(analysis.found_suspects)
     self.assertEqual('core', analysis.solution)
 
   def testAnalysisAborted(self):
-    chrome_version = '1'
-    signature = 'signature'
-    platform = 'win'
-    crash_identifiers = {
-      'chrome_version': chrome_version,
-      'signature': signature,
-      'channel': 'canary',
-      'platform': platform,
-      'process_type': 'browser',
-    }
-    analysis = FracasCrashAnalysis.Create(crash_identifiers)
+    analysis = FracasCrashAnalysis.Create(DUMMY_CRASH_IDENTIFIERS1)
     analysis.status = analysis_status.RUNNING
     analysis.put()
 
-    pipeline = crash_pipeline.CrashAnalysisPipeline(crash_identifiers, 'fracas')
-    pipeline._SetErrorIfAborted(True)
-    analysis = FracasCrashAnalysis.Get(crash_identifiers)
+    pipeline = crash_pipeline.CrashAnalysisPipeline(DUMMY_CRASH_IDENTIFIERS1, 'fracas')
+    # TODO(wrengr): we can't actually set |was_aborted|, so we'll have to call |finalized()| some other way. Of re-factor it out again :(
+    pipeline.was_aborted = True

[stgao, 2016/10/12 15:46:58]

Yes, we have to go with the original approach for testing purpose.
was_aborted is not an mutable property.

[wrengr, 2016/10/12 21:14:51]

Acknowledged.

+    pipeline.finalized()
+    analysis = FracasCrashAnalysis.Get(DUMMY_CRASH_IDENTIFIERS1)
     self.assertEqual(analysis_status.ERROR, analysis.status)