> On Nov. 13, 2014, 12:36 a.m., Kevin Sweeney wrote:
> > src/main/python/apache/aurora/client/cli/jobs.py, line 331
> > <https://reviews.apache.org/r/27848/diff/3/?file=757409#file757409line331>
> >
> >     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`.

Done.


> On Nov. 13, 2014, 12:36 a.m., Kevin Sweeney wrote:
> > src/main/python/apache/aurora/client/cli/options.py, line 63
> > <https://reviews.apache.org/r/27848/diff/3/?file=757410#file757410line63>
> >
> >     ```py
> >     if self.kwargs.get('dest'):
> >       return self.kwargs['dest']
> >     ```

Fixed.


> On Nov. 13, 2014, 12:36 a.m., Kevin Sweeney wrote:
> > src/main/python/apache/aurora/client/cli/options.py, lines 70-72
> > <https://reviews.apache.org/r/27848/diff/3/?file=757410#file757410line70>
> >
> >     return self.kwargs.get('default')

Fixed.


> On Nov. 13, 2014, 12:36 a.m., Kevin Sweeney wrote:
> > src/test/python/apache/aurora/client/cli/test_create.py, lines 74-77
> > <https://reviews.apache.org/r/27848/diff/3/?file=757411#file757411line74>
> >
> >     ```py
> >     with pytest.raises(Context.CommandError):
> >       command.execute(fake_context)
> >     ```
> >     
> >     Otherwise this test will succeed when the command doesn't raise.

Well the assertion still fails if the message isn't added to the output. This 
suggestion is better than the try except anyway. Done.


> On Nov. 13, 2014, 12:36 a.m., Kevin Sweeney wrote:
> > src/test/python/apache/aurora/client/cli/test_create.py, line 79
> > <https://reviews.apache.org/r/27848/diff/3/?file=757411#file757411line79>
> >
> >     extract get_job_config's return value above and replace with
> >     
> >     mock_api.assert_called_once_with(mock_job_config)

Done.


> On Nov. 13, 2014, 12:36 a.m., Kevin Sweeney wrote:
> > src/test/python/apache/aurora/client/cli/test_kill.py, line 75
> > <https://reviews.apache.org/r/27848/diff/3/?file=757412#file757412line75>
> >
> >     mock_api.kill_job.assert_called_once_with(...) to prove the setup above 
> > isn't a noop

Done.


> On Nov. 13, 2014, 12:36 a.m., Kevin Sweeney wrote:
> > src/test/python/apache/aurora/client/cli/test_kill.py, line 103
> > <https://reviews.apache.org/r/27848/diff/3/?file=757412#file757412line103>
> >
> >     assert_called_once_with(...) to prove your fake object setup was needed

Done.


> On Nov. 13, 2014, 12:36 a.m., Kevin Sweeney wrote:
> > src/test/python/apache/aurora/client/cli/test_restart.py, line 33
> > <https://reviews.apache.org/r/27848/diff/3/?file=757413#file757413line33>
> >
> >     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.

The idea is that we'd eventually get full coverage of these commands using this 
style. In this case I'm still only using classes for grouping while they live 
next to the existing tests. I could split the new style tests into their own 
files and just use functions? But I sort of wanted them to be next to the 
existing integrationy tests so people cargo cult the correct thing when adding 
more tests. Thoughts?


> On Nov. 13, 2014, 12:36 a.m., Kevin Sweeney wrote:
> > src/test/python/apache/aurora/client/cli/test_restart.py, line 54
> > <https://reviews.apache.org/r/27848/diff/3/?file=757413#file757413line54>
> >
> >     fake_api.restart.assert_called_once_with(...)

Done.


> On Nov. 13, 2014, 12:36 a.m., Kevin Sweeney wrote:
> > src/test/python/apache/aurora/client/cli/test_supdate.py, lines 67-70
> > <https://reviews.apache.org/r/27848/diff/3/?file=757414#file757414line67>
> >
> >     with pytest.raises(...), see above for rationale

Done.


> On Nov. 13, 2014, 12:36 a.m., Kevin Sweeney wrote:
> > src/test/python/apache/aurora/client/cli/test_supdate.py, line 73
> > <https://reviews.apache.org/r/27848/diff/3/?file=757414#file757414line73>
> >
> >     mock_api.start_job_update.assert_called_once_with(...)

Done.


> On Nov. 13, 2014, 12:36 a.m., Kevin Sweeney wrote:
> > src/test/python/apache/aurora/client/cli/test_update.py, line 76
> > <https://reviews.apache.org/r/27848/diff/3/?file=757415#file757415line76>
> >
> >     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.

It's not just the traceback, the message comes from check_and_log_response. I'm 
not a huge fan of this design (or the context heirarchy in general...) but I 
don't have enough context yet to go in and tear it up in favour of just 
throwing exceptions (AFAICT the main hurdle is that the current design allows 
for multiple messages of different 'types' to be propagated to the user, 
including messages from hooks et al.).


> On Nov. 13, 2014, 12:36 a.m., Kevin Sweeney wrote:
> > src/test/python/apache/aurora/client/cli/test_update.py, lines 71-74
> > <https://reviews.apache.org/r/27848/diff/3/?file=757415#file757415line71>
> >
> >     pytest.raises

Done.


> On Nov. 13, 2014, 12:36 a.m., Kevin Sweeney wrote:
> > src/test/python/apache/aurora/client/cli/test_update.py, line 77
> > <https://reviews.apache.org/r/27848/diff/3/?file=757415#file757415line77>
> >
> >     mock_api.assert_called_once_with(...)

Done.


- David


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


On Nov. 11, 2014, 12:58 a.m., David McLaughlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27848/
> -----------------------------------------------------------
> 
> (Updated Nov. 11, 2014, 12:58 a.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
> 
>

Reply via email to