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