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