mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ben Mahler <benjamin.mah...@gmail.com>
Subject Re: Review Request 46175: Added a test for slavePostFetchHook.
Date Wed, 13 Apr 2016 23:00:00 GMT

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




src/examples/test_hook_module.cpp (line 197)
<https://reviews.apache.org/r/46175/#comment192239>

    Mind committing this as a separate patch?



src/tests/hook_tests.cpp (lines 751 - 753)
<https://reviews.apache.org/r/46175/#comment192249>

    Why did you need the mock here?



src/tests/hook_tests.cpp (line 761)
<https://reviews.apache.org/r/46175/#comment192246>

    How about no newline here to make the code block more clear and to be consistent with
StartMaster above?



src/tests/hook_tests.cpp (lines 764 - 768)
<https://reviews.apache.org/r/46175/#comment192250>

    Ditto here, do you need the mock?



src/tests/hook_tests.cpp (line 776)
<https://reviews.apache.org/r/46175/#comment192247>

    How about no newline here to make the code block more clear and to be consistent with
StartMaster above?



src/tests/hook_tests.cpp (line 797)
<https://reviews.apache.org/r/46175/#comment192244>

    `offers->size`



src/tests/hook_tests.cpp (line 806)
<https://reviews.apache.org/r/46175/#comment192241>

    How about the following to make the sandbox from TemporaryDirectoryTest::sandbox usage
more explicit?
    
    ```
    const string file = path::join(sandbox.get(), "post_fetch_hook");
    ```



src/tests/hook_tests.cpp (line 823)
<https://reviews.apache.org/r/46175/#comment192251>

    Seems like you could remove the repeatedly case, we don't expect more updates, right?



src/tests/hook_tests.cpp (line 837)
<https://reviews.apache.org/r/46175/#comment192243>

    Why `docker.get()->ps` instead of `docker->ps`?



src/tests/hook_tests.cpp (line 843)
<https://reviews.apache.org/r/46175/#comment192242>

    Why `docker.get()->rm` instead of `docker->rm`?


- Ben Mahler


On April 13, 2016, 10:33 p.m., Jie Yu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46175/
> -----------------------------------------------------------
> 
> (Updated April 13, 2016, 10:33 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-5209
>     https://issues.apache.org/jira/browse/MESOS-5209
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added a test for slavePostFetchHook.
> 
> 
> Diffs
> -----
> 
>   src/examples/test_hook_module.cpp 3ff9fd71c275fd1a705ab3aca7a8041f29289bb0 
>   src/tests/hook_tests.cpp 97ff55ac7ec875b5ba5d4f17d80646c2d1dd4142 
> 
> Diff: https://reviews.apache.org/r/46175/diff/
> 
> 
> Testing
> -------
> 
> `sudo bin/mesos-tests.sh --gtest_filter=*ROOT_DOCKER_VerifySlavePostFetchHook* `
> 
> 
> Thanks,
> 
> Jie Yu
> 
>


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