mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Schwartzmeyer <and...@schwartzmeyer.com>
Subject Re: Review Request 63862: Windows: Ported docker_tests.cpp.
Date Thu, 11 Jan 2018 18:13:53 GMT


> On Jan. 10, 2018, 5:27 a.m., Alexander Rukletsov wrote:
> > src/tests/containerizer/docker_tests.cpp
> > Lines 139-153 (patched)
> > <https://reviews.apache.org/r/63862/diff/5/?file=1932789#file1932789line139>
> >
> >     Again, this is something we should likely do in a broader scope. Chances are,
someone else in a different place will try to solve the same problem.
> 
> Andrew Schwartzmeyer wrote:
>     Same as above. At the point of someone else solving the same problem, then it would
make sense to move the code into a more general purpose location and reuse it. But we shouldn't
do it until then, because otherwise we're just guessing at future technical needs.
> 
> Akash Gupta wrote:
>     This is specific to Docker and only for test code that checks the container exit
value, which is onlt used here. So, I don't think other places will ever use this.

I was talking with Alex, and while I don't think `stout` is necessarily the right place for
this kind of code, there is potential clean up here:

```
src/tests/containerizer/docker_containerizer_tests.cpp
1288:  AWAIT_EXPECT_WEXITSTATUS_EQ(128 + SIGKILL, orphanRun);
1425:  AWAIT_EXPECT_WEXITSTATUS_EQ(128 + SIGKILL, orphanRun);

src/tests/containerizer/ns_tests.cpp
101:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s->status());

src/tests/containerizer/docker_tests.cpp
170:  AWAIT_EXPECT_WEXITSTATUS_EQ(128 + SIGKILL, status);
250:  AWAIT_EXPECT_WEXITSTATUS_EQ(128 + SIGKILL, status);
318:  AWAIT_EXPECT_WEXITSTATUS_EQ(128 + SIGKILL, run);
449:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, run);
528:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, run);
574:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, run);
623:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, run);
844:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, status);
918:  AWAIT_EXPECT_WEXITSTATUS_EQ(128 + SIGKILL, status);

src/tests/containerizer/capabilities_tests.cpp
98:  AWAIT_EXPECT_WEXITSTATUS_NE(0, s->status());
122:  AWAIT_EXPECT_WEXITSTATUS_NE(0, s->status());
151:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s->status());

3rdparty/libprocess/src/tests/reap_tests.cpp
98:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, status);
172:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, status);

3rdparty/libprocess/include/process/gtest.hpp
623:#define AWAIT_EXPECT_WEXITSTATUS_EQ_FOR(expected, actual, duration)     \
627:#define AWAIT_EXPECT_WEXITSTATUS_EQ(expected, actual)                   \
628:  AWAIT_EXPECT_WEXITSTATUS_EQ_FOR(expected, actual, Seconds(15))
664:#define AWAIT_EXPECT_WEXITSTATUS_NE_FOR(expected, actual, duration)     \
668:#define AWAIT_EXPECT_WEXITSTATUS_NE(expected, actual)                   \
669:  AWAIT_EXPECT_WEXITSTATUS_NE_FOR(expected, actual, Seconds(15))

3rdparty/libprocess/src/tests/subprocess_tests.cpp
72:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
249:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
264:  AWAIT_EXPECT_WEXITSTATUS_EQ(1, s.get().status());
323:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
344:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
371:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
411:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
442:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
465:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
497:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
531:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
562:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
599:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
617:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
700:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
763:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
789:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
818:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
847:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
879:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
```

Especially the ones that do `128 + SIGKILL`; just WTF is that even doing? No one should have
to go to [Stack Overflow](https://unix.stackexchange.com/a/99134) to understand what an assertion
in a test is doing.

It may be reasonable to go to `include/process/gtest.hpp` and make this easier, with `AWAIT_EXPECT_EXIT_SIGKILL(status)`
(and like `AWAIT_EXPECT_EXIT_SUCCESS(status)` for `== 0`).


- Andrew


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63862/#review195133
-----------------------------------------------------------


On Jan. 5, 2018, 10:33 a.m., Akash Gupta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63862/
> -----------------------------------------------------------
> 
> (Updated Jan. 5, 2018, 10:33 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, Gaston Kleiman,
Jie Yu, John Kordich, Joseph Wu, and Michael Park.
> 
> 
> Bugs: MESOS-7342
>     https://issues.apache.org/jira/browse/MESOS-7342
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Ported the disabled tests to run on Windows. The following tests
> could not be ported due to Windows platform limitations and remain
> diabled:
>   - ROOT_DOCKER_MountRelativeContainerPath (can't mount volumes inside
>     sandbox).
>   - ROOT_DOCKER_NVIDIA_GPU_DeviceAllow (no GPU container support).
>   - ROOT_DOCKER_NVIDIA_GPU_InspectDevices (no GPU container support).
> 
> 
> Diffs
> -----
> 
>   src/tests/containerizer/docker_tests.cpp 0ac4a79e03d5e11ead5a57a9749e26c20a1ddd57 
> 
> 
> Diff: https://reviews.apache.org/r/63862/diff/5/
> 
> 
> Testing
> -------
> 
> Windows mesos-test:
> [==========] 754 tests from 77 test cases ran. (270586 ms total)
> [  PASSED  ] 754 tests.
> 
> 
> Windows DockerTest only:
> [==========] 11 tests from 1 test case ran. (85617 ms total)
> [  PASSED  ] 11 tests.
> 
> Linux DockerTest (only 12 tests instead of 14, because I don't have Nvidia GPU):
> [==========] 12 tests from 1 test case ran. (12413 ms total)
> [  PASSED  ] 12 tests.
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>


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