----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22471/#review46636 -----------------------------------------------------------
src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82141> Please separate those includes. It's ok that you include json.hpp, net.hpp, tests/flags.hpp and mesos_containerizer.hpp, without the ifdef guard. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82145> Can you remind me why this needs to be MesosTest? src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82144> Please stick to style guide. We put { in the next line for functions. Here and everywhere else. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82146> Could you please explain why we need to get external name servers? For example, to do DNS testing? to get an valid external host? src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82147> You should be able to combine these two lines: foreach (const string& line, strings::split(read.get(), "\n")) { ... } src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82143> Please stick to style guide. The indent for those parameters should be 4 spaces. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82149> Would you please swap these two arguments to be consistent with the launcher code? src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82154> Would you please move this down below ommandInfo.set_value(command); src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82150> New lines above and below. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82153> We've changed to MesosContainerizerLaunch::NAME src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82157> I would suggest not making 'pipes' and 'command1' 'command2' a class field. Especially for pipes, make sure all pipes you created get closed. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82158> Style. 4 spaces please. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82159> Ditto. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82162> You can combine these two: EXPECT_EQ(0, os::system( ... ...)); src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82160> Ditto. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82161> Not checking the error code? EXPECT_EQ(0, os::system(...)); Here and every where else. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82163> Ditto. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82164> Ditto. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82165> Ditto. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82166> Ditto. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82167> Ditto. - Jie Yu On June 25, 2014, 2:10 a.m., Chi Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22471/ > ----------------------------------------------------------- > > (Updated June 25, 2014, 2:10 a.m.) > > > Review request for mesos, Ian Downes, Jie Yu, Vinod Kone, and Cong Wang. > > > Repository: mesos-git > > > Description > ------- > > These tests test different different protocols (TCP, UDP, ICMP, ARP, DNS) in > different connection scenarios (C2C, H2C). > > > Diffs > ----- > > src/tests/isolator_tests.cpp 0bbec09 > > Diff: https://reviews.apache.org/r/22471/diff/ > > > Testing > ------- > > make check. > > > Thanks, > > Chi Zhang > >
