> 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? > > Niklas Nielsen wrote: > Ping :) > > Colin Williams wrote: > Sorry, my day job's been really all-consuming the last few days. I was > planning to update the ticket. > > Colin Williams wrote: > Ticket updated > > Niklas Nielsen wrote: > Colin; thanks! should we keep this patch then, or will you be providing a > new one? > > Colin Williams wrote: > I'm not planning on providing a new one.
Sorry for the long turn around time, Colin! To BenM's point; let's just rename to key1,value1 consistently instead of just hoisting the constants inline. Would you be up for doing this? We can land that immediately. - 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 > >