Issue 940123005: Adding ability to bisect recipe to bisect into dependency repos.

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+5086 lines, -200 lines)			Patch
M	scripts/slave/recipe_modules/auto_bisect/__init__.py	View	1 2 3 4 5 6 7 8 9	1 chunk	+10 lines, -7 lines	0 comments	Download
M	scripts/slave/recipe_modules/auto_bisect/api.py	View	1 2 3 4 5 6 7 8	1 chunk	+10 lines, -3 lines	0 comments	Download
M	scripts/slave/recipe_modules/auto_bisect/bisector.py	View	1 2 3 4 5 6 7 8	5 chunks	+205 lines, -20 lines	0 comments	Download
M	scripts/slave/recipe_modules/auto_bisect/bisector_test.py	View	1 2 3 4 5 6 7 8	6 chunks	+110 lines, -102 lines	0 comments	Download
A	scripts/slave/recipe_modules/auto_bisect/depot_config.py	View	1 2	1 chunk	+60 lines, -0 lines	0 comments	Download
A	scripts/slave/recipe_modules/auto_bisect/example.py	View	1 2 3 4 5 6 7 8 9	1 chunk	+274 lines, -0 lines	0 comments	Download
A	scripts/slave/recipe_modules/auto_bisect/example.expected/bad_deps_syntax.json	View	1 2 3 4 5 6 7 8 9	1 chunk	+560 lines, -0 lines	0 comments	Download
A	scripts/slave/recipe_modules/auto_bisect/example.expected/basic.json	View	1 2 3 4 5 6 7 8 9	1 chunk	+670 lines, -0 lines	0 comments	Download
A	scripts/slave/recipe_modules/auto_bisect/example.expected/failed_git_hash_object.json	View	1 2 3 4 5 6 7 8 9	1 chunk	+612 lines, -0 lines	0 comments	Download
A	scripts/slave/recipe_modules/auto_bisect/example.expected/missing_deps_entry.json	View	1 2 3 4 5 6 7 8 9	1 chunk	+560 lines, -0 lines	0 comments	Download
A	scripts/slave/recipe_modules/auto_bisect/example.expected/missing_vars_entry.json	View	1 2 3 4 5 6 7 8 9	1 chunk	+571 lines, -0 lines	0 comments	Download
A	scripts/slave/recipe_modules/auto_bisect/example.expected/reversed_basic.json	View	1 2 3 4 5 6 7 8 9	1 chunk	+670 lines, -0 lines	0 comments	Download
M	scripts/slave/recipe_modules/auto_bisect/perf_revision_state.py	View	1 2 3 4 5 6 7 8 9	6 chunks	+45 lines, -10 lines	0 comments	Download
M	scripts/slave/recipe_modules/auto_bisect/revision_state.py	View	1 2 3 4 5 6	3 chunks	+118 lines, -21 lines	0 comments	Download
A	scripts/slave/recipe_modules/auto_bisect/test_results_cache.py	View	1 2 3 4 5 6 7	1 chunk	+53 lines, -0 lines	0 comments	Download
M	scripts/slave/recipes/bisect.py	View	1 2 3 4 5 6	1 chunk	+2 lines, -1 line	0 comments	Download
M	scripts/slave/recipes/bisect.expected/basic.json	View	1 2 3 4 5 6 7 8 9	6 chunks	+278 lines, -18 lines	0 comments	Download
M	scripts/slave/recipes/bisect.expected/encoded_config_test.json	View	1 2 3 4 5 6 7 8 9	6 chunks	+278 lines, -18 lines	0 comments	Download

Messages

Total messages: 22 (7 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

RobertoCN

This CL is getting big, I thought it would be a good idea to ask ...

5 years, 10 months ago (2015-02-21 00:05:06 UTC) #4

qyearsley

Some feedback ^_^ https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_modules/auto_bisect/bisector.py File scripts/slave/recipe_modules/auto_bisect/bisector.py (right): https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_modules/auto_bisect/bisector.py#newcode10 scripts/slave/recipe_modules/auto_bisect/bisector.py:10: DEPS_SHA_PATCH = """diff --git DEPS.sha DEPS.sha ...

5 years, 10 months ago (2015-02-22 20:37:55 UTC) #5

RobertoCN

Addressed first round of feedback. https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_modules/auto_bisect/bisector.py File scripts/slave/recipe_modules/auto_bisect/bisector.py (right): https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_modules/auto_bisect/bisector.py#newcode10 scripts/slave/recipe_modules/auto_bisect/bisector.py:10: DEPS_SHA_PATCH = """diff --git ...

5 years, 10 months ago (2015-02-24 20:01:14 UTC) #6

Addressed first round of feedback.

https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_mod...
File scripts/slave/recipe_modules/auto_bisect/bisector.py (right):

https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/auto_bisect/bisector.py:10: DEPS_SHA_PATCH =
"""diff --git DEPS.sha DEPS.sha
On 2015/02/22 20:37:54, qyearsley wrote:
> Would it make sense to mark this as private with a underscore prefix?

Done.

https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/auto_bisect/bisector.py:82: Returns: A string
containing a git diff.
On 2015/02/22 20:37:54, qyearsley wrote:
> Usually the description of the return value goes on the next line after
> "Returns:". Same below.

Done.

https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/auto_bisect/bisector.py:84: return '\n' +
DEPS_SHA_PATCH % {'deps_sha': deps_sha}
On 2015/02/22 20:37:54, qyearsley wrote:
> Maybe the newline should be part of DEPS_PATCH_SHA?

Done.

https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/auto_bisect/bisector.py:88: """Make a diff patch
that updates a specifc dependency revision.
On 2015/02/22 20:37:54, qyearsley wrote:
> specifc -> specific

Done.

https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/auto_bisect/bisector.py:91: base_revision
(RevisionState): The revision whose DEPS is to be patched.
On 2015/02/22 20:37:53, qyearsley wrote:
> Possible alternative phrasing:
> The revision for which the DEPS file is to be patched.

Done.

https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/auto_bisect/bisector.py:97: full text of such
modified DEPS, both as strings.
On 2015/02/22 20:37:53, qyearsley wrote:
> full text of the modified DEPS file, ...

Done.

https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/auto_bisect/bisector.py:105: re.MULTILINE)
On 2015/02/22 20:37:54, qyearsley wrote:
> This variable name could be changed -- maybe something like deps_item_pattern
or
> deps_item_regexp?

Done.

https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/auto_bisect/bisector.py:111: cmd = 'hash-object -t
blob -w --stdin'.split(' ')
On 2015/02/22 20:37:53, qyearsley wrote:
> 1. split() with no args would have the same effect here. This way is also OK.
> 2. It would be good to have an explanation of what it means to "intern" a
patch.

Leaving the .split(' ') as it is explicit.

Separated the intern file logic into its own private function with its own
docstring.

https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/auto_bisect/bisector.py:131: return patch_text,
patched_contents
On 2015/02/22 20:37:54, qyearsley wrote:
> I think that parts of this function could potentially be more understandable
if
> they were broken into individual functions with more comments. For example,
you
> could have functions like:
> _modify_deps (input: deps_var, new_commit_hash, original_contents; output:
> patched_contents)
> _intern_modified_deps (input: patched_contents, output: interned_deps_hash)
> _gen_diff_patch (input: base_revision, interned_deps_hash, patch_text).

Done.

https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/auto_bisect/bisector.py:151: """This method
populates the revisions attribute.
On 2015/02/22 20:37:53, qyearsley wrote:
> You could drop the words "This method" here.

Done.

https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/auto_bisect/bisector.py:153: After running method
self.revisions should contain all the revisions
On 2015/02/22 20:37:53, qyearsley wrote:
> -> After running this method, self.revisions should contain...

Done.

https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/auto_bisect/bisector.py:158: revision_to_expand: A
revision where there is a deps change.
On 2015/02/22 20:37:54, qyearsley wrote:
> Would it make sense to possibly have two separate functions, one that takes a
> revision_to_expand and inserts deps revisions, and one that just initially
> populates the revisions attribute?

Done.

https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/auto_bisect/bisector.py:168:
max_revision.read_deps()
On 2015/02/22 20:37:54, qyearsley wrote:
> What does read_deps() do here? It sets the deps attribute right?

Yes, added in-line comment.

https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/auto_bisect/bisector.py:171: if (deps_var in
min_revision.deps and deps_var in max_revision.deps):
On 2015/02/22 20:37:53, qyearsley wrote:
> Parentheses unnecessary.

Done.

https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/auto_bisect/bisector.py:185: warning_text = ("Could
not expand dependency revisions for " +
On 2015/02/22 20:37:53, qyearsley wrote:
> Single quotes.

Done.

https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_mod...
File scripts/slave/recipe_modules/auto_bisect/bisector_test.py (right):

https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/auto_bisect/bisector_test.py:246:
self.assertTrue(called_abort)
On 2015/02/22 20:37:55, qyearsley wrote:
> Would it be possible to do this without global variables?
> 
> One possibility if you want to avoid global variables would be to make
separate
> TestCase class and make them class variables (which could be reset to False in
> the setUp method). This would make it so that when reading this method, one
> doesn't have to consider other possible places where these variables could be
> used.

Done.

https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_mod...
File scripts/slave/recipe_modules/auto_bisect/depot_config.py (right):

https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/auto_bisect/depot_config.py:7: FILE_DEPS = 'DEPS'
On 2015/02/22 20:37:55, qyearsley wrote:
> Possible alternative names: DEPS_GIT_FILENAME, DEPS_FILENAME

Done.

https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/auto_bisect/depot_config.py:13: DEPOT_DEPS_NAME = {
On 2015/02/22 20:37:55, qyearsley wrote:
> Optional: describe the purpose of this dict.
> Also optional: Describe how "src", "recurse", "from", "viewvc", "platform",
and
> "deps_var" are used.

Acknowledged.

This is a copy-pasted version of the current code (bisect_perf_regression).
I plan to clean it up when I have a clearer idea of how the recursive deps
bisecting will work.

https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_mod...
File scripts/slave/recipe_modules/auto_bisect/example.py (right):

https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/auto_bisect/example.py:17: #  
api.chromium_tests.sync_and_configure_build
On 2015/02/22 20:37:55, qyearsley wrote:
> # This would ordinarily be done by
> # `api.chromium_tests.sync_and_configure_build`.
> 
> Is this saying that api.path['checkout'] would normally be set in
> api.chromium_tests.sync_and_configure_build?

Yes. Changing the comment to make it more clear.

https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/auto_bisect/example.py:20: #Create a bisector
On 2015/02/22 20:37:55, qyearsley wrote:
> Space after "#".

Done.

https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/auto_bisect/example.py:45: 'commit_pos': '314015',
On 2015/02/22 20:37:55, qyearsley wrote:
> Indentation alignment.

Done.

https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/auto_bisect/example.py:61: ]
On 2015/02/22 20:37:55, qyearsley wrote:
> Indentation alignment.

Done.

https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/auto_bisect/example.py:74:
('src/tools/perf/run_benchmark -v --browser=release smoothness.'
On 2015/02/22 20:37:55, qyearsley wrote:
> Indent 4 spaces.

Done.

https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_mod...
File scripts/slave/recipe_modules/auto_bisect/perf_revision_state.py (right):

https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/auto_bisect/perf_revision_state.py:50: def
_put_diff_file(self):
On 2015/02/22 20:37:55, qyearsley wrote:
> 1. This might be improved with a docstring or a different name. Would
> _write_deps_patch_file describe it? (I'm not sure.)
> 
> 2. General question: Do you think it's better to say "diff file" or "patch
> file"?
>   In favor of "diff file":
>     a) The man page for `patch` says "apply a diff file"
>     b) The suffix below is .diff.
>     c) It's the output of `diff` command.
>   In favor of "patch file":
>     a) RevisionState has a property called patch_file.
>     b) It can be thought of as "a patch".
>     c) "patch file" is a more common search query than "diff file", but "patch
> file" is more common in India and "diff file" is more common in the US :-/ 
>
http://www.google.com/trends/explore#q=%22diff%20file%22%2C%20%22patch%20file%22

Diff is the name of a tool to find and list differences between 2 files, also
the name of the produced output and its customary extension 'diff file/.diff'

Patch is the tool used to apply a transform to a file, typically a .diff file,
or the process of applying such transform, or the transform itself that is to be
applied.

I guess this is a hard question because our particular case is right were patch
and diff overlap. By diff'ing we generate a diff file that is used as a patch
file by the builder.

I guess it is slightly more correct to say diff if we are agnostic to the fact
that it will be used to modify a file. But if we see it as instructions to
modify a file regardless of how they were produced (think the DEPS.sha portion,
which is not created by diff'ing anything) then it makes a little more sense to
call it patch.

I guess the difference is more philosophical in nature than anything else.

I will rename the method for now, and aim for consistency in any future changes.

https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_mod...
File scripts/slave/recipe_modules/auto_bisect/revision_state.py (right):

https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/auto_bisect/revision_state.py:18: class
RevisionState(object):
On 2015/02/22 20:37:55, qyearsley wrote:
> The other files have two blank lines between import section and first class or
> function.

Done.

https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/auto_bisect/revision_state.py:27: - Creating a
revision state for a chromium revision plus a deps change.
On 2015/02/22 20:37:55, qyearsley wrote:
> I'm not sure, but for other people without context I think the phrase "deps
> change" might be unclear. Not sure if it would be more clear to say "...a
> chromium revision plus an explicitly specified revision of a dependency
> repository (a DEPS change)".

Done.

https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/auto_bisect/revision_state.py:147: 'Var': lambda _:
deps_data["vars"][_],
On 2015/02/22 20:37:55, qyearsley wrote:
> Single quotes.

Done.

https://codereview.chromium.org/940123005/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/auto_bisect/revision_state.py:165: rxp =
re.compile(".git@(?P<revision>[a-fA-F0-9]+)")
On 2015/02/22 20:37:55, qyearsley wrote:
> 1. Variable name could probably be improved.
> 2. Not sure if you want to use [a-fA-F0-9]{40} instead of [a-fA-F0-9]+ (are
all
> the commit hashes in deps full sha1 hashes?)
> 3. Single quotes.

Done.

RobertoCN

Could you please take a look? I have expanded the coverage of the new code ...

5 years, 10 months ago (2015-02-26 21:32:59 UTC) #7

qyearsley

Started to take a look, but I still don't really understand how example.py (or recipe ...

5 years, 9 months ago (2015-02-27 23:12:03 UTC) #8

RobertoCN

https://codereview.chromium.org/940123005/diff/60001/scripts/slave/recipe_modules/auto_bisect/bisector.py File scripts/slave/recipe_modules/auto_bisect/bisector.py (right): https://codereview.chromium.org/940123005/diff/60001/scripts/slave/recipe_modules/auto_bisect/bisector.py#newcode225 scripts/slave/recipe_modules/auto_bisect/bisector.py:225: min_revision.read_deps() # Parses DEPS file and sets the `.deps` ...

5 years, 9 months ago (2015-03-03 21:45:30 UTC) #9

RobertoCN

Bisect recipe works locally with DEPS (non recursive) please review.

5 years, 9 months ago (2015-03-10 17:02:34 UTC) #11

qyearsley

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_modules/auto_bisect/api.py File scripts/slave/recipe_modules/auto_bisect/api.py (right): https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_modules/auto_bisect/api.py#newcode24 scripts/slave/recipe_modules/auto_bisect/api.py:24: BOT_EMAIL = 'chrome_bot@chromium.org' Note: Just looking at the names, ...

5 years, 9 months ago (2015-03-10 23:20:49 UTC) #12

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/auto_bisect/api.py (right):

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/api.py:24: BOT_EMAIL =
'chrome_bot@chromium.org'
Note: Just looking at the names, I can't tell what all of these things are used
for. In this case, I think it may be a good idea to give individual
documentation for most of the constants.

My guesses about their purpose:

POLLING_INTERVAL: Interval in seconds for checking whether there are new test
results from a tester?
BUCKET, RESULTS_GS_DIR, GS_RESULTS_URL: Location of test results after tester
finishes?
SVN_REPO_URL: SVN repo for git try used when triggering tester?
BOT_EMAIL: ?

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/api.py:32: bisect_config_dict =
bisect_config
Could you just call the argument bisect_config_dict and remove this line?

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/auto_bisect/bisector.py (right):

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:37:
'dummy_regression_confidence', None)
The default value None is redundant if bisect_config is a dict, since None is
the default default.

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:81: deps_sha (str): Hash of
the diff that patches DEPS.
Specifically, a SHA1 hash of the diff that patches DEPS.
Or even more specifically, the hex digest of the SHA1 hash of ...

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:96: Returns:
Blank line between the Args section and Returns section.

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:123: the resulting diff
patch.
Optional: You could also put each of these on their own line, e.g.:

git_object_a (str): A tree-ish git object identifier.
git_object_b (str): Another tree-ish git object identifier.
src_alias (str): Label to replace the identifier for the first file of the first
tree-ish git object (?)
dst_alias (str): ...

I'm not sure what pylint says about it.

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:131: cmd = 'diff %s %s
--src-prefix=IAMSRC: --dst-prefix=IAMDST:'
Possible explanation comment that could be added:
What do --src-prefix and --dst-prefix do?

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/auto_bisect/bisector_test.py (right):

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector_test.py:38: class
BisectorTest(unittest.TestCase): # pragma: no cover
Nit: Two spaces before #.

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector_test.py:84: new_bisector =
Bisector(self.dummy_api, self.bisect_config,
If new_bisector is renamed to bisector, then MockRevisionClass fits on this
line. Same for all the lines below, I think.

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector_test.py:204: class
BisectorAbortTest(unittest.TestCase): # pragma: no cover
Nit: Two spaces before #.
Also, it seems strange that a test case class would need a "no cover" pragma.
All of these lines should be run when "the tests" are run. Are these tests not
included when calculating the coverage?

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector_test.py:270: #
TODO(robertocn): Add test for bisector.check_bisect_finished.
Probably want two blank lines between the test case class above and the next
non-blank line.

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/auto_bisect/example.py (right):

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/example.py:47: def GenTests(api):
Optional: You could add notes about when GenSteps and GenTests are usually
called, for the benefit of people that are reading this without much knowledge
of any of the other recipe modules.

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/example.py:211: test_results =
revision_data['test_results']
You don't have to do anything about this comment, but since I wrote it out I'll
keep it here:

One alternative to having lists of dicts that have particular field names is to
have named tuples. Named tuples are lightweight immutable objects with
particular attributes; my readability reviewer suggested I do this for the
"anomaly dicts" that were returned by FindAnomalies on the dashboard, and the
result was defining a class like this:

ChangePoint = collections.namedtuple(
    'ChangePoint', (
        # The x-value of the first point after a step.
        'x_value',
        # Median of the segments before and after the change point.
        'median_before', 'median_after',
        # Number of points before and after the change point.
        'size_before', 'size_after',
        # X-values of the first and last point in the series window used.
        'window_start', 'window_end',
        # Relative change from before to after.
        'relative_change',
        # Standard deviation of points before.
        'std_dev_before',
        # Results of the Welch's t-test for values before and after.
        't_statistic', 'degrees_of_freedom', 'p_value'
    ))

And then FindAnomalies returned objects of type ChangePoint. I think the main
advantage of doing this is that instead of saying that this function takes a
"revision data dict" with fields "commit_pos", "hash", etc., you could say, for
example, that it takes a list of RevisionInfo objects or something.

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/auto_bisect/perf_revision_state.py (right):

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/perf_revision_state.py:58: file_name,
self.deps_patch + self.deps_sha_patch)
Nit: Alignment.

RobertoCN

Does this LGTY now? https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_modules/auto_bisect/api.py File scripts/slave/recipe_modules/auto_bisect/api.py (right): https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_modules/auto_bisect/api.py#newcode24 scripts/slave/recipe_modules/auto_bisect/api.py:24: BOT_EMAIL = 'chrome_bot@chromium.org' On 2015/03/10 ...

5 years, 9 months ago (2015-03-13 20:55:59 UTC) #14

Does this LGTY now?

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/auto_bisect/api.py (right):

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/api.py:24: BOT_EMAIL =
'chrome_bot@chromium.org'
On 2015/03/10 23:20:48, qyearsley wrote:
> Note: Just looking at the names, I can't tell what all of these things are
used
> for. In this case, I think it may be a good idea to give individual
> documentation for most of the constants.
> 
> My guesses about their purpose:
> 
> POLLING_INTERVAL: Interval in seconds for checking whether there are new test
> results from a tester?
> BUCKET, RESULTS_GS_DIR, GS_RESULTS_URL: Location of test results after tester
> finishes?
> SVN_REPO_URL: SVN repo for git try used when triggering tester?
> BOT_EMAIL: ?

Done.

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/api.py:32: bisect_config_dict =
bisect_config
On 2015/03/10 23:20:48, qyearsley wrote:
> Could you just call the argument bisect_config_dict and remove this line?

Done.

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/auto_bisect/bisector.py (right):

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:37:
'dummy_regression_confidence', None)
On 2015/03/10 23:20:48, qyearsley wrote:
> The default value None is redundant if bisect_config is a dict, since None is
> the default default.

Done.

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:81: deps_sha (str): Hash of
the diff that patches DEPS.
On 2015/03/10 23:20:48, qyearsley wrote:
> Specifically, a SHA1 hash of the diff that patches DEPS.
> Or even more specifically, the hex digest of the SHA1 hash of ...

Done.

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:96: Returns:
On 2015/03/10 23:20:48, qyearsley wrote:
> Blank line between the Args section and Returns section.

Done.

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:123: the resulting diff
patch.
On 2015/03/10 23:20:48, qyearsley wrote:
> Optional: You could also put each of these on their own line, e.g.:
> 
> git_object_a (str): A tree-ish git object identifier.
> git_object_b (str): Another tree-ish git object identifier.
> src_alias (str): Label to replace the identifier for the first file of the
first
> tree-ish git object (?)
> dst_alias (str): ...
> 
> I'm not sure what pylint says about it.

Done.

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:131: cmd = 'diff %s %s
--src-prefix=IAMSRC: --dst-prefix=IAMDST:'
On 2015/03/10 23:20:48, qyearsley wrote:
> Possible explanation comment that could be added:
> What do --src-prefix and --dst-prefix do?

Done.

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/auto_bisect/bisector_test.py (right):

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector_test.py:38: class
BisectorTest(unittest.TestCase): # pragma: no cover
On 2015/03/10 23:20:48, qyearsley wrote:
> Nit: Two spaces before #.

Done.

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector_test.py:84: new_bisector =
Bisector(self.dummy_api, self.bisect_config,
On 2015/03/10 23:20:48, qyearsley wrote:
> If new_bisector is renamed to bisector, then MockRevisionClass fits on this
> line. Same for all the lines below, I think.

Done.

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector_test.py:204: class
BisectorAbortTest(unittest.TestCase): # pragma: no cover
On 2015/03/10 23:20:48, qyearsley wrote:
> Nit: Two spaces before #.
> Also, it seems strange that a test case class would need a "no cover" pragma.
> All of these lines should be run when "the tests" are run. Are these tests not
> included when calculating the coverage?

These tests are not covered when calculating coverage. Presubmit and recipe
simulation test work specifically by ensuring that GenTests for each recipe
cover the code. It would be odd if GenTests somehow triggered these unittests.

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector_test.py:270: #
TODO(robertocn): Add test for bisector.check_bisect_finished.
On 2015/03/10 23:20:48, qyearsley wrote:
> Probably want two blank lines between the test case class above and the next
> non-blank line.

Done.

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/auto_bisect/example.py (right):

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/example.py:47: def GenTests(api):
On 2015/03/10 23:20:49, qyearsley wrote:
> Optional: You could add notes about when GenSteps and GenTests are usually
> called, for the benefit of people that are reading this without much knowledge
> of any of the other recipe modules.

Done.

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/example.py:211: test_results =
revision_data['test_results']
On 2015/03/10 23:20:49, qyearsley wrote:
> You don't have to do anything about this comment, but since I wrote it out
I'll
> keep it here:
> 
> One alternative to having lists of dicts that have particular field names is
to
> have named tuples. Named tuples are lightweight immutable objects with
> particular attributes; my readability reviewer suggested I do this for the
> "anomaly dicts" that were returned by FindAnomalies on the dashboard, and the
> result was defining a class like this:
> 
> ChangePoint = collections.namedtuple(
>     'ChangePoint', (
>         # The x-value of the first point after a step.
>         'x_value',
>         # Median of the segments before and after the change point.
>         'median_before', 'median_after',
>         # Number of points before and after the change point.
>         'size_before', 'size_after',
>         # X-values of the first and last point in the series window used.
>         'window_start', 'window_end',
>         # Relative change from before to after.
>         'relative_change',
>         # Standard deviation of points before.
>         'std_dev_before',
>         # Results of the Welch's t-test for values before and after.
>         't_statistic', 'degrees_of_freedom', 'p_value'
>     ))
> 
> And then FindAnomalies returned objects of type ChangePoint. I think the main
> advantage of doing this is that instead of saying that this function takes a
> "revision data dict" with fields "commit_pos", "hash", etc., you could say,
for
> example, that it takes a list of RevisionInfo objects or something.

Acknowledged.

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/auto_bisect/perf_revision_state.py (right):

https://codereview.chromium.org/940123005/diff/140001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/perf_revision_state.py:58: file_name,
self.deps_patch + self.deps_sha_patch)
On 2015/03/10 23:20:49, qyearsley wrote:
> Nit: Alignment.

Done.

qyearsley

Yep, LGTM, now pending review of Prasad & Robbie :-) https://codereview.chromium.org/940123005/diff/160001/scripts/slave/recipe_modules/auto_bisect/api.py File scripts/slave/recipe_modules/auto_bisect/api.py (right): https://codereview.chromium.org/940123005/diff/160001/scripts/slave/recipe_modules/auto_bisect/api.py#newcode30 ...

5 years, 9 months ago (2015-03-13 23:38:59 UTC) #15

iannucci

lgtm w/ some high level comments for now or later :) https://chromiumcodereview.appspot.com/940123005/diff/60001/scripts/slave/recipe_modules/auto_bisect/example.py File scripts/slave/recipe_modules/auto_bisect/example.py (right): ...

5 years, 9 months ago (2015-03-18 23:43:28 UTC) #16

lgtm w/ some high level comments for now or later :)

https://chromiumcodereview.appspot.com/940123005/diff/60001/scripts/slave/rec...
File scripts/slave/recipe_modules/auto_bisect/example.py (right):

https://chromiumcodereview.appspot.com/940123005/diff/60001/scripts/slave/rec...
scripts/slave/recipe_modules/auto_bisect/example.py:99: basic_test += step_data
basic_test += sum(_get_step_data_for_revision(api, revision_data))

probably works too, I think.

Not sure that _get_step_data_for_revision needs to be a generator.

https://chromiumcodereview.appspot.com/940123005/diff/160001/scripts/slave/re...
File scripts/slave/recipe_modules/auto_bisect/example.py (right):

https://chromiumcodereview.appspot.com/940123005/diff/160001/scripts/slave/re...
scripts/slave/recipe_modules/auto_bisect/example.py:39:
bisector.bad_rev.start_job()
potentially combine 'good_rev' and 'last_known_good_rev' into the same api (same
for bad_rev), so that the bisector API can be a bit smaller.

https://chromiumcodereview.appspot.com/940123005/diff/160001/scripts/slave/re...
scripts/slave/recipe_modules/auto_bisect/example.py:42: assert
bisector.check_improvement_direction()
lets maybe do assert_improvement_direction, which throws a structured exception
about WHY it failed? same for the next line. Otherwise you'll just get a purple
build with an unhelpful 'AssertionError' stack trace.

You may also want the exception to derive from StepFailure, so that the build
goes RED instead of purple, and then you won't get a stack trace either (which
is probably not as helpful as the data you put into the custom failure
exception)

https://chromiumcodereview.appspot.com/940123005/diff/160001/scripts/slave/re...
scripts/slave/recipe_modules/auto_bisect/example.py:47:
bisector.wait_for_any(revisions_to_check)
idea: have wait_for_any() set step text for which revisions it waited for, and
which one it eventually got. May need to have underlying python script pass this
data back as json so the recipe can populate.

potential bug/feature request (against recipes): Allow the step_text to be
modified BEFORE the actual step executes. Since you know which revisions you're
waiting for ahead of time, there's no reason to wait for the step to return
these before displaying them.

https://chromiumcodereview.appspot.com/940123005/diff/160001/scripts/slave/re...
scripts/slave/recipe_modules/auto_bisect/example.py:48:
bisector.check_bisect_finished(revisions_to_check[0])
let's rephrase this example as a loop which is closer to real-life usage

https://chromiumcodereview.appspot.com/940123005/diff/160001/scripts/slave/re...
scripts/slave/recipe_modules/auto_bisect/example.py:51: revisions_to_check =
bisector.get_revisions_to_eval(2)
let's comment on the `2` here, since we did `1` earlier.

On that note... why not have the Nsection factor be part of the bisector object
at construction time?

Also, could we merge this and 'check_bisect_finished'? They seem to have
overlapping logic. If our revisions_to_check is empty, then it would mean we're
done?

We'd probably can have wait_for_any directly update the bisector state instead
of having the caller pass the revision data around on the outside of the class,
too.

https://chromiumcodereview.appspot.com/940123005/diff/160001/scripts/slave/re...
scripts/slave/recipe_modules/auto_bisect/example.py:53:
revisions_to_check[0].start_job()
could have revisions_to_check be a RevisionList object which is-a list, but also
has a .start_all() method

https://chromiumcodereview.appspot.com/940123005/diff/160001/scripts/slave/re...
scripts/slave/recipe_modules/auto_bisect/example.py:54:
revisions_to_check[0].read_deps()  # Only added for coverage.
Later TODO: make gclient have an API to do this parsing for us (if it doesn't
already)

robertocn1

Added bugs with the Cr-Tests-AutoBisect-RecipeMigration label for these new TODO's as well as #TODO comments ...

5 years, 9 months ago (2015-03-19 21:48:24 UTC) #18

Added bugs with the Cr-Tests-AutoBisect-RecipeMigration label for these new
TODO's as well as #TODO comments in the code.

https://chromiumcodereview.appspot.com/940123005/diff/60001/scripts/slave/rec...
File scripts/slave/recipe_modules/auto_bisect/example.py (right):

https://chromiumcodereview.appspot.com/940123005/diff/60001/scripts/slave/rec...
scripts/slave/recipe_modules/auto_bisect/example.py:99: basic_test += step_data
On 2015/03/18 23:43:28, iannucci wrote:
> basic_test += sum(_get_step_data_for_revision(api, revision_data))
> 
> probably works too, I think.
> 
> Not sure that _get_step_data_for_revision needs to be a generator.

Done.

https://chromiumcodereview.appspot.com/940123005/diff/160001/scripts/slave/re...
File scripts/slave/recipe_modules/auto_bisect/example.py (right):

https://chromiumcodereview.appspot.com/940123005/diff/160001/scripts/slave/re...
scripts/slave/recipe_modules/auto_bisect/example.py:29: # More information is
available in scripts/slave/README.recipes.md
On 2015/03/13 23:38:59, qyearsley wrote:
> Thanks for adding this :-)

Acknowledged.

https://chromiumcodereview.appspot.com/940123005/diff/160001/scripts/slave/re...
scripts/slave/recipe_modules/auto_bisect/example.py:39:
bisector.bad_rev.start_job()
On 2015/03/18 23:43:28, iannucci wrote:
> potentially combine 'good_rev' and 'last_known_good_rev' into the same api
(same
> for bad_rev), so that the bisector API can be a bit smaller.

Acknowledged.

https://chromiumcodereview.appspot.com/940123005/diff/160001/scripts/slave/re...
scripts/slave/recipe_modules/auto_bisect/example.py:42: assert
bisector.check_improvement_direction()
On 2015/03/18 23:43:28, iannucci wrote:
> lets maybe do assert_improvement_direction, which throws a structured
exception
> about WHY it failed? same for the next line. Otherwise you'll just get a
purple
> build with an unhelpful 'AssertionError' stack trace.
> 
> You may also want the exception to derive from StepFailure, so that the build
> goes RED instead of purple, and then you won't get a stack trace either (which
> is probably not as helpful as the data you put into the custom failure
> exception)

Acknowledged.

https://chromiumcodereview.appspot.com/940123005/diff/160001/scripts/slave/re...
scripts/slave/recipe_modules/auto_bisect/example.py:47:
bisector.wait_for_any(revisions_to_check)
On 2015/03/18 23:43:28, iannucci wrote:
> idea: have wait_for_any() set step text for which revisions it waited for, and
> which one it eventually got. May need to have underlying python script pass
this
> data back as json so the recipe can populate.
> 
> potential bug/feature request (against recipes): Allow the step_text to be
> modified BEFORE the actual step executes. Since you know which revisions
you're
> waiting for ahead of time, there's no reason to wait for the step to return
> these before displaying them.

Acknowledged.

https://chromiumcodereview.appspot.com/940123005/diff/160001/scripts/slave/re...
scripts/slave/recipe_modules/auto_bisect/example.py:48:
bisector.check_bisect_finished(revisions_to_check[0])
On 2015/03/18 23:43:28, iannucci wrote:
> let's rephrase this example as a loop which is closer to real-life usage

Done.

https://chromiumcodereview.appspot.com/940123005/diff/160001/scripts/slave/re...
scripts/slave/recipe_modules/auto_bisect/example.py:51: revisions_to_check =
bisector.get_revisions_to_eval(2)
On 2015/03/18 23:43:28, iannucci wrote:
> let's comment on the `2` here, since we did `1` earlier.
> 
> On that note... why not have the Nsection factor be part of the bisector
object
> at construction time?
> 
> Also, could we merge this and 'check_bisect_finished'? They seem to have
> overlapping logic. If our revisions_to_check is empty, then it would mean
we're
> done?
> 
> We'd probably can have wait_for_any directly update the bisector state instead
> of having the caller pass the revision data around on the outside of the
class,
> too.

Acknowledged.

https://chromiumcodereview.appspot.com/940123005/diff/160001/scripts/slave/re...
scripts/slave/recipe_modules/auto_bisect/example.py:53:
revisions_to_check[0].start_job()
On 2015/03/18 23:43:28, iannucci wrote:
> could have revisions_to_check be a RevisionList object which is-a list, but
also
> has a .start_all() method

Acknowledged.

https://chromiumcodereview.appspot.com/940123005/diff/160001/scripts/slave/re...
scripts/slave/recipe_modules/auto_bisect/example.py:54:
revisions_to_check[0].read_deps()  # Only added for coverage.
On 2015/03/18 23:43:28, iannucci wrote:
> Later TODO: make gclient have an API to do this parsing for us (if it doesn't
> already)

Acknowledged.

commit-bot: I haz the power

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

5 years, 9 months ago (2015-03-19 21:48:41 UTC) #21

commit-bot: I haz the power

5 years, 9 months ago (2015-03-19 21:53:11 UTC) #22

Message was sent while issue was closed.

Committed patchset #10 (id:180001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=294505

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages