You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Akash Gupta <ak...@hotmail.com> on 2017/12/06 18:17:46 UTC
Review Request 64387: Windows: Ported docker health check tests.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64387/
-----------------------------------------------------------
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
Re: Review Request 64387: Windows: Ported docker health check tests.
Posted by Akash Gupta <ak...@hotmail.com>.
> 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
>
>
Re: Review Request 64387: Windows: Ported docker health check tests.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
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
>
>