mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Niklas Nielsen" <...@qni.dk>
Subject Re: Review Request 34361: converted hard-coded strings to consts
Date Mon, 15 Jun 2015 18:31:39 GMT


> On June 9, 2015, 6:25 p.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?
> 
> Ben Mahler wrote:
>     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?
> 
> Niklas Nielsen wrote:
>     Try to see how testLabelKey and testLabelValue was used previously and "foo", "bar",
"qux" was used on others and I created this ticket to unify the way we do this, along with
seeing these key value pair being created in the slave and master tests.
>     
>     Dispite the current patch, I do think we can simply and unify the test label creation.
Maybe create a CREATE_LABEL_PAIR(id) to make it more obvious which key pairs are being tested.
The comments in the test code need to carry a bunch of context, to make sense out of the label
transformations (especially across the library border for the hooks tests).
>     
>     This is all in spirit of reducing the tech debt we introduced. We are on the same
page on not introducing more lines/key pairs than before. Let us iterate on this and get back
to you.
> 
> Colin Williams wrote:
>     Ben: I'm more in agreement with your sentiment, I'm not sure I see the point of unifying
label names into a centralized variable that aren't at all related.
>     
>     Niklas: I was a little confused by the original task description, so I thought a
sample patch would be a good discussion point. I don't entirely understand the tech debt you're
trying to solve. For example, it seems like you're asking to have some constants defined somewhere
that are used by both master_test and slave_test, but the the similarities between these two
only seem incidental. I'm concerned that creating something like CREATE_LABEL_PAIR(id) would
instead obfuscate the intent of the code.
> 
> Niklas Nielsen wrote:
>     Okay - thinking about this; I am on board with "key1", "value1" etc.
>     
>     Colin: the tech debt is that we have some inlined constants (like "foo", "bar", etc)
and some which are constants (which have to be kept in sync between hooks modules and test
body).
>     The idea was to unify the way we name the key value pairs.
>     
>     Do you want to update this ticket to reflect this, or come with a new patch set which
tackles streaming the two?

Ping :)


- Niklas


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


On June 8, 2015, 12: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, 12: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