----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37082/#review94292 -----------------------------------------------------------
Tests look good! Mainly stylistic issues and generous use of 'auto'. Please make sure you fix all the tests when you fix a given issue (I might not have caught all of them in all the tests). src/tests/http_api_tests.cpp (line 69) <https://reviews.apache.org/r/37082/#comment148841> I would have this return a Try<Event> src/tests/http_api_tests.cpp (line 73) <https://reviews.apache.org/r/37082/#comment148842> what if this fails? see above: this should return an Error. src/tests/http_api_tests.cpp (line 79) <https://reviews.apache.org/r/37082/#comment148843> what if this fails? just return parse here. src/tests/http_api_tests.cpp (line 91) <https://reviews.apache.org/r/37082/#comment148840> 2 blank lines between outer elements. src/tests/http_api_tests.cpp (line 97) <https://reviews.apache.org/r/37082/#comment148844> 2 blank lines. src/tests/http_api_tests.cpp (line 126) <https://reviews.apache.org/r/37082/#comment148845> 2 blank lines. src/tests/http_api_tests.cpp (line 127) <https://reviews.apache.org/r/37082/#comment148848> s/client/scheduler/ src/tests/http_api_tests.cpp (lines 135 - 136) <https://reviews.apache.org/r/37082/#comment148850> Put this line close to where it is used, above #150. src/tests/http_api_tests.cpp (line 145) <https://reviews.apache.org/r/37082/#comment148867> don't need to do this anymore now that the default framework info sets the user, right? src/tests/http_api_tests.cpp (line 164) <https://reviews.apache.org/r/37082/#comment148868> why auto? it's not clear at all what the return type is here. src/tests/http_api_tests.cpp (line 174) <https://reviews.apache.org/r/37082/#comment148870> ditto. no auto please. src/tests/http_api_tests.cpp (lines 185 - 186) <https://reviews.apache.org/r/37082/#comment148877> This comment and test name is misleading because this is not testing failover. This test is just testing subscription retry (simulating the situation of a ZK blip). Mind fixing it? src/tests/http_api_tests.cpp (line 192) <https://reviews.apache.org/r/37082/#comment148885> ditto. src/tests/http_api_tests.cpp (line 201) <https://reviews.apache.org/r/37082/#comment148872> ditto. src/tests/http_api_tests.cpp (line 219) <https://reviews.apache.org/r/37082/#comment148873> ditto. src/tests/http_api_tests.cpp (line 229) <https://reviews.apache.org/r/37082/#comment148874> ditto. src/tests/http_api_tests.cpp (line 238) <https://reviews.apache.org/r/37082/#comment148878> new line here. src/tests/http_api_tests.cpp (line 244) <https://reviews.apache.org/r/37082/#comment148876> ditto. src/tests/http_api_tests.cpp (line 251) <https://reviews.apache.org/r/37082/#comment148879> ditto. src/tests/http_api_tests.cpp (line 258) <https://reviews.apache.org/r/37082/#comment148880> this is not a failover. please rephrase. src/tests/http_api_tests.cpp (lines 265 - 266) <https://reviews.apache.org/r/37082/#comment148881> EXPECT_EQ(arg1, arg2); src/tests/http_api_tests.cpp (line 273) <https://reviews.apache.org/r/37082/#comment148882> s/http/HTTP/ src/tests/http_api_tests.cpp (line 281) <https://reviews.apache.org/r/37082/#comment148883> ditto. src/tests/http_api_tests.cpp (line 285) <https://reviews.apache.org/r/37082/#comment148884> ditto. src/tests/http_api_tests.cpp (line 299) <https://reviews.apache.org/r/37082/#comment148887> s/http/HTTP/ src/tests/http_api_tests.cpp (line 311) <https://reviews.apache.org/r/37082/#comment148888> s/http/HTTP/ src/tests/http_api_tests.cpp (lines 317 - 318) <https://reviews.apache.org/r/37082/#comment148889> reorder. src/tests/http_api_tests.cpp (lines 351 - 353) <https://reviews.apache.org/r/37082/#comment148891> you don't need to pull out value. we have equality operator defined for FrameworkID. also alignment. src/tests/http_api_tests.cpp (line 362) <https://reviews.apache.org/r/37082/#comment148892> s/pid/PID/ s/http/HTTP/ src/tests/http_api_tests.cpp (line 365) <https://reviews.apache.org/r/37082/#comment148894> s/UpdatePidToHttpSchedulerForceNotSetFailure/UpdatePidToHttpSchedulerWithoutForce/ src/tests/http_api_tests.cpp (line 376) <https://reviews.apache.org/r/37082/#comment148895> ditto. src/tests/http_api_tests.cpp (line 395) <https://reviews.apache.org/r/37082/#comment148896> s/http/HTTP/ s/framework/framework without setting 'force' field/ src/tests/http_api_tests.cpp (line 419) <https://reviews.apache.org/r/37082/#comment148897> ditto. src/tests/http_api_tests.cpp (line 429) <https://reviews.apache.org/r/37082/#comment148898> ditto. src/tests/http_api_tests.cpp (line 432) <https://reviews.apache.org/r/37082/#comment148899> s/framework/PID framework/ - Vinod Kone On Aug. 5, 2015, midnight, Anand Mazumdar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37082/ > ----------------------------------------------------------- > > (Updated Aug. 5, 2015, midnight) > > > Review request for mesos, Ben Mahler and Vinod Kone. > > > Repository: mesos > > > Description > ------- > > This implements the tests for http framework subscribe/failover/upgrade from > a pid based framework. The test are parameterized on content type and hence > test both json/protobuf responses. > > > Diffs > ----- > > src/tests/http_api_tests.cpp 586d11288828fe9997e54f5dbd7d28c200e973f5 > > Diff: https://reviews.apache.org/r/37082/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Anand Mazumdar > >
