----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52612/#review151842 -----------------------------------------------------------
src/tests/containerizer/runtime_isolator_tests.cpp (lines 342 - 348) <https://reviews.apache.org/r/52612/#comment220447> I don't like the fact that we're switching from v0 to v1 and from v1 to v0 in the test. Since this is a top level end to end test, can we use v1 consistently? src/tests/containerizer/runtime_isolator_tests.cpp (lines 344 - 348) <https://reviews.apache.org/r/52612/#comment220437> Can you add an optional type (with a default value) to `createExecutorInfo`? src/tests/containerizer/runtime_isolator_tests.cpp (lines 344 - 348) <https://reviews.apache.org/r/52612/#comment220440> Why setting `executorInfo` here? Can you move is down where you did `executorInfo.mutable_framework_id()->CopyFrom(devolve(frameworkId));`? src/tests/containerizer/runtime_isolator_tests.cpp (line 357) <https://reviews.apache.org/r/52612/#comment220446> one line above. src/tests/containerizer/runtime_isolator_tests.cpp (line 369) <https://reviews.apache.org/r/52612/#comment220434> any reason not 'using v1::scheduler::Event' above? src/tests/containerizer/runtime_isolator_tests.cpp (lines 382 - 385) <https://reviews.apache.org/r/52612/#comment220439> I would factor this into `createCallSubscribe(frameworkInfo)` src/tests/containerizer/runtime_isolator_tests.cpp (lines 406 - 412) <https://reviews.apache.org/r/52612/#comment220443> Let's create `createCommandInfo` helper which takes a value and a vector of arguments. src/tests/containerizer/runtime_isolator_tests.cpp (lines 415 - 417) <https://reviews.apache.org/r/52612/#comment220444> We already have `createImageFromDocker` src/tests/containerizer/runtime_isolator_tests.cpp (lines 419 - 421) <https://reviews.apache.org/r/52612/#comment220445> We have `createContainerInfo` src/tests/containerizer/runtime_isolator_tests.cpp (lines 432 - 440) <https://reviews.apache.org/r/52612/#comment220441> This is too tedious! Can we introduce `createCallAccept` (in test/mesos.hpp) with a bunch of parameters? src/tests/containerizer/runtime_isolator_tests.cpp (line 451) <https://reviews.apache.org/r/52612/#comment220448> Remove the 60 seconds here. I think this is before we strip down the linux rootfs src/tests/containerizer/runtime_isolator_tests.cpp (lines 458 - 465) <https://reviews.apache.org/r/52612/#comment220442> Same here. - Jie Yu On Oct. 6, 2016, 6:36 p.m., Gilbert Song wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52612/ > ----------------------------------------------------------- > > (Updated Oct. 6, 2016, 6:36 p.m.) > > > Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, and > Timothy Chen. > > > Bugs: MESOS-6292 > https://issues.apache.org/jira/browse/MESOS-6292 > > > Repository: mesos > > > Description > ------- > > Added nested container tests for docker runtime isolator. > > > Diffs > ----- > > src/tests/containerizer/runtime_isolator_tests.cpp > c0c11607024b6a80d5bf5a486b91f7905a9083d7 > > Diff: https://reviews.apache.org/r/52612/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Gilbert Song > >
