> On March 21, 2014, 11:53 p.m., Bill Farner wrote:
> > src/test/python/apache/aurora/client/cli/test_quota.py, line 47
> > <https://reviews.apache.org/r/19509/diff/2/?file=532010#file532010line47>
> >
> >     > Currently, the mocked calls can't detect renamed and missing thrift 
> > structs.
> >     
> >     This is one unfortunate aspect of the thrift API, you can set arbitrary 
> > attributes.  However, you can harden the tests a bit more if you use the 
> > constructors.  These would trip up on a refactor.  I find the python repl 
> > in pants helpful to see the APIs (see below).  I suggest taking the route 
> > of using constructors everywhere, probably introducing a helper method or 
> > two to cut down on the verbosity.
> >     
> >     $ ./pants py src/main/thrift/org/apache/aurora/gen:py-thrift-packaged
> >     Python 2.6.8 (unknown, Aug 25 2013, 00:04:29)
> >     [GCC 4.2.1 Compatible Apple LLVM 5.0 (clang-500.0.68)] on darwin
> >     Type "help", "copyright", "credits" or "license" for more information.
> >     (InteractiveConsole)
> >     >>> from gen.apache.aurora.ttypes import *
> >     >>> help(GetQuotaResult)
> >     
> >     Help on class GetQuotaResult in module gen.apache.aurora.ttypes:
> >     
> >     class GetQuotaResult(__builtin__.object)
> >      |  Attributes:
> >      |   - quota
> >      |   - prodConsumption
> >      |   - nonProdConsumption
> >      |
> >      |  Methods defined here:
> >      |
> >      |  __eq__(self, other)
> >      |
> >      |  __init__(self, quota=None, prodConsumption=None, 
> > nonProdConsumption=None)

Agreed that a constructing an object using the constructor is the best way 
here. Updated the code. 


> On March 21, 2014, 11:53 p.m., Bill Farner wrote:
> > src/test/python/apache/aurora/client/cli/test_quota.py, line 77
> > <https://reviews.apache.org/r/19509/diff/2/?file=532010#file532010line77>
> >
> >     Be consistent about single and double quotes, here and below.  That 
> > said, this might be easier to grok with a multiline string:
> >     
> >     
> >     '''Allocated:
> >     CPU: 5
> >     RAM: 20.000000 GB
> >     Disk: 40.000000 GB'''
> >     
> >     While you're at it, can you add formatting to the float values?  %g as 
> > opposed to %s is probably appropriate.

We are already using %f here which per the docs is a human readable version of 
%g. So, leaving it as is. Updated quoting to be consistent.

Looked into using a multi-line string, but the text doesn't format that well 
because of additional spaces in the output. Since this is also the output users 
see and is a stylistic change, refraining from making any output format changes 
in this diff.


> On March 21, 2014, 11:53 p.m., Bill Farner wrote:
> > src/test/python/apache/aurora/client/cli/test_quota.py, line 91
> > <https://reviews.apache.org/r/19509/diff/2/?file=532010#file532010line91>
> >
> >     Thanks for this coverage!  Do you know if the JSON stuff will be 
> > brittle w.r.t. field ordering?  That's bitten us in the past.  In these 
> > cases, it's probably best to deserialize the json coming from the SUT and 
> > check for object (dict) equality.

Good one. Updated the code, to turn json into dict and comparing the dicts.


- Suman


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19509/#review38235
-----------------------------------------------------------


On March 21, 2014, 10:14 p.m., Suman Karumuri wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19509/
> -----------------------------------------------------------
> 
> (Updated March 21, 2014, 10:14 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney, Mark Chu-Carroll, and Brian Wickman.
> 
> 
> Bugs: AURORA-246
>     https://issues.apache.org/jira/browse/AURORA-246
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Updated the client to consume the new getQuota API which contains 
> nonProdConsumption(resources consumed by non-prod tasks) information also. 
> 
> Currently, the mocked calls can't detect renamed and missing thrift structs. 
> Added tests to look for expected fields in GetQuotaResult and  
> ResourceAggregate structs. Refactored tests in test_quota to remove 
> duplicated code and added a missing test.
> 
> 
> Diffs
> -----
> 
>   src/main/python/apache/aurora/client/cli/quota.py 
> d06f21a80575058aefa3dffc72b365805d7a5ce2 
>   src/main/python/apache/aurora/client/commands/core.py 
> 9977c725528086d3e8cf58de294adee542570411 
>   src/test/python/apache/aurora/client/cli/test_quota.py 
> 44afd74aa5b11296951f45fe7edca8cb58b0ec18 
>   src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 
> 27b745ea31189a0ea0731619eb4c06f802aa04b9 
> 
> Diff: https://reviews.apache.org/r/19509/diff/
> 
> 
> Testing
> -------
> 
> ./pants src/test/python:all -vxs 
> 
> Works for all quota related test. 
> src.test.python.apache.thermos.bin.test_thermos fails on my laptop because of 
> build issues. Sending out a code review since it is an un-related issue. Will 
> look into this tomorrow, if it's a blocker.
> 
> Found all the code to update by running the following query.
> 
> ?  git:(mansu/AURORA-246_getQuotaAPI) ag getQuotaResult -G '.py'              
>                                                                           
> ~/workspace/incubator-aurora
> src/main/python/apache/aurora/client/api/quota_check.py
> 90:    allocated = CapacityRequest(resp.result.getQuotaResult.quota)
> 91:    consumed = CapacityRequest(resp.result.getQuotaResult.prodConsumption)
> 
> src/main/python/apache/aurora/client/cli/quota.py
> 65:      return serialize(quota_resp.result.getQuotaResult,
> 69:      result += get_quota_str(quota_resp.result.getQuotaResult.quota)
> 70:      if quota_resp.result.getQuotaResult.prodConsumption:
> 72:        result += 
> get_quota_str(quota_resp.result.getQuotaResult.prodConsumption)
> 73:      if quota_resp.result.getQuotaResult.nonProdConsumption:
> 75:        result += 
> get_quota_str(quota_resp.result.getQuotaResult.nonProdConsumption)
> 
> src/main/python/apache/aurora/client/commands/admin.py
> 199:  quota = resp.result.getQuotaResult.quota
> 
> src/main/python/apache/aurora/client/commands/core.py
> 632:  print_quota(resp.result.getQuotaResult.quota, 'Total allocated quota', 
> role)
> 634:  if resp.result.getQuotaResult.prodConsumption:
> 635:    print_quota(resp.result.getQuotaResult.prodConsumption,
> 639:  if resp.result.getQuotaResult.nonProdConsumption:
> 640:    print_quota(resp.result.getQuotaResult.nonProdConsumption,
> 
> src/test/python/apache/aurora/client/api/test_quota_check.py
> 49:        getQuotaResult=GetQuotaResult(
> 
> src/test/python/apache/aurora/client/cli/test_quota.py
> 35:    response.result.getQuotaResult = GetQuotaResult()
> 36:    response.result.getQuotaResult.quota = ResourceAggregate()
> 37:    response.result.getQuotaResult.quota.numCpus = 5
> 38:    response.result.getQuotaResult.quota.ramMb = 20480
> 39:    response.result.getQuotaResult.quota.diskMb = 40960
> 40:    response.result.getQuotaResult.consumed = None
> 47:    response.result.getQuotaResult = GetQuotaResult()
> 48:    response.result.getQuotaResult.quota = ResourceAggregate()
> 49:    response.result.getQuotaResult.quota.numCpus = 5
> 50:    response.result.getQuotaResult.quota.ramMb = 20480
> 51:    response.result.getQuotaResult.quota.diskMb = 40960
> 52:    response.result.getQuotaResult.prodConsumption = ResourceAggregate()
> 53:    response.result.getQuotaResult.prodConsumption.numCpus = 1
> 54:    response.result.getQuotaResult.prodConsumption.ramMb = 1024
> 55:    response.result.getQuotaResult.prodConsumption.diskMb = 2048
> 56:    response.result.getQuotaResult.nonProdConsumption = ResourceAggregate()
> 57:    response.result.getQuotaResult.nonProdConsumption.numCpus = 1
> 58:    response.result.getQuotaResult.nonProdConsumption.ramMb = 1024
> 59:    response.result.getQuotaResult.nonProdConsumption.diskMb = 2048
> ?  git:(mansu/AURORA-246_getQuotaAPI)                                         
>                                                                           
> ~/workspace/incubator-aurora
> 
> 
> Thanks,
> 
> Suman Karumuri
> 
>

Reply via email to