> On 五月 2, 2016, 9:44 p.m., Jie Yu wrote: > > src/tests/containerizer/docker_volume_isolator_tests.cpp, lines 172-181 > > <https://reviews.apache.org/r/46140/diff/2/?file=1363610#file1363610line172> > > > > Why copy the code here? I don't think we need a 'rootfs' to test some > > basic functionalities. > > > > Let's not test the rootfs path yet.
Here I want to test about two kind of different cases: With rootfs and without rootfs, can you please show more detail why do you think that we do not need to test the rootfs path now? > On 五月 2, 2016, 9:44 p.m., Jie Yu wrote: > > src/tests/containerizer/docker_volume_isolator_tests.cpp, lines 269-271 > > <https://reviews.apache.org/r/46140/diff/2/?file=1363610#file1363610line269> > > > > Hum, please do not blindly copy the comments. Does it apply here?? Sorry, I have fixed this in a non publihsed patch. > On 五月 2, 2016, 9:44 p.m., Jie Yu wrote: > > src/tests/containerizer/docker_volume_isolator_tests.cpp, line 94 > > <https://reviews.apache.org/r/46140/diff/2/?file=1363610#file1363610line94> > > > > Why do you need the additional `_dvdcli` parameter? Is it used? Because the constructor of `DriverClient` need this parameter and the `MockDockerVolumeDriverClient` is derived from that class. > On 五月 2, 2016, 9:44 p.m., Jie Yu wrote: > > src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp, line 72 > > <https://reviews.apache.org/r/46140/diff/2/?file=1363609#file1363609line72> > > > > I don't think you need this change. See my comments below. Since I need to make the class `MockDockerVolumeDriverClient` derived from `DriverClient`, so here need to udpate it to `protected`. I was following the `docker` test style here: https://github.com/apache/mesos/blob/master/src/docker/docker.hpp#L171-L173 https://github.com/apache/mesos/blob/master/src/tests/mesos.cpp#L606-L610 > On 五月 2, 2016, 9:44 p.m., Jie Yu wrote: > > src/tests/containerizer/docker_volume_isolator_tests.cpp, lines 113-124 > > <https://reviews.apache.org/r/46140/diff/2/?file=1363610#file1363610line113> > > > > This is not needed. Let's make it a pure mock object. The `mount` will failed if I did not create the `mountPoint` in this mock method as the `bind mount` need a real path mount to the container. > On 五月 2, 2016, 9:44 p.m., Jie Yu wrote: > > src/tests/containerizer/docker_volume_isolator_tests.cpp, line 91 > > <https://reviews.apache.org/r/46140/diff/2/?file=1363610#file1363610line91> > > > > I would call it: > > ``` > > MockDockerVolumeDriverClient > > ``` > > > > Let's make it a pure Mock without a concrete implementation. You can > > setup Mock expectation in the following tests. I saw that my test cases are very similar with docker test, so I was following the style of docker here, https://github.com/apache/mesos/blob/master/src/tests/mesos.cpp#L606-L626 https://github.com/apache/mesos/blob/master/src/tests/mesos.hpp#L1498-L1527 - Guangya ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46140/#review131378 ----------------------------------------------------------- On 四月 27, 2016, 4:28 p.m., Guangya Liu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46140/ > ----------------------------------------------------------- > > (Updated 四月 27, 2016, 4:28 p.m.) > > > Review request for mesos, Gilbert Song and Jie Yu. > > > Bugs: MESOS-5266 > https://issues.apache.org/jira/browse/MESOS-5266 > > > Repository: mesos > > > Description > ------- > > WIP: Added dvd isolator test "ROOT_LaunchCommandExecutorWithVolumes". > > > Diffs > ----- > > src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca > src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp > 070d52018e82ed3e46fb1b79714ffc4716f6a306 > src/tests/containerizer/docker_volume_isolator_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/46140/diff/ > > > Testing > ------- > > make > make check > > > Thanks, > > Guangya Liu > >
