----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63862/#review191223 -----------------------------------------------------------
src/tests/containerizer/docker_tests.cpp Lines 62 (patched) <https://reviews.apache.org/r/63862/#comment268887> If you want to use it, we have a `SLEEP_COMMAND` macro that takes an int and generates either `sleep x` on Linuxish and `powershell -noprofile -command start-sleep x` on Windows. Using `ping` can be annoying because it emits output. src/tests/containerizer/docker_tests.cpp Lines 67 (patched) <https://reviews.apache.org/r/63862/#comment268889> You could declare a `Seconds(30)` and not have to use `Seconds(DOCKER_WAIT)` below. It'd be nicer if we didn't have to adjust the timeouts at all... src/tests/containerizer/docker_tests.cpp Lines 79-82 (patched) <https://reviews.apache.org/r/63862/#comment268891> Is this assumed not fail? I don't know, but if it can fail, we should `CHECK_SOME` it before `get()`. src/tests/containerizer/docker_tests.cpp Lines 84-85 (patched) <https://reviews.apache.org/r/63862/#comment268894> If you don't override it, you get this from the base class MesosTest : TemporaryDirectoryTest [here](https://github.com/apache/mesos/blob/72752fc6deb8ebcbfbd5448dc599ef3774339d31/3rdparty/stout/include/stout/tests/utils.hpp#L41), so you could call `MesosTest::SetUp()` and get the freebies from the base classes. src/tests/containerizer/docker_tests.cpp Lines 90-91 (patched) <https://reviews.apache.org/r/63862/#comment268897> Jesus... src/tests/containerizer/docker_tests.cpp Lines 121-124 (patched) <https://reviews.apache.org/r/63862/#comment268896> Would it be possible to instead change the default network setting to `bridge`? We may have talked about this, I don't remember. What should the default be if `host` doesn't work on Windows? src/tests/containerizer/docker_tests.cpp Lines 125 (patched) <https://reviews.apache.org/r/63862/#comment268898> Nit: indentation (I put clang-format up on #windows). src/tests/containerizer/docker_tests.cpp Lines 130-133 (patched) <https://reviews.apache.org/r/63862/#comment268899> Nit: indentation. src/tests/containerizer/docker_tests.cpp Lines 231 (patched) <https://reviews.apache.org/r/63862/#comment268900> We're not checking the status code? src/tests/containerizer/docker_tests.cpp Lines 316 (patched) <https://reviews.apache.org/r/63862/#comment268901> Ditto. src/tests/containerizer/docker_tests.cpp Lines 399-401 (patched) <https://reviews.apache.org/r/63862/#comment268906> Would this read better with an `all_of`? src/tests/containerizer/docker_tests.cpp Lines 400 (patched) <https://reviews.apache.org/r/63862/#comment268903> Should we maybe do `strings::remove(PREFIX, "/", container.name)` for a safer test? (Otherwise we're just assuming here that `container.name` will always be prefixed with a slash.) src/tests/containerizer/docker_tests.cpp Lines 405-412 (patched) <https://reviews.apache.org/r/63862/#comment268904> Would this read better with an `any_of`? We're really just checking that at least one of the elements of `containers.get()` matches `containerName`. src/tests/containerizer/docker_tests.cpp Lines 407 (patched) <https://reviews.apache.org/r/63862/#comment268908> Ditto on the "/". src/tests/containerizer/docker_tests.cpp Lines 417 (patched) <https://reviews.apache.org/r/63862/#comment268907> Oh I see, actually I think you want `find_if` since we're looking for that particular `containerName`. src/tests/containerizer/docker_tests.cpp Lines 420 (patched) <https://reviews.apache.org/r/63862/#comment268910> This is checking that the ID is not an empty string, but the coment says we're checking that it hasn't changed. We should probably save the ID earlier and check its value matches here. src/tests/containerizer/docker_tests.cpp Lines 421 (patched) <https://reviews.apache.org/r/63862/#comment268909> Ditto on the "/". src/tests/containerizer/docker_tests.cpp Lines 422 (patched) <https://reviews.apache.org/r/63862/#comment268911> Sweet. src/tests/containerizer/docker_tests.cpp Lines 611 (patched) <https://reviews.apache.org/r/63862/#comment268913> Should we get an actual Windows temp directory instead of `C:\tmp`? src/tests/containerizer/docker_tests.cpp Lines 578-624 (original), 703-752 (patched) <https://reviews.apache.org/r/63862/#comment268915> Wait, why did we remove it from the list of skipped tests? src/tests/containerizer/docker_tests.cpp Line 582 (original), 708-709 (patched) <https://reviews.apache.org/r/63862/#comment268914> Nit: Join these lines again. src/tests/containerizer/docker_tests.cpp Lines 629-630 (original), 757-758 (patched) <https://reviews.apache.org/r/63862/#comment268916> Nit: Join the lines again if it's < 80. src/tests/containerizer/docker_tests.cpp Line 642 (original), 770-774 (patched) <https://reviews.apache.org/r/63862/#comment268917> Oh, nit and for elsewhere: `path::join` is preferably to hard coding this. src/tests/containerizer/docker_tests.cpp Line 652 (original), 784-788 (patched) <https://reviews.apache.org/r/63862/#comment268918> `/mnt/mesos/sandbox/` is now a value in a variable somewhere, isn't it? src/tests/containerizer/docker_tests.cpp Lines 790-919 (original), 926-1058 (patched) <https://reviews.apache.org/r/63862/#comment268919> Why are these being compiled out? src/tests/containerizer/docker_tests.cpp Lines 924-925 (original), 1063-1064 (patched) <https://reviews.apache.org/r/63862/#comment268920> Nit: join. - Andrew Schwartzmeyer On Nov. 16, 2017, 10:18 a.m., Akash Gupta wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63862/ > ----------------------------------------------------------- > > (Updated Nov. 16, 2017, 10:18 a.m.) > > > Review request for mesos, Andrew Schwartzmeyer and John Kordich. > > > Bugs: MESOS-7342 > https://issues.apache.org/jira/browse/MESOS-7342 > > > Repository: mesos > > > Description > ------- > > Ported the disabled tests to run on Windows. The following tests > could not be ported due to Windows platform limitations and were > simply #ifdef'd out: > - ROOT_DOCKER_MountRelativeContainerPath (can't mount volumes inside > sandbox). > - ROOT_DOCKER_NVIDIA_GPU_DeviceAllow (no GPU container support). > - ROOT_DOCKER_NVIDIA_GPU_InspectDevices (no GPU container support). > > > Diffs > ----- > > src/tests/containerizer/docker_tests.cpp > 5cabf5a0b0164f9923008677c58806c8931cbc8d > > > Diff: https://reviews.apache.org/r/63862/diff/1/ > > > Testing > ------- > > Windows mesos-test: > [==========] 754 tests from 77 test cases ran. (270586 ms total) > [ PASSED ] 754 tests. > > > Windows DockerTest only: > [==========] 11 tests from 1 test case ran. (85617 ms total) > [ PASSED ] 11 tests. > > Linux DockerTest (only 12 tests instead of 14, because I don't have Nvidia > GPU): > [==========] 12 tests from 1 test case ran. (12413 ms total) > [ PASSED ] 12 tests. > > > Thanks, > > Akash Gupta > >
