[Predator] Fix bug in min_distance after refactor and add back skip added/deleted deps.

[Predator] Fix bug in min_distance after refactor and add back skip added/deleted deps.

Also added culprit_test for culprit.py

BUG=660621, 659346
Committed: https://chromium.googlesource.com/infra/infra/+/e50200467b7e522a03b2d9d73d6cd27d0b49874c

[Sharu Jiang, 2016-10-29 01:23:17]

Description was changed from

==========
[Predator] Fix bug in min_distance after refactor and add back skip
added/deleted deps.

BUG=
==========

to

==========
[Predator] Fix bug in min_distance after refactor and add back skip
added/deleted deps.

BUG=660621
==========

[Sharu Jiang, 2016-10-29 01:23:17]

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

[Sharu Jiang, 2016-10-29 01:26:21]

PTAL :)

[Sharu Jiang, 2016-10-31 20:57:28]

Description was changed from

==========
[Predator] Fix bug in min_distance after refactor and add back skip
added/deleted deps.

BUG=660621
==========

to

==========
[Predator] Fix bug in min_distance after refactor and add back skip
added/deleted deps.

Also added culprit_test for culprit.py

BUG=660621
==========

[Sharu Jiang, 2016-10-31 20:57:49]

Patchset #2 (id:20001) has been deleted

[Sharu Jiang, 2016-10-31 20:58:20]

Description was changed from

==========
[Predator] Fix bug in min_distance after refactor and add back skip
added/deleted deps.

Also added culprit_test for culprit.py

BUG=660621
==========

to

==========
[Predator] Fix bug in min_distance after refactor and add back skip
added/deleted deps.

Also added culprit_test for culprit.py

BUG=660621, 659346
==========

[Sharu Jiang, 2016-10-31 21:00:31]

Patchset #2 (id:40001) has been deleted

[wrengr, 2016-10-31 23:48:35]

lgtm, with nits.

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

https://codereview.chromium.org/2449853012/diff/60001/appengine/findit/crash/...
appengine/findit/crash/culprit.py:101: result['regression_range'] =
self.regression_range
self.regression_range stores a tuple; however, JSON and the unittests expect a
list. We should insert a coercion here to make sure we produce a list.

https://codereview.chromium.org/2449853012/diff/60001/appengine/findit/crash/...
File appengine/findit/crash/scorers/test/aggregated_scorer_test.py (right):

https://codereview.chromium.org/2449853012/diff/60001/appengine/findit/crash/...
appengine/findit/crash/scorers/test/aggregated_scorer_test.py:25: 'a.cc':
AnalysisInfo(
+1 to using AnalysisInfo everywhere :)

https://codereview.chromium.org/2449853012/diff/60001/appengine/findit/crash/...
File appengine/findit/crash/test/changelist_classifier_test.py (right):

https://codereview.chromium.org/2449853012/diff/60001/appengine/findit/crash/...
appengine/findit/crash/test/changelist_classifier_test.py:143: del
expected_regression_deps_rolls['src/dep']
Could you add a comment about why this ``del`` is necessary?

https://codereview.chromium.org/2449853012/diff/60001/appengine/findit/crash/...
File appengine/findit/crash/test/culprit_test.py (right):

https://codereview.chromium.org/2449853012/diff/60001/appengine/findit/crash/...
appengine/findit/crash/test/culprit_test.py:13: self.assertEqual(culprit.fields,
('project', 'components', 'cls',
N.B., this is fragile. In general Culprit is not defined when the
``regression_range`` is None rather than a pair

https://codereview.chromium.org/2449853012/diff/60001/appengine/findit/crash/...
appengine/findit/crash/test/culprit_test.py:16: culprit = Culprit(None, None,
None, None, None)
Again, this is fragile. Culprit expects all the fields to actually be defined.

https://codereview.chromium.org/2449853012/diff/60001/appengine/findit/crash/...
appengine/findit/crash/test/culprit_test.py:25: cl = self.GetDummyChangeLog()
Below from here should be broken out into a separate test from the stuff above
here.

[Sharu Jiang, 2016-11-01 23:54:09]

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

https://codereview.chromium.org/2449853012/diff/60001/appengine/findit/crash/...
appengine/findit/crash/culprit.py:101: result['regression_range'] =
self.regression_range
On 2016/10/31 23:48:35, wrengr wrote:
> self.regression_range stores a tuple; however, JSON and the unittests expect a
> list. We should insert a coercion here to make sure we produce a list.

Done.

https://codereview.chromium.org/2449853012/diff/60001/appengine/findit/crash/...
appengine/findit/crash/culprit.py:102: if self.project is not None:
Since the project_classifier can return '' and component_classifier can return
[], to make a safe move, changed this to "if self.*" instead of "if self.* is
not None".

https://codereview.chromium.org/2449853012/diff/60001/appengine/findit/crash/...
File appengine/findit/crash/test/changelist_classifier_test.py (right):

https://codereview.chromium.org/2449853012/diff/60001/appengine/findit/crash/...
appengine/findit/crash/test/changelist_classifier_test.py:143: del
expected_regression_deps_rolls['src/dep']
On 2016/10/31 23:48:35, wrengr wrote:
> Could you add a comment about why this ``del`` is necessary?

Done.

https://codereview.chromium.org/2449853012/diff/60001/appengine/findit/crash/...
File appengine/findit/crash/test/culprit_test.py (right):

https://codereview.chromium.org/2449853012/diff/60001/appengine/findit/crash/...
appengine/findit/crash/test/culprit_test.py:13: self.assertEqual(culprit.fields,
('project', 'components', 'cls',
On 2016/10/31 23:48:35, wrengr wrote:
> N.B., this is fragile. In general Culprit is not defined when the
> ``regression_range`` is None rather than a pair

Actually we can still get suspected_project and suspected_components from crash
stack even though we cannot find any regression_range.

We return None only when there is no parsed stacktrace object.

https://codereview.chromium.org/2449853012/diff/60001/appengine/findit/crash/...
appengine/findit/crash/test/culprit_test.py:16: culprit = Culprit(None, None,
None, None, None)
On 2016/10/31 23:48:35, wrengr wrote:
> Again, this is fragile. Culprit expects all the fields to actually be defined.

Still, it's possible that some of those fields are empty. This is just to test
we can successfully drop those empty fields.

So it's possible that we return Culprit('', [], [], [], 'core_algorithm') if we
failed detecting regression range and all the classifiers failed.

[stgao, 2016-11-02 01:11:27]

lgtm

[Sharu Jiang, 2016-11-02 01:29:48]

Description was changed from

==========
[Predator] Fix bug in min_distance after refactor and add back skip
added/deleted deps.

Also added culprit_test for culprit.py

BUG=660621, 659346
==========

to

==========
[Predator] Fix bug in min_distance after refactor and add back skip
added/deleted deps.

Also added culprit_test for culprit.py

BUG=660621, 659346
R=wrengr@chromium.org
TBR=stgao@chromium.org
==========

[Sharu Jiang, 2016-11-02 01:35:34]

Description was changed from

==========
[Predator] Fix bug in min_distance after refactor and add back skip
added/deleted deps.

Also added culprit_test for culprit.py

BUG=660621, 659346
R=wrengr@chromium.org
TBR=stgao@chromium.org
==========

to

==========
[Predator] Fix bug in min_distance after refactor and add back skip
added/deleted deps.

Also added culprit_test for culprit.py

BUG=660621, 659346
==========

[Sharu Jiang, 2016-11-02 01:35:40]

The CQ bit was checked by katesonia@chromium.org

[Sharu Jiang, 2016-11-02 01:35:40]

The patchset sent to the CQ was uploaded after l-g-t-m from wrengr@chromium.org,
stgao@chromium.org
Link to the patchset: https://codereview.chromium.org/2449853012/#ps120001
(title: "Rebase and add logging.")

[commit-bot: I haz the power, 2016-11-02 01:35:52]

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

[commit-bot: I haz the power, 2016-11-02 01:43:22]

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

[commit-bot: I haz the power, 2016-11-02 01:43:23]

Try jobs failed on following builders:
  Infra Win Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/323b7f450e9d9910)

[Sharu Jiang, 2016-11-02 02:40:02]

The CQ bit was checked by katesonia@chromium.org

[Sharu Jiang, 2016-11-02 02:40:02]

The patchset sent to the CQ was uploaded after l-g-t-m from wrengr@chromium.org,
stgao@chromium.org
Link to the patchset: https://codereview.chromium.org/2449853012/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2016-11-02 02:40:07]

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

[commit-bot: I haz the power, 2016-11-02 03:08:21]

Description was changed from

==========
[Predator] Fix bug in min_distance after refactor and add back skip
added/deleted deps.

Also added culprit_test for culprit.py

BUG=660621, 659346
==========

to

==========
[Predator] Fix bug in min_distance after refactor and add back skip
added/deleted deps.

Also added culprit_test for culprit.py

BUG=660621, 659346

Committed:
https://chromium.googlesource.com/infra/infra/+/e50200467b7e522a03b2d9d73d6cd...
==========

[commit-bot: I haz the power, 2016-11-02 03:08:23]

Committed patchset #6 (id:140001) as
https://chromium.googlesource.com/infra/infra/+/e50200467b7e522a03b2d9d73d6cd...

[wrengr, 2016-11-03 16:14:33]

https://codereview.chromium.org/2449853012/diff/60001/appengine/findit/crash/...
File appengine/findit/crash/test/culprit_test.py (right):

https://codereview.chromium.org/2449853012/diff/60001/appengine/findit/crash/...
appengine/findit/crash/test/culprit_test.py:13: self.assertEqual(culprit.fields,
('project', 'components', 'cls',
On 2016/11/01 23:54:09, sharu jiang wrote:
> On 2016/10/31 23:48:35, wrengr wrote:
> > N.B., this is fragile. In general Culprit is not defined when the
> > ``regression_range`` is None rather than a pair
> 
> Actually we can still get suspected_project and suspected_components from
crash
> stack even though we cannot find any regression_range.
> 
> We return None only when there is no parsed stacktrace object.

No, FindCulprit should return None (or rather: not return a Culprit) whenever it
fails for any reason. The regression_range is a required field for the
CrashReport, which Predator.FindCulprit takes as an argument, therefore
regression_range is guaranteed to be provided in every Culprit that
Predator.FindCulprit returns, thus the semantics of Culprit also have
regression_range be required.

https://codereview.chromium.org/2449853012/diff/60001/appengine/findit/crash/...
appengine/findit/crash/test/culprit_test.py:16: culprit = Culprit(None, None,
None, None, None)
On 2016/11/01 23:54:09, sharu jiang wrote:
> On 2016/10/31 23:48:35, wrengr wrote:
> > Again, this is fragile. Culprit expects all the fields to actually be
defined.
> 
> Still, it's possible that some of those fields are empty. This is just to test
> we can successfully drop those empty fields.
> 
> So it's possible that we return Culprit('', [], [], [], 'core_algorithm') if
we
> failed detecting regression range and all the classifiers failed.

No, again. The Culprit class is defined to encapsulate the successful results of
Predator.FindCulprit. The regression_range is provided by the CrashReport, and
therefore is always available. Similarly, the algorithm always knows about
itself, therefore Culprit.algorithm can always be filled in. For the cls and
components, "failure" means returning the empty list of alternatives; None
should not be used as it is not iterable. For the project, since the empty
string is an invalid project name, we do not want to distinguish between '' vs
None. Therefore, none of the fields should ever be None. Allowing any of the
fields to be None means all the consumers of Culprits must go through and add a
bunch of boilerplate to deal with ruling out Culprits that don't make sense in
the first place. The whole point of making Culprit a class rather than an
arbitrary dict is precisely to capture these invariants.

I can add code to Culprit.__init__ to check all these invariants, if you'd
prefer. I've avoided doing so only because that's against Python's spirit of
"consenting adults" (which I disagree with, but well, Python culture is Python
culture).