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 64387: Windows: Ported docker health check tests.
Date Thu, 07 Dec 2017 22:30:07 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64387/#review193181
-----------------------------------------------------------




src/tests/health_check_tests.cpp
Lines 102-111 (patched)
<https://reviews.apache.org/r/64387/#comment271709>

    Can we reuse those added to the other test file?



src/tests/health_check_tests.cpp
Lines 110 (patched)
<https://reviews.apache.org/r/64387/#comment271710>

    Oh, oh! You're making a custom Docker image for this anyway. Add a symlink or something
so you can just call `powershell.exe` to call `pwsh.exe`. Then some code disappears.



src/tests/health_check_tests.cpp
Lines 946 (patched)
<https://reviews.apache.org/r/64387/#comment271711>

    Probably not applicable when running in a container, but `-NoProfile` is generally recommended
for scripted code.



src/tests/health_check_tests.cpp
Lines 946-953 (patched)
<https://reviews.apache.org/r/64387/#comment271713>

    I've tried to consistently not use aliases (`New-Item -ItemType Directory` over `mkdir`)
and use the right casing `Set-Content`. It's probably not important, but so long as I'm nitpicking
I'll mention it. (And I don't necessarily agree with myself currently on not using `mkdir`).



src/tests/health_check_tests.cpp
Lines 947 (patched)
<https://reviews.apache.org/r/64387/#comment271712>

    Why save `$ri_err`, we using it? What about `$null`?



src/tests/health_check_tests.cpp
Lines 948 (patched)
<https://reviews.apache.org/r/64387/#comment271714>

    _Could_ shorten it with `if (-Not (Remove-Item ... ))` rather than `$?`.



src/tests/health_check_tests.cpp
Lines 2266 (patched)
<https://reviews.apache.org/r/64387/#comment271715>

    nit picking but your other message ended in `...` which I quite liked!



src/tests/health_check_tests.cpp
Line 2227 (original), 2279-2282 (patched)
<https://reviews.apache.org/r/64387/#comment271716>

    Should we file a `TODO` issue to come back to this when IPv6 does work? I expect that's
coming eventually.



src/tests/health_check_tests.cpp
Line 2249 (original), 2304-2307 (patched)
<https://reviews.apache.org/r/64387/#comment271717>

    Ditto, and also, would it make sense to make these functions a no-op on Windows for now
so we don't have to worry about other code calling them?



src/tests/health_check_tests.cpp
Line 2258 (original), 2318-2324 (patched)
<https://reviews.apache.org/r/64387/#comment271718>

    Could probably not repeat some of this with just:
    ```
    DockerContainerizerHealthCheckTest,
    ::testing::Values(
    #ifdef __WINDOWS__
    NetworkInfo::IPv4
    #else
    NetworkInfo::IPv4, NetworkInfo::IPv6
    #endif
    ));
    ```
    
    but it's a style choice and I know some other committers prefer the extra code over breaking
up a piece of it.



src/tests/health_check_tests.cpp
Lines 2562-2565 (original), 2647-2653 (patched)
<https://reviews.apache.org/r/64387/#comment271719>

    Is the same as the first one above?


- Andrew Schwartzmeyer


On Dec. 6, 2017, 10:17 a.m., Akash Gupta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64387/
> -----------------------------------------------------------
> 
> (Updated Dec. 6, 2017, 10:17 a.m.)
> 
> 
> Review request for mesos and Andrew Schwartzmeyer.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The `HealthCheckTest.ROOT_DOCKER_*` and
> `DockerContainerizerHealthCheckTest.*` tests now work on Windows.
> 
> 
> Diffs
> -----
> 
>   src/tests/health_check_tests.cpp 56721fac4a464f3cad40dbf4e6bc4fb7b1a780d4 
> 
> 
> Diff: https://reviews.apache.org/r/64387/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>


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