aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Joshua Cohen" <jco...@apache.org>
Subject Re: Review Request 41428: Refactoring HealthCheckConfig into separate structs
Date Thu, 17 Dec 2015 01:14:51 GMT


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


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