> 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
> 
>

Reply via email to