mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Guangya Liu <gyliu...@gmail.com>
Subject Re: Review Request 49689: Added Appc runtime isolator tests.
Date Wed, 27 Jul 2016 06:47:54 GMT

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



More comments:
1) Seems you are losing two cases to handle the case of `sh=0,value=1,argv=0,exec=1` and `sh=0,value=1,argv=1,exec=1`,
do you want to add those two cases?
2) can you please create another patch to rename the `runtime_isolator_tests.cpp` to `docker_runtime_isolator_tests.cpp`?


src/Makefile.am (line 2127)
<https://reviews.apache.org/r/49689/#comment209566>

    move this to `mesos_tests_SOURCES` as this is only valid for linux platform



src/tests/containerizer/appc_archive.hpp (line 42)
<https://reviews.apache.org/r/49689/#comment209548>

    s/a/an



src/tests/containerizer/appc_archive.hpp (line 47)
<https://reviews.apache.org/r/49689/#comment209549>

    s/a/an



src/tests/containerizer/appc_archive.hpp (lines 52 - 57)
<https://reviews.apache.org/r/49689/#comment209552>

    s/string/std::string



src/tests/containerizer/appc_archive.hpp (lines 52 - 55)
<https://reviews.apache.org/r/49689/#comment209557>

    Move this right before line 99



src/tests/containerizer/appc_archive.hpp (line 53)
<https://reviews.apache.org/r/49689/#comment209550>

    kill this line



src/tests/containerizer/appc_archive.hpp (line 99)
<https://reviews.apache.org/r/49689/#comment209553>

    s/string/std::string



src/tests/containerizer/appc_archive.hpp (line 103)
<https://reviews.apache.org/r/49689/#comment209554>

    kill this line



src/tests/containerizer/appc_archive.hpp (lines 111 - 112)
<https://reviews.apache.org/r/49689/#comment209555>

    new line here



src/tests/containerizer/appc_archive.hpp (line 117)
<https://reviews.apache.org/r/49689/#comment209556>

    s/string/std::string



src/tests/containerizer/appc_runtime_isolator_tests.cpp (line 17)
<https://reviews.apache.org/r/49689/#comment209574>

    kill this



src/tests/containerizer/appc_runtime_isolator_tests.cpp (line 36)
<https://reviews.apache.org/r/49689/#comment209561>

    We generally don't rely on transitive includes, because it's easier to maintain (quickly
scan a file and add includes for all symbols present). It is better adding #include<string>
in this header file.



src/tests/containerizer/appc_runtime_isolator_tests.cpp (line 38)
<https://reviews.apache.org/r/49689/#comment209560>

    kill this



src/tests/containerizer/appc_runtime_isolator_tests.cpp (line 40)
<https://reviews.apache.org/r/49689/#comment209562>

    include header file for this



src/tests/containerizer/appc_runtime_isolator_tests.cpp (line 42)
<https://reviews.apache.org/r/49689/#comment209563>

    ditto



src/tests/containerizer/appc_runtime_isolator_tests.cpp (line 78)
<https://reviews.apache.org/r/49689/#comment209567>

    kill this



src/tests/containerizer/appc_runtime_isolator_tests.cpp (line 82)
<https://reviews.apache.org/r/49689/#comment209571>

    All of the test cases should use `ROOT_` as prefix so as to filter out those test cases
if the test was not running as `root`.
    
    Ditto for other test cases.



src/tests/containerizer/appc_runtime_isolator_tests.cpp (line 90)
<https://reviews.apache.org/r/49689/#comment209568>

    You are losing `AWAIR_READY` here, you should also call an `ASSERT_TRUE(os::exists(xxx))`
here to make sure the image exist.



src/tests/containerizer/appc_runtime_isolator_tests.cpp (lines 102 - 103)
<https://reviews.apache.org/r/49689/#comment209569>

    new line here



src/tests/containerizer/appc_runtime_isolator_tests.cpp (lines 176 - 177)
<https://reviews.apache.org/r/49689/#comment209570>

    You should also call an `ASSERT_TRUE(os::exists(xxx))` here to make sure the image exist.



src/tests/containerizer/appc_runtime_isolator_tests.cpp (lines 262 - 263)
<https://reviews.apache.org/r/49689/#comment209573>

    You should also call an `ASSERT_TRUE(os::exists(xxx))` here to make sure the image exist.



src/tests/containerizer/appc_runtime_isolator_tests.cpp (lines 346 - 347)
<https://reviews.apache.org/r/49689/#comment209572>

    You should also call an `ASSERT_TRUE(os::exists(xxx))` here to make sure the image exist.



src/tests/containerizer/appc_runtime_isolator_tests.cpp (line 419)
<https://reviews.apache.org/r/49689/#comment209575>

    kill this


- Guangya Liu


On 七月 14, 2016, 5:28 p.m., Srinivas Brahmaroutu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49689/
> -----------------------------------------------------------
> 
> (Updated 七月 14, 2016, 5:28 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-4778
>     https://issues.apache.org/jira/browse/MESOS-4778
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added Appc runtime isolator tests.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 9f2b0585fa901918431530b967b6bbb0f286c595 
>   src/tests/containerizer/appc_archive.hpp PRE-CREATION 
>   src/tests/containerizer/appc_runtime_isolator_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/49689/diff/
> 
> 
> Testing
> -------
> 
> Make check
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>


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