> 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.
> 
> Joshua Cohen wrote:
>     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.

Deleted (under protest)! I don't want to separate them into classes as I don't 
want to drag out this diff even further and get blocked on comments in tests. :)


> 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?
> 
> Joshua Cohen wrote:
>     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?

If this is not a blocker, then I would prefer to leave this as is.

I am totally onboard with `mock.patch` and you can see test_base.py where I 
used injections all around.


- Dmitriy


-----------------------------------------------------------
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