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