> On Dec. 15, 2015, 4:35 a.m., Anand Mazumdar wrote: > > src/tests/slave_tests.cpp, line 2147 > > <https://reviews.apache.org/r/41188/diff/4/?file=1163918#file1163918line2147> > > > > I am assuming that we are testing that port labels are populated > > correctly in the `DiscoveryInfo` object. > > > > How about: > > ``` > > // This test verifies that port labels are exposed over the slave state > > endpoint. > > ```
Well actually it tests both DiscoveryInfo and port labels. Let me be a bit more explicit about it and change it to: // This test verifies that DiscoveryInfo and Port messages, set in TaskInfo, // are exposed over the slave state endpoint. The test launches a task with // the DiscoveryInfo and Port message fields populated. It then makes an HTTP // request to the state endpoint of the slave and retrieves the JSON data from // the endpoint. The test passes if the DiscoveryInfo and Port message data in // JSON matches the corresponding data set in the TaskInfo used to launch the // task. > On Dec. 15, 2015, 4:35 a.m., Anand Mazumdar wrote: > > src/tests/slave_tests.cpp, line 2149 > > <https://reviews.apache.org/r/41188/diff/4/?file=1163918#file1163918line2149> > > > > s/DiscoveryInfoAndPorts/PortLabels This test the setting of both DiscoveryInfo and Ports hence the function name. > On Dec. 15, 2015, 4:35 a.m., Anand Mazumdar wrote: > > src/tests/slave_tests.cpp, line 2205 > > <https://reviews.apache.org/r/41188/diff/4/?file=1163918#file1163918line2205> > > > > Why not just do this: > > > > ``` > > EXPECT_CALL(exec, registered(_, _, _, _, _)); > > ``` > > > > AFAICT, you don't seem to be using any of the arguments from the > > `registered` callback. All you are interested in is that the expectation is > > fullfilled exactly once. Agrred, I thought we were using the execDriver further down to get the response but we are not. > On Dec. 15, 2015, 4:35 a.m., Anand Mazumdar wrote: > > src/tests/slave_tests.cpp, line 2209 > > <https://reviews.apache.org/r/41188/diff/4/?file=1163918#file1163918line2209> > > > > You don't seem to be using `execTask` i.e. `TaskInfo` attributed. Why > > not just do: > > > > ``` > > Future<Nothing> launchTask; > > EXPECT_CALL(exec, launchTask(_, _)) > > .WillOnce(FutureSatisfy(&launchTask)); > > ``` > > > > and then later do > > > > ```AWAIT_READY(launchTask);``` Cool !! Still getting comfortable with Google mock . This is nice :) > On Dec. 15, 2015, 4:35 a.m., Anand Mazumdar wrote: > > src/tests/slave_tests.cpp, line 2222 > > <https://reviews.apache.org/r/41188/diff/4/?file=1163918#file1163918line2222> > > > > We generally avoid magic string constants. > > > > Can you just do > > > > ``` > > ASSERT_SOME_EQ( > > APPLICATION_JSON, > > response.get().headers.get("Content-Type")); > > ``` > > > > Can we also check if the response code is OK() ? This is pre-existing code from TEST_F(SlaveTest, TaskLabels) ?? They are using "application/json. Is APPLICATION_JSON defined somewhere? > On Dec. 15, 2015, 4:35 a.m., Anand Mazumdar wrote: > > src/tests/slave_tests.cpp, line 2245 > > <https://reviews.apache.org/r/41188/diff/4/?file=1163918#file1163918line2245> > > > > New line before this line. > > > > Also, > > > > `// Verify that the ports match.` These are comments. Lets me more explicit than implicit. Think about it from a newbie's perspective more info is better. Might not be perfect but better. > On Dec. 15, 2015, 4:35 a.m., Anand Mazumdar wrote: > > src/tests/slave_tests.cpp, line 2190 > > <https://reviews.apache.org/r/41188/diff/4/?file=1163918#file1163918line2190> > > > > How about: > > > > ``` > > Port *port1 = ports.add_ports(); > > port1->set_number(80); > > port1->mutable_labels()->CopyFrom(labels1); > > > > Port *port2 = ports.add_ports(); > > port2->set_number(8081); > > port2->mutable_labels()->CopyFrom(labels2); > > ``` > > > > This makes it a bit cleaner. Yup looks cleaner :). - Avinash ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41188/#review110387 ----------------------------------------------------------- On Dec. 14, 2015, 11:34 p.m., Avinash sridharan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/41188/ > ----------------------------------------------------------- > > (Updated Dec. 14, 2015, 11:34 p.m.) > > > Review request for mesos, Adam B and Anand Mazumdar. > > > Bugs: MESOS-3962 > https://issues.apache.org/jira/browse/MESOS-3962 > > > Repository: mesos > > > Description > ------- > > Providing JSON bindings to so that mesos modules can expose DiscoveryInfo > protobuf messages to HTTP endpoints > > > Diffs > ----- > > src/slave/http.cpp d1b1158c0a80d72c32c9b28977043b6be2295239 > src/tests/common/http_tests.cpp 3aca5b0437a012664f58ff331cc7cf682d442699 > src/tests/slave_tests.cpp 4975bea8a7a701e0414426760692720f73dea7f5 > > Diff: https://reviews.apache.org/r/41188/diff/ > > > Testing > ------- > > make check. > Added Unit tests to verify setting of DiscoveryInfo in state.json in slave. > Also added Unit test to test that DiscoveryInfo gets exposed in master when > TaskInfo protobuf is converted to JSON objects. > > > Thanks, > > Avinash sridharan > >
