aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Kevin Sweeney" <kevi...@apache.org>
Subject Re: Review Request 27848: Add friendly error message to the client when lock is held.
Date Thu, 13 Nov 2014 00:36:39 GMT

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



src/main/python/apache/aurora/client/cli/jobs.py
<https://reviews.apache.org/r/27848/#comment102595>

    weird wrapping - consider wrapping the whole statement in parens so there is one condition
per line
    
    ```py
    if (resp.responseCode != ResponseCode.OK
        or self.wait_kill_tasks(...) != EXIT_OK):
        
      log.error(...)
    ```
    
    Also, ResponseCodes are ints - use `!=` instead of `is not`.



src/main/python/apache/aurora/client/cli/options.py
<https://reviews.apache.org/r/27848/#comment102601>

    ```py
    if self.kwargs.get('dest'):
      return self.kwargs['dest']
    ```



src/main/python/apache/aurora/client/cli/options.py
<https://reviews.apache.org/r/27848/#comment102600>

    return self.kwargs.get('default')



src/test/python/apache/aurora/client/cli/test_create.py
<https://reviews.apache.org/r/27848/#comment102603>

    ```py
    with pytest.raises(Context.CommandError):
      command.execute(fake_context)
    ```
    
    Otherwise this test will succeed when the command doesn't raise.



src/test/python/apache/aurora/client/cli/test_create.py
<https://reviews.apache.org/r/27848/#comment102606>

    extract get_job_config's return value above and replace with
    
    mock_api.assert_called_once_with(mock_job_config)



src/test/python/apache/aurora/client/cli/test_kill.py
<https://reviews.apache.org/r/27848/#comment102609>

    mock_api.kill_job.assert_called_once_with(...) to prove the setup above isn't a noop



src/test/python/apache/aurora/client/cli/test_kill.py
<https://reviews.apache.org/r/27848/#comment102610>

    assert_called_once_with(...) to prove your fake object setup was needed



src/test/python/apache/aurora/client/cli/test_restart.py
<https://reviews.apache.org/r/27848/#comment102611>

    nit: does this need to be a TestCase class? as far as I can tell you aren't using any
per-test state or setup methods so you could just define this as a method: `def test_...`
and py.test will still pick it up fine.



src/test/python/apache/aurora/client/cli/test_restart.py
<https://reviews.apache.org/r/27848/#comment102612>

    fake_api.restart.assert_called_once_with(...)



src/test/python/apache/aurora/client/cli/test_supdate.py
<https://reviews.apache.org/r/27848/#comment102614>

    with pytest.raises(...), see above for rationale



src/test/python/apache/aurora/client/cli/test_supdate.py
<https://reviews.apache.org/r/27848/#comment102613>

    mock_api.start_job_update.assert_called_once_with(...)



src/test/python/apache/aurora/client/cli/test_update.py
<https://reviews.apache.org/r/27848/#comment102615>

    pytest.raises



src/test/python/apache/aurora/client/cli/test_update.py
<https://reviews.apache.org/r/27848/#comment102642>

    something to keep in mind here - if the return value of get_err is just the traceback
that you're throwing away above you can consider just doing this assertion off the traceback
above (and possibly removing the get_err()) method.



src/test/python/apache/aurora/client/cli/test_update.py
<https://reviews.apache.org/r/27848/#comment102616>

    mock_api.assert_called_once_with(...)



src/test/python/apache/aurora/client/cli/util.py
<https://reviews.apache.org/r/27848/#comment102643>

    cool


- Kevin Sweeney


On Nov. 10, 2014, 4:58 p.m., David McLaughlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27848/
> -----------------------------------------------------------
> 
> (Updated Nov. 10, 2014, 4:58 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-885
>     https://issues.apache.org/jira/browse/AURORA-885
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Add friendly error message to the client when lock is held.
> 
> 
> Diffs
> -----
> 
>   src/main/python/apache/aurora/client/cli/__init__.py 67cf40365b38b6bf395c697faf0cdb334322bdc3

>   src/main/python/apache/aurora/client/cli/context.py a5ebbdc2ca37c5fd1813854d0a34f15511e6ca06

>   src/main/python/apache/aurora/client/cli/jobs.py 28f9475c5accb8c73cbc5f7a1010920479a0388e

>   src/main/python/apache/aurora/client/cli/options.py 1f5cbb616828932742e6482867e2eca9401aad61

>   src/test/python/apache/aurora/client/cli/test_create.py 1dec54c0da234cccc6d4091bb3fda4508836aac0

>   src/test/python/apache/aurora/client/cli/test_kill.py 78f5f04507d7fe080a1ed5ddda692e52f66cc18d

>   src/test/python/apache/aurora/client/cli/test_restart.py a8180a3264ac1aa2ade654985755a4dbe262dc47

>   src/test/python/apache/aurora/client/cli/test_supdate.py 09f6a85aebdbf0ad9c9816684f4574132205ee65

>   src/test/python/apache/aurora/client/cli/test_update.py a5e59e4924618ab97f18ea056ef8225e864a317d

>   src/test/python/apache/aurora/client/cli/util.py 154fb3a7170ae81548fcbc9f3cdd6dcf9bf1942d

> 
> Diff: https://reviews.apache.org/r/27848/diff/
> 
> 
> Testing
> -------
> 
> ./pants src/test/python/apache/aurora/client/cli/:all
> ./build-support/python/isort-check
> ./build-support/python/checkstyle-check
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message