mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Schwartzmeyer <and...@schwartzmeyer.com>
Subject Re: Review Request 63862: Ported docker_tests.cpp to Windows.
Date Thu, 16 Nov 2017 19:25:06 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message