----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64387/#review196738 -----------------------------------------------------------
src/tests/environment.cpp Lines 350-356 (patched) <https://reviews.apache.org/r/64387/#comment276568> Could this be const-qualified if it were brace-initialized? src/tests/environment.cpp Lines 364-365 (patched) <https://reviews.apache.org/r/64387/#comment276570> This is going to error if `!status.isSome()`, as then `status.get()` (in the stringify) is invalid. src/tests/environment.cpp Lines 370 (patched) <https://reviews.apache.org/r/64387/#comment276572> Will there be a log message before we await so it doesn't look like we're hung for 30 seconds? src/tests/environment.cpp Lines 382-390 (patched) <https://reviews.apache.org/r/64387/#comment276573> Nit: `const int result` src/tests/environment.cpp Lines 388-389 (patched) <https://reviews.apache.org/r/64387/#comment276574> Is this the only possible way it could error? Is there a way we could run this and keep the `stderr` output? src/tests/environment.cpp Lines 402 (patched) <https://reviews.apache.org/r/64387/#comment276575> Just a heads up, Jame's chain https://reviews.apache.org/r/65202/ is replace `Seconds(15)` with a flag-configured variable. src/tests/environment.cpp Lines 412-413 (patched) <https://reviews.apache.org/r/64387/#comment276576> Is it not possible to run them with `--rm`? I don't like possibly leaving stale containers around if we crash or are interrupted or something. src/tests/health_check_tests.cpp Lines 96-99 (original), 93-96 (patched) <https://reviews.apache.org/r/64387/#comment276577> While we're in here, I guess I never fixed this, but we should have `-NoProfile` for sure on this. src/tests/health_check_tests.cpp Lines 973 (patched) <https://reviews.apache.org/r/64387/#comment276579> Should the value be `foo` to match the other command? src/tests/health_check_tests.cpp Lines 2283-2284 (patched) <https://reviews.apache.org/r/64387/#comment276581> Oof... We might want a test filter for the tests that can wait up to an hour, or fill a machine's disk. I'm thinking slow/small CI machines are going to run into a problem. src/tests/health_check_tests.cpp Lines 2404 (patched) <https://reviews.apache.org/r/64387/#comment276582> Is this something we should put into a constant at the top? I think it's liable to change in the future as Server Core is updated. src/tests/health_check_tests.cpp Lines 2544-2547 (original), 2663-2669 (patched) <https://reviews.apache.org/r/64387/#comment276583> This whole block keeps getting repeated. I twas repeated originally, but maybe now is the time to refactor ;) src/tests/mesos.hpp Lines 2173-2174 (patched) <https://reviews.apache.org/r/64387/#comment276584> It seems that this a feature Windows will support in the future; should we have an issue and TODO to track it? - Andrew Schwartzmeyer On Jan. 30, 2018, 2:21 a.m., Akash Gupta wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64387/ > ----------------------------------------------------------- > > (Updated Jan. 30, 2018, 2:21 a.m.) > > > Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, Gaston > Kleiman, Jie Yu, Joseph Wu, and Michael Park. > > > Bugs: MESOS-8498 > https://issues.apache.org/jira/browse/MESOS-8498 > > > Repository: mesos > > > Description > ------- > > The `HealthCheckTest.ROOT_DOCKER_*` and > `DockerContainerizerHealthCheckTest.*` tests now work on Windows. > > > Diffs > ----- > > src/tests/environment.cpp 13a4c9514fcd3016fe623c597decd067457e86cd > src/tests/health_check_tests.cpp 1893c85169f5e94e164434b93e6a24268224225d > src/tests/mesos.hpp a35c68e8645384f6244d17e37cad71373aba6893 > > > Diff: https://reviews.apache.org/r/64387/diff/10/ > > > Testing > ------- > > Windows Server: > [==========] Running 5 tests from 2 test cases. > [----------] Global test environment set-up. > [----------] 2 tests from HealthCheckTest > [ RUN ] HealthCheckTest.ROOT_DOCKER_DockerHealthyTask > [ OK ] HealthCheckTest.ROOT_DOCKER_DockerHealthyTask (21263 ms) > [ RUN ] HealthCheckTest.ROOT_DOCKER_DockerHealthStatusChange > [ OK ] HealthCheckTest.ROOT_DOCKER_DockerHealthStatusChange (23512 ms) > [----------] 2 tests from HealthCheckTest (44835 ms total) > > [----------] 3 tests from NetworkProtocol/DockerContainerizerHealthCheckTest > [ RUN ] > NetworkProtocol/DockerContainerizerHealthCheckTest.ROOT_DOCKER_USERNETWORK_NETNAMESPACE_HealthyTaskViaHTTP/0 > [ OK ] > NetworkProtocol/DockerContainerizerHealthCheckTest.ROOT_DOCKER_USERNETWORK_NETNAMESPACE_HealthyTaskViaHTTP/0 > (28487 ms) > [ RUN ] > NetworkProtocol/DockerContainerizerHealthCheckTest.ROOT_DOCKER_USERNETWORK_NETNAMESPACE_HealthyTaskViaHTTPS/0 > [ OK ] > NetworkProtocol/DockerContainerizerHealthCheckTest.ROOT_DOCKER_USERNETWORK_NETNAMESPACE_HealthyTaskViaHTTPS/0 > (26447 ms) > [ RUN ] > NetworkProtocol/DockerContainerizerHealthCheckTest.ROOT_DOCKER_USERNETWORK_NETNAMESPACE_HealthyTaskViaTCP/0 > [ OK ] > NetworkProtocol/DockerContainerizerHealthCheckTest.ROOT_DOCKER_USERNETWORK_NETNAMESPACE_HealthyTaskViaTCP/0 > (26264 ms) > [----------] 3 tests from NetworkProtocol/DockerContainerizerHealthCheckTest > (81268 ms total) > > [----------] Global test environment tear-down > [==========] 5 tests from 2 test cases ran. (126559 ms total) > [ PASSED ] 5 tests > > Rest of tests pass. > > Windows Client (Disabled network health checks): > Proof that network health checks are skipped on Windows Client. > C:\Program Files\Docker\Docker\Resources\bin\docker.exe: Error response from > daemon: sharing of hyperv containers network is not supported. > 356b087e7fa640f83fe27ebeb3396bfc7b2bbebd917aeaec0508b887b41d31f4 > ------------------------------------------------------------- > We cannot run any Docker health checks tests because: > Running in another container's namespace is not supported on this version of > Windows. > > Rest rests pass. > > Linux: > make check passes > > > Thanks, > > Akash Gupta > >
