----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56213/#review165049 -----------------------------------------------------------
src/tests/check_tests.cpp (lines 62 - 63) <https://reviews.apache.org/r/56213/#comment236889> i don't see tests for these in this review. if they are added in the latter part of the chain, please don't mention them here. or add a TODO in front of them. src/tests/check_tests.cpp (line 68) <https://reviews.apache.org/r/56213/#comment236886> Remove `call` prefixes for these functions. src/tests/check_tests.cpp (line 73) <https://reviews.apache.org/r/56213/#comment236905> Any particular reason for using v1 scheduler API? The general recommendation has been to use v0 scheduler API unless you are testing v1 features. The main reason being most users are still using v0 scheduler API. src/tests/check_tests.cpp (line 138) <https://reviews.apache.org/r/56213/#comment236887> no need for this. src/tests/check_tests.cpp (line 141) <https://reviews.apache.org/r/56213/#comment236888> s/reconciledTask/reconcile/ src/tests/check_tests.cpp (line 171) <https://reviews.apache.org/r/56213/#comment236890> Instead of these long test names how about you create a fixture for CommandExecutor check tests for better namespacing? class CommandExecutorCheckTest : public CheckTest {}; TEST_F(CommandExecutorCheckTest, CommandCheckDeliveredAndReconciled) src/tests/check_tests.cpp (line 177) <https://reviews.apache.org/r/56213/#comment236893> s/agent/slave/ here and everywhere else. we are not doing this change yet. src/tests/check_tests.cpp (line 237) <https://reviews.apache.org/r/56213/#comment236894> s/checkCommand/command/ here and below. src/tests/check_tests.cpp (line 241) <https://reviews.apache.org/r/56213/#comment236895> s/exitStatusVar/variable/ here and below. src/tests/check_tests.cpp (line 249) <https://reviews.apache.org/r/56213/#comment236892> why auto here? it's not clear to me what the type is from just looking at this statement. please fix it here and everywhere else. src/tests/check_tests.cpp (line 391) <https://reviews.apache.org/r/56213/#comment236898> s/checkCommandString/command/ src/tests/check_tests.cpp (lines 505 - 515) <https://reviews.apache.org/r/56213/#comment236900> this description needs to be updated to reflect the sleep? src/tests/check_tests.cpp (line 517) <https://reviews.apache.org/r/56213/#comment236899> s/checkCommandString/command/ src/tests/check_tests.cpp (lines 673 - 677) <https://reviews.apache.org/r/56213/#comment236902> also check for health check status? - Vinod Kone On Feb. 8, 2017, 4:56 p.m., Alexander Rukletsov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56213/ > ----------------------------------------------------------- > > (Updated Feb. 8, 2017, 4:56 p.m.) > > > Review request for mesos, Gastón Kleiman and Vinod Kone. > > > Bugs: MESOS-6906 > https://issues.apache.org/jira/browse/MESOS-6906 > > > Repository: mesos > > > Description > ------- > > See summary. > > > Diffs > ----- > > src/tests/check_tests.cpp b5a5d8e80c80b480992a3c8160ee7d0e4443111c > src/tests/mesos.hpp b450a04dfbf3bbeeb6b605fb78097dca390cbdbe > > Diff: https://reviews.apache.org/r/56213/diff/ > > > Testing > ------- > > See https://reviews.apache.org/r/56218/ > > > Thanks, > > Alexander Rukletsov > >
