> On Dec. 16, 2015, 10:14 p.m., Joshua Cohen wrote:
> > src/test/python/apache/aurora/client/test_config.py, line 187
> > <https://reviews.apache.org/r/41428/diff/4/?file=1166716#file1166716line187>
> >
> >     Kill this comment? It seems superfluous.
> 
> Dmitriy Shirchenko wrote:
>     I added because I had a hard time finding my tests. I like comments to 
> logically separate tests into sections. If you feel like they should be 
> separated into individual classes, then I can do that as well but comments 
> seemed good enough for tests. Lmk.

I generally prefer to rely on explicitly named tests for this. Adding a comment 
to delineate a group of tests is vulnerable to someone unknowingly sneaking an 
unrelated test in the middle of that section. If you feel that there are tests 
that should be logically grouped, adding them to a class is the way to go.


> On Dec. 16, 2015, 10:14 p.m., Joshua Cohen wrote:
> > src/test/python/apache/aurora/client/test_config.py, lines 240-243
> > <https://reviews.apache.org/r/41428/diff/4/?file=1166716#file1166716line240>
> >
> >     I'm somewhat uncomfortable with using monkeypatch to accomplish this. 
> > We had generally been trending towards adding injection points for mocks, 
> > rather than monkeypatching globals. How would you feel about adding a 
> > logger arg to `_validate_health_check_config` that just defaults to `log`? 
> > and passing in a mock from this test case?
> 
> Dmitriy Shirchenko wrote:
>     I personally use mock.patch decorators myself but I try not to add 
> dependencies as well so I had to figure out how to make this work with 
> pytest. I feel like mock.patch is the way to go but I also feel like we 
> should be consistent and swapping to mock is a separate chunk of work as it 
> may be required affect other tests as well. Like it would be weird to just 
> have one test using mock.
>     
>     In any case: is this a nit or you feel like this is a blocker?

We use mock.patch elsewhere throughout the codebase, so adding a dependency on 
that is not a problem.

That said, I think using mock.patch is essentially the same thing as using 
pytest monkeypatch. Perhaps it's due to the fact that this is a python codebase 
maintained primarily by a bunch of Java developers, but we've tended to prefer 
injection over patching. The way we accomplish these sort of assertions 
elsewhere in the client is through the use of the 
[FakeAuroraCommandContext](https://github.com/apache/aurora/blob/master/src/test/python/apache/aurora/client/cli/util.py#L71)
 which abstracts away the log statements in a way that makes them easy to mock. 
However config is at a higher level and `AuroraCommandContext` is not used, but 
we can still follow a similar pattern with regards to asserting on cli logging.

So... long story short, this is more than a nit but less than a blocker, 
hopefully the above makes a compelling case for "injecting" a mock?


- Joshua


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


On Dec. 16, 2015, 10:43 p.m., Dmitriy Shirchenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41428/
> -----------------------------------------------------------
> 
> (Updated Dec. 16, 2015, 10:43 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Bill Farner.
> 
> 
> Bugs: AURORA-1562
>     https://issues.apache.org/jira/browse/AURORA-1562
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Refactoring HealthCheckConfig into separate structs
> 
> 
> Diffs
> -----
> 
>   docs/configuration-reference.md 07149c7f49371e39b70e8744d60affeffd73c77f 
>   src/main/python/apache/aurora/client/config.py 
> 161c3627db0afa8fdd8afff5abf6a94556f53180 
>   src/main/python/apache/aurora/config/schema/base.py 
> e752482b95ae6377d0cf12cf0ecf0fa60e2a5d46 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> cba4e8c879fae2b9f56a36bf8fa4b702163667d1 
>   src/test/python/apache/aurora/client/test_config.py 
> 8fd112fd8a10120f57324d8efec22a009b93404b 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> 8561abc3e7df8803785b70064bb76553d3210399 
>   src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora 
> 37f2e9c6ebc4a4bbd6089e821420e5a02179c6b9 
> 
> Diff: https://reviews.apache.org/r/41428/diff/
> 
> 
> Testing
> -------
> 
> Added unit tests.
> Changed end to end test.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Reply via email to