----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22471/#review46562 -----------------------------------------------------------
My comments are only for the first test. Apply the same comments to all other tests. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment81998> Please move this up (below 'using namespace process;'). src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82003> Result<string> _eth0 = link::eth0(); ... eth0 = _eth0.get(); src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82004> This fits in one line? src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment81999> LOG(INFO) << "Using " << eth0 << " ..."; Here and everywhere else. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82005> Result<net::IP> _hostIP = net::ip(eth0); ... hostIP = _hostIP.get(); src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82001> CHECK_SOME should be sufficient. (CHECK_SOME checks isSome()) src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82002> Do not use '+' when streaming. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82006> Fit in one line? src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82033> Should we parse them into net::IP? src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82007> ASSERT_SOME(os::user()); src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82008> Why option here? src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82035> can you rename 'file1' to 'container1OK'. It's more easy to read in that way. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82037> Ditto. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82039> Rename 'fileLoopback' to 'contentViaLoopback' src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82040> Ditto. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82041> It's a little hard to follow here. Would you please add some comments to explain the flow here? src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82043> Create a helper function to create the launchFlags? src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82044> You don't need these two 'None()' here. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82046> Why this comment? src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82047> Probably add a sleep here (instead of busy waiting)? like os::sleep(Milliseconds(10)); src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82048> I would prefer using 'command1' and 'command2'. That's more clear and align with container1 and container2. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82049> Ditto. A helper function can remove all these duplicated logics. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/22471/#comment82051> Ditto. - Jie Yu On June 24, 2014, 5:02 p.m., Chi Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22471/ > ----------------------------------------------------------- > > (Updated June 24, 2014, 5:02 p.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 > >
