[Findit] Add three sub-pipelines to analyze build failure.

[Findit] Add three sub-pipelines to analyze build failure.

With this patch, if a CL changes a file in the failure message or a related file (like xyz.h vs. xyz_impl.cc), it will be filtered out.

Committed: https://chromium.googlesource.com/infra/infra/+/ed59b401885b3b9f67cd73ae184540d9d54b73c5

[stgao, 2015-01-09 02:46:28]

Patchset #1 (id:1) has been deleted

[stgao, 2015-01-10 00:58:18]

Patchset #1 (id:20001) has been deleted

[stgao, 2015-01-13 23:54:15]

Patchset #1 (id:40001) has been deleted

[stgao, 2015-01-14 00:15:55]

stgao@chromium.org changed reviewers:
+ qyearsley@chromium.org

[stgao, 2015-01-14 00:16:55]

ptal

It would be great if you could review the files listed int he description in the
first round.

[stgao, 2015-01-15 00:19:04]

Patchset #2 (id:80001) has been deleted

[stgao, 2015-01-15 19:02:14]

+cc stip, agable, ojan

[qyearsley, 2015-01-15 19:10:28]

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
File appengine/findit/waterfall/build_failure_analysis.py (right):

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis.py:26: def
JoinAsFilePath(file_dir, file_name):
Should this function be marked as private? (Same question for the functions
below.)

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis.py:49: COMMON_POSTFIXES = [
1) s/POSTFIX/SUFFIX/
2) If this is only used in this file, it might also be marked as private. (Same
question for the functions below.)

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis.py:54: 'aura', 'x', 'x11']
Style nit: Closing parenthesis goes on its own line.

(Based on example at
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Whites...)

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis.py:60: ]
It seems like these four lines should be un-indented four spaces?

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis.py:116: def
_AddHint(change_action, src_file, file_path):
Nested functions don't actually need to be marked as private, since they're
hidden from module scope anyway, so they can't be used by other modules that
import this module.

Although, in this case, would CheckFiles be more readable if _AddHint and
_CheckFile were module-level functions?

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
File appengine/findit/waterfall/build_failure_analysis_pipelines.py (right):

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis_pipelines.py:28: def
finalized(self):
When is this method called? Why is no BuildAnalysis created/put when was_aborted
is False? Is it created and put at some other time?

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis_pipelines.py:44: failure_info
= yield DetectFirstFailurePipeline(
It may be worth adding a comment here saying what failure_info will be set to...
not sure if there's any Pipeline documentation related to this?

[qyearsley, 2015-01-15 21:15:03]

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
File appengine/findit/waterfall/build_failure_analysis.py (right):

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis.py:46: return
JoinAsFilePath(file_dir, file_name)
For both uses of JoinAsFilePath in this file, you could actually use
os.path.join instead. e.g.:

path = 'foo/bar.o'
dir = os.path.dirname(path)  # = 'foo'
file = os.path.dirname(path)  # = 'bar.o'
joined = os.path.join(dir, file)  # = 'foo/bar.o'

path = 'bar.o'
dir = os.path.dirname(path)  # = ''
file = os.path.dirname(path)  # = 'bar.o'
joined = os.path.join(dir, file)  # = 'bar.o'

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
File appengine/findit/waterfall/extract_signal_pipeline.py (right):

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
appengine/findit/waterfall/extract_signal_pipeline.py:24: It is a json like
below:
The below is a dict, not JSON. It is true that the return value of a pipeline's
run() method is converted to JSON at some point, but what is returned directly
from the run() method is a dict.

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
appengine/findit/waterfall/extract_signal_pipeline.py:34: def run(self,
failure_info):
Rather than putting Input and Output above, I suggest adding a docstring for the
run() method, which should have Args and Returns sections.

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
appengine/findit/waterfall/extract_signal_pipeline.py:72: master_name,
builder_name, step_name, None, stdio_log).ToJson()
Looking at the FailureSignal.ToJson method[1] - it doesn't return JSON, it
returns a dict (which is JSON-serializable, but it is not a string that contains
JSON-formatted data). This is a bit misleading.

I think that method should be renamed to ToDict.

[1]
https://code.google.com/p/chromium/codesearch#chromium/infra/appengine/findit...

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
File appengine/findit/waterfall/test/build_failure_analysis_test.py (right):

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
appengine/findit/waterfall/test/build_failure_analysis_test.py:51:
self.assertTrue(build_failure_analysis.IsCorrelated('a/b/x.cc', 'a/b/y.cc'))
Maybe add a negative case -- when is IsCorrelated going to return false? Also,
why are the above examples expected to be true?

[stgao, 2015-01-16 20:20:39]

Patchset #2 (id:100001) has been deleted

[stgao, 2015-01-16 20:21:39]

Many thanks for the review!
I've addressed your comments.

For rename ToJson/FromJson to ToDict/FromDict, I will do it in a separate CL.

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
File appengine/findit/waterfall/build_failure_analysis.py (right):

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis.py:26: def
JoinAsFilePath(file_dir, file_name):
On 2015/01/15 19:10:28, qyearsley wrote:
> Should this function be marked as private? (Same question for the functions
> below.)

Done.

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis.py:46: return
JoinAsFilePath(file_dir, file_name)
On 2015/01/15 21:15:03, qyearsley wrote:
> For both uses of JoinAsFilePath in this file, you could actually use
> os.path.join instead. e.g.:
> 
> path = 'foo/bar.o'
> dir = os.path.dirname(path)  # = 'foo'
> file = os.path.dirname(path)  # = 'bar.o'
> joined = os.path.join(dir, file)  # = 'foo/bar.o'
> 
> path = 'bar.o'
> dir = os.path.dirname(path)  # = ''
> file = os.path.dirname(path)  # = 'bar.o'
> joined = os.path.join(dir, file)  # = 'bar.o'

Done.

Switch to os.path.join, but need to do a replace of os.sep with '/'.
Because on Windows, os.path.join uses os.sep ('\\'), but git change logs always
use '/'.

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis.py:49: COMMON_POSTFIXES = [
On 2015/01/15 19:10:28, qyearsley wrote:
> 1) s/POSTFIX/SUFFIX/
> 2) If this is only used in this file, it might also be marked as private.
(Same
> question for the functions below.)

Done.

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis.py:54: 'aura', 'x', 'x11']
On 2015/01/15 19:10:28, qyearsley wrote:
> Style nit: Closing parenthesis goes on its own line.
> 
> (Based on example at
>
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Whites...)

Done.

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis.py:60: ]
On 2015/01/15 19:10:28, qyearsley wrote:
> It seems like these four lines should be un-indented four spaces?

Done.

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis.py:116: def
_AddHint(change_action, src_file, file_path):
On 2015/01/15 19:10:28, qyearsley wrote:
> Nested functions don't actually need to be marked as private, since they're
> hidden from module scope anyway, so they can't be used by other modules that
> import this module.
> 
> Although, in this case, would CheckFiles be more readable if _AddHint and
> _CheckFile were module-level functions?

Done.

Moved _AddHint to a new class _Justification.
Moved _CheckFile to module-level.

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
File appengine/findit/waterfall/build_failure_analysis_pipelines.py (right):

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis_pipelines.py:28: def
finalized(self):
On 2015/01/15 19:10:28, qyearsley wrote:
> When is this method called? Why is no BuildAnalysis created/put when
was_aborted
> is False? Is it created and put at some other time?

This method is called when all sub-pipelines finish running.
If not aborted, the analysis entity is updated in identify_culprit_pipeline.py,
so there is no need to update here.
The analysis is created before the pipeline starts.

I also added a comment in the code to explain this.

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis_pipelines.py:44: failure_info
= yield DetectFirstFailurePipeline(
On 2015/01/15 19:10:28, qyearsley wrote:
> It may be worth adding a comment here saying what failure_info will be set
to...
> not sure if there's any Pipeline documentation related to this?

Done.
I added a comment.

Yes, the documentation for the yield is
https://github.com/GoogleCloudPlatform/appengine-pipelines/wiki/Python#defini...

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
File appengine/findit/waterfall/extract_signal_pipeline.py (right):

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
appengine/findit/waterfall/extract_signal_pipeline.py:24: It is a json like
below:
On 2015/01/15 21:15:03, qyearsley wrote:
> The below is a dict, not JSON. It is true that the return value of a
pipeline's
> run() method is converted to JSON at some point, but what is returned directly
> from the run() method is a dict.

Good point.
Docstring was updated.

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
appengine/findit/waterfall/extract_signal_pipeline.py:34: def run(self,
failure_info):
On 2015/01/15 21:15:03, qyearsley wrote:
> Rather than putting Input and Output above, I suggest adding a docstring for
the
> run() method, which should have Args and Returns sections.

Done.

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
appengine/findit/waterfall/extract_signal_pipeline.py:72: master_name,
builder_name, step_name, None, stdio_log).ToJson()
On 2015/01/15 21:15:03, qyearsley wrote:
> Looking at the FailureSignal.ToJson method[1] - it doesn't return JSON, it
> returns a dict (which is JSON-serializable, but it is not a string that
contains
> JSON-formatted data). This is a bit misleading.
> 
> I think that method should be renamed to ToDict.
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/infra/appengine/findit...

Good point.
But let's do the rename in a separate CL to avoid making this CL even larger.

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
File appengine/findit/waterfall/test/build_failure_analysis_test.py (right):

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
appengine/findit/waterfall/test/build_failure_analysis_test.py:51:
self.assertTrue(build_failure_analysis.IsCorrelated('a/b/x.cc', 'a/b/y.cc'))
On 2015/01/15 21:15:03, qyearsley wrote:
> Maybe add a negative case -- when is IsCorrelated going to return false? Also,
> why are the above examples expected to be true?

Negative case added.

If a CL changed file a.h, and a_test.cc or a_impl.cc or a_impl_test.o show up in
the failure, the CL is suspected. Because a.cc is tested by a_test.cc, or some
class in a.h is implemented in a_impl.cc, or some class in a_impl.cc
implementing a.h and it is tested in a_impl_test.cc (compiled into
a_impl_test.o).
The correlation between files here are 'tested', 'implemented', or combination
of 'implemented' and 'tested'.

These rules caught quite a few build failures in the past.

The last case, a/b/x.cc and a/b/y.cc are in the same directory. I'm not sure if
it will lead to some noisy result as I commented in IsCorrelated. What do you
think?

[qyearsley, 2015-01-16 22:55:26]

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
File appengine/findit/waterfall/build_failure_analysis.py (right):

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis.py:46: return
JoinAsFilePath(file_dir, file_name)
On 2015/01/16 20:21:38, Shuotao wrote:
> On 2015/01/15 21:15:03, qyearsley wrote:
> > For both uses of JoinAsFilePath in this file, you could actually use
> > os.path.join instead. e.g.:
> > 
> > path = 'foo/bar.o'
> > dir = os.path.dirname(path)  # = 'foo'
> > file = os.path.dirname(path)  # = 'bar.o'
> > joined = os.path.join(dir, file)  # = 'foo/bar.o'
> > 
> > path = 'bar.o'
> > dir = os.path.dirname(path)  # = ''
> > file = os.path.dirname(path)  # = 'bar.o'
> > joined = os.path.join(dir, file)  # = 'bar.o'
> 
> Done.
> 
> Switch to os.path.join, but need to do a replace of os.sep with '/'.
> Because on Windows, os.path.join uses os.sep ('\\'), but git change logs
always
> use '/'.

Ah, very observant. Sounds good.

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
File appengine/findit/waterfall/extract_signal_pipeline.py (right):

https://codereview.chromium.org/838003004/diff/60001/appengine/findit/waterfa...
appengine/findit/waterfall/extract_signal_pipeline.py:72: master_name,
builder_name, step_name, None, stdio_log).ToJson()
On 2015/01/16 20:21:39, Shuotao wrote:
> On 2015/01/15 21:15:03, qyearsley wrote:
> > Looking at the FailureSignal.ToJson method[1] - it doesn't return JSON, it
> > returns a dict (which is JSON-serializable, but it is not a string that
> contains
> > JSON-formatted data). This is a bit misleading.
> > 
> > I think that method should be renamed to ToDict.
> > 
> > [1]
> >
>
https://code.google.com/p/chromium/codesearch#chromium/infra/appengine/findit...
> 
> Good point.
> But let's do the rename in a separate CL to avoid making this CL even larger.

SGTM

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/common...
File appengine/findit/common/git_repository.py (right):

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/common...
appengine/findit/common/git_repository.py:59: Some email is like:
person@@chromium.org@bbb929c8-8fbe-4397-9dbb-9b2b20218538.
In the test, you have an example that looks like
"person@@chromium.org@bbb929c8-8fbe..." (only one @ between email address
username and domain). Will there be one @ or two?

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
File appengine/findit/waterfall/build_failure_analysis.py (right):

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:14: """Guess if the two
files are the same.
The first line of the docstring should use third-person singular verb form (e.g.
guesses, not guess). Same for other docstrings in this file (and probably other
files).

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:33: 
Probably no need to have visual separation above and below file_dir = ...

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:42: _COMMON_SUFFIXES= [
Missing a space after "=".

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:51:
re.compile('.*(\_[^\_]*test)$'),
This also matches "foo_blahtest", which means the "foo.cc" and "foo_blahtest.cc"
are considered related. Is this intended? Maybe you could just add "test" and
"unittest" into the list of suffixes?

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:58: """Strip extension and
common postfixes from file name to guess correlation.
Strip -> Strips
Postfix -> Suffix

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:67: for postfix_patten in
_COMMON_SUFFIX_PATTERNS:
postfix_patten -> suffix_pattern

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:80: """Check if two files
are correlated.
Maybe just "related", rather than "correlated"?

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:102: class
_Justification(object):
What does this class contain/represent? Evidence for why a particular commit may
be a culprit? Also, what's a hint? A string describing a reason for suspecting a
CL, which may be shown to the user?

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:105: self.scores = 0
Using a plural word in a variable name usually suggests that it's a list (or
collection). I might name these "suspect_points" and "score".

Also, what's the difference between the suspect point number and the score?

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:106: self.hints = list()
Do these all need to be public? Also, you could use a literal [] instead of
list().

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:112: change_action (str):
The change type (modify/add/delete) of the file.
Saying what the arg type is is useful, but args sections usually aren't
formatted like this. Depending on how you word the arg descriptions, it may be
unnecessary to explicitly say what the types are. e.g.:

Args:
  change_action: Change type, one of the change types in common.diff.ChangeType.
  src_file_path: Changed file path in a CL.
  file_path: File path appearing in the failure log.
  suspect: Number of suspect points for the file change.
  score: Score number for the file change.

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:158: def
CheckFiles(failure_signal, change_log):
Non-trivial public functions (CheckFiles and AnalyzeBuildFailure) should have a
docstring describing arg types and return type.

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:209: justification =
CheckFiles(failure_signal, change_logs[revision])
Maybe justification_dict, since this (as is) is either None or a dict, but not a
_Justification object.

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
File appengine/findit/waterfall/identify_culprit_pipeline.py (right):

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/identify_culprit_pipeline.py:14: def run(self,
failure_info, change_logs, signals):
A docstring would be helpful here.

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
File appengine/findit/waterfall/pull_changelog_pipeline.py (right):

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/pull_changelog_pipeline.py:13: """ A pipeline to pull
change log of CLs.
Extra space after """

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/pull_changelog_pipeline.py:16: It is output of
pipeline DetectFirstFailurePipeline.
"It is" can be omitted, I think -- you could say:
Input:
  The output of DetectFirstFailurePipeline.

Output:
  A dictionary with the following form:
  ...

Again, it might be a bit clearer to put this in Args and Returns sections in the
docstring for run(). During execution, the output of the run() method will be
serialized as JSON, so the "output" of the pipeline class could be said to be a
dict or JSON with some data; but the return value of run() is definitely a dict.

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/pull_changelog_pipeline.py:19: It is a json like
below:
json -> dict

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
File appengine/findit/waterfall/test/build_failure_analysis_pipelines_test.py
(right):

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/test/build_failure_analysis_pipelines_test.py:27: def
testAnanlysIsNeededWhenBuildWasNeverAnalyzed(self):
Ananlys -> Analysis (also below)

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/test/build_failure_analysis_pipelines_test.py:41:
self._CreateAndSaveBuildAnanlysis(master_name, builder_name, build_number,
Ananlysis -> Analysis (also below)

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/test/build_failure_analysis_pipelines_test.py:54:
BuildAnalysisStatus.ANALYZED)
Style nit: Alignment.

[stgao, 2015-01-21 20:22:22]

Patchset #3 (id:140001) has been deleted

[stgao, 2015-01-21 20:29:23]

Thanks for the code review!

I've addressed the comments. Would you mind another review?

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/common...
File appengine/findit/common/git_repository.py (right):

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/common...
appengine/findit/common/git_repository.py:59: Some email is like:
person@@chromium.org@bbb929c8-8fbe-4397-9dbb-9b2b20218538.
On 2015/01/16 22:55:25, qyearsley wrote:
> In the test, you have an example that looks like
> "person@@chromium.org@bbb929c8-8fbe..." (only one @ between email address
> username and domain). Will there be one @ or two?

Good catch.
Typo was corrected.

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
File appengine/findit/waterfall/build_failure_analysis.py (right):

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:14: """Guess if the two
files are the same.
On 2015/01/16 22:55:26, qyearsley wrote:
> The first line of the docstring should use third-person singular verb form
(e.g.
> guesses, not guess). Same for other docstrings in this file (and probably
other
> files).

Done for files in this CL.
Will do a cleanup of other files in appengine/findit later in a separate CL.

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:33: 
On 2015/01/16 22:55:25, qyearsley wrote:
> Probably no need to have visual separation above and below file_dir = ...

Done.

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:42: _COMMON_SUFFIXES= [
On 2015/01/16 22:55:25, qyearsley wrote:
> Missing a space after "=".

Ooops, done.

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:51:
re.compile('.*(\_[^\_]*test)$'),
On 2015/01/16 22:55:25, qyearsley wrote:
> This also matches "foo_blahtest", which means the "foo.cc" and
"foo_blahtest.cc"
> are considered related. Is this intended? Maybe you could just add "test" and
> "unittest" into the list of suffixes?

OK. Restrict this pattern to a list of common suffixes for testing files as
added above. We could add more suffixes later if needed.

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:58: """Strip extension and
common postfixes from file name to guess correlation.
On 2015/01/16 22:55:26, qyearsley wrote:
> Strip -> Strips
> Postfix -> Suffix

Done.

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:67: for postfix_patten in
_COMMON_SUFFIX_PATTERNS:
On 2015/01/16 22:55:25, qyearsley wrote:
> postfix_patten -> suffix_pattern

Done.

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:80: """Check if two files
are correlated.
On 2015/01/16 22:55:25, qyearsley wrote:
> Maybe just "related", rather than "correlated"?

Done.

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:102: class
_Justification(object):
On 2015/01/16 22:55:25, qyearsley wrote:
> What does this class contain/represent? Evidence for why a particular commit
may
> be a culprit? Also, what's a hint? A string describing a reason for suspecting
a
> CL, which may be shown to the user?

Added a docstring to explain its usage, hints, and difference between suspect
points and score.

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:105: self.scores = 0
On 2015/01/16 22:55:26, qyearsley wrote:
> Using a plural word in a variable name usually suggests that it's a list (or
> collection). I might name these "suspect_points" and "score".
> 
> Also, what's the difference between the suspect point number and the score?

Done.

Explanation of the difference between the suspect points and the score is added
in the docstring.

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:106: self.hints = list()
On 2015/01/16 22:55:26, qyearsley wrote:
> Do these all need to be public? Also, you could use a literal [] instead of
> list().

Done.

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:112: change_action (str):
The change type (modify/add/delete) of the file.
On 2015/01/16 22:55:26, qyearsley wrote:
> Saying what the arg type is is useful, but args sections usually aren't
> formatted like this. Depending on how you word the arg descriptions, it may be
> unnecessary to explicitly say what the types are. e.g.:
> 
> Args:
>   change_action: Change type, one of the change types in
common.diff.ChangeType.
>   src_file_path: Changed file path in a CL.
>   file_path: File path appearing in the failure log.
>   suspect: Number of suspect points for the file change.
>   score: Score number for the file change.

I also get used to the docstring format as you suggested.
But the infra repo is moving toward ReST formatting which requires specifying
the type explictly.
So I still kept the type as is, but also updated the descriptions.
https://storage.googleapis.com/chromium-infra-docs/infra/html/documenting.html

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:158: def
CheckFiles(failure_signal, change_log):
On 2015/01/16 22:55:26, qyearsley wrote:
> Non-trivial public functions (CheckFiles and AnalyzeBuildFailure) should have
a
> docstring describing arg types and return type.

Done.

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:209: justification =
CheckFiles(failure_signal, change_logs[revision])
On 2015/01/16 22:55:26, qyearsley wrote:
> Maybe justification_dict, since this (as is) is either None or a dict, but not
a
> _Justification object.

Done.

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
File appengine/findit/waterfall/identify_culprit_pipeline.py (right):

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/identify_culprit_pipeline.py:14: def run(self,
failure_info, change_logs, signals):
On 2015/01/16 22:55:26, qyearsley wrote:
> A docstring would be helpful here.

Done.

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
File appengine/findit/waterfall/pull_changelog_pipeline.py (right):

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/pull_changelog_pipeline.py:13: """ A pipeline to pull
change log of CLs.
On 2015/01/16 22:55:26, qyearsley wrote:
> Extra space after """

Done.

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/pull_changelog_pipeline.py:16: It is output of
pipeline DetectFirstFailurePipeline.
On 2015/01/16 22:55:26, qyearsley wrote:
> "It is" can be omitted, I think -- you could say:
> Input:
>   The output of DetectFirstFailurePipeline.
> 
> Output:
>   A dictionary with the following form:
>   ...
> 
> Again, it might be a bit clearer to put this in Args and Returns sections in
the
> docstring for run(). During execution, the output of the run() method will be
> serialized as JSON, so the "output" of the pipeline class could be said to be
a
> dict or JSON with some data; but the return value of run() is definitely a
dict.

Done.

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/pull_changelog_pipeline.py:19: It is a json like
below:
On 2015/01/16 22:55:26, qyearsley wrote:
> json -> dict

Done.

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
File appengine/findit/waterfall/test/build_failure_analysis_pipelines_test.py
(right):

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/test/build_failure_analysis_pipelines_test.py:27: def
testAnanlysIsNeededWhenBuildWasNeverAnalyzed(self):
On 2015/01/16 22:55:26, qyearsley wrote:
> Ananlys -> Analysis (also below)

Ooops, good catch!
Done.

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/test/build_failure_analysis_pipelines_test.py:41:
self._CreateAndSaveBuildAnanlysis(master_name, builder_name, build_number,
On 2015/01/16 22:55:26, qyearsley wrote:
> Ananlysis -> Analysis (also below)

Done.

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/test/build_failure_analysis_pipelines_test.py:54:
BuildAnalysisStatus.ANALYZED)
On 2015/01/16 22:55:26, qyearsley wrote:
> Style nit: Alignment.

Done.

[stgao, 2015-01-22 02:23:37]

Patchset #4 (id:180001) has been deleted

[stgao, 2015-01-22 02:29:53]

Patchset #4 (id:200001) has been deleted

[stgao, 2015-01-22 02:33:09]

Patchset #4 (id:220001) has been deleted

[qyearsley, 2015-01-27 00:58:48]

lgtm

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
File appengine/findit/waterfall/build_failure_analysis.py (right):

https://codereview.chromium.org/838003004/diff/120001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:112: change_action (str):
The change type (modify/add/delete) of the file.
On 2015/01/21 20:29:22, Shuotao wrote:
> On 2015/01/16 22:55:26, qyearsley wrote:
> > Saying what the arg type is is useful, but args sections usually aren't
> > formatted like this. Depending on how you word the arg descriptions, it may
be
> > unnecessary to explicitly say what the types are. e.g.:
> > 
> > Args:
> >   change_action: Change type, one of the change types in
> common.diff.ChangeType.
> >   src_file_path: Changed file path in a CL.
> >   file_path: File path appearing in the failure log.
> >   suspect: Number of suspect points for the file change.
> >   score: Score number for the file change.
> 
> I also get used to the docstring format as you suggested.
> But the infra repo is moving toward ReST formatting which requires specifying
> the type explictly.
> So I still kept the type as is, but also updated the descriptions.
> https://storage.googleapis.com/chromium-infra-docs/infra/html/documenting.html

Alright, that looks good -- if it's in the style guide then it's right :-)

[stgao, 2015-01-27 02:06:37]

The CQ bit was checked by stgao@chromium.org

[commit-bot: I haz the power, 2015-01-27 02:07:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/838003004/260001

[commit-bot: I haz the power, 2015-01-27 02:10:13]

Committed patchset #5 (id:260001) as
https://chromium.googlesource.com/infra/infra/+/ed59b401885b3b9f67cd73ae18454...