[Findit] Add skeleton code for delta test script.

appengine/findit/crash/findit_for_chromecrash.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 logging
 from collections import namedtuple
 
 from common import chromium_deps
 from crash import detect_regression_range
 from crash import findit_for_crash
 from crash.chromecrash_parser import ChromeCrashParser
 from crash.project_classifier import ProjectClassifier
 from crash.component_classifier import Component
 from crash.component_classifier import ComponentClassifier
 from model.crash.crash_config import CrashConfig
 
 # TODO(katesonia): Remove the default value after adding validity check to
 # config.
 _DEFAULT_TOP_N = 7
 
 
 # TODO(wrengr): move this to its own file, so it can be shared. When we do
 # so, we'll need to also pass in the 'solution' argument for the tag_dict.
 class Culprit(namedtuple('Culprit',
     ['project', 'components', 'cls', 'regression_range'])):
 
+  @property
+  def fields(self):

[wrengr, 2016/10/11 22:57:43]

What's the purpose of this? Since _fields is a private member of namedtuple, it
make sense to leave it private so that it's clear when callers are depending on
the internals of this class in ways they shouldn't be (i.e., which are fragile).

[Sharu Jiang, 2016/10/12 01:18:20]

This is used in delta test, and later may used to drop empty field in Culprit.

[wrengr, 2016/10/12 20:15:17]

I still don't understand. Why can't delta use _fields directly? If all we need
is to compare one Culprit to another, then we should provide the appropriate
comparison functions rather than having client code rely on internals.

This class serves as the API for the library to pass information to the
application; thus, it needs to serve the purposes of the library first and
foremost. I assume you want to drop fields because of serializing to JSON, but
that's an application concern not a library concern. The library shouldn't have
to care at all about how the application decides to serialize things.

[Sharu Jiang, 2016/10/12 22:58:27]

shouldn't _fields supposed to be private and not used directly? This is not for
comparison. 

For "that's an application concern not a library concern", in this case, I think
the serialization part should be in clients' code (findit_for_*) instead of
Culprit, to do that, the "fields" may still be needed.

[wrengr, 2016/10/24 18:09:06]

(FWIW, whenever I talk about "client code" or "clients (of X)", I mean any code
that uses X; as distinct from "users", i.e. humans, who use X. I do not
specifically mean the AppEngine applications. I believe this has been a source
of confusion, so I will avoid the word "client" below.)

If we're going to be turning Culprit objects into anonymous dicts, then that
should be done in/by the Culprit class itself. That way, code using the class
isn't forced to muck around with the innards of the class— thereby becoming
fragile to changes in/to the class. If code using the class can simply call a
ToDict method (or similar) and expect that the method will return some sort of
JSON-serializable dictionary, then the code using the class becomes independent
of how the class is implemented; which in turn frees us up to change the
internal representation of Culprit objects without needing to change all the
code that uses the Culprit class.

If we're serializing Culprit objects into JSON, then we surely want to serialize
them in the same way regardless of which AppEngine application happens to be
working with that serialized format. Thus, serialization is not
application-specific, so it should not be implemented by/in application code. It
is much less fragile for application code to assume that objects already know
how to serialize *themselves*, and then just ask them to do so (by calling the
ToDict method) whenever necessary. Yes, in terms of execution control flow, the
application code is where the request for serialization is made, but that
doesn't mean that's where the implementation of serialization should live.

[stgao, 2016/10/24 18:17:26]

+1 for Wren's suggestion here

[Sharu Jiang, 2016/10/24 18:50:20]

I agree on what you said about the serializing culprit object into JSON, however
I still have one question, should I just use _fields directly in delta test?
Shouldn't _smth be private?

[wrengr, 2016/10/24 21:22:56]

Things with a single underscore are generally "protected" in the Java
terminology, whereas things with two underscores are "private" in the Java sense
(though double underscore is also used to refer to builtins, so there's some
confusion here. Also, I don't know whether Google follows the one- vs
two-underscore tradition of the Python community at large).

As far as the Delta class goes, there are two salient approaches to take. We can
either make the class specific to being a delta of Culprit objects, or we can
try to make it generic for some large class of values (e.g., JSON-serializable
things).

If we make it Culprit-specific, then the Delta class is a "friend" in the C++
terminology, so the fact that it accesses the internals of the Culprit class
isn't unexpected. Whenever we update the Culprit class we'll have to make sure
Delta is kept in sync; but if Delta is the only "friend" of Culprit, then it's
cleaner to have Delta depend directly on the innards of Culprit rather than
setting up a bunch of boilerplate for other potential "friend" classes which
don't actually exist.

On the other hand, if we want Delta to be generic over a large class of values,
then it makes more sense IMO to define Delta to operate over dict (and list,
set, etc; i.e., Python built-in types, or JSON-serializable types, or something
similar). In this case we'd call ToDict before computing the delta, and so Delta
doesn't need to know what fields the Culprit object itself has (it can query the
dict for its keys). Of course, if we only ever use Delta for the deltas of
Culprits, then being generic over all dicts(etc) may make things unnecessarily
complicated. (I'm guessing in our case it wouldn't actually be too complicated,
but the concern is still worth mentioning.)

(A third option is to be generic over types which provide certain helper
functions. This would allow avoiding the conversion to Dict, while still letting
the Delta class remain agnostic to the internals of the classes it operates
over. The idea here is to generalize the notion of "delta" or "equality" to the
notion of "zipping" two structures together. My MSE thesis was on how to do this
cleanly and efficiently, so if we want to pursue this path I already know how to
go about it; but that may be like using a cannon as a flyswatter.)

+    return self._fields
+
   # TODO(wrengr): better name for this method.
   def ToDicts(self):
     """Convert this object to a pair of anonymous dicts for JSON.
 
     Returns:
       (analysis_result_dict, tag_dict)
       The analysis result is a dict like below:
       {
           # Indicate if Findit found any suspects_cls, project,
           # components or regression_range.
@@ skipping 171 matching lines @@
 
     crash_stack = stacktrace.crash_stack
     suspected_project = self.project_classifier.Classify(
         suspected_cls, crash_stack)
 
     suspected_components = self.component_classifier.Classify(
         suspected_cls, crash_stack)
 
     return Culprit(suspected_project, suspected_components, suspected_cls,
         regression_range)