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




src/tests/containerizer/provisioner_appc_tests.cpp (line 240)
<https://reviews.apache.org/r/43969/#comment182591>

    you can do return JSON::parse(...) here, right?



src/tests/containerizer/provisioner_appc_tests.cpp (lines 241 - 265)
<https://reviews.apache.org/r/43969/#comment182592>

    Can you use c++11 string literal here (to avoid backslashes and quotes).
    
    http://en.cppreference.com/w/cpp/language/string_literal
    
    See example here:
    
https://github.com/apache/mesos/blob/master/src/tests/containerizer/docker_spec_tests.cpp#L104



src/tests/containerizer/provisioner_appc_tests.cpp (line 455)
<https://reviews.apache.org/r/43969/#comment182595>

    Can you call it TestAppcImageServer?



src/tests/containerizer/provisioner_appc_tests.cpp (line 479)
<https://reviews.apache.org/r/43969/#comment182594>

    Ditto on using c++ string literals. You can also use strings::format to 
avoid inlining varaibles.
    
    ```
    %s/TestAppcImageServer/image
    ```



src/tests/containerizer/provisioner_appc_tests.cpp (line 510)
<https://reviews.apache.org/r/43969/#comment182598>

    We might want to serve multiple images. Can you make it more general so 
that we can serve any files under 'server' directory?



src/tests/containerizer/provisioner_appc_tests.cpp (line 528)
<https://reviews.apache.org/r/43969/#comment182597>

    We avoid static non-pod. Can you just use:
    ```
    const string serverDir = path::join(os::getcwd(), "server");
    ```



src/tests/containerizer/provisioner_appc_tests.cpp (line 550)
<https://reviews.apache.org/r/43969/#comment182596>

    We don't use 'override' in our code base. Can you remove them for now for 
consistency?



src/tests/containerizer/provisioner_appc_tests.cpp (lines 555 - 558)
<https://reviews.apache.org/r/43969/#comment182599>

    I'd like this to be called by the test. We might want to prepare multiple 
images to test the code path of fetching dependencies.


- Jie Yu


On Feb. 24, 2016, 11:19 p.m., Jojy Varghese wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43969/
> -----------------------------------------------------------
> 
> (Updated Feb. 24, 2016, 11:19 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4748
>     https://issues.apache.org/jira/browse/MESOS-4748
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added simple appc Fetcher test with mock HTTP image server.
> 
> 
> Diffs
> -----
> 
>   src/tests/containerizer/provisioner_appc_tests.cpp 
> 9d9779ac04e32348ce0e28da54fd7aa039a7fbcd 
> 
> Diff: https://reviews.apache.org/r/43969/diff/
> 
> 
> Testing
> -------
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Reply via email to