----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53679/#review164651 -----------------------------------------------------------
src/tests/master_quota_tests.cpp (line 1134) <https://reviews.apache.org/r/53679/#comment236415> `EXPECT_FALSE(offers->empty());` src/tests/master_quota_tests.cpp (line 1136) <https://reviews.apache.org/r/53679/#comment236416> Let's move this just before the invocation of `driver->launchTasks`. src/tests/master_quota_tests.cpp (line 1150) <https://reviews.apache.org/r/53679/#comment236417> `status->state()` src/tests/master_quota_tests.cpp (lines 1158 - 1159) <https://reviews.apache.org/r/53679/#comment236419> Any reason we need to reference `defaultAgentResources` here at all? I see you make sure above that `defaultAgentResources == agent1TotalResources`, but we could work with less symbols in the scope if we'd just use `agent1TotalResources`. src/tests/master_quota_tests.cpp (line 1165) <https://reviews.apache.org/r/53679/#comment236420> No need for the explicit `false` here since it is the default. I also find it not very readible (_what is `false`?_). src/tests/master_quota_tests.cpp (line 1168) <https://reviews.apache.org/r/53679/#comment236418> `response->body` src/tests/master_quota_tests.cpp (line 1172) <https://reviews.apache.org/r/53679/#comment236414> I am surprised this would not be called deterministically. Is that the intended behavior or a bug? src/tests/master_quota_tests.cpp (line 1201) <https://reviews.apache.org/r/53679/#comment236421> Let's use `agent1Id` or similar instead to not confuse this with the id of `agent2`'s possible ID. src/tests/master_quota_tests.cpp (line 1203) <https://reviews.apache.org/r/53679/#comment236422> We don't make use of this anywhere in the test. src/tests/master_quota_tests.cpp (line 1215) <https://reviews.apache.org/r/53679/#comment236423> We don't make use of this anywhere in the test. src/tests/master_quota_tests.cpp (line 1221) <https://reviews.apache.org/r/53679/#comment236425> Can you use explicit capture here? src/tests/master_quota_tests.cpp (line 1227) <https://reviews.apache.org/r/53679/#comment236430> This implicit conversion does not return a reference, but a new value instead; we do not use const ref to extend lifetime of temporaries. Instead just make the copy explicit by dropping the ref. src/tests/master_quota_tests.cpp (line 1252) <https://reviews.apache.org/r/53679/#comment236431> Unneeded capture. src/tests/master_quota_tests.cpp (line 1255) <https://reviews.apache.org/r/53679/#comment236432> Unneeded capture. src/tests/master_quota_tests.cpp (line 1298) <https://reviews.apache.org/r/53679/#comment236433> We don't rely on this anywhere in the test. src/tests/master_quota_tests.cpp (line 1318) <https://reviews.apache.org/r/53679/#comment236437> Let's not put expressions which can fail in the expected part here. How about EXPECT_EQ(agent2Resources, stringify(agent2TotalResources.get()); Note that this requires inserting some spaces into `agent2Resources`, e.g., `cpus(%s):1; mem:512` etc. src/tests/master_quota_tests.cpp (line 1325) <https://reviews.apache.org/r/53679/#comment236413> Extra whitespace at EOL. src/tests/master_quota_tests.cpp (line 1328) <https://reviews.apache.org/r/53679/#comment236438> `// Satisfied` src/tests/master_quota_tests.cpp (line 1337) <https://reviews.apache.org/r/53679/#comment236439> Let's add an expectation for `response->body`. src/tests/master_quota_tests.cpp (line 1377) <https://reviews.apache.org/r/53679/#comment236440> We could use `agentTotalResources` and avoid pulling more variables into this scope. src/tests/master_quota_tests.cpp (line 1422) <https://reviews.apache.org/r/53679/#comment236442> Let's add an expectation for `response->body`. src/tests/master_quota_tests.cpp (line 1465) <https://reviews.apache.org/r/53679/#comment236443> We could just use `agent1TotalResources` in this test, and avoid pulling another variable into scope. src/tests/master_quota_tests.cpp (line 1478) <https://reviews.apache.org/r/53679/#comment236444> `slaveRegisteredMessage->to` src/tests/master_quota_tests.cpp (line 1492) <https://reviews.apache.org/r/53679/#comment236445> Let's add an expectation for `response->body`. - Benjamin Bannier On Feb. 7, 2017, 8:11 a.m., Zhitao Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53679/ > ----------------------------------------------------------- > > (Updated Feb. 7, 2017, 8:11 a.m.) > > > Review request for mesos, Alexander Rukletsov, Xiaojian Huang, and Kunal > Thakar. > > > Bugs: MESOS-6840 > https://issues.apache.org/jira/browse/MESOS-6840 > > > Repository: mesos > > > Description > ------- > > Implemented capacity heuristic check related tests. > > > Diffs > ----- > > src/tests/master_quota_tests.cpp b25f5911579c435549b9bc65994627414357dcb6 > > Diff: https://reviews.apache.org/r/53679/diff/ > > > Testing > ------- > > > Thanks, > > Zhitao Li > >
