Adding AvailabilityFinder to Doc Server

Adding AvailabilityFinder to Doc Server. 
This is a large part of automatically generating API intro tables.
This patch does not connect AvailabilityFinder with the rest of the server; a future patch will take care of that.

BUG=233968
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=208075

[epeterson, 2013-06-20 01:29:49]

Here's the AvailabilityFinder, along with its test and test data. It hasn't been
linked up with the rest of the server yet, and the test file is a little beefier
now.

[not at google - send to devlin, 2013-06-20 15:25:42]

add a BUG= plz

[not at google - send to devlin, 2013-06-20 16:15:20]

this looks fantastic, thanks.

https://codereview.chromium.org/17397010/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/availability_finder.py (right):

https://codereview.chromium.org/17397010/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/availability_finder.py:151: # otherwise a
numerical version will be passed in.
I see. Yeah special test-divergent logic is bad. It reduces test coverage - I
guess the availability tests never manage to trigger any of these code paths
then?

I think we'll need to add a way in TestBranchUtility to return numbers - which
means injecting a configuration into it. It would be a pretty simple mapping,
but it would let you test a whole bunch of new things. yay.

https://codereview.chromium.org/17397010/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/availability_finder.py:157: if version <
5:
"We don't have any data below version 5"?

https://codereview.chromium.org/17397010/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/availability_finder.py:207: break
maybe add this thing to branch_utility? channel =
self._branch_utility.GetChannelForVersion? There is already a
GetBranchForVersion. It makes sense to me.

https://codereview.chromium.org/17397010/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/availability_finder.py:225: if
api_info.get('channel') == _STABLE else None)
maybe save a reference to channel so that this can fit on 1 line. python ternary
syntax is confusing enough already.

https://codereview.chromium.org/17397010/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/availability_finder.py:235: if
(available_channel is not None and BranchUtility.NewestChannel(
would prefer a line break after the 'and'. Easier to read.

https://codereview.chromium.org/17397010/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/availability_finder_test.py (right):

https://codereview.chromium.org/17397010/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/availability_finder_test.py:23:
ConfigureFakeFetchers()
I know I keep on banging on about this, but unit tests shouldn't need to
ConfigureFakeFetchers, they should be able to inject their own dependencies.

https://codereview.chromium.org/17397010/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/availability_finder_test.py:173:
self._avail_ds.GetApiAvailability('events').version)
awesome tests!

https://codereview.chromium.org/17397010/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/test_data/canned_data.py (right):

https://codereview.chromium.org/17397010/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/test_data/canned_data.py:39:
'intro_tables.json': '{"test": [{"Permissions": "probably none"}]}'
is there some kind of JSON pretty printer you can put this through?

(and afterwards standardise on single quotes not double quotes)

[epeterson, 2013-06-20 16:41:34]

Just a quick note before I jump into these changes:

https://codereview.chromium.org/17397010/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/availability_finder.py (right):

https://codereview.chromium.org/17397010/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/availability_finder.py:151: # otherwise a
numerical version will be passed in.
On 2013/06/20 16:15:20, kalman wrote:
> I see. Yeah special test-divergent logic is bad. It reduces test coverage - I
> guess the availability tests never manage to trigger any of these code paths
> then?
> 
> I think we'll need to add a way in TestBranchUtility to return numbers - which
> means injecting a configuration into it. It would be a pretty simple mapping,
> but it would let you test a whole bunch of new things. yay.

Thanks for the review. I'll get right to work on fixing it all up.
I did want to clear this comment up first, though.

The code paths here in _FindEarliestStableAvailability are indeed run, since a
real BranchUtility is being used in the AvailabilityFinder test. Only
TestBranchUtility sends the 'funky' data - BranchUtility sending fake (but
validly-formatted) data from test_data/branch_utility/ works just fine here.

If TestBranchUtility is changed to return valid numerical versions, then it can
be used in this test in place of the regular BranchUtility, and the
test-divergent logic can be removed.

I can still write more tests for these code paths, if you'd like, but they are
being run right now. promise.

[epeterson, 2013-06-21 00:48:26]

A few small changes.
I spent a while trying to get canned_data nicely formatted.
If the changes to test_branch_utility aren't at all what you're looking for, I'd
appreciate it if you could give me some pointers on how to properly accomplish
your request to "inject a config."

https://codereview.chromium.org/17397010/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/availability_finder.py (right):

https://codereview.chromium.org/17397010/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/availability_finder.py:151: # otherwise a
numerical version will be passed in.
On 2013/06/20 16:41:34, epeterson wrote:
> On 2013/06/20 16:15:20, kalman wrote:
> > I see. Yeah special test-divergent logic is bad. It reduces test coverage -
I
> > guess the availability tests never manage to trigger any of these code paths
> > then?
> > 
> > I think we'll need to add a way in TestBranchUtility to return numbers -
which
> > means injecting a configuration into it. It would be a pretty simple
mapping,
> > but it would let you test a whole bunch of new things. yay.
> 
> Thanks for the review. I'll get right to work on fixing it all up.
> I did want to clear this comment up first, though.
> 
> The code paths here in _FindEarliestStableAvailability are indeed run, since a
> real BranchUtility is being used in the AvailabilityFinder test. Only
> TestBranchUtility sends the 'funky' data - BranchUtility sending fake (but
> validly-formatted) data from test_data/branch_utility/ works just fine here.
> 
> If TestBranchUtility is changed to return valid numerical versions, then it
can
> be used in this test in place of the regular BranchUtility, and the
> test-divergent logic can be removed.
> 
> I can still write more tests for these code paths, if you'd like, but they are
> being run right now. promise.
> 
test-divergent logic is gone.

https://codereview.chromium.org/17397010/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/availability_finder.py:157: if version <
5:
On 2013/06/20 16:15:20, kalman wrote:
> "We don't have any data below version 5"?

Done.

https://codereview.chromium.org/17397010/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/availability_finder.py:207: break
On 2013/06/20 16:15:20, kalman wrote:
> maybe add this thing to branch_utility? channel =
> self._branch_utility.GetChannelForVersion? There is already a
> GetBranchForVersion. It makes sense to me.

Done.

https://codereview.chromium.org/17397010/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/availability_finder.py:225: if
api_info.get('channel') == _STABLE else None)
On 2013/06/20 16:15:20, kalman wrote:
> maybe save a reference to channel so that this can fit on 1 line. python
ternary
> syntax is confusing enough already.

Done.

https://codereview.chromium.org/17397010/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/availability_finder.py:235: if
(available_channel is not None and BranchUtility.NewestChannel(
On 2013/06/20 16:15:20, kalman wrote:
> would prefer a line break after the 'and'. Easier to read.

Done.

https://codereview.chromium.org/17397010/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/availability_finder_test.py (right):

https://codereview.chromium.org/17397010/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/availability_finder_test.py:23:
ConfigureFakeFetchers()
On 2013/06/20 16:15:20, kalman wrote:
> I know I keep on banging on about this, but unit tests shouldn't need to
> ConfigureFakeFetchers, they should be able to inject their own dependencies.

Done.

https://codereview.chromium.org/17397010/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/test_data/canned_data.py (right):

https://codereview.chromium.org/17397010/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/test_data/canned_data.py:39:
'intro_tables.json': '{"test": [{"Permissions": "probably none"}]}'
On 2013/06/20 16:15:20, kalman wrote:
> is there some kind of JSON pretty printer you can put this through?
> 
> (and afterwards standardise on single quotes not double quotes)

I had some adventures with this change... But this is the end result.

https://codereview.chromium.org/17397010/diff/9001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/branch_utility.py (right):

https://codereview.chromium.org/17397010/diff/9001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/branch_utility.py:156: return
channel_info.channel
Here's the new GetChannelForVersion method, created from code in
AvailabilityFinder.

https://codereview.chromium.org/17397010/diff/9001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/test_branch_utility.py (right):

https://codereview.chromium.org/17397010/diff/9001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/test_branch_utility.py:26: return channel
Can we reuse dicts from canned_data? This way we don't have a bunch of inlined
data within TestBranchUtility, and the servlet tests also don't need to worry
about passing in some kind of config.

[not at google - send to devlin, 2013-06-21 00:54:11]

lgtm

https://codereview.chromium.org/17397010/diff/9001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/availability_finder.py (right):

https://codereview.chromium.org/17397010/diff/9001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/availability_finder.py:202: for
channel_info in self._branch_utility.GetAllChannelInfo():
forgot to delete this :)

https://codereview.chromium.org/17397010/diff/9001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/branch_utility.py (right):

https://codereview.chromium.org/17397010/diff/9001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/branch_utility.py:156: return
channel_info.channel
On 2013/06/21 00:48:26, epeterson wrote:
> Here's the new GetChannelForVersion method, created from code in
> AvailabilityFinder.

cool. no else after returns though. just if .. if...

https://codereview.chromium.org/17397010/diff/9001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/test_branch_utility.py (right):

https://codereview.chromium.org/17397010/diff/9001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/test_branch_utility.py:11: '''
almost what I meant. except, pass the branches and channels into a constructor
here, then whoever constructs the TestBranchUtility can pass in the canned data
if they wish. or, pass in their own data.

def __init__(self, branches, channels):
  self._branches = branches
  self._channels = channels

... etc

https://codereview.chromium.org/17397010/diff/9001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/test_branch_utility.py:26: return channel
On 2013/06/21 00:48:26, epeterson wrote:
> Can we reuse dicts from canned_data? This way we don't have a bunch of inlined
> data within TestBranchUtility, and the servlet tests also don't need to worry
> about passing in some kind of config.

answered above

[epeterson, 2013-06-22 01:42:40]

Some final changes. A config can now be passed into TestBranchUtility, but I
also created a public static method that callers can use to create a
TestBranchUtility with a canned_data config by default.

https://codereview.chromium.org/17397010/diff/9001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/availability_finder.py (right):

https://codereview.chromium.org/17397010/diff/9001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/availability_finder.py:202: for
channel_info in self._branch_utility.GetAllChannelInfo():
On 2013/06/21 00:54:11, kalman wrote:
> forgot to delete this :)

Oops.

https://codereview.chromium.org/17397010/diff/9001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/branch_utility.py (right):

https://codereview.chromium.org/17397010/diff/9001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/branch_utility.py:156: return
channel_info.channel
On 2013/06/21 00:54:11, kalman wrote:
> On 2013/06/21 00:48:26, epeterson wrote:
> > Here's the new GetChannelForVersion method, created from code in
> > AvailabilityFinder.
> 
> cool. no else after returns though. just if .. if...

Done.

https://codereview.chromium.org/17397010/diff/9001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/test_branch_utility.py (right):

https://codereview.chromium.org/17397010/diff/9001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/test_branch_utility.py:11: '''
On 2013/06/21 00:54:11, kalman wrote:
> almost what I meant. except, pass the branches and channels into a constructor
> here, then whoever constructs the TestBranchUtility can pass in the canned
data
> if they wish. or, pass in their own data.
> 
> def __init__(self, branches, channels):
>   self._branches = branches
>   self._channels = channels
> 
> ... etc

Done. I also made a method similar to BranchUtility's Create method, so that
things creating TestBranchUtilities (like the servlets) don't necessarily need
to include canned_data.py.

[commit-bot: I haz the power, 2013-06-22 01:46:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/evan.peterson.EP@gmail.com/17397010/21001

[commit-bot: I haz the power, 2013-06-22 02:40:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/evan.peterson.EP@gmail.com/17397010/21001

[commit-bot: I haz the power, 2013-06-22 03:05:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/evan.peterson.EP@gmail.com/17397010/21001

[commit-bot: I haz the power, 2013-06-22 03:33:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/evan.peterson.EP@gmail.com/17397010/21001

[commit-bot: I haz the power, 2013-06-22 04:43:05]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2013-06-22 15:34:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/evan.peterson.EP@gmail.com/17397010/21001

[commit-bot: I haz the power, 2013-06-22 17:18:57]

Change committed as 208075