> On Dec. 7, 2017, 10:30 p.m., Andrew Schwartzmeyer wrote: > > src/tests/health_check_tests.cpp > > Lines 102-111 (patched) > > <https://reviews.apache.org/r/64387/diff/1/?file=1909687#file1909687line102> > > > > Can we reuse those added to the other test file?
Yeah. I can add the constant to `tests/mesos.hpp`, where the other global test constants are defined. However, I didn't make this patch dependent on the docker tests patch, so I think it's better to have another patch, so the change isn't duplicated. > On Dec. 7, 2017, 10:30 p.m., Andrew Schwartzmeyer wrote: > > src/tests/health_check_tests.cpp > > Lines 110 (patched) > > <https://reviews.apache.org/r/64387/diff/1/?file=1909687#file1909687line110> > > > > 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. Unfortunately that doesn't work :(. > On Dec. 7, 2017, 10:30 p.m., Andrew Schwartzmeyer wrote: > > src/tests/health_check_tests.cpp > > Lines 946 (patched) > > <https://reviews.apache.org/r/64387/diff/1/?file=1909687#file1909687line947> > > > > Probably not applicable when running in a container, but `-NoProfile` > > is generally recommended for scripted code. `-NoProfile` doesn't exist in either in the container image or powershell core. > On Dec. 7, 2017, 10:30 p.m., Andrew Schwartzmeyer wrote: > > src/tests/health_check_tests.cpp > > Lines 946-953 (patched) > > <https://reviews.apache.org/r/64387/diff/1/?file=1909687#file1909687line947> > > > > 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`). I'll clean up the powershell script a bit. > On Dec. 7, 2017, 10:30 p.m., Andrew Schwartzmeyer wrote: > > src/tests/health_check_tests.cpp > > Lines 948 (patched) > > <https://reviews.apache.org/r/64387/diff/1/?file=1909687#file1909687line949> > > > > _Could_ shorten it with `if (-Not (Remove-Item ... ))` rather than `$?`. That changes the logic. It should alternate between creating the directory and not, which tests that health check going healthy <-> unhealthy. I based it off the bash code that was used on Linux. > On Dec. 7, 2017, 10:30 p.m., Andrew Schwartzmeyer wrote: > > src/tests/health_check_tests.cpp > > Line 2227 (original), 2279-2282 (patched) > > <https://reviews.apache.org/r/64387/diff/1/?file=1909687#file1909687line2280> > > > > Should we file a `TODO` issue to come back to this when IPv6 does work? > > I expect that's coming eventually. Yeah. I expect it to some eventually. > On Dec. 7, 2017, 10:30 p.m., Andrew Schwartzmeyer wrote: > > src/tests/health_check_tests.cpp > > Line 2249 (original), 2304-2307 (patched) > > <https://reviews.apache.org/r/64387/diff/1/?file=1909687#file1909687line2305> > > > > 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? Yeah. That's test code only, but it makes sense to me to have the `#ifdef` inside it so that it doesn't break Windows when other people use it. > On Dec. 7, 2017, 10:30 p.m., Andrew Schwartzmeyer wrote: > > src/tests/health_check_tests.cpp > > Line 2258 (original), 2318-2324 (patched) > > <https://reviews.apache.org/r/64387/diff/1/?file=1909687#file1909687line2319> > > > > 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. Personally, having the `#ifdef` outside looks cleaner to me. > On Dec. 7, 2017, 10:30 p.m., Andrew Schwartzmeyer wrote: > > src/tests/health_check_tests.cpp > > Lines 2562-2565 (original), 2647-2653 (patched) > > <https://reviews.apache.org/r/64387/diff/1/?file=1909687#file1909687line2649> > > > > Is the same as the first one above? Yeah, it's the same. - Akash ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64387/#review193181 ----------------------------------------------------------- On Dec. 7, 2017, 10:30 p.m., Akash Gupta wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64387/ > ----------------------------------------------------------- > > (Updated Dec. 7, 2017, 10:30 p.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Jie Yu, Joseph Wu, and > Michael Park. > > > 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 > >
