----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17013/#review33963 -----------------------------------------------------------
Ship it! Looks good Ian. Let's get all this stuff committed please! src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/17013/#comment64000> ASSERT_SOME(mounted); src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/17013/#comment64001> . src/tests/cluster.hpp <https://reviews.apache.org/r/17013/#comment64002> Please change wrapping to match style guide. src/tests/cluster.hpp <https://reviews.apache.org/r/17013/#comment64003> Please fix wrapping. src/tests/cluster.hpp <https://reviews.apache.org/r/17013/#comment64004> Wrapping. src/tests/containerizer.hpp <https://reviews.apache.org/r/17013/#comment64005> Formatting. src/tests/containerizer.hpp <https://reviews.apache.org/r/17013/#comment64008> Why a map here but hashmap below? A comment would be great for posterity. src/tests/containerizer.hpp <https://reviews.apache.org/r/17013/#comment63815> Functions first, variables second please. src/tests/containerizer.hpp <https://reviews.apache.org/r/17013/#comment64006> Don't forget to kill newlines across all these files please! src/tests/containerizer.cpp <https://reviews.apache.org/r/17013/#comment64007> Wrapping. src/tests/containerizer.cpp <https://reviews.apache.org/r/17013/#comment64009> Use 'contains' if you can use a hashmap. src/tests/containerizer.cpp <https://reviews.apache.org/r/17013/#comment64010> Wrapping. src/tests/fault_tolerance_tests.cpp <https://reviews.apache.org/r/17013/#comment64011> Consider wrapping. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/17013/#comment64012> Reorganize the includes. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/17013/#comment64024> Same comment about calling initialize on a process as in the previous reviews. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/17013/#comment64025> Please wrap after 'CopyFrom('. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/17013/#comment64014> I'd prefer not to require 'openssl' ... can you think of an alternative? src/tests/isolator_tests.cpp <https://reviews.apache.org/r/17013/#comment64026> Newline before comment please. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/17013/#comment64027> Please wrap. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/17013/#comment64028> Use the expected value first, actual value second (i.e., swapping _GE for _LT). That's what gtest expects (and prints out "expected XXX actual YYY" too). src/tests/isolator_tests.cpp <https://reviews.apache.org/r/17013/#comment64029> Wrap. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/17013/#comment64030> Newline before comment please. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/17013/#comment64031> Wrap. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/17013/#comment64033> Ah, I see now that this wasn't your bug, but please swap if you don't mind. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/17013/#comment64034> const & for Bytes? src/tests/isolator_tests.cpp <https://reviews.apache.org/r/17013/#comment64039> This is likely doing more than async-signal safe functions. Maybe worth mentioning above this function? Also, could you not invoke the balloon program? src/tests/isolator_tests.cpp <https://reviews.apache.org/r/17013/#comment64035> Wrap. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/17013/#comment64036> Newline. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/17013/#comment64037> Wrap. ;) src/tests/isolator_tests.cpp <https://reviews.apache.org/r/17013/#comment64038> Suggestion: feel free to just do 'AWAIT_READY(isolator.method(...))' if you don't care about the future for anything else (i.e., for isolator.cleanup, isolator.destroy, etc for all of these tests). src/tests/master_tests.cpp <https://reviews.apache.org/r/17013/#comment64017> Put a newline in between lines and comments please. src/tests/mesos.cpp <https://reviews.apache.org/r/17013/#comment64018> ASSERT_SOME(mounted); src/tests/slave_recovery_tests.cpp <https://reviews.apache.org/r/17013/#comment64019> Reorganize. src/tests/slave_recovery_tests.cpp <https://reviews.apache.org/r/17013/#comment64020> You only have to tell the compiler you're using it once, it's a pretty good listener. ;) src/tests/slave_recovery_tests.cpp <https://reviews.apache.org/r/17013/#comment64022> I think it's worthwhile to note that while you're explicit about the MesosContainerizer type you're not using 'TypeParam' to create the Containerizer but instead using Containerizer::create which ultimately determines the Containerizer type that gets created. That way in the future if someone wants to add another Containerizer type they won't have to do as much code spelunking to determine what they need to change. src/tests/slave_recovery_tests.cpp <https://reviews.apache.org/r/17013/#comment64023> ASSERT_SOME(containerizer); - Benjamin Hindman On Feb. 7, 2014, 3:38 a.m., Ian Downes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17013/ > ----------------------------------------------------------- > > (Updated Feb. 7, 2014, 3:38 a.m.) > > > Review request for mesos, Benjamin Hindman, Ben Mahler, Niklas Nielsen, Jason > Dusek, and Vinod Kone. > > > Repository: mesos-git > > > Description > ------- > > Updated tests to use Containerizer. > > A few isolator specific tests haven't been updated. > > The tests require a different cgroup mount configuration; please see > https://issues.apache.org/jira/browse/MESOS-926 for preliminary documentation. > > > Diffs > ----- > > src/slave/containerizer/cgroups_launcher.cpp PRE-CREATION > src/slave/containerizer/mesos_containerizer.cpp PRE-CREATION > src/tests/allocator_tests.cpp c9592d4baf9f8a29624722930eefe82ea9c1e129 > src/tests/cgroups_isolator_tests.cpp > 1f5ce764a895fefb24126e39a283a8efc290d580 > src/tests/cgroups_tests.cpp 0e9316d6561a1339bd2a3fb3482277658beba12b > src/tests/cluster.hpp 065976c19170e995bd3766bcc7a9b0a244776108 > src/tests/containerizer.hpp PRE-CREATION > src/tests/containerizer.cpp PRE-CREATION > src/tests/environment.cpp 41b8a71f63b959abdaafca09bb3e0a5eeeae691b > src/tests/fault_tolerance_tests.cpp > 60e06cc52d102ade687b095db474c0278ad508cc > src/tests/gc_tests.cpp 6638a4ac37cf80c86cf0a7ccd609868d2581716c > src/tests/isolator.hpp 6431dd2b6f441764c1e193aa2f19a6d38848e458 > src/tests/isolator_tests.cpp 45a41ca177efbc200c5e83448f92b4f1d8ca30e1 > src/tests/master_contender_detector_tests.cpp > 5223200ee59b9ce0e1d228320e8ad9e84a567288 > src/tests/master_tests.cpp 815149a6f6794e534c57b5767fdd18c776878dc0 > src/tests/mesos.hpp d7bdaee3cfd2bad95a12daf8737896becd1dcf18 > src/tests/mesos.cpp 1b1b4cc1fd6bd2823a82e08c8e37005ab65f5874 > src/tests/monitor_tests.cpp 7988c90b4b383bd822d1294a7f0318fbd9160dc6 > src/tests/paths_tests.cpp 40c644cf945c08e08ec9bc44e7d828c2547ba4cf > src/tests/slave_recovery_tests.cpp 4779509f95f6a62c22241aff5fc12d4fc2e92084 > src/tests/test_framework_test.sh 277245d563dc129ebeaabff1fae8707110e7879a > > Diff: https://reviews.apache.org/r/17013/diff/ > > > Testing > ------- > > make check # OSX and Linux > > > Thanks, > > Ian Downes > >
