mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Akash Gupta <akash-gu...@hotmail.com>
Subject Re: Review Request 64387: Windows: Ported docker health check tests.
Date Wed, 13 Dec 2017 01:46:39 GMT


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


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