> On March 25, 2015, 5:38 p.m., Alexander Rukletsov wrote: > > src/tests/master_tests.cpp, line 2816 > > <https://reviews.apache.org/r/32463/diff/1/?file=904857#file904857line2816> > > > > Why do you check the type here, but not above for e.g. `DATE`, `TIME`, > > and `USER`?
Ah nice catch, `"id"` needs it but the ones below don't need it! For DATE, TIME, and USER, we can check equality (note that the `std::string build::DATE` implicitly becomes a `JSON::String`, which is `==` comparable to a `JSON::Value`). However, for `"id"`, we want to confirm that it's a non-empty `JSON::String`. If you check for `"" != value` then this holds for all non-empty strings, but it also holds for JSON::Number, JSON::Object, etc. So I check the type explicitly, make sense? > On March 25, 2015, 5:38 p.m., Alexander Rukletsov wrote: > > src/tests/master_tests.cpp, line 66 > > <https://reviews.apache.org/r/32463/diff/1/?file=904857#file904857line66> > > > > I noticed you do drive-by clean-up of some `process::` occurrences. Do > > you want to go further and remove all of them or prefer creating a newbie > > JIRA for that? There are e.g. `process::Message` and `Process::Time`. Thanks for noting this, I originally added this to avoid having to add using clauses for a lot of things from process::http. However, we generally are avoiding using namespaces like this so I've reverted back to fine-grained using clauses, and a namespace alias at the top to deal with the `proces::http` problem. > On March 25, 2015, 5:38 p.m., Alexander Rukletsov wrote: > > src/tests/master_tests.cpp, lines 2847-2849 > > <https://reviews.apache.org/r/32463/diff/1/?file=904857#file904857line2847> > > > > How about a one-liner here and below: > > ``` > > EXPECT_TRUE(state.values["slaves"].as<JSON::Array>().values.empty()); > > ``` Thanks! > On March 25, 2015, 5:38 p.m., Alexander Rukletsov wrote: > > src/tests/slave_tests.cpp, line 792 > > <https://reviews.apache.org/r/32463/diff/1/?file=904858#file904858line792> > > > > Not yours, but can you do a drive-by fix and remove `process::`? I'd like to avoid the noise in the diff since its in an unrelated test :) > On March 25, 2015, 5:38 p.m., Alexander Rukletsov wrote: > > src/tests/master_tests.cpp, line 2814 > > <https://reviews.apache.org/r/32463/diff/1/?file=904857#file904857line2814> > > > > This fails on my Mac. > > > > Also, since you're checking start time, you can add one more > > `EXPECT_GT` check caching time before starting master. It turns out this fails because `Time::create` compensates for the advanced state the clock by adding `clock::advanced`. That has already been added from `Clock::now` in the endpoint and so the times are not equal. > On March 25, 2015, 5:38 p.m., Alexander Rukletsov wrote: > > src/tests/master_tests.cpp, lines 2807-2809 > > <https://reviews.apache.org/r/32463/diff/1/?file=904857#file904857line2807> > > > > Does it make sense to use `Clock::now().secs()` and compare doubles to > > avoid multiple conversions and mimic the way it is actually done in master > > code? Hm.. but you have to pause the clock, yes? I've gone with your suggestion since its cleaner, paused the Clock at the top of the test to ensure Clock::now matches the master's time. > On March 25, 2015, 5:38 p.m., Alexander Rukletsov wrote: > > src/tests/slave_tests.cpp, line 937 > > <https://reviews.apache.org/r/32463/diff/1/?file=904858#file904858line937> > > > > Kill `process::` prefix? Will leave the unrelated tests for now. :) - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32463/#review77738 ----------------------------------------------------------- On March 24, 2015, 11:42 p.m., Ben Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/32463/ > ----------------------------------------------------------- > > (Updated March 24, 2015, 11:42 p.m.) > > > Review request for mesos, Alexander Rukletsov and Vinod Kone. > > > Repository: mesos > > > Description > ------- > > This improves the slave test, and adds a new test for the master, to help > prevent API regressions. > > > Diffs > ----- > > src/tests/master_tests.cpp e69348be676a80017062e3abbd15b8008a6009d7 > src/tests/slave_tests.cpp fd09d65bf34136c0959419b451e54105300740c4 > > Diff: https://reviews.apache.org/r/32463/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Ben Mahler > >
