mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ben Mahler" <benjamin.mah...@gmail.com>
Subject Re: Review Request 34361: converted hard-coded strings to consts
Date Thu, 11 Jun 2015 01:35:49 GMT


> On June 10, 2015, 1:25 a.m., Ben Mahler wrote:
> > src/tests/master_tests.cpp, lines 3031-3034
> > <https://reviews.apache.org/r/34361/diff/3/?file=971359#file971359line3031>
> >
> >     Why bother with all this? Why not just have `"key1"`, `"value1"`, `"key2"`,
`"value2"` inlined appropriately throughout these tests. Very straightforward to read.
> 
> Colin Williams wrote:
>     I think the issue with the changes remaining is that the test depends on the same
value occurring in several places. By consolidating to a variable it's no longer possible
for these values to get out of sync.
> 
> Niklas Nielsen wrote:
>     Colin: exactly
>     
>     Ben: We have label tests three places; in the master, slave and in the modules and
they use the same "foo", "bar", "baz", "qux" key value pairs.
>     The idea was to centralize them, so they don't get out of date and to avoid code
duplication.
>     
>     Does that make sense?

Well, then let's just keep it simple and get rid of these special strings by just making the
tests use "key1", "value1", "key2", "value2", etc.

I'm not following your code duplication point, this introduces more code (now there are additional
global constants, which we like to avoid if unnecessary).

Also, each test is ideally independent, why does the master's test for labels affect the slave's
test for labels? Let's say I need a test with 5 labels, you want to have 5x2=10 global constants
to address this?


- Ben


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


On June 8, 2015, 7:05 p.m., Colin Williams wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34361/
> -----------------------------------------------------------
> 
> (Updated June 8, 2015, 7:05 p.m.)
> 
> 
> Review request for mesos and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2637
>     https://issues.apache.org/jira/browse/MESOS-2637
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> converted hard-coded strings to consts
> 
> 
> Diffs
> -----
> 
>   src/tests/hook_tests.cpp 3ffde6d 
>   src/tests/master_tests.cpp ba3858f 
>   src/tests/slave_tests.cpp acae497 
> 
> Diff: https://reviews.apache.org/r/34361/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Colin Williams
> 
>


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