----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52620/#review151890 -----------------------------------------------------------
Thanks Ilya. Looks pretty good! - Mostly minor comments around moving expectations. - Renaming some `EXPECT_EQ` to `ASSERT_EQ`. They are not related to this patch per se and I am fine if you want to create another patch to address them. Also, can you run the maintenance tests with `--gtest_repeat=1000 --gtest_break_on_failure` to ensure we do not introduce any flakiness to the tests as part of this refactoring? src/tests/master_maintenance_tests.cpp (lines 1159 - 1161) <https://reviews.apache.org/r/52620/#comment220520> We prefer to keep expectations close to the actual business logic for readability. Can you move this down to L1192? src/tests/master_maintenance_tests.cpp (lines 1189 - 1191) <https://reviews.apache.org/r/52620/#comment220521> Move this below the `launchTask()` expectation on L1198 src/tests/master_maintenance_tests.cpp (line 1222) <https://reviews.apache.org/r/52620/#comment220538> Not yours: s/EXPECT_EQ/ASSERT_EQ src/tests/master_maintenance_tests.cpp (lines 1251 - 1252) <https://reviews.apache.org/r/52620/#comment220522> Move this above L1273 src/tests/master_maintenance_tests.cpp (line 1281) <https://reviews.apache.org/r/52620/#comment220539> Not yours: s/EXPECT_EQ/ASSERT_EQ src/tests/master_maintenance_tests.cpp (lines 1316 - 1317) <https://reviews.apache.org/r/52620/#comment220523> Move this before L1338 src/tests/master_maintenance_tests.cpp (line 1346) <https://reviews.apache.org/r/52620/#comment220540> Not yours: s/EXPECT_EQ/ASSERT_EQ src/tests/master_maintenance_tests.cpp (lines 1380 - 1381) <https://reviews.apache.org/r/52620/#comment220524> You can remove this expectation i.e., the `master` would be deleted on the stack *after* the scheduler library, so the library won't ever see the disconnected callback. This was not the case earlier when this test was written but we can get rid of this now. src/tests/master_maintenance_tests.cpp (lines 1496 - 1498) <https://reviews.apache.org/r/52620/#comment220526> Similar comment as per previous test. Move this expectation. src/tests/master_maintenance_tests.cpp (line 1540) <https://reviews.apache.org/r/52620/#comment220527> s/firstUpdate/update1 To be consistent on how we name these variables else where in our code. src/tests/master_maintenance_tests.cpp (line 1541) <https://reviews.apache.org/r/52620/#comment220529> s/firstUpdate/update2 src/tests/master_maintenance_tests.cpp (line 1580) <https://reviews.apache.org/r/52620/#comment220541> Not yours: s/EXPECT_EQ/ASSERT_EQ src/tests/master_maintenance_tests.cpp (lines 1594 - 1595) <https://reviews.apache.org/r/52620/#comment220530> Nit: No need to create the temporary `updateStatus`, directly do `update1->status().state()` src/tests/master_maintenance_tests.cpp (lines 1615 - 1616) <https://reviews.apache.org/r/52620/#comment220531> Ditto src/tests/master_maintenance_tests.cpp (lines 1670 - 1671) <https://reviews.apache.org/r/52620/#comment220533> Move this above L1689 src/tests/master_maintenance_tests.cpp (line 1695) <https://reviews.apache.org/r/52620/#comment220545> Not yours: s/EXPECT_EQ/ASSERT_EQ src/tests/master_maintenance_tests.cpp (lines 1705 - 1706) <https://reviews.apache.org/r/52620/#comment220534> Move this before L1725 src/tests/master_maintenance_tests.cpp (line 1731) <https://reviews.apache.org/r/52620/#comment220547> Not yours: s/EXPECT_EQ/ASSERT_EQ src/tests/master_maintenance_tests.cpp (lines 1742 - 1743) <https://reviews.apache.org/r/52620/#comment220536> Kill this as per my earlier comment. - Anand Mazumdar On Oct. 7, 2016, 5:29 p.m., Ilya Pronin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52620/ > ----------------------------------------------------------- > > (Updated Oct. 7, 2016, 5:29 p.m.) > > > Review request for mesos and Anand Mazumdar. > > > Bugs: MESOS-4948 > https://issues.apache.org/jira/browse/MESOS-4948 > > > Repository: mesos > > > Description > ------- > > Updated InverseOffers* maintenance tests to use the new scheduler mock. > > > Diffs > ----- > > src/tests/master_maintenance_tests.cpp > 77eb405ab7314da906bed9ec1d0018c24928d8d8 > > Diff: https://reviews.apache.org/r/52620/diff/ > > > Testing > ------- > > `make check` on OS X 10.11 and Ubuntu 14.04. > > > Thanks, > > Ilya Pronin > >
