----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22471/#review45851 -----------------------------------------------------------
src/tests/port_mapping_tests.cpp <https://reviews.apache.org/r/22471/#comment80909> Why is this a ContainerizerTest rather than an IsolatorTest? Directly testing the isolator itself should simplify the tests! Don't need to worry about offers, can set the ContainerID, .... src/tests/port_mapping_tests.cpp <https://reviews.apache.org/r/22471/#comment80908> Could implement CreateSlaveFlags here to selectively enable the network isolator. src/tests/port_mapping_tests.cpp <https://reviews.apache.org/r/22471/#comment80862> I've seen this used at least twice to get the main interface - what about implementing it in stout? src/tests/port_mapping_tests.cpp <https://reviews.apache.org/r/22471/#comment80859> Correct indentation. src/tests/port_mapping_tests.cpp <https://reviews.apache.org/r/22471/#comment80865> Also seen this more than once - put an implementation into stout? src/tests/port_mapping_tests.cpp <https://reviews.apache.org/r/22471/#comment80878> Why is this randomized? If there's a particular reason could you please comment on it, else just pick a port. src/tests/port_mapping_tests.cpp <https://reviews.apache.org/r/22471/#comment80866> It won't ever make sense to run these tests again any other containerizer would it? Could just use ContainerizerTest<MesosContainerizer> and avoid having typed tests. src/tests/port_mapping_tests.cpp <https://reviews.apache.org/r/22471/#comment80914> What does it mean if it already exists...? src/tests/port_mapping_tests.cpp <https://reviews.apache.org/r/22471/#comment80883> can you get each "nc" command on a single line so it's easier to read? src/tests/port_mapping_tests.cpp <https://reviews.apache.org/r/22471/#comment80884> Why is this-> needed? src/tests/port_mapping_tests.cpp <https://reviews.apache.org/r/22471/#comment80888> Can you comment at the start of the test what it's doing and which connections should work? src/tests/port_mapping_tests.cpp <https://reviews.apache.org/r/22471/#comment80891> ditto to have each command chain on a separate line src/tests/port_mapping_tests.cpp <https://reviews.apache.org/r/22471/#comment80885> Other tests clear and re-use the tasks vector. src/tests/port_mapping_tests.cpp <https://reviews.apache.org/r/22471/#comment80890> s/nee/need/ src/tests/port_mapping_tests.cpp <https://reviews.apache.org/r/22471/#comment80905> use EXPECT_ unless you absolutely require something for the test to be able to continue. src/tests/port_mapping_tests.cpp <https://reviews.apache.org/r/22471/#comment80906> ditto. src/tests/port_mapping_tests.cpp <https://reviews.apache.org/r/22471/#comment80900> ditto here and elsewhere src/tests/port_mapping_tests.cpp <https://reviews.apache.org/r/22471/#comment80901> style, move first "echo..." to next line src/tests/port_mapping_tests.cpp <https://reviews.apache.org/r/22471/#comment80910> This only needs to be done explicitly when there's no slave + containerizer in the cluster that knows about the container(s), otherwise it gets done in Shutdown(). src/tests/port_mapping_tests.cpp <https://reviews.apache.org/r/22471/#comment80902> ditto. src/tests/port_mapping_tests.cpp <https://reviews.apache.org/r/22471/#comment80903> ditto src/tests/port_mapping_tests.cpp <https://reviews.apache.org/r/22471/#comment80904> ditto src/tests/port_mapping_tests.cpp <https://reviews.apache.org/r/22471/#comment80915> better to pluralize this and use containers2 later when you call again src/tests/port_mapping_tests.cpp <https://reviews.apache.org/r/22471/#comment80922> ditto src/tests/port_mapping_tests.cpp <https://reviews.apache.org/r/22471/#comment80916> style src/tests/port_mapping_tests.cpp <https://reviews.apache.org/r/22471/#comment80921> ditto src/tests/port_mapping_tests.cpp <https://reviews.apache.org/r/22471/#comment80924> ditto. src/tests/port_mapping_tests.cpp <https://reviews.apache.org/r/22471/#comment80926> style for the command string src/tests/port_mapping_tests.cpp <https://reviews.apache.org/r/22471/#comment80925> ditto. src/tests/port_mapping_tests.cpp <https://reviews.apache.org/r/22471/#comment80927> ditto src/tests/port_mapping_tests.cpp <https://reviews.apache.org/r/22471/#comment80929> ditto, not necessary. - Ian Downes On June 13, 2014, 11:02 a.m., Chi Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22471/ > ----------------------------------------------------------- > > (Updated June 13, 2014, 11:02 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/Makefile.am c91b438 > src/tests/port_mapping_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/22471/diff/ > > > Testing > ------- > > make check. > > > Thanks, > > Chi Zhang > >
