Issue 2414523002: [Findit] Reorganizing findit_for_*.py

wrengr

Description was changed from ========== [Findit] Reorganizing findit_for_*.py BUG= ========== to ========== [Findit] Reorganizing findit_for_*.py ...

4 years, 2 months ago (2016-10-11 22:01:44 UTC) #1

wrengr

wrengr@chromium.org changed reviewers: + katesonia@chromium.org, mbarbella@chromium.org, stgao@chromium.org

4 years, 2 months ago (2016-10-11 22:01:44 UTC) #2

wrengr

Some of the unittests don't pass, but it's not entirely clear how to fix them. ...

4 years, 2 months ago (2016-10-11 22:05:50 UTC) #3

stgao

I really like the change in this CL. And it should improve the code a ...

4 years, 2 months ago (2016-10-12 15:46:58 UTC) #4

Sharu Jiang

Post some comments first, will take a further look, for failed tests, can you post ...

4 years, 2 months ago (2016-10-12 17:46:30 UTC) #5

wrengr

Thanks for the comments. I have a few followup questions included below. Also, still need ...

4 years, 2 months ago (2016-10-12 21:14:51 UTC) #6

Sharu Jiang

https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/crash/crash_pipeline.py File appengine/findit/crash/crash_pipeline.py (right): https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/crash/crash_pipeline.py#newcode28 appengine/findit/crash/crash_pipeline.py:28: # TODO(wrengr): this should prolly be a method on ...

4 years, 2 months ago (2016-10-12 22:35:03 UTC) #9

https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/crash/...
File appengine/findit/crash/crash_pipeline.py (right):

https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/crash_pipeline.py:28: # TODO(wrengr): this should prolly
be a method on CrashAnalysis rather
I prefer to put this in CrashAnalysis.

https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/crash_pipeline.py:75: return
findit.FinditForClusterfuzz()
We'd better create a findit_for_clusterfuzz for class FinditForClusterfuzz

https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/crash_pipeline.py:161: def
_NeedsNewAnalysis(findit_client, crash_identifiers, report,
historical_metadata=None):
instead of passing in historical_metadata, we should pass customized_data, since
for clusterfuzz, there is no historical_metadata, we can directly get
regression_range from customized_data.

https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/crash_pipeline.py:181: def
ScheduleNewAnalysisForCrash(findit_client, crash_identifiers, report,
?
In crash_handler, this function is called as below:

crash_pipeline.ScheduleNewAnalysisForCrash(
    crash_data['crash_identifiers'],
    crash_data['chrome_version'],
    crash_data['signature'],
    crash_data['client_id'],
    crash_data['platform'],
    crash_data['stack_trace'],
    crash_data['customized_data']['channel'],
    queue_name=constants.CRASH_ANALYSIS_QUEUE[crash_data['client_id']])

however the parameters here are findit_client, crash_identifiers... should we
create findit_client and report somewhere?

https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/crash_pipeline.py:197: analysis_pipeline =
CrashWrapperPipeline(crash_identifiers, findit_client.client_id)
Assume we already created findit_client here, why just pass the findit_client
instead of passing the client_id and creating another self.findit inside
CrashBasePipeline

https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/crash/...
File appengine/findit/crash/findit.py (right):

https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/findit.py:82: return None
On 2016/10/12 21:14:51, wrengr wrote:
> On 2016/10/12 17:46:29, sharu jiang wrote:
> > shouldn't this raise NotImplementedError()?
> 
> That's what I originally had, but a lot of the unittests rely on it returning
> None.
> 
> As written it fits the docstring (in this case: using the policy to reject all
> reports), so I'm not too concerned.  I'm also fine with raising
> NotImplementedError, but if we do that then I'll need someone to go through
and
> update the unittests to handle that appropriately.

For those Not Implemented methods, we can simply skip testing, add "# pragma: no
cover." to skip code coverage and only test those implemented methods.

https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/findit.py:93: def ResetAnalysis(self, model, report):
The create order should be:

client-input-messeage --> ndb.CrashAnalysis --> CrashReport

So the parameters here should be (model, signature, platform, chrome_versions,
...), CrashReport should be the parsed object which is ready for analysis.

The name may be a little confusing, however changing name of ndb.CrashAnalysis
would cause some legacy data issue, so I prefer keep it as it is.

https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/findit.py:132: class FinditForClusterfuzz(Findit):
On 2016/10/12 21:14:51, wrengr wrote:
> On 2016/10/12 17:46:29, sharu jiang wrote:
> > Why is this in findit.py?
> 
> Because it's as-yet too short to bother breaking out into its own file? I
could
> break it out if you like, I just figured you'd do that as part of the CL
> implementing it.

Can you break it out to findit_for_clustefuzz.py? since you already wrote the
skeleton code. Thanks :) 

No findit_for_clustefuzz_test.py is needed, so the test coverage won't skan this
file.

https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/crash/...
File appengine/findit/crash/findit_for_chromecrash.py (right):

https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/findit_for_chromecrash.py:36: self.chromecrash_parser =
ChromeCrashParser()
this can be renamed to more general name like stacktrace_parser, since it's
already inside findit_for_chromecrash.

https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/findit_for_chromecrash.py:39: self.azalea = Azalea(
If we lift the FulCulprit to findit class, the azalea can be lifted too.

https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/findit_for_chromecrash.py:96: historical_metadata =
kwargs.get('historical_metadata', None)
On 2016/10/12 21:14:51, wrengr wrote:
> On 2016/10/12 17:46:29, sharu jiang wrote:
> > We already got regression range before running analysis while determining
> > whether to rerun or not.
> 
> Is the regression_range stored in the CrashAnalysis model? Or should it be
> passed in as a separate argument?

It should be stored in CrashAnalysis model before running analysis.

https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/handle...
File appengine/findit/handlers/crash/crash_handler.py (right):

https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/handle...
appengine/findit/handlers/crash/crash_handler.py:112:
crash_data['customized_data']['channel'],
On 2016/10/12 21:14:51, wrengr wrote:
> On 2016/10/12 17:46:29, sharu jiang wrote:
> > ? This is not correct, the 'historical_metadata' got discarded.
> 
> ScheduleNewAnalysisForCrash (the version in this CL, at least) doesn't use the
> historical_metadata, it only uses the channel; hence passing it directly
rather
> than bundling it up in a dict.

The function is to schedule analysis for any client, however the 'channel' is
fracas/cracas-specific. We should pass in the 'customized_data' and handle it
accordingly.

stgao

https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/crash/crash_pipeline.py File appengine/findit/crash/crash_pipeline.py (right): https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/crash/crash_pipeline.py#newcode100 appengine/findit/crash/crash_pipeline.py:100: logging.error('Aborted analysis for %s', repr(self.crash_identifiers)) On 2016/10/12 21:14:51, wrengr ...

4 years, 2 months ago (2016-10-13 06:03:17 UTC) #10

wrengr

I've posted a newer version of the CL. Still having issues with crash.test.findit_for_chromecrash_test.testFindCulpritForChromeCrash being flaky ...

4 years, 2 months ago (2016-10-18 23:13:54 UTC) #11

I've posted a newer version of the CL. Still having issues with
crash.test.findit_for_chromecrash_test.testFindCulpritForChromeCrash being flaky
about mocking DetectRegressionRange. Also, a number of tests fail because of
things that can't be serialized to JSON.

https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/crash/...
File appengine/findit/crash/crash_pipeline.py (right):

https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/crash_pipeline.py:28: # TODO(wrengr): this should prolly
be a method on CrashAnalysis rather
On 2016/10/12 22:35:03, sharu jiang wrote:
> I prefer to put this in CrashAnalysis.

Done.

https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/crash_pipeline.py:75: return
findit.FinditForClusterfuzz()
On 2016/10/12 22:35:03, sharu jiang wrote:
> We'd better create a findit_for_clusterfuzz for class FinditForClusterfuzz

Done.

https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/crash_pipeline.py:100: logging.error('Aborted analysis
for %s', repr(self.crash_identifiers))
On 2016/10/13 06:03:16, stgao wrote:
> On 2016/10/12 21:14:51, wrengr wrote:
> > On 2016/10/12 15:46:58, stgao wrote:
> > > For testing purpose, we need to factor this out as before.
> > 
> > I've been told elsewhere that the main code shouldn't be written to the
> > requirements of tests, but rather the tests should be written around the
main
> > code. 
> 
> Yep, I agreed with this in general.
> 
> > So why does this need to be factored out for that one test? Why can't the
> > test be rewritten?
> 
> But in this case, I failed to find a way to simulate an abortion of pipeline
> after playing around with it for a couple hours. If some way is found to
> simulate such a scenario, I'd be happy to change other code to adopt the
> simulation.

Done.

https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/crash_pipeline.py:161: def
_NeedsNewAnalysis(findit_client, crash_identifiers, report,
historical_metadata=None):
On 2016/10/12 22:35:03, sharu jiang wrote:
> instead of passing in historical_metadata, we should pass customized_data,
since
> for clusterfuzz, there is no historical_metadata, we can directly get
> regression_range from customized_data.

I can add another optional argument for regression_range, and document how the
two arguments interact. But I dislike passing around a dictionary with an
unhelpful name like "customized_data". As far as callsites go, they can always
unpack the dict to pass them as keyword arguments via `**customized_data`
(afaict, the dict is always small enough there shouldn't be any performance
concerns). I'd prefer either that, or else defining some helper type that makes
clear what all is expected to be in "customized_data", rather than having an
anonymous dict.

https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/crash_pipeline.py:181: def
ScheduleNewAnalysisForCrash(findit_client, crash_identifiers, report,
On 2016/10/12 22:35:03, sharu jiang wrote:
> ?
> In crash_handler, this function is called as below:
> 
> crash_pipeline.ScheduleNewAnalysisForCrash(
>     crash_data['crash_identifiers'],
>     crash_data['chrome_version'],
>     crash_data['signature'],
>     crash_data['client_id'],
>     crash_data['platform'],
>     crash_data['stack_trace'],
>     crash_data['customized_data']['channel'],
>     queue_name=constants.CRASH_ANALYSIS_QUEUE[crash_data['client_id']])
> 
> however the parameters here are findit_client, crash_identifiers... should we
> create findit_client and report somewhere?

Whoops, I missed that callsite when reordering/reorganizing the arguments. Good
catch!

Yeah, the old code constructed a new findit client (or the moral equivalent
thereof), so we'll want to construct one from the crash_data['client_id'] since
the old client_id and the new findit_client serve the same role. There're some
tricky bits about getting the regression range and parsing the callstack without
duplicating code. I'll see if I can come up with a good way of doing that.

https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/crash_pipeline.py:197: analysis_pipeline =
CrashWrapperPipeline(crash_identifiers, findit_client.client_id)
On 2016/10/12 22:35:03, sharu jiang wrote:
> Assume we already created findit_client here, why just pass the findit_client
> instead of passing the client_id and creating another self.findit inside
> CrashBasePipeline

The general goal is to avoid passing the client_id around. That is, we want to
inspect client_id only once, when constructing the Findit object (done in
FinditForClientID). Since the Findit instance encapsulates the knowledge of
which client_id we're working with, we can pass that object around everywhere
and not need to worry about having a bunch of case switching on the client_id.

I think part of the problem is that it's not really clear (to me) what the goal
of crash_pipeline.py is. It feels like there's an inversion of responsibilities
here; that is, rather than having crash_pipeline.py call into findit.py, it
feels like it should go the other way with findit.py calling into
crash_pipeline.py. That way the Findit class would encapsulate not just the
Azalea instance but also all the pipeline instances. Alas, I'm not familiar
enough with appengine to be sure of doing that change correctly.

https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/crash/...
File appengine/findit/crash/findit.py (right):

https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/findit.py:93: def ResetAnalysis(self, model, report):
On 2016/10/12 22:35:03, sharu jiang wrote:
> The create order should be:
> 
> client-input-messeage --> ndb.CrashAnalysis --> CrashReport
> 
> So the parameters here should be (model, signature, platform, chrome_versions,
> ...), CrashReport should be the parsed object which is ready for analysis.
> 
> The name may be a little confusing, however changing name of ndb.CrashAnalysis
> would cause some legacy data issue, so I prefer keep it as it is.

It's not clear to me when/why one would call ResetAnalysis, regardless of how
the arguments are phrased. The previous version was undocumented so I have no
idea what it's supposed to mean/do. The collection of arguments it takes are the
same as other places taking CrashReports, so that doesn't really help. And since
CrashAnalysis comprises information from both the CrashReport and the Culprit,
when I hear "reset" I think of discarding whatever's there and replacing it with
just the CrashReport to be analysed; which fits with the fact that it sets the
status to PENDING.

Provided the current code works, I'd rather land the CL and try to perfect
things later. All of this stuff will be moving to ./service/azalea-app/ anyways,
since it's appengine code not library code. And that reorg will entail changes
to the rest of the appengine code anyways.

https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/findit.py:132: class FinditForClusterfuzz(Findit):
On 2016/10/12 22:35:03, sharu jiang wrote:
> On 2016/10/12 21:14:51, wrengr wrote:
> > On 2016/10/12 17:46:29, sharu jiang wrote:
> > > Why is this in findit.py?
> > 
> > Because it's as-yet too short to bother breaking out into its own file? I
> could
> > break it out if you like, I just figured you'd do that as part of the CL
> > implementing it.
> 
> Can you break it out to findit_for_clustefuzz.py? since you already wrote the
> skeleton code. Thanks :) 
> 
> No findit_for_clustefuzz_test.py is needed, so the test coverage won't skan
this
> file.

Done.

https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/crash/...
File appengine/findit/crash/findit_for_chromecrash.py (right):

https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/findit_for_chromecrash.py:39: self.azalea = Azalea(
On 2016/10/12 22:35:03, sharu jiang wrote:
> If we lift the FulCulprit to findit class, the azalea can be lifted too.

We can't move this creation of Azalea to the base Findit class, since it depends
on client-specific details for constructing the different classifier objects. I
could break the creation of the ComponentClassifier out into a private method,
since that part seems fairly client agnostic. Dunno if it's worth it though.

https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/findit_for_chromecrash.py:96: historical_metadata =
kwargs.get('historical_metadata', None)
On 2016/10/12 22:35:03, sharu jiang wrote:
> On 2016/10/12 21:14:51, wrengr wrote:
> > On 2016/10/12 17:46:29, sharu jiang wrote:
> > > We already got regression range before running analysis while determining
> > > whether to rerun or not.
> > 
> > Is the regression_range stored in the CrashAnalysis model? Or should it be
> > passed in as a separate argument?
> 
> It should be stored in CrashAnalysis model before running analysis.

Done.

https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/crash/...
File appengine/findit/crash/test/crash_pipeline_test.py (right):

https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/test/crash_pipeline_test.py:169: def
testRunningAnalysis(self):
On 2016/10/13 06:03:17, stgao wrote:
> On 2016/10/12 21:14:51, wrengr wrote:
> > On 2016/10/12 15:46:58, stgao wrote:
> > > Hopefully https://codereview.chromium.org/2414523002/ could help on
testing
> > the
> > > pipeline logic here.
> > 
> > Um, that's this CL. Did you have another one in mind?
> 
> 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.

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?

https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/handle...
File appengine/findit/handlers/crash/crash_handler.py (right):

https://codereview.chromium.org/2414523002/diff/40001/appengine/findit/handle...
appengine/findit/handlers/crash/crash_handler.py:112:
crash_data['customized_data']['channel'],
On 2016/10/12 22:35:03, sharu jiang wrote:
> On 2016/10/12 21:14:51, wrengr wrote:
> > On 2016/10/12 17:46:29, sharu jiang wrote:
> > > ? This is not correct, the 'historical_metadata' got discarded.
> > 
> > ScheduleNewAnalysisForCrash (the version in this CL, at least) doesn't use
the
> > historical_metadata, it only uses the channel; hence passing it directly
> rather
> > than bundling it up in a dict.
> 
> The function is to schedule analysis for any client, however the 'channel' is
> fracas/cracas-specific. We should pass in the 'customized_data' and handle it
> accordingly.

As mentioned elsewhere, I mislike having this 'customized_data' thing, because
it's very underspecified what exactly it's supposed to be. Since we presently
don't support Clusterfuzz, passing a ChromeCrash-specific thing isn't a problem
yet. I agree things should be cleaned up in the future, but that's outside of
the scope of the current CL.

wrengr

I've included some stack traces for unittests which are still failing. I need help debugging ...

4 years, 2 months ago (2016-10-18 23:28:10 UTC) #12

I've included some stack traces for unittests which are still failing. I need
help debugging these.

https://codereview.chromium.org/2414523002/diff/200001/appengine/findit/crash...
File appengine/findit/crash/test/crash_pipeline_test.py (right):

https://codereview.chromium.org/2414523002/diff/200001/appengine/findit/crash...
appengine/findit/crash/test/crash_pipeline_test.py:154:
self.assertTrue(_MockFinditForFracas().ScheduleNewAnalysis(crash_data))
This line ultimately causes a bunch of tests to fail. An example stacktrace:

  File
"/usr/local/google/home/wrengr/chromium-srcs/infra/appengine/findit/crash/test/crash_pipeline_test.py",
line 206, in testRunningAnalysis
    analysis_result, analysis_tags)
  File
"/usr/local/google/home/wrengr/chromium-srcs/infra/appengine/findit/crash/test/crash_pipeline_test.py",
line 154, in _TestRunningAnalysisForResult
    self.assertTrue(_MockFinditForFracas().ScheduleNewAnalysis(crash_data))
  File
"/usr/local/google/home/wrengr/chromium-srcs/infra/appengine/findit/crash/findit.py",
line 149, in ScheduleNewAnalysis
    analysis_pipeline.start(queue_name=queue_name)
  File
"/usr/local/google/home/wrengr/chromium-srcs/infra/appengine/findit/third_party/pipeline_utils/appengine_third_party_pipeline_python_src_pipeline/pipeline.py",
line 673, in start
    self, idempotence_key, str(e)))
PipelineSetupError: Error starting
crash.crash_pipeline.CrashWrapperPipeline(*(<crash.test.crash_pipeline_test._MockFinditForFracas
object at 0x7f193ad56390>, {'channel': 'canary', 'chrome_version':
'50.2500.0.1', 'platform': 'win', 'process_type': 'browser', 'signature':
'sign... (208 bytes), **{})#695f7662ebca40688d7598e104d784ff:
<crash.test.crash_pipeline_test._MockFinditForFracas object at 0x7f193ad56390>
is not JSON serializable

==== captured logging output ====
LOG:
INFO: Config
CrashConfig<component_classifier=JsonProperty('component_classifier',
default={}, compressed=True), cracas=JsonProperty('cracas', default={}),
fracas=JsonProperty('fracas', default={}),
project_classifier=JsonProperty('project_classifier', default={}),
updated_by=StringProperty('updated_by', indexed=False),
updated_ts=DateTimeProperty('updated_ts', indexed=False, auto_now=True)> was
updated by admin

STDOUT:
(empty)

STDERR:
(empty)

https://codereview.chromium.org/2414523002/diff/200001/appengine/findit/crash...
File appengine/findit/crash/test/findit_for_chromecrash_test.py (right):

https://codereview.chromium.org/2414523002/diff/200001/appengine/findit/crash...
appengine/findit/crash/test/findit_for_chromecrash_test.py:98:
self.assertDictEqual(expected_results, results)
The mocking of DetectRegressionRange doesn't work for some reason. An example
stacktrace:

  File
"/usr/local/google/home/wrengr/chromium-srcs/infra/appengine/findit/crash/test/findit_for_chromecrash_test.py",
line 98, in testFindCulpritForChromeCrash
    self.assertDictEqual(expected_results, results)
  File "/usr/lib/python2.7/unittest/case.py", line 837, in assertDictEqual
    self.fail(self._formatMessage(msg, standardMsg))
  File "/usr/lib/python2.7/unittest/case.py", line 412, in fail
    raise self.failureException(msg)
AssertionError: {'found': True, 'suspected_project': '', 'suspected_cls':
[{'reasons': None, 'co [truncated]... != {'suspected_components': [], 'found':
True, 'suspected_project': '', 'suspected_ [truncated]...
  {'found': True,
-  'regression_range': ['50.0.1233.0', '50.0.1234.0'],
+  'regression_range': None,
   'suspected_cls': [{'author': 'r@chromium.org',
                      'changed_files': None,
                      'confidence': None,
                      'project_path': 'src/',
                      'reasons': None,
                      'review_url': 'https://codereview.chromium.org/3281',
                      'revision': '1',
                      'time': 'Thu Mar 31 21:24:43 2016',
                      'url': 'https://repo.test/+/1'}],
   'suspected_components': [],
   'suspected_project': ''}

==== captured logging output ====
LOG:
INFO: Config
CrashConfig<component_classifier=JsonProperty('component_classifier',
default={}, compressed=True), cracas=JsonProperty('cracas', default={}),
fracas=JsonProperty('fracas', default={}),
project_classifier=JsonProperty('project_classifier', default={}),
updated_by=StringProperty('updated_by', indexed=False),
updated_ts=DateTimeProperty('updated_ts', indexed=False, auto_now=True)> was
updated by admin

STDOUT:
(empty)

STDERR:
(empty)

https://codereview.chromium.org/2414523002/diff/200001/appengine/findit/handl...
File appengine/findit/handlers/crash/test/crash_handler_test.py (right):

https://codereview.chromium.org/2414523002/diff/200001/appengine/findit/handl...
appengine/findit/handlers/crash/test/crash_handler_test.py:80:
self.assertEqual(1, len(requested_crashes))
The mocking of ScheduleNewAnalysis isn't working here. An example stacktrace:

  File
"/usr/local/google/home/wrengr/chromium-srcs/infra/appengine/findit/handlers/crash/test/crash_handler_test.py",
line 80, in testAnalysisScheduled
    self.assertEqual(1, len(requested_crashes))
  File "/usr/lib/python2.7/unittest/case.py", line 515, in assertEqual
    assertion_func(first, second, msg=msg)
  File "/usr/lib/python2.7/unittest/case.py", line 508, in _baseAssertEqual
    raise self.failureException(msg)
AssertionError: 1 != 0

==== captured logging output ====
LOG:
INFO: Config
CrashConfig<component_classifier=JsonProperty('component_classifier',
default={}, compressed=True), cracas=JsonProperty('cracas', default={}),
fracas=JsonProperty('fracas', default={}),
project_classifier=JsonProperty('project_classifier', default={}),
updated_by=StringProperty('updated_by', indexed=False),
updated_ts=DateTimeProperty('updated_ts', indexed=False, auto_now=True)> was
updated by admin
INFO: Processing message id from subscription subscription.
INFO: Crash data is {"stack_trace": "frame1\nframe2\nframe3",
"crash_identifiers": {"platform": "supported_platform", "signature":
"signature/here", "chrome_version": "50.2500.0.0", "process_type": "renderer",
"channel": "supported_channel"}, "customized_data": {"channel":
"supported_channel", "historical_metadata": [{"chrome_version": "50.2500.0.0",
"cpm": 0.6}]}, "platform": "supported_platform", "client_id": "fracas",
"signature": "signature/here", "chrome_version": "50.2500.0.0"}
WARNING: Failed to find spikes in history data [{u'chrome_version':
u'50.2500.0.0', u'cpm': 0.6}]
WARNING: suspended generator transaction(context.py:1018) raised
KeyError('crashed_version')
ERROR: Failed to process crash message
Traceback (most recent call last):
  File
"/usr/local/google/home/wrengr/chromium-srcs/infra/appengine/findit/handlers/crash/crash_handler.py",
line 126, in HandlePost
    queue_name=constants.CRASH_ANALYSIS_QUEUE[client_id])
  File
"/usr/local/google/home/wrengr/chromium-srcs/infra/appengine/findit/crash/findit.py",
line 141, in ScheduleNewAnalysis
    if not self._NeedsNewAnalysis(crash_data):
  File
"/usr/local/google/home/wrengr/chromium-srcs/google_appengine/google/appengine/ext/ndb/utils.py",
line 197, in inner_wrapper
    return wrapped_decorator(func, args, kwds, **options)
  File
"/usr/local/google/home/wrengr/chromium-srcs/google_appengine/google/appengine/ext/ndb/model.py",
line 3834, in transactional
    func, args, kwds, **options).get_result()
  File
"/usr/local/google/home/wrengr/chromium-srcs/google_appengine/google/appengine/ext/ndb/tasklets.py",
line 383, in get_result
    self.check_success()
  File
"/usr/local/google/home/wrengr/chromium-srcs/google_appengine/google/appengine/ext/ndb/tasklets.py",
line 430, in _help_tasklet_along
    value = gen.send(val)
  File
"/usr/local/google/home/wrengr/chromium-srcs/google_appengine/google/appengine/ext/ndb/context.py",
line 1014, in transaction
    result = callback()
  File
"/usr/local/google/home/wrengr/chromium-srcs/google_appengine/google/appengine/ext/ndb/model.py",
line 3842, in <lambda>
    return transaction_async(lambda: func(*args, **kwds), **options)
  File
"/usr/local/google/home/wrengr/chromium-srcs/infra/appengine/findit/crash/findit_for_chromecrash.py",
line 89, in _NeedsNewAnalysis
    self._InitializeAnalysis(model, crash_data)
  File
"/usr/local/google/home/wrengr/chromium-srcs/infra/appengine/findit/crash/findit_for_chromecrash.py",
line 59, in _InitializeAnalysis
    super(FinditForChromeCrash, self)._InitializeAnalysis(model, crash_data)
  File
"/usr/local/google/home/wrengr/chromium-srcs/infra/appengine/findit/crash/findit.py",
line 117, in _InitializeAnalysis
    model.crashed_version = crash_data['crashed_version']
KeyError: 'crashed_version'
INFO: {"message": {"data":
"eyJzdGFja190cmFjZSI6ICJmcmFtZTFcbmZyYW1lMlxuZnJhbWUzIiwgImNyYXNoX2lkZW50aWZpZXJzIjogeyJwbGF0Zm9ybSI6ICJzdXBwb3J0ZWRfcGxhdGZvcm0iLCAicHJvY2Vzc190eXBlIjogInJlbmRlcmVyIiwgImNocm9tZV92ZXJzaW9uIjogIjUwLjI1MDAuMC4wIiwgImNoYW5uZWwiOiAic3VwcG9ydGVkX2NoYW5uZWwiLCAic2lnbmF0dXJlIjogInNpZ25hdHVyZS9oZXJlIn0sICJjaHJvbWVfdmVyc2lvbiI6ICI1MC4yNTAwLjAuMCIsICJwbGF0Zm9ybSI6ICJzdXBwb3J0ZWRfcGxhdGZvcm0iLCAiY2xpZW50X2lkIjogImZyYWNhcyIsICJzaWduYXR1cmUiOiAic2lnbmF0dXJlL2hlcmUiLCAiY3VzdG9taXplZF9kYXRhIjogeyJjaGFubmVsIjogInN1cHBvcnRlZF9jaGFubmVsIiwgImhpc3RvcmljYWxfbWV0YWRhdGEiOiBbeyJjaHJvbWVfdmVyc2lvbiI6ICI1MC4yNTAwLjAuMCIsICJjcG0iOiAwLjZ9XX19",
"message_id": "id"}, "subscription": "subscription"}

STDOUT:
(empty)

STDERR:
(empty)

Sharu Jiang

https://codereview.chromium.org/2414523002/diff/200001/appengine/findit/crash/findit_for_chromecrash.py File appengine/findit/crash/findit_for_chromecrash.py (right): https://codereview.chromium.org/2414523002/diff/200001/appengine/findit/crash/findit_for_chromecrash.py#newcode64 appengine/findit/crash/findit_for_chromecrash.py:64: crash_data.get('customized_data', {}).get('historical_metadata', [])) Should also set model.regression_range here or ...

4 years, 2 months ago (2016-10-19 00:20:06 UTC) #13

Sharu Jiang

https://codereview.chromium.org/2414523002/diff/200001/appengine/findit/handlers/crash/test/crash_handler_test.py File appengine/findit/handlers/crash/test/crash_handler_test.py (right): https://codereview.chromium.org/2414523002/diff/200001/appengine/findit/handlers/crash/test/crash_handler_test.py#newcode80 appengine/findit/handlers/crash/test/crash_handler_test.py:80: self.assertEqual(1, len(requested_crashes)) On 2016/10/18 23:28:10, wrengr wrote: > The ...

4 years, 2 months ago (2016-10-19 00:53:07 UTC) #14

https://codereview.chromium.org/2414523002/diff/200001/appengine/findit/handl...
File appengine/findit/handlers/crash/test/crash_handler_test.py (right):

https://codereview.chromium.org/2414523002/diff/200001/appengine/findit/handl...
appengine/findit/handlers/crash/test/crash_handler_test.py:80:
self.assertEqual(1, len(requested_crashes))
On 2016/10/18 23:28:10, wrengr wrote:
> The mocking of ScheduleNewAnalysis isn't working here. An example stacktrace:
> 
>   File
>
"/usr/local/google/home/wrengr/chromium-srcs/infra/appengine/findit/handlers/crash/test/crash_handler_test.py",
> line 80, in testAnalysisScheduled
>     self.assertEqual(1, len(requested_crashes))
>   File "/usr/lib/python2.7/unittest/case.py", line 515, in assertEqual
>     assertion_func(first, second, msg=msg)
>   File "/usr/lib/python2.7/unittest/case.py", line 508, in _baseAssertEqual
>     raise self.failureException(msg)
> AssertionError: 1 != 0
> 
> ==== captured logging output ====
> LOG:
> INFO: Config
> CrashConfig<component_classifier=JsonProperty('component_classifier',
> default={}, compressed=True), cracas=JsonProperty('cracas', default={}),
> fracas=JsonProperty('fracas', default={}),
> project_classifier=JsonProperty('project_classifier', default={}),
> updated_by=StringProperty('updated_by', indexed=False),
> updated_ts=DateTimeProperty('updated_ts', indexed=False, auto_now=True)> was
> updated by admin
> INFO: Processing message id from subscription subscription.
> INFO: Crash data is {"stack_trace": "frame1\nframe2\nframe3",
> "crash_identifiers": {"platform": "supported_platform", "signature":
> "signature/here", "chrome_version": "50.2500.0.0", "process_type": "renderer",
> "channel": "supported_channel"}, "customized_data": {"channel":
> "supported_channel", "historical_metadata": [{"chrome_version": "50.2500.0.0",
> "cpm": 0.6}]}, "platform": "supported_platform", "client_id": "fracas",
> "signature": "signature/here", "chrome_version": "50.2500.0.0"}
> WARNING: Failed to find spikes in history data [{u'chrome_version':
> u'50.2500.0.0', u'cpm': 0.6}]
> WARNING: suspended generator transaction(context.py:1018) raised
> KeyError('crashed_version')
> ERROR: Failed to process crash message
> Traceback (most recent call last):
>   File
>
"/usr/local/google/home/wrengr/chromium-srcs/infra/appengine/findit/handlers/crash/crash_handler.py",
> line 126, in HandlePost
>     queue_name=constants.CRASH_ANALYSIS_QUEUE[client_id])
>   File
>
"/usr/local/google/home/wrengr/chromium-srcs/infra/appengine/findit/crash/findit.py",
> line 141, in ScheduleNewAnalysis
>     if not self._NeedsNewAnalysis(crash_data):
>   File
>
"/usr/local/google/home/wrengr/chromium-srcs/google_appengine/google/appengine/ext/ndb/utils.py",
> line 197, in inner_wrapper
>     return wrapped_decorator(func, args, kwds, **options)
>   File
>
"/usr/local/google/home/wrengr/chromium-srcs/google_appengine/google/appengine/ext/ndb/model.py",
> line 3834, in transactional
>     func, args, kwds, **options).get_result()
>   File
>
"/usr/local/google/home/wrengr/chromium-srcs/google_appengine/google/appengine/ext/ndb/tasklets.py",
> line 383, in get_result
>     self.check_success()
>   File
>
"/usr/local/google/home/wrengr/chromium-srcs/google_appengine/google/appengine/ext/ndb/tasklets.py",
> line 430, in _help_tasklet_along
>     value = gen.send(val)
>   File
>
"/usr/local/google/home/wrengr/chromium-srcs/google_appengine/google/appengine/ext/ndb/context.py",
> line 1014, in transaction
>     result = callback()
>   File
>
"/usr/local/google/home/wrengr/chromium-srcs/google_appengine/google/appengine/ext/ndb/model.py",
> line 3842, in <lambda>
>     return transaction_async(lambda: func(*args, **kwds), **options)
>   File
>
"/usr/local/google/home/wrengr/chromium-srcs/infra/appengine/findit/crash/findit_for_chromecrash.py",
> line 89, in _NeedsNewAnalysis
>     self._InitializeAnalysis(model, crash_data)
>   File
>
"/usr/local/google/home/wrengr/chromium-srcs/infra/appengine/findit/crash/findit_for_chromecrash.py",
> line 59, in _InitializeAnalysis
>     super(FinditForChromeCrash, self)._InitializeAnalysis(model, crash_data)
>   File
>
"/usr/local/google/home/wrengr/chromium-srcs/infra/appengine/findit/crash/findit.py",
> line 117, in _InitializeAnalysis
>     model.crashed_version = crash_data['crashed_version']
> KeyError: 'crashed_version'
> INFO: {"message": {"data":
>
"eyJzdGFja190cmFjZSI6ICJmcmFtZTFcbmZyYW1lMlxuZnJhbWUzIiwgImNyYXNoX2lkZW50aWZpZXJzIjogeyJwbGF0Zm9ybSI6ICJzdXBwb3J0ZWRfcGxhdGZvcm0iLCAicHJvY2Vzc190eXBlIjogInJlbmRlcmVyIiwgImNocm9tZV92ZXJzaW9uIjogIjUwLjI1MDAuMC4wIiwgImNoYW5uZWwiOiAic3VwcG9ydGVkX2NoYW5uZWwiLCAic2lnbmF0dXJlIjogInNpZ25hdHVyZS9oZXJlIn0sICJjaHJvbWVfdmVyc2lvbiI6ICI1MC4yNTAwLjAuMCIsICJwbGF0Zm9ybSI6ICJzdXBwb3J0ZWRfcGxhdGZvcm0iLCAiY2xpZW50X2lkIjogImZyYWNhcyIsICJzaWduYXR1cmUiOiAic2lnbmF0dXJlL2hlcmUiLCAiY3VzdG9taXplZF9kYXRhIjogeyJjaGFubmVsIjogInN1cHBvcnRlZF9jaGFubmVsIiwgImhpc3RvcmljYWxfbWV0YWRhdGEiOiBbeyJjaHJvbWVfdmVyc2lvbiI6ICI1MC4yNTAwLjAuMCIsICJjcG0iOiAwLjZ9XX19",
> "message_id": "id"}, "subscription": "subscription"}
> 
> STDOUT:
> (empty)
> 
> STDERR:
> (empty)

"/usr/local/google/home/wrengr/chromium-srcs/infra/appengine/findit/crash/findit.py",
line 117, in _InitializeAnalysis
    model.crashed_version = crash_data['crashed_version']

It's 'chrome_version' in crash_data

wrengr

Any idea about where the "FinditForWhatever is not JSON serializable" errors are coming from? I ...

4 years, 2 months ago (2016-10-19 18:05:16 UTC) #15

Any idea about where the "FinditForWhatever is not JSON serializable" errors are
coming from? I don't even see who's trying to serialize these objects (which
they shouldn't be).

https://codereview.chromium.org/2414523002/diff/200001/appengine/findit/crash...
File appengine/findit/crash/findit_for_chromecrash.py (right):

https://codereview.chromium.org/2414523002/diff/200001/appengine/findit/crash...
appengine/findit/crash/findit_for_chromecrash.py:64:
crash_data.get('customized_data', {}).get('historical_metadata', []))
On 2016/10/19 00:20:06, sharu jiang wrote:
> Should also set model.regression_range here or in
> superclass._InitializeAnalysis.

IIRC, the regression range isn't available from the crash_data for
fracas/cracas. Do you want it computed here if it's not provided (e.g., via
_NedsNewAnalysis)?

Also, is regression_range considered part of customized_data, or is it in the
top-level of crash_data?

https://codereview.chromium.org/2414523002/diff/200001/appengine/findit/crash...
File appengine/findit/crash/test/findit_for_chromecrash_test.py (right):

https://codereview.chromium.org/2414523002/diff/200001/appengine/findit/crash...
appengine/findit/crash/test/findit_for_chromecrash_test.py:76:
analysis.historical_metadata = ['50.0.1233.0', '50.0.1234.0']
On 2016/10/19 00:20:06, sharu jiang wrote:
> This should be analysis.regression_range = ['50.0.1233.0', '50.0.1234.0']
(this
> argument is regression_range in the old FindCulprit.), then the test would be
> fixed.

Done. Thanks

https://codereview.chromium.org/2414523002/diff/200001/appengine/findit/handl...
File appengine/findit/handlers/crash/test/crash_handler_test.py (right):

https://codereview.chromium.org/2414523002/diff/200001/appengine/findit/handl...
appengine/findit/handlers/crash/test/crash_handler_test.py:80:
self.assertEqual(1, len(requested_crashes))
On 2016/10/19 00:53:07, sharu jiang wrote:
> On 2016/10/18 23:28:10, wrengr wrote:
> > The mocking of ScheduleNewAnalysis isn't working here. An example
stacktrace:
> > 
> >   File
> >
>
"/usr/local/google/home/wrengr/chromium-srcs/infra/appengine/findit/handlers/crash/test/crash_handler_test.py",
> > line 80, in testAnalysisScheduled
> >     self.assertEqual(1, len(requested_crashes))
> >   File "/usr/lib/python2.7/unittest/case.py", line 515, in assertEqual
> >     assertion_func(first, second, msg=msg)
> >   File "/usr/lib/python2.7/unittest/case.py", line 508, in _baseAssertEqual
> >     raise self.failureException(msg)
> > AssertionError: 1 != 0
> > 
> > ==== captured logging output ====
> > LOG:
> > INFO: Config
> > CrashConfig<component_classifier=JsonProperty('component_classifier',
> > default={}, compressed=True), cracas=JsonProperty('cracas', default={}),
> > fracas=JsonProperty('fracas', default={}),
> > project_classifier=JsonProperty('project_classifier', default={}),
> > updated_by=StringProperty('updated_by', indexed=False),
> > updated_ts=DateTimeProperty('updated_ts', indexed=False, auto_now=True)> was
> > updated by admin
> > INFO: Processing message id from subscription subscription.
> > INFO: Crash data is {"stack_trace": "frame1\nframe2\nframe3",
> > "crash_identifiers": {"platform": "supported_platform", "signature":
> > "signature/here", "chrome_version": "50.2500.0.0", "process_type":
"renderer",
> > "channel": "supported_channel"}, "customized_data": {"channel":
> > "supported_channel", "historical_metadata": [{"chrome_version":
"50.2500.0.0",
> > "cpm": 0.6}]}, "platform": "supported_platform", "client_id": "fracas",
> > "signature": "signature/here", "chrome_version": "50.2500.0.0"}
> > WARNING: Failed to find spikes in history data [{u'chrome_version':
> > u'50.2500.0.0', u'cpm': 0.6}]
> > WARNING: suspended generator transaction(context.py:1018) raised
> > KeyError('crashed_version')
> > ERROR: Failed to process crash message
> > Traceback (most recent call last):
> >   File
> >
>
"/usr/local/google/home/wrengr/chromium-srcs/infra/appengine/findit/handlers/crash/crash_handler.py",
> > line 126, in HandlePost
> >     queue_name=constants.CRASH_ANALYSIS_QUEUE[client_id])
> >   File
> >
>
"/usr/local/google/home/wrengr/chromium-srcs/infra/appengine/findit/crash/findit.py",
> > line 141, in ScheduleNewAnalysis
> >     if not self._NeedsNewAnalysis(crash_data):
> >   File
> >
>
"/usr/local/google/home/wrengr/chromium-srcs/google_appengine/google/appengine/ext/ndb/utils.py",
> > line 197, in inner_wrapper
> >     return wrapped_decorator(func, args, kwds, **options)
> >   File
> >
>
"/usr/local/google/home/wrengr/chromium-srcs/google_appengine/google/appengine/ext/ndb/model.py",
> > line 3834, in transactional
> >     func, args, kwds, **options).get_result()
> >   File
> >
>
"/usr/local/google/home/wrengr/chromium-srcs/google_appengine/google/appengine/ext/ndb/tasklets.py",
> > line 383, in get_result
> >     self.check_success()
> >   File
> >
>
"/usr/local/google/home/wrengr/chromium-srcs/google_appengine/google/appengine/ext/ndb/tasklets.py",
> > line 430, in _help_tasklet_along
> >     value = gen.send(val)
> >   File
> >
>
"/usr/local/google/home/wrengr/chromium-srcs/google_appengine/google/appengine/ext/ndb/context.py",
> > line 1014, in transaction
> >     result = callback()
> >   File
> >
>
"/usr/local/google/home/wrengr/chromium-srcs/google_appengine/google/appengine/ext/ndb/model.py",
> > line 3842, in <lambda>
> >     return transaction_async(lambda: func(*args, **kwds), **options)
> >   File
> >
>
"/usr/local/google/home/wrengr/chromium-srcs/infra/appengine/findit/crash/findit_for_chromecrash.py",
> > line 89, in _NeedsNewAnalysis
> >     self._InitializeAnalysis(model, crash_data)
> >   File
> >
>
"/usr/local/google/home/wrengr/chromium-srcs/infra/appengine/findit/crash/findit_for_chromecrash.py",
> > line 59, in _InitializeAnalysis
> >     super(FinditForChromeCrash, self)._InitializeAnalysis(model, crash_data)
> >   File
> >
>
"/usr/local/google/home/wrengr/chromium-srcs/infra/appengine/findit/crash/findit.py",
> > line 117, in _InitializeAnalysis
> >     model.crashed_version = crash_data['crashed_version']
> > KeyError: 'crashed_version'
> > INFO: {"message": {"data":
> >
>
"eyJzdGFja190cmFjZSI6ICJmcmFtZTFcbmZyYW1lMlxuZnJhbWUzIiwgImNyYXNoX2lkZW50aWZpZXJzIjogeyJwbGF0Zm9ybSI6ICJzdXBwb3J0ZWRfcGxhdGZvcm0iLCAicHJvY2Vzc190eXBlIjogInJlbmRlcmVyIiwgImNocm9tZV92ZXJzaW9uIjogIjUwLjI1MDAuMC4wIiwgImNoYW5uZWwiOiAic3VwcG9ydGVkX2NoYW5uZWwiLCAic2lnbmF0dXJlIjogInNpZ25hdHVyZS9oZXJlIn0sICJjaHJvbWVfdmVyc2lvbiI6ICI1MC4yNTAwLjAuMCIsICJwbGF0Zm9ybSI6ICJzdXBwb3J0ZWRfcGxhdGZvcm0iLCAiY2xpZW50X2lkIjogImZyYWNhcyIsICJzaWduYXR1cmUiOiAic2lnbmF0dXJlL2hlcmUiLCAiY3VzdG9taXplZF9kYXRhIjogeyJjaGFubmVsIjogInN1cHBvcnRlZF9jaGFubmVsIiwgImhpc3RvcmljYWxfbWV0YWRhdGEiOiBbeyJjaHJvbWVfdmVyc2lvbiI6ICI1MC4yNTAwLjAuMCIsICJjcG0iOiAwLjZ9XX19",
> > "message_id": "id"}, "subscription": "subscription"}
> > 
> > STDOUT:
> > (empty)
> > 
> > STDERR:
> > (empty)
> 
>
"/usr/local/google/home/wrengr/chromium-srcs/infra/appengine/findit/crash/findit.py",
> line 117, in _InitializeAnalysis
>     model.crashed_version = crash_data['crashed_version']
> 
> It's 'chrome_version' in crash_data

Except that it's 'crashed_version' for everyone else who calls
ScheduleNewAnalysis (e.g., in crash_pipeline_test.py)

stgao

https://codereview.chromium.org/2414523002/diff/240001/appengine/findit/crash/crash_pipeline.py File appengine/findit/crash/crash_pipeline.py (right): https://codereview.chromium.org/2414523002/diff/240001/appengine/findit/crash/crash_pipeline.py#newcode120 appengine/findit/crash/crash_pipeline.py:120: run_analysis = yield CrashAnalysisPipeline(findit_client, crash_identifiers) And same here for ...

4 years, 2 months ago (2016-10-19 18:20:53 UTC) #16

Sharu Jiang

https://codereview.chromium.org/2414523002/diff/200001/appengine/findit/crash/findit_for_chromecrash.py File appengine/findit/crash/findit_for_chromecrash.py (right): https://codereview.chromium.org/2414523002/diff/200001/appengine/findit/crash/findit_for_chromecrash.py#newcode64 appengine/findit/crash/findit_for_chromecrash.py:64: crash_data.get('customized_data', {}).get('historical_metadata', [])) On 2016/10/19 18:05:16, wrengr wrote: > ...

4 years, 2 months ago (2016-10-19 21:03:10 UTC) #17

https://codereview.chromium.org/2414523002/diff/200001/appengine/findit/crash...
File appengine/findit/crash/findit_for_chromecrash.py (right):

https://codereview.chromium.org/2414523002/diff/200001/appengine/findit/crash...
appengine/findit/crash/findit_for_chromecrash.py:64:
crash_data.get('customized_data', {}).get('historical_metadata', []))
On 2016/10/19 18:05:16, wrengr wrote:
> On 2016/10/19 00:20:06, sharu jiang wrote:
> > Should also set model.regression_range here or in
> > superclass._InitializeAnalysis.
> 
> IIRC, the regression range isn't available from the crash_data for
> fracas/cracas. Do you want it computed here if it's not provided (e.g., via
> _NedsNewAnalysis)?
> 
> Also, is regression_range considered part of customized_data, or is it in the
> top-level of crash_data?

Conceptually, it would be better to let it be top-level of crash_data, since the
'customized_data' should be raw client-specific data provided by clients and
shouldn't contain computed(un-customized) data.

You already computed regression_range and insert it in
crash_data['customized_data'] in line#88, the only thing left is to set the
value of analysis here using that data.

https://codereview.chromium.org/2414523002/diff/200001/appengine/findit/handl...
File appengine/findit/handlers/crash/test/crash_handler_test.py (right):

https://codereview.chromium.org/2414523002/diff/200001/appengine/findit/handl...
appengine/findit/handlers/crash/test/crash_handler_test.py:80:
self.assertEqual(1, len(requested_crashes))
On 2016/10/19 18:05:16, wrengr wrote:
> On 2016/10/19 00:53:07, sharu jiang wrote:
> > On 2016/10/18 23:28:10, wrengr wrote:
> > > The mocking of ScheduleNewAnalysis isn't working here. An example
> stacktrace:
> > > 
> > >   File
> > >
> >
>
"/usr/local/google/home/wrengr/chromium-srcs/infra/appengine/findit/handlers/crash/test/crash_handler_test.py",
> > > line 80, in testAnalysisScheduled
> > >     self.assertEqual(1, len(requested_crashes))
> > >   File "/usr/lib/python2.7/unittest/case.py", line 515, in assertEqual
> > >     assertion_func(first, second, msg=msg)
> > >   File "/usr/lib/python2.7/unittest/case.py", line 508, in
_baseAssertEqual
> > >     raise self.failureException(msg)
> > > AssertionError: 1 != 0
> > > 
> > > ==== captured logging output ====
> > > LOG:
> > > INFO: Config
> > > CrashConfig<component_classifier=JsonProperty('component_classifier',
> > > default={}, compressed=True), cracas=JsonProperty('cracas', default={}),
> > > fracas=JsonProperty('fracas', default={}),
> > > project_classifier=JsonProperty('project_classifier', default={}),
> > > updated_by=StringProperty('updated_by', indexed=False),
> > > updated_ts=DateTimeProperty('updated_ts', indexed=False, auto_now=True)>
was
> > > updated by admin
> > > INFO: Processing message id from subscription subscription.
> > > INFO: Crash data is {"stack_trace": "frame1\nframe2\nframe3",
> > > "crash_identifiers": {"platform": "supported_platform", "signature":
> > > "signature/here", "chrome_version": "50.2500.0.0", "process_type":
> "renderer",
> > > "channel": "supported_channel"}, "customized_data": {"channel":
> > > "supported_channel", "historical_metadata": [{"chrome_version":
> "50.2500.0.0",
> > > "cpm": 0.6}]}, "platform": "supported_platform", "client_id": "fracas",
> > > "signature": "signature/here", "chrome_version": "50.2500.0.0"}
> > > WARNING: Failed to find spikes in history data [{u'chrome_version':
> > > u'50.2500.0.0', u'cpm': 0.6}]
> > > WARNING: suspended generator transaction(context.py:1018) raised
> > > KeyError('crashed_version')
> > > ERROR: Failed to process crash message
> > > Traceback (most recent call last):
> > >   File
> > >
> >
>
"/usr/local/google/home/wrengr/chromium-srcs/infra/appengine/findit/handlers/crash/crash_handler.py",
> > > line 126, in HandlePost
> > >     queue_name=constants.CRASH_ANALYSIS_QUEUE[client_id])
> > >   File
> > >
> >
>
"/usr/local/google/home/wrengr/chromium-srcs/infra/appengine/findit/crash/findit.py",
> > > line 141, in ScheduleNewAnalysis
> > >     if not self._NeedsNewAnalysis(crash_data):
> > >   File
> > >
> >
>
"/usr/local/google/home/wrengr/chromium-srcs/google_appengine/google/appengine/ext/ndb/utils.py",
> > > line 197, in inner_wrapper
> > >     return wrapped_decorator(func, args, kwds, **options)
> > >   File
> > >
> >
>
"/usr/local/google/home/wrengr/chromium-srcs/google_appengine/google/appengine/ext/ndb/model.py",
> > > line 3834, in transactional
> > >     func, args, kwds, **options).get_result()
> > >   File
> > >
> >
>
"/usr/local/google/home/wrengr/chromium-srcs/google_appengine/google/appengine/ext/ndb/tasklets.py",
> > > line 383, in get_result
> > >     self.check_success()
> > >   File
> > >
> >
>
"/usr/local/google/home/wrengr/chromium-srcs/google_appengine/google/appengine/ext/ndb/tasklets.py",
> > > line 430, in _help_tasklet_along
> > >     value = gen.send(val)
> > >   File
> > >
> >
>
"/usr/local/google/home/wrengr/chromium-srcs/google_appengine/google/appengine/ext/ndb/context.py",
> > > line 1014, in transaction
> > >     result = callback()
> > >   File
> > >
> >
>
"/usr/local/google/home/wrengr/chromium-srcs/google_appengine/google/appengine/ext/ndb/model.py",
> > > line 3842, in <lambda>
> > >     return transaction_async(lambda: func(*args, **kwds), **options)
> > >   File
> > >
> >
>
"/usr/local/google/home/wrengr/chromium-srcs/infra/appengine/findit/crash/findit_for_chromecrash.py",
> > > line 89, in _NeedsNewAnalysis
> > >     self._InitializeAnalysis(model, crash_data)
> > >   File
> > >
> >
>
"/usr/local/google/home/wrengr/chromium-srcs/infra/appengine/findit/crash/findit_for_chromecrash.py",
> > > line 59, in _InitializeAnalysis
> > >     super(FinditForChromeCrash, self)._InitializeAnalysis(model,
crash_data)
> > >   File
> > >
> >
>
"/usr/local/google/home/wrengr/chromium-srcs/infra/appengine/findit/crash/findit.py",
> > > line 117, in _InitializeAnalysis
> > >     model.crashed_version = crash_data['crashed_version']
> > > KeyError: 'crashed_version'
> > > INFO: {"message": {"data":
> > >
> >
>
"eyJzdGFja190cmFjZSI6ICJmcmFtZTFcbmZyYW1lMlxuZnJhbWUzIiwgImNyYXNoX2lkZW50aWZpZXJzIjogeyJwbGF0Zm9ybSI6ICJzdXBwb3J0ZWRfcGxhdGZvcm0iLCAicHJvY2Vzc190eXBlIjogInJlbmRlcmVyIiwgImNocm9tZV92ZXJzaW9uIjogIjUwLjI1MDAuMC4wIiwgImNoYW5uZWwiOiAic3VwcG9ydGVkX2NoYW5uZWwiLCAic2lnbmF0dXJlIjogInNpZ25hdHVyZS9oZXJlIn0sICJjaHJvbWVfdmVyc2lvbiI6ICI1MC4yNTAwLjAuMCIsICJwbGF0Zm9ybSI6ICJzdXBwb3J0ZWRfcGxhdGZvcm0iLCAiY2xpZW50X2lkIjogImZyYWNhcyIsICJzaWduYXR1cmUiOiAic2lnbmF0dXJlL2hlcmUiLCAiY3VzdG9taXplZF9kYXRhIjogeyJjaGFubmVsIjogInN1cHBvcnRlZF9jaGFubmVsIiwgImhpc3RvcmljYWxfbWV0YWRhdGEiOiBbeyJjaHJvbWVfdmVyc2lvbiI6ICI1MC4yNTAwLjAuMCIsICJjcG0iOiAwLjZ9XX19",
> > > "message_id": "id"}, "subscription": "subscription"}
> > > 
> > > STDOUT:
> > > (empty)
> > > 
> > > STDERR:
> > > (empty)
> > 
> >
>
"/usr/local/google/home/wrengr/chromium-srcs/infra/appengine/findit/crash/findit.py",
> > line 117, in _InitializeAnalysis
> >     model.crashed_version = crash_data['crashed_version']
> > 
> > It's 'chrome_version' in crash_data
> 
> Except that it's 'crashed_version' for everyone else who calls
> ScheduleNewAnalysis (e.g., in crash_pipeline_test.py)

Right, the name is not consistent..., however changing name in CrashAnalysis
will cause some legacy data issue, and changing name in client message will
cause some cross-team back and forth, so we just live with it :)

Sharu Jiang

https://codereview.chromium.org/2414523002/diff/320001/appengine/findit/crash/findit.py File appengine/findit/crash/findit.py (right): https://codereview.chromium.org/2414523002/diff/320001/appengine/findit/crash/findit.py#newcode187 appengine/findit/crash/findit.py:187: analysis_pipeline = self._pipeline_cls(self.client_id, crash_identifiers) This is like we construct ...

4 years, 2 months ago (2016-10-21 21:57:06 UTC) #18

Sharu Jiang

https://codereview.chromium.org/2414523002/diff/320001/appengine/findit/crash/findit_for_chromecrash.py File appengine/findit/crash/findit_for_chromecrash.py (right): https://codereview.chromium.org/2414523002/diff/320001/appengine/findit/crash/findit_for_chromecrash.py#newcode42 appengine/findit/crash/findit_for_chromecrash.py:42: crash_config = CrashConfig.Get() There is already a config property ...

4 years, 2 months ago (2016-10-21 22:06:33 UTC) #19

wrengr

Okay, I finally have a version that's actually ready for review, as such. There arere ...

4 years, 2 months ago (2016-10-22 00:18:49 UTC) #23

Okay, I finally have a version that's actually ready for review, as such.

There arere still three unittests which are failing because it's not clear how
to fix the mocking for them; but Abhishek said we should get the CL landed
anyways, and deal with fixing those up in a separate CL. At the very least,
these tests give clean errors, rather than going over the wire and exploding in
spectacular ways.

PTAL

https://codereview.chromium.org/2414523002/diff/200001/appengine/findit/crash...
File appengine/findit/crash/findit_for_chromecrash.py (right):

https://codereview.chromium.org/2414523002/diff/200001/appengine/findit/crash...
appengine/findit/crash/findit_for_chromecrash.py:64:
crash_data.get('customized_data', {}).get('historical_metadata', []))
On 2016/10/19 21:03:09, sharu jiang wrote:
> On 2016/10/19 18:05:16, wrengr wrote:
> > On 2016/10/19 00:20:06, sharu jiang wrote:
> > > Should also set model.regression_range here or in
> > > superclass._InitializeAnalysis.
> > 
> > IIRC, the regression range isn't available from the crash_data for
> > fracas/cracas. Do you want it computed here if it's not provided (e.g., via
> > _NedsNewAnalysis)?
> > 
> > Also, is regression_range considered part of customized_data, or is it in
the
> > top-level of crash_data?
> 
> Conceptually, it would be better to let it be top-level of crash_data, since
the
> 'customized_data' should be raw client-specific data provided by clients and
> shouldn't contain computed(un-customized) data.
> 
> You already computed regression_range and insert it in
> crash_data['customized_data'] in line#88, the only thing left is to set the
> value of analysis here using that data.

I couldn't remember where it was before I started trying to replace crash_data
with CrashReport. I've put it at the top level everywhere i the latest version
of the cl.

https://codereview.chromium.org/2414523002/diff/240001/appengine/findit/crash...
File appengine/findit/crash/findit.py (right):

https://codereview.chromium.org/2414523002/diff/240001/appengine/findit/crash...
appengine/findit/crash/findit.py:157: analysis_pipeline =
CrashWrapperPipeline(self, crash_identifiers)
On 2016/10/19 18:20:53, stgao wrote:
> The problem is here -- the `self`.
> Only JSON-serializable objects could be passed over as parameters to an
> AppEngine pipeline. 
> 
> What specific data or info from `self` has to be passed over into the
pipeline?
> Is it possible to pass over the raw data and reconstruct the object in the
> pipeline?

IIRC the previous version just passed the client_id here. In principle that's
not enough, since the `self` object also encapsulates various data from
CrashConfig (so when we try to reconstruct `self` from the client_id, various
other config-specified stuff may change, meaning the new Findit object may be
different from the old one; opening the possibility of some hard-to-diagnose
incoherence bugs in the future).

I'll try just passing the client_id and see if that'll get the tests to pass at
least. Thanks for tracking this down.

https://codereview.chromium.org/2414523002/diff/320001/appengine/findit/crash...
File appengine/findit/crash/findit.py (right):

https://codereview.chromium.org/2414523002/diff/320001/appengine/findit/crash...
appengine/findit/crash/findit.py:187: analysis_pipeline =
self._pipeline_cls(self.client_id, crash_identifiers)
On 2016/10/21 21:57:06, sharu jiang wrote:
> This is like we construct a Findit object (in crash_handler), and construct a
> pipeline inside it, and yet construct another identical Findit object in this
> pipeline and run analysis using this Findit object.
> 
> How about we move this function out of Findit class, and make this stand-alone
> function which simply creates the CrashWrapperPipeline and run its _findit for
> analysis.

The Findit object contains all the stuff for actually doing the analysis (by
calling the Azalea object to do so), but objects in general (and Findit objects
in particular) aren't JSON-serializable; thus, no matter how we organize things,
the pipelines are going to have to reconstruct Findit objects every time they're
initialized.

The goal of putting ScheduleNewAnalysis (and _NeedsNewAnalysis, and...) in this
class is to try and encapsulate/hide all the gross implementation details of how
we spawn off a bunch of processes to actually do things. That is, conceptually
the Findit object fires off a bunch of pipeline tasks and outlives all of them
(even if, technically, due to implementation details we must actually
reconstruct the Findit object every time we start up a new process to execute a
given task).

https://codereview.chromium.org/2414523002/diff/320001/appengine/findit/crash...
File appengine/findit/crash/findit_for_chromecrash.py (right):

https://codereview.chromium.org/2414523002/diff/320001/appengine/findit/crash...
appengine/findit/crash/findit_for_chromecrash.py:50: top_n_frames =
crash_config.fracas.get('top_n', _DEFAULT_TOP_N)),
On 2016/10/21 22:06:33, sharu jiang wrote:
> Should be updated.

To... what?

wrengr

Ping. https://codereview.chromium.org/2414523002/diff/240001/appengine/findit/crash/crash_pipeline.py File appengine/findit/crash/crash_pipeline.py (right): https://codereview.chromium.org/2414523002/diff/240001/appengine/findit/crash/crash_pipeline.py#newcode120 appengine/findit/crash/crash_pipeline.py:120: run_analysis = yield CrashAnalysisPipeline(findit_client, crash_identifiers) On 2016/10/19 18:20:53, ...

4 years, 1 month ago (2016-10-24 18:36:14 UTC) #24

Sharu Jiang

https://codereview.chromium.org/2414523002/diff/320001/appengine/findit/crash/findit_for_chromecrash.py File appengine/findit/crash/findit_for_chromecrash.py (right): https://codereview.chromium.org/2414523002/diff/320001/appengine/findit/crash/findit_for_chromecrash.py#newcode42 appengine/findit/crash/findit_for_chromecrash.py:42: crash_config = CrashConfig.Get() On 2016/10/21 22:06:33, sharu jiang wrote: ...

4 years, 1 month ago (2016-10-24 19:38:23 UTC) #25

wrengr

https://codereview.chromium.org/2414523002/diff/320001/appengine/findit/crash/findit_for_chromecrash.py File appengine/findit/crash/findit_for_chromecrash.py (right): https://codereview.chromium.org/2414523002/diff/320001/appengine/findit/crash/findit_for_chromecrash.py#newcode50 appengine/findit/crash/findit_for_chromecrash.py:50: top_n_frames = crash_config.fracas.get('top_n', _DEFAULT_TOP_N)), On 2016/10/24 19:38:23, sharu jiang ...

4 years, 1 month ago (2016-10-24 22:29:48 UTC) #26

wrengr

https://codereview.chromium.org/2414523002/diff/320001/appengine/findit/crash/findit_for_chromecrash.py File appengine/findit/crash/findit_for_chromecrash.py (right): https://codereview.chromium.org/2414523002/diff/320001/appengine/findit/crash/findit_for_chromecrash.py#newcode42 appengine/findit/crash/findit_for_chromecrash.py:42: crash_config = CrashConfig.Get() On 2016/10/21 22:06:33, sharu jiang wrote: ...

4 years, 1 month ago (2016-10-24 22:39:00 UTC) #27

aarya

lgtm. lets land this, this CL is already getting huge. And we can track individual ...

4 years, 1 month ago (2016-10-25 02:45:40 UTC) #29

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2414523002/400001

4 years, 1 month ago (2016-10-25 17:23:52 UTC) #31

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 1 month ago (2016-10-25 17:23:54 UTC) #32

commit-bot: I haz the power

No L-G-T-M from a valid reviewer yet. CQ run can only be started by full ...

4 years, 1 month ago (2016-10-25 17:23:55 UTC) #33

stgao

On 2016/10/25 17:23:55, commit-bot: I haz the power wrote: > No L-G-T-M from a valid ...

4 years, 1 month ago (2016-10-25 17:25:00 UTC) #34

stgao

lgtm as I didn't see any big issue. Regarding the cycle dependency, I'd like to ...

4 years, 1 month ago (2016-10-25 18:03:42 UTC) #35

stgao

I also agree with Abhishek that we'd better avoid huge CL like this in the ...

4 years, 1 month ago (2016-10-25 18:23:11 UTC) #36

Sharu Jiang

lgtm with some comments. https://codereview.chromium.org/2414523002/diff/400001/appengine/findit/crash/culprit.py File appengine/findit/crash/culprit.py (right): https://codereview.chromium.org/2414523002/diff/400001/appengine/findit/crash/culprit.py#newcode119 appengine/findit/crash/culprit.py:119: class NullCulprit(object): # pragma: no ...

4 years, 1 month ago (2016-10-25 18:31:44 UTC) #37

wrengr

https://codereview.chromium.org/2414523002/diff/400001/appengine/findit/crash/crash_pipeline.py File appengine/findit/crash/crash_pipeline.py (right): https://codereview.chromium.org/2414523002/diff/400001/appengine/findit/crash/crash_pipeline.py#newcode146 appengine/findit/crash/crash_pipeline.py:146: client_config = self._findit.config On 2016/10/25 18:03:41, stgao wrote: > ...

4 years, 1 month ago (2016-10-25 19:49:54 UTC) #38

https://codereview.chromium.org/2414523002/diff/400001/appengine/findit/crash...
File appengine/findit/crash/crash_pipeline.py (right):

https://codereview.chromium.org/2414523002/diff/400001/appengine/findit/crash...
appengine/findit/crash/crash_pipeline.py:146: client_config =
self._findit.config
On 2016/10/25 18:03:41, stgao wrote:
> IMO, it's OK for the pipeline to depends on the config as it is part of the
flow
> controller.
> 
> Later, we should move the pipelines out from this lib to the app layer.

My concern is that the config is volatile. Since we cache various things in
objects (e.g., compiled regexes, particular top-N values, etc), and do so at
different times, that means we can end up in an inconsistent state. If we pass
all the relevant parts to the constructor when we create the object, then we're
guaranteed it will be consistent/coherent. (There may still be inconsistency
issues *between* objects, if those objects are constructed at different times;
but that's easier to address.)

N.B., the problem isn't that things change, the problem is that they change out
from under us silently. If the objects got some notification whenever the
CrashConfig changes, then we could update the object to match the new config (or
throw away the old object and make a new one).

https://codereview.chromium.org/2414523002/diff/400001/appengine/findit/crash...
File appengine/findit/crash/culprit.py (right):

https://codereview.chromium.org/2414523002/diff/400001/appengine/findit/crash...
appengine/findit/crash/culprit.py:119: class NullCulprit(object): # pragma: no
cover
On 2016/10/25 18:31:44, sharu jiang wrote:
> How about let NullCulprit inherits Culprit and do something like
> super(NullCulprit, self).__init__('', [], [], None, None)? or drop those empty
> fields so we can combine them. 
> 
> If so, it's ok to have this in another cl.

The big question here is whether the JSON encoding supports combining them. The
current NullCulprit.ToDicts returns minimal dictionaries whereas using
Culprit.ToDicts (as it's currently written) would produce larger dictionaries. I
don't know how fragile the JSON parsing would be to such differences.

https://codereview.chromium.org/2414523002/diff/400001/appengine/findit/crash...
File appengine/findit/crash/findit.py (right):

https://codereview.chromium.org/2414523002/diff/400001/appengine/findit/crash...
appengine/findit/crash/findit.py:210: analysis_pipeline =
self._pipeline_cls(self.client_id, crash_identifiers)
On 2016/10/25 18:03:41, stgao wrote:
> This is a bit unfortunate for the cycle dependency. Later, we could consider
> factoring this out and make the caller of this code to start the pipeline.

Yeah, the cyclic dependency problem is unfortunate. Let's set up a meeting to
see if we can't come up with some cleaner way of breaking the cycle.

https://codereview.chromium.org/2414523002/diff/400001/appengine/findit/crash...
File appengine/findit/crash/findit_for_chromecrash.py (right):

https://codereview.chromium.org/2414523002/diff/400001/appengine/findit/crash...
appengine/findit/crash/findit_for_chromecrash.py:74: model.regression_range =
crash_data.get('regression_range', None)
On 2016/10/25 18:31:44, sharu jiang wrote:
> This should be lifted to findit superclass, since the regression_range is the
> top-level field that all clients(user) should have.
> 
> The _InitializeAnalysis here should just handle data in customized_data

Acknowledged.

https://codereview.chromium.org/2414523002/diff/400001/appengine/findit/crash...
File appengine/findit/crash/test/crash_pipeline_test.py (right):

https://codereview.chromium.org/2414523002/diff/400001/appengine/findit/crash...
appengine/findit/crash/test/crash_pipeline_test.py:199: 
On 2016/10/25 18:03:41, stgao wrote:
> nit: too many empty lines.

Acknowledged.

https://codereview.chromium.org/2414523002/diff/400001/appengine/findit/crash...
File appengine/findit/crash/test/stacktrace_test.py (right):

https://codereview.chromium.org/2414523002/diff/400001/appengine/findit/crash...
appengine/findit/crash/test/stacktrace_test.py:11: from
crash.test.stacktrace_test_suite import StacktraceTestSuite
On 2016/10/25 18:03:41, stgao wrote:
> style nit: order of import.

Acknowledged.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2414523002/400001

4 years, 1 month ago (2016-10-25 19:50:11 UTC) #40

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 1 month ago (2016-10-25 19:51:59 UTC) #41

commit-bot: I haz the power

Try jobs failed on following builders: Infra Linux Precise 32 Tester on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/321636432cb9fb10) ...

4 years, 1 month ago (2016-10-25 19:52:01 UTC) #42

wrengr

The patchset sent to the CQ was uploaded after l-g-t-m from katesonia@chromium.org, stgao@chromium.org, aarya@google.com Link ...

4 years, 1 month ago (2016-10-25 23:34:06 UTC) #44

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2414523002/420001

4 years, 1 month ago (2016-10-25 23:34:17 UTC) #45

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 1 month ago (2016-10-25 23:47:25 UTC) #46

commit-bot: I haz the power

Try jobs failed on following builders: Infra Linux Trusty 64 Tester on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/321703714e52ab10)

4 years, 1 month ago (2016-10-25 23:47:27 UTC) #47

Sharu Jiang

https://codereview.chromium.org/2414523002/diff/420001/appengine/findit/crash/test/crash_pipeline_test.py File appengine/findit/crash/test/crash_pipeline_test.py (right): https://codereview.chromium.org/2414523002/diff/420001/appengine/findit/crash/test/crash_pipeline_test.py#newcode111 appengine/findit/crash/test/crash_pipeline_test.py:111: #analysis_pipeline.run() the self._client_id, self._crash_identifiers should be passed to run ...

4 years, 1 month ago (2016-10-26 01:16:00 UTC) #48

wrengr

https://codereview.chromium.org/2414523002/diff/420001/appengine/findit/crash/test/crash_pipeline_test.py File appengine/findit/crash/test/crash_pipeline_test.py (right): https://codereview.chromium.org/2414523002/diff/420001/appengine/findit/crash/test/crash_pipeline_test.py#newcode111 appengine/findit/crash/test/crash_pipeline_test.py:111: #analysis_pipeline.run() On 2016/10/26 01:16:00, sharu jiang wrote: > the ...

4 years, 1 month ago (2016-10-26 17:05:19 UTC) #49

Sharu Jiang

https://codereview.chromium.org/2414523002/diff/420001/appengine/findit/crash/test/crash_pipeline_test.py File appengine/findit/crash/test/crash_pipeline_test.py (right): https://codereview.chromium.org/2414523002/diff/420001/appengine/findit/crash/test/crash_pipeline_test.py#newcode111 appengine/findit/crash/test/crash_pipeline_test.py:111: #analysis_pipeline.run() On 2016/10/26 17:05:19, wrengr wrote: > On 2016/10/26 ...

4 years, 1 month ago (2016-10-26 19:09:32 UTC) #50

wrengr

The patchset sent to the CQ was uploaded after l-g-t-m from katesonia@chromium.org, stgao@chromium.org, aarya@google.com Link ...

4 years, 1 month ago (2016-10-26 21:01:08 UTC) #52

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2414523002/460001

4 years, 1 month ago (2016-10-26 21:01:20 UTC) #53

commit-bot: I haz the power

Description was changed from ========== [Findit] Reorganizing findit_for_*.py BUG= ========== to ========== [Findit] Reorganizing findit_for_*.py ...

4 years, 1 month ago (2016-10-26 21:42:50 UTC) #54

commit-bot: I haz the power

4 years, 1 month ago (2016-10-26 21:42:52 UTC) #55

Message was sent while issue was closed.

Committed patchset #19 (id:460001) as
https://chromium.googlesource.com/infra/infra/+/6a8b24da0d0a58339da9f3c86e365...

Issue 2414523002: [Findit] Reorganizing findit_for_*.py (Closed)

Description

Patch Set 1 #

Patch Set 2 : rebase_update #

Patch Set 3 : trying to fix some tests #

Patch Set 4 : addressing comments and some unittest bugfixing #

Patch Set 5 : Fixing some bugs in changelist_classifier_test.py #

Patch Set 6 : Checking off todos #

Patch Set 7 : moving GetPublishResultFromAnalysis to be a method of CrashAnalysis, breaking out findit_for_cluste… #

Patch Set 8 : debugging the unittests #

Patch Set 9 : Fixing call to ScheduleNewAnalysis in handlers/crash/crash_handler.py #

Patch Set 10 : crash_pipeline_test.py: cleaning up the crash_data to better match the old code. #

Patch Set 11 : more debugging #

Patch Set 12 : linting for coverage #

Patch Set 13 : Fixed crash_handler_test.py. Also various other unittest debugging. #

Patch Set 14 : cleaning up where regression_range lives in crash_data #

Patch Set 15 : rebase-update #

Patch Set 16 : Addressing the crash_config.fracas issue #

Patch Set 17 : rebasing against recently landed cls #

Patch Set 18 : more debugging of mocking #

Patch Set 19 : Finally fixed the mock tests! #

Messages