Refactor stages 1, 2 and test_api overhaul.

go/bisectrefactor

Highlights:
 - revision_state no longer has a status property.
 - revision_state.start_job runs sequentially:
     ([request build, wait for it], test it)
 - removed lotsa legacy cruft.
 - hypothesis testing is offloaded to catapult.
    (https://codereview.chromium.org/2089833002/)
 - step_test_data for all bisect related steps is generated
     from simple descriptions of each revision's result. (See
     test_api.py and example.py

Committed: https://chromium.googlesource.com/chromium/tools/build/+/3d68b83bc307ab0f55225956ce533d18fce26bac

[RobertoCN, 2016-08-23 00:26:22]

robertocn@chromium.org changed reviewers:
+ dtu@chromium.org, eakuefner@chromium.org, prasadv@chromium.org

[RobertoCN, 2016-08-23 00:26:24]

Feel free to start reviewing this in depth. I still haven't covered 100% of the
lines with expectations, but I think this is mostly it.

Also, the issues that came up during the walkthrough might not be addressed yet,
but are on my radar and will be addressed on a subsequent patcheset to this CL

https://codereview.chromium.org/2247373002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/auto_bisect/bisector.py (right):

https://codereview.chromium.org/2247373002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/bisector.py:197: self.loopCHECK[key] +=
1
Remove loop check and debug prints

https://codereview.chromium.org/2247373002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/auto_bisect/local_bisect.py (right):

https://codereview.chromium.org/2247373002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/local_bisect.py:38: raise
Remove?

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

https://codereview.chromium.org/2247373002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:188: revision_a.mean =
result['sample_a']['mean']
Make mean and std_dev properties of revision_state and generate the values on
the fly

https://codereview.chromium.org/2247373002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:193: if result['result'] ==
'needMoreData':
Make 3 constants for true, false and needMore.

https://codereview.chromium.org/2247373002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:593: if
self.bypass_stats_check:
Remove this flag

[RobertoCN, 2016-09-07 00:33:25]

PTAL,

Ethan, this CL is quite big and we need help reviewing it. If you could take a
look, specially the parts that interact with the compare_samples function from
catapult, that'd be awesome.

https://codereview.chromium.org/2247373002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/auto_bisect/bisector.py (right):

https://codereview.chromium.org/2247373002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/bisector.py:197: self.loopCHECK[key] +=
1
On 2016/08/23 00:26:23, RobertoCN wrote:
> Remove loop check and debug prints

Done.

https://codereview.chromium.org/2247373002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/auto_bisect/local_bisect.py (right):

https://codereview.chromium.org/2247373002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/local_bisect.py:38: raise
On 2016/08/23 00:26:23, RobertoCN wrote:
> Remove?

Done.

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

https://codereview.chromium.org/2247373002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:188: revision_a.mean =
result['sample_a']['mean']
On 2016/08/23 00:26:24, RobertoCN wrote:
> Make mean and std_dev properties of revision_state and generate the values on
> the fly

Done.

https://codereview.chromium.org/2247373002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:193: if result['result'] ==
'needMoreData':
On 2016/08/23 00:26:24, RobertoCN wrote:
> Make 3 constants for true, false and needMore.

Done.

https://codereview.chromium.org/2247373002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:593: if
self.bypass_stats_check:
On 2016/08/23 00:26:24, RobertoCN wrote:
> Remove this flag

Acknowledged.

[RobertoCN, 2016-09-07 00:40:21]

Description was changed from

==========
WIP for partial review

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

to

==========
go/bisectrefactor

Highlights:
 - revision_state no longer has a status property.
 - revision_state.start_job runs sequentially:
     ([request build, wait for it], test it)
 - removed lotsa legacy cruft.
 - hypothesis testing is offloaded to catapult.
    (https://codereview.chromium.org/2089833002/)
 - step_test_data for all bisect related steps is generated
     from simple descriptions of each revision's result. (See
     test_api.py and example.py
==========

[prasadv, 2016-09-09 18:52:31]

Review not yet completed, some initial comments and clarification.

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

https://codereview.chromium.org/2247373002/diff/70001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/api.py:132:
self.resource('fetch_revision_info.py'),
Can we replace this with gitiles recipe_module?

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

https://codereview.chromium.org/2247373002/diff/70001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:167: Returns:
I think it is better to have a consistent datatype value to return. 
In this case if there might be a confusion to evaluate False and None.
May be we should use some kind of enum here.

Same for the "needMoreData"

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

https://codereview.chromium.org/2247373002/diff/80001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/api.py:171: def run_bisect_script(self,
**kwargs):  # pragma: no cover
Do we still want to support this? As far as I know all all bisects kicked off by
dashboard are recipe based.

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

https://codereview.chromium.org/2247373002/diff/80001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:64: SIGNIFICANTLY_DIFFERENT
= True
I think declaring enum here would be better.

https://codereview.chromium.org/2247373002/diff/80001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:181: output_format =
'valueset'
shouldn't we re-assign values_a and values_b here?

https://codereview.chromium.org/2247373002/diff/80001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:290: def
_gen_diff_patch(self, git_object_a, git_object_b, src_alias, dst_alias,
Do we still need this? Since we are using buildbucket properties to set the DEPS
revision, I'm wondering where else we are going to use this patch.

https://codereview.chromium.org/2247373002/diff/80001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/auto_bisect/exceptions.py (right):

https://codereview.chromium.org/2247373002/diff/80001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/exceptions.py:1: # Copyright 2016 The
Chromium Authors. All rights reserved.
By looking at the file I (may be other too) get confused with in-build exception
module, is it possible to rename filename as bisect_exceptions or something
similar?

[eakuefner, 2016-09-09 20:35:42]

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

https://codereview.chromium.org/2247373002/diff/80001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/api.py:104: def
gsutil_file_exists(self, path, **kwargs):
I don't like this idea of putting **kwargs everywhere -- do we have specific
motivation for threading particular arguments through? Can we just pick those
instead?

https://codereview.chromium.org/2247373002/diff/80001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/api.py:431: output_format: either
'chartjson' or 'valueset'
will 'buildbot' be supported?

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

https://codereview.chromium.org/2247373002/diff/80001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:49:
REGRESSION_CHECK_TIMEOUT = 2 * 60 * 60
Why? Comment?

https://codereview.chromium.org/2247373002/diff/80001/scripts/slave/recipes/b...
File scripts/slave/recipes/bisection/android_bisect.py (right):

https://codereview.chromium.org/2247373002/diff/80001/scripts/slave/recipes/b...
scripts/slave/recipes/bisection/android_bisect.py:397:
'page_cycler.intl_ar_fa_he',
strictly speaking, this should be page_cycler_v2 -- not only have the original
page_cycler benchmarks been deleted, they're not capable of producing
valueset-format output.

https://codereview.chromium.org/2247373002/diff/80001/scripts/slave/recipes/b...
scripts/slave/recipes/bisection/android_bisect.py:423:
'page_cycler.intl_ar_fa_he',
and here

https://codereview.chromium.org/2247373002/diff/80001/scripts/slave/recipes/b...
scripts/slave/recipes/bisection/android_bisect.py:659:
'page_cycler.intl_ar_fa_he',
and here

https://codereview.chromium.org/2247373002/diff/80001/scripts/slave/recipes/b...
File scripts/slave/recipes/bisection/desktop_bisect.py (left):

https://codereview.chromium.org/2247373002/diff/80001/scripts/slave/recipes/b...
scripts/slave/recipes/bisection/desktop_bisect.py:342: yield (
What happens to the test coverage that you're deleting here? Is it still covered
elsewhere?

[RobertoCN, 2016-09-13 22:11:41]

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

https://codereview.chromium.org/2247373002/diff/70001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/api.py:132:
self.resource('fetch_revision_info.py'),
On 2016/09/09 18:52:30, prasadv wrote:
> Can we replace this with gitiles recipe_module?

Yes we can, but we don't want to do it on this CL because that would require
that we modify the gitiles module to take step_test_data, adding yet another
file to the CL

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

https://codereview.chromium.org/2247373002/diff/70001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:167: Returns:
On 2016/09/09 18:52:30, prasadv wrote:
> I think it is better to have a consistent datatype value to return. 
> In this case if there might be a confusion to evaluate False and None.
> May be we should use some kind of enum here.
> 
> Same for the "needMoreData"

Done.

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

https://codereview.chromium.org/2247373002/diff/80001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/api.py:104: def
gsutil_file_exists(self, path, **kwargs):
On 2016/09/09 20:35:42, eakuefner wrote:
> I don't like this idea of putting **kwargs everywhere -- do we have specific
> motivation for threading particular arguments through? Can we just pick those
> instead?

I agree, but this is a pattern that is used all over recipes. I tried to thread
only step_test_data but the behavior resulting from that was unexpected. I think
it had something to do with `'step_test_data' not in kwargs` being different
from `step_test_data == None` or somesuch.

https://codereview.chromium.org/2247373002/diff/80001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/api.py:171: def run_bisect_script(self,
**kwargs):  # pragma: no cover
On 2016/09/09 18:52:30, prasadv wrote:
> Do we still want to support this? As far as I know all all bisects kicked off
by
> dashboard are recipe based.

Done.

https://codereview.chromium.org/2247373002/diff/80001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/api.py:431: output_format: either
'chartjson' or 'valueset'
On 2016/09/09 20:35:42, eakuefner wrote:
> will 'buildbot' be supported?

yeah, done.

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

https://codereview.chromium.org/2247373002/diff/80001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:49:
REGRESSION_CHECK_TIMEOUT = 2 * 60 * 60
On 2016/09/09 20:35:42, eakuefner wrote:
> Why? Comment?

I don't know. I'll set it back to what it was. A later CL that is unrelated to
the refactor should change this if needed.

https://codereview.chromium.org/2247373002/diff/80001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:64: SIGNIFICANTLY_DIFFERENT
= True
On 2016/09/09 18:52:30, prasadv wrote:
> I think declaring enum here would be better.

Done.

https://codereview.chromium.org/2247373002/diff/80001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:181: output_format =
'valueset'
On 2016/09/09 18:52:30, prasadv wrote:
> shouldn't we re-assign values_a and values_b here?
Nice catch!
Done.

https://codereview.chromium.org/2247373002/diff/80001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:290: def
_gen_diff_patch(self, git_object_a, git_object_b, src_alias, dst_alias,
On 2016/09/09 18:52:30, prasadv wrote:
> Do we still need this? Since we are using buildbucket properties to set the
DEPS
> revision, I'm wondering where else we are going to use this patch.

Removed it. Now, we'll hash self.revision_string() and use that.

https://codereview.chromium.org/2247373002/diff/80001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/auto_bisect/exceptions.py (right):

https://codereview.chromium.org/2247373002/diff/80001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/exceptions.py:1: # Copyright 2016 The
Chromium Authors. All rights reserved.
On 2016/09/09 18:52:30, prasadv wrote:
> By looking at the file I (may be other too) get confused with in-build
exception
> module, is it possible to rename filename as bisect_exceptions or something
> similar?

Done.

https://codereview.chromium.org/2247373002/diff/80001/scripts/slave/recipes/b...
File scripts/slave/recipes/bisection/android_bisect.py (right):

https://codereview.chromium.org/2247373002/diff/80001/scripts/slave/recipes/b...
scripts/slave/recipes/bisection/android_bisect.py:397:
'page_cycler.intl_ar_fa_he',
On 2016/09/09 20:35:42, eakuefner wrote:
> strictly speaking, this should be page_cycler_v2 -- not only have the original
> page_cycler benchmarks been deleted, they're not capable of producing
> valueset-format output.

Done.

https://codereview.chromium.org/2247373002/diff/80001/scripts/slave/recipes/b...
scripts/slave/recipes/bisection/android_bisect.py:423:
'page_cycler.intl_ar_fa_he',
On 2016/09/09 20:35:42, eakuefner wrote:
> and here

Done.

https://codereview.chromium.org/2247373002/diff/80001/scripts/slave/recipes/b...
scripts/slave/recipes/bisection/android_bisect.py:659:
'page_cycler.intl_ar_fa_he',
On 2016/09/09 20:35:42, eakuefner wrote:
> and here

Done.

https://codereview.chromium.org/2247373002/diff/80001/scripts/slave/recipes/b...
File scripts/slave/recipes/bisection/desktop_bisect.py (left):

https://codereview.chromium.org/2247373002/diff/80001/scripts/slave/recipes/b...
scripts/slave/recipes/bisection/desktop_bisect.py:342: yield (
On 2016/09/09 20:35:42, eakuefner wrote:
> What happens to the test coverage that you're deleting here? Is it still
covered
> elsewhere?

Yeah. I am overall reducing the amount of non-covered code because I adding
almost no new `# pragma: no cover`s and I am removing many. Also these tests are
being removed because either: the code that they tested is being removed, or the
way we generated test data was overly complicated.

[dtu, 2016-09-13 23:57:28]

Here's my comments for test_api.py!
Still have yet to look at the other files in the CL :)

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

https://codereview.chromium.org/2247373002/diff/90001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:258: step_test_data=lambda:
self.api._test_data['revision_list'][depot]
How do you decide whether to pass step_test_data here or in _revision_range()?
Sorry, I'm finding it confusing when step_test_data is passed here, but this is
not the function whose result is being mocked, it's actually "python
fetch_intervening_revisions.py". Same is true of gsutil_exists().

As an aside, a brief rant on recipes: I'm not liking how step_test_data works.
You want to mock out function X, but function X passes down step_test_data until
it reaches the subprocess being called. It means you have to dig down until you
reach the process that's being called to find out its type signature. That's
interesting if everything is an integration test, but I don't want to test the
gsutil API, I want to test bisect.

https://codereview.chromium.org/2247373002/diff/90001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/auto_bisect/revision_state.py (right):

https://codereview.chromium.org/2247373002/diff/90001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/revision_state.py:29:
NOT_SIGNIFICANTLY_DIFFERENT, SIGNIFICANTLY_DIFFERENT, NEED_MORE_DATA = range(3)
I think Nat or Tony? said that whenever you're going to use an enum, if it's not
performance-critical, you should use strings instead.

NOT_SIGNIFICANTLY_DIFFERENT = 'not significantly different'
SIGNIFICANTLY_DIFFERENT = 'significantly different'
NEED_MORE_DATA = 'need more data'

The reasons are that it's easier to print to a UI and easier to debug.

https://codereview.chromium.org/2247373002/diff/90001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/revision_state.py:254: else
collections.namedtuple('retcode_attr', ['retcode'])(0))
Maybe this is:

api._test_data.get('gsutil_exists', {}).get(self.commit_hash,
[collections.namedtuple('retcode_attr', ['retcode'])(0)]).pop()

https://codereview.chromium.org/2247373002/diff/90001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/revision_state.py:390: def
run_test_step_test_data():
Hmm, this feels out of place. Is there a way to put this in AutoBisectTestApi?

Taking this thought even further, if that's possible, maybe more or all of the
@mod_test_data could be rewritten as functions? Then you don't have to deal with
assigning them all to result/_test_data. E.g.:

# in test_api.py
def hash_cp_map(self, commit_position):
  for item in self._test_data.items:
    if item.get('commit_pos') == commit_position:
      return self.m.json.output_stream({'git_sha': item['hash']})

and

# in bisector.py
return self._api.m.crrev.to_commit_hash(
    commit_position,
    step_test_data=lambda: self.api.hash_cp_map(rev))

https://codereview.chromium.org/2247373002/diff/90001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/auto_bisect/test_api.py (right):

https://codereview.chromium.org/2247373002/diff/90001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/test_api.py:22: def
compare_samples_data(self, data, rev_a, rev_b):
This logic feels too complex for a test, and is in danger of just repeating the
code being tested.

Is there a way to mock out the results of this function? You don't have to
handle arbitrary values, just the values given by the tests. And the tests don't
have to cover arbitrary values either, just representative values of each
scenario.

https://codereview.chromium.org/2247373002/diff/90001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/test_api.py:73: if 'hash' in item and
'commit_pos' in item:
Is there ever a case where 'hash' (and/or 'commit_pos') is not specified? If
not, I'd remove the checks to simplify the code.

https://codereview.chromium.org/2247373002/diff/90001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/test_api.py:80: def
revision_data(items):
As far as I can tell, this is only used in one place, to pass 'parsed_values' to
compare_samples_data(). Maybe a function with a narrower scope would be one that
only returns 'parsed_values'? Another bold possibility: make
compare_samples_data() return results by looking up the commit hashes, and not
have values at all.

https://codereview.chromium.org/2247373002/diff/90001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/test_api.py:96: result['chromium'] =
result['chromium'][1:]
Special-casing chromium is weird. Should this apply to all depots?

https://codereview.chromium.org/2247373002/diff/90001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/test_api.py:111: result[item['hash']] =
self.m.raw_io.stream_output(git_output)
This line assumes 'hash' is in item, whereas the previous line 107 does not.

https://codereview.chromium.org/2247373002/diff/90001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/test_api.py:145: def
exists_result(exists=True):
Make protected or private.

style guide: Don't use @staticmethod.

https://codereview.chromium.org/2247373002/diff/90001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/test_api.py:148: return 'GS location
does not exist', 1, ''
Sorry, I can't figure out why this is the return value here. This is returned by
m.gsutil(['ls']) ?

https://codereview.chromium.org/2247373002/diff/90001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/test_api.py:156: for i in
item['gsutil_exists']]
Does it need to be a list? I don't see any case where it changes within a
revision.

https://codereview.chromium.org/2247373002/diff/90001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/test_api.py:165: data=v.get('stdout',
'text from actual benchmark, (ignored)'))
nit: Can you assign v.get(...) to a variable and reuse it in both places? Or
does that not work?

https://codereview.chromium.org/2247373002/diff/90001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/test_api.py:178: if 'cl_info' in item:
info = item.get('cl_info', {})

[RobertoCN, 2016-09-21 22:22:49]

Addressed comments.

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

https://codereview.chromium.org/2247373002/diff/90001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:258: step_test_data=lambda:
self.api._test_data['revision_list'][depot]
On 2016/09/13 23:57:27, dtu wrote:
> How do you decide whether to pass step_test_data here or in _revision_range()?
> Sorry, I'm finding it confusing when step_test_data is passed here, but this
is
> not the function whose result is being mocked, it's actually "python
> fetch_intervening_revisions.py". Same is true of gsutil_exists().
> 
> As an aside, a brief rant on recipes: I'm not liking how step_test_data works.
> You want to mock out function X, but function X passes down step_test_data
until
> it reaches the subprocess being called. It means you have to dig down until
you
> reach the process that's being called to find out its type signature. That's
> interesting if everything is an integration test, but I don't want to test the
> gsutil API, I want to test bisect.

I also do not like this. Ideally gsutil (for example) would have reasonable
defaults for test data and I wouldn't have to send it anything myself. There may
or may not be a test_api for gsutil but then I would have to add +
api.gsutil.exists(file_name) and so on for every file there is for each of the
tests yielded, or something like that, sorta like it was before when we used
step names to match results to step_data.

What I wanted most of all was that the list of revision descriptors passed to
auto bisect's test api would suffice to specify the whole bisect process so that
new tests were easily added and it was easy to follow the tests.

I wanted to specify the step_test_data in the topmost call because I wanted
whoever was following the code to have it handy without having to go deep at the
cost of making debugging a little more tedious, the guiding principle being that
we wanted bisect to be easier to hack on.

https://codereview.chromium.org/2247373002/diff/90001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/auto_bisect/revision_state.py (right):

https://codereview.chromium.org/2247373002/diff/90001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/revision_state.py:29:
NOT_SIGNIFICANTLY_DIFFERENT, SIGNIFICANTLY_DIFFERENT, NEED_MORE_DATA = range(3)
On 2016/09/13 23:57:27, dtu wrote:
> I think Nat or Tony? said that whenever you're going to use an enum, if it's
not
> performance-critical, you should use strings instead.
> 
> NOT_SIGNIFICANTLY_DIFFERENT = 'not significantly different'
> SIGNIFICANTLY_DIFFERENT = 'significantly different'
> NEED_MORE_DATA = 'need more data'
> 
> The reasons are that it's easier to print to a UI and easier to debug.

Done.

https://codereview.chromium.org/2247373002/diff/90001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/revision_state.py:254: else
collections.namedtuple('retcode_attr', ['retcode'])(0))
On 2016/09/13 23:57:27, dtu wrote:
> Maybe this is:
> 
> api._test_data.get('gsutil_exists', {}).get(self.commit_hash,
> [collections.namedtuple('retcode_attr', ['retcode'])(0)]).pop()

That wouldn't work because after popping the last one,
.get(self.commit_hash,...) will return [] instead of ...

I rewrote thus:
         step_test_data=lambda: (api._test_data.get('gsutil_exists', {}).get(
             self.commit_hash) or [collections.namedtuple(
                 'retcode_attr', ['retcode'])(0)]).pop())

https://codereview.chromium.org/2247373002/diff/90001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/revision_state.py:390: def
run_test_step_test_data():
On 2016/09/13 23:57:27, dtu wrote:
> Hmm, this feels out of place. Is there a way to put this in AutoBisectTestApi?
> 
> Taking this thought even further, if that's possible, maybe more or all of the
> @mod_test_data could be rewritten as functions? Then you don't have to deal
with
> assigning them all to result/_test_data. E.g.:
> 
> # in test_api.py
> def hash_cp_map(self, commit_position):
>   for item in self._test_data.items:
>     if item.get('commit_pos') == commit_position:
>       return self.m.json.output_stream({'git_sha': item['hash']})
> 
> and
> 
> # in bisector.py
> return self._api.m.crrev.to_commit_hash(
>     commit_position,
>     step_test_data=lambda: self.api.hash_cp_map(rev)

That makes sense. I'll give it a try in a separate patch/patchset.

https://codereview.chromium.org/2247373002/diff/90001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/auto_bisect/test_api.py (right):

https://codereview.chromium.org/2247373002/diff/90001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/test_api.py:22: def
compare_samples_data(self, data, rev_a, rev_b):
On 2016/09/13 23:57:28, dtu wrote:
> This logic feels too complex for a test, and is in danger of just repeating
the
> code being tested.
> 
> Is there a way to mock out the results of this function? You don't have to
> handle arbitrary values, just the values given by the tests. And the tests
don't
> have to cover arbitrary values either, just representative values of each
> scenario.

Simplified it a lot, PTAL. Now any revision that has 1 as one of its values is
considered pre-regression and any that has 7 is considered post-regression. With
this we can return all three cases and also cover retest scenarios (by not
having either 1 or 7 until later in the list)

https://codereview.chromium.org/2247373002/diff/90001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/test_api.py:73: if 'hash' in item and
'commit_pos' in item:
On 2016/09/13 23:57:28, dtu wrote:
> Is there ever a case where 'hash' (and/or 'commit_pos') is not specified? If
> not, I'd remove the checks to simplify the code.
'hash' will always be there, 'commit_pos' may not. Simplifying the check.

https://codereview.chromium.org/2247373002/diff/90001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/test_api.py:80: def
revision_data(items):
On 2016/09/13 23:57:28, dtu wrote:
> As far as I can tell, this is only used in one place, to pass 'parsed_values'
to
> compare_samples_data(). Maybe a function with a narrower scope would be one
that
> only returns 'parsed_values'? Another bold possibility: make
> compare_samples_data() return results by looking up the commit hashes, and not
> have values at all.

compare_sample_data cannot look up data on the config items directly. I will on
a later patch replace all mod_test_data with functions that operate directly on
list of dicts sent by the testcase via __call__, which will be the only contents
of _test_data.

https://codereview.chromium.org/2247373002/diff/90001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/test_api.py:96: result['chromium'] =
result['chromium'][1:]
On 2016/09/13 23:57:28, dtu wrote:
> Special-casing chromium is weird. Should this apply to all depots?

We special case the topmost repo because the last revision in the roll is
equivalent to the roll itself.

https://codereview.chromium.org/2247373002/diff/90001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/test_api.py:111: result[item['hash']] =
self.m.raw_io.stream_output(git_output)
On 2016/09/13 23:57:28, dtu wrote:
> This line assumes 'hash' is in item, whereas the previous line 107 does not.

Done.

https://codereview.chromium.org/2247373002/diff/90001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/test_api.py:145: def
exists_result(exists=True):
On 2016/09/13 23:57:28, dtu wrote:
> Make protected or private.
> 
> style guide: Don't use @staticmethod.

https://cs.chromium.org/search/?q=@staticmethod+file:%5Ebuild/scripts/slave/r...

This seems like an accepted pattern throughout. Does it make more sense to
receive self and do nothing with it?

https://codereview.chromium.org/2247373002/diff/90001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/test_api.py:148: return 'GS location
does not exist', 1, ''
On 2016/09/13 23:57:28, dtu wrote:
> Sorry, I can't figure out why this is the return value here. This is returned
by
> m.gsutil(['ls']) ?

the placeholder_step_data decorator converts this into step_test_data. I just
rewrote it to be more clear.

https://codereview.chromium.org/2247373002/diff/90001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/test_api.py:156: for i in
item['gsutil_exists']]
On 2016/09/13 23:57:28, dtu wrote:
> Does it need to be a list? I don't see any case where it changes within a
> revision.

We need to be able to return false for a number of times and then true to test
certain code paths.  It currently does change within a revision, because when
you pop out all Falses it defaults to True, see the code in
revision_state._is_build_archived(), it would be nice if that defaulting logic
was defined here rather than there, maybe when we change these to be functions.
I tried several things but the mod_test_data thing required that the results be
serializable, so I couldn't pass a function.

I thought having this as a list was more readable.

https://codereview.chromium.org/2247373002/diff/90001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/test_api.py:165: data=v.get('stdout',
'text from actual benchmark, (ignored)'))
On 2016/09/13 23:57:28, dtu wrote:
> nit: Can you assign v.get(...) to a variable and reuse it in both places? Or
> does that not work?

Done.

https://codereview.chromium.org/2247373002/diff/90001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/test_api.py:178: if 'cl_info' in item:
On 2016/09/13 23:57:28, dtu wrote:
> info = item.get('cl_info', {})

Done.

[RobertoCN, 2016-09-21 22:22:54]

Addressed comments.

[RobertoCN, 2016-10-12 17:49:08]

Dave, PTAL this is the CL we discussed yesterday.

[RobertoCN, 2016-10-12 17:49:08]

Dave, PTAL this is the CL we discussed yesterday.

[RobertoCN, 2016-10-17 18:26:17]

robertocn@chromium.org changed reviewers:
+ sullivan@chromium.org

[RobertoCN, 2016-10-17 18:26:20]

Guys, can I get a Rubberstamp LGTM? 

This is going to staging and not to production, so there'll be plenty of chance
to fix any qualms you may have about this code.

The purpose of this is to get end to end testing on real bots. A separate CL
would later push these changes to prod, and that would be a good place to ensure
all code changes meet the quality bar.

[sullivan, 2016-10-17 18:27:18]

lgtm

[RobertoCN, 2016-10-17 19:32:49]

The CQ bit was checked by robertocn@chromium.org

[RobertoCN, 2016-10-17 19:32:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
sullivan@chromium.org
Link to the patchset: https://codereview.chromium.org/2247373002/#ps130001
(title: "Rebase and retrain")

[commit-bot: I haz the power, 2016-10-17 19:32:56]

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-10-17 20:08:55]

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

[commit-bot: I haz the power, 2016-10-17 20:08:57]

Try jobs failed on following builders:
  Build Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/31ecf39e55684910)

[RobertoCN, 2016-10-17 23:47:12]

The CQ bit was checked by robertocn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-17 23:47:23]

Dry run: 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-10-18 00:27:08]

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

[commit-bot: I haz the power, 2016-10-18 00:27:10]

Dry run: Try jobs failed on following builders:
  Build Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/31eddc9419e88710)

[RobertoCN, 2016-10-18 07:17:07]

The CQ bit was checked by robertocn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 07:17:20]

Dry run: 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-10-18 07:27:09]

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

[commit-bot: I haz the power, 2016-10-18 07:27:11]

Dry run: This issue passed the CQ dry run.

[RobertoCN, 2016-10-18 15:39:42]

The CQ bit was checked by robertocn@chromium.org

[RobertoCN, 2016-10-18 15:39:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
sullivan@chromium.org
Link to the patchset: https://codereview.chromium.org/2247373002/#ps160001
(title: "Relative intra-module imports.")

[commit-bot: I haz the power, 2016-10-18 15:39:54]

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-10-18 16:22:04]

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

[commit-bot: I haz the power, 2016-10-18 16:22:07]

Try jobs failed on following builders:
  Build Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/31f14db8f514da10)

[RobertoCN, 2016-10-18 18:09:47]

robertocn@chromium.org changed reviewers:
+ sergeyberezin@chromium.org

[RobertoCN, 2016-10-18 18:09:49]

Sergey, we need an owners' stamp for chromium_tests/steps.py

Thanks!

[Sergey Berezin, 2016-10-18 18:24:34]

RS LGTM for chromium_tests/steps.py

[RobertoCN, 2016-10-18 18:32:00]

The CQ bit was checked by robertocn@chromium.org

[RobertoCN, 2016-10-18 18:44:37]

The CQ bit was unchecked by robertocn@chromium.org

[RobertoCN, 2016-10-18 18:44:42]

The CQ bit was checked by robertocn@chromium.org

[commit-bot: I haz the power, 2016-10-18 18:47:14]

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

[RobertoCN, 2016-10-18 18:49:03]

The CQ bit was checked by robertocn@chromium.org

[commit-bot: I haz the power, 2016-10-18 18:49:14]

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-10-18 18:59:54]

Description was changed from

==========
go/bisectrefactor

Highlights:
 - revision_state no longer has a status property.
 - revision_state.start_job runs sequentially:
     ([request build, wait for it], test it)
 - removed lotsa legacy cruft.
 - hypothesis testing is offloaded to catapult.
    (https://codereview.chromium.org/2089833002/)
 - step_test_data for all bisect related steps is generated
     from simple descriptions of each revision's result. (See
     test_api.py and example.py
==========

to

==========
go/bisectrefactor

Highlights:
 - revision_state no longer has a status property.
 - revision_state.start_job runs sequentially:
     ([request build, wait for it], test it)
 - removed lotsa legacy cruft.
 - hypothesis testing is offloaded to catapult.
    (https://codereview.chromium.org/2089833002/)
 - step_test_data for all bisect related steps is generated
     from simple descriptions of each revision's result. (See
     test_api.py and example.py

Committed:
https://chromium.googlesource.com/chromium/tools/build/+/3d68b83bc307ab0f5522...
==========

[commit-bot: I haz the power, 2016-10-18 18:59:56]

Committed patchset #14 (id:160001) as
https://chromium.googlesource.com/chromium/tools/build/+/3d68b83bc307ab0f5522...