mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Benjamin Hindman" <b...@berkeley.edu>
Subject Re: Review Request 26862: Fix docker flaky tests
Date Sun, 26 Oct 2014 01:11:06 GMT

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



src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/26862/#comment99607>

    If we're going to take ownership than we need to make that explicit in the constructor
please!
    
    Also, why a 'const Docker'?



src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/26862/#comment99608>

    Can you please redirect people to the explanation in src/tests/containerizer.cpp which
explains why we set up defaults with EXPECT_CALL/WillRepeatedly instead of ON_CALL/WillByDefault.
As an example, see MockGarbageCollector() in src/tests/mesos.hpp. Also, did you copy this
code from someplace that doesn't have a comment? Because it would be great to get that everywhere
so people don't end up making that change (like I once did) only to have to undo it because
of gmock annoyances.



src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/26862/#comment99609>

    Hmm, I feel like I ask this question everytime, but can we not just s/MockDocker::_logs/Docker::logs/
here and it does the right thing?



src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/26862/#comment99613>

    This doesn't look like it was used by any call sites.



src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/26862/#comment99610>

    s/> >/>>/



src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/26862/#comment99611>

    Shouldn't this just be an automatic upcast so the explicit cast isn't necessary?



src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/26862/#comment99612>

    I don't understand, isn't the point of this test to show that the container was at least
stopped (since now we're not removing them until after the delay)? The same for the other
tests where this was removed too?


- Benjamin Hindman


On Oct. 22, 2014, 12:46 a.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26862/
> -----------------------------------------------------------
> 
> (Updated Oct. 22, 2014, 12:46 a.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Docker tests are flaky, mostly around getting expected output from the docker container
forwarded to stdout/stderr.
> 
> This is due to Docker not always have the stdout/stderr output available for docker logs
if kill/rm is called.
> 
> 
> Diffs
> -----
> 
>   src/docker/docker.hpp 9656f15 
>   src/docker/docker.cpp e09b51c 
>   src/slave/containerizer/docker.hpp fbbd45d 
>   src/slave/containerizer/docker.cpp 9a29489 
>   src/tests/docker_containerizer_tests.cpp 67d60a8 
>   src/tests/docker_tests.cpp 04139af 
>   src/tests/environment.cpp 4dd78e7 
> 
> Diff: https://reviews.apache.org/r/26862/diff/
> 
> 
> Testing
> -------
> 
> make with gtest_repeat=-1 gtest_shuffle=1
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>


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