----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70534/#review215500 -----------------------------------------------------------
Overall looking pretty good, thanks! Can you be sure to run the test in repetition to ensure there's no flakyness? src/tests/master/update_framework_tests.cpp Lines 88 (patched) <https://reviews.apache.org/r/70534/#comment302217> {}; can go on the previous line src/tests/master/update_framework_tests.cpp Lines 93 (patched) <https://reviews.apache.org/r/70534/#comment302213> It seems strange to pass the `id` here.. can't we use the one in info and ensure that callers set it? src/tests/master/update_framework_tests.cpp Lines 98-103 (patched) <https://reviews.apache.org/r/70534/#comment302212> In general I would suggest we prefer the following syntax pattern: ``` *call.mutable_framework_id() = id; *call.mutable_update_framework()->mutable_framework_info() = info; *call.mutable_update_framework()->mutable_framework_info() ->mutable_id() = id; ``` Because it allows moves to happen when the rhs is an rvalue: ``` *msg.mutable_foo() = func(); // will move :) msg.mutable_foo()->CopyFrom(func()); // will copy :( ``` src/tests/master/update_framework_tests.cpp Lines 124 (patched) <https://reviews.apache.org/r/70534/#comment302214> pull this up into the previous line? src/tests/master/update_framework_tests.cpp Lines 131 (patched) <https://reviews.apache.org/r/70534/#comment302215> newline src/tests/master/update_framework_tests.cpp Lines 134 (patched) <https://reviews.apache.org/r/70534/#comment302216> newline src/tests/master/update_framework_tests.cpp Lines 145 (patched) <https://reviews.apache.org/r/70534/#comment302218> s/C/c/ src/tests/master/update_framework_tests.cpp Lines 147-149 (patched) <https://reviews.apache.org/r/70534/#comment302211> How about a CHECK_EQ with a helpful `<<` message so that the reader knows why this is failing from the logs without having to come in here and read the code? src/tests/master/update_framework_tests.cpp Lines 176 (patched) <https://reviews.apache.org/r/70534/#comment302219> s/FrameworkInfosDiff/diff/? src/tests/master/update_framework_tests.cpp Lines 182 (patched) <https://reviews.apache.org/r/70534/#comment302220> This seems odd, can't the caller fill this in for the initial FrameworkInfo so that we don't ignore it? if the `id` is set to two different values, we want to catch that. src/tests/master/update_framework_tests.cpp Lines 197-198 (patched) <https://reviews.apache.org/r/70534/#comment302221> Can you declare this lower down closer to where it's used? We can also pass in the DEFAULT_FRAMEWORK_INFO **with** `id` assigned based on the subscription. Ditto for the other tests src/tests/master/update_framework_tests.cpp Lines 219 (patched) <https://reviews.apache.org/r/70534/#comment302222> newline, ditto for the other tests src/tests/master/update_framework_tests.cpp Lines 230 (patched) <https://reviews.apache.org/r/70534/#comment302236> newline, ditto other tests src/tests/master/update_framework_tests.cpp Lines 325 (patched) <https://reviews.apache.org/r/70534/#comment302237> A role not in the whitelist? Don't we have a test for that somewhere already? Or are you thinking specifically when an UPDATE_FRAMEWORK uses a non-whitelisted role? src/tests/master/update_framework_tests.cpp Lines 326 (patched) <https://reviews.apache.org/r/70534/#comment302238> Ditto here, I guess we test this already somewhere (maybe) but this is just to make sure the UPDATE_FRAMEWORK path goes throug that check? src/tests/master/update_framework_tests.cpp Lines 332 (patched) <https://reviews.apache.org/r/70534/#comment302239> exit code? src/tests/master/update_framework_tests.cpp Lines 391 (patched) <https://reviews.apache.org/r/70534/#comment302234> remove this? src/tests/master/update_framework_tests.cpp Lines 397 (patched) <https://reviews.apache.org/r/70534/#comment302235> newline src/tests/master/update_framework_tests.cpp Lines 417 (patched) <https://reviews.apache.org/r/70534/#comment302224> Can you explain what this is testing? src/tests/master/update_framework_tests.cpp Lines 421 (patched) <https://reviews.apache.org/r/70534/#comment302223> newline src/tests/master/update_framework_tests.cpp Lines 424-425 (patched) <https://reviews.apache.org/r/70534/#comment302228> See my comment below, it seems we don't need this? src/tests/master/update_framework_tests.cpp Lines 429-430 (patched) <https://reviews.apache.org/r/70534/#comment302226> Why is this customization needed? src/tests/master/update_framework_tests.cpp Lines 434-438 (patched) <https://reviews.apache.org/r/70534/#comment302227> This looks racy? The expectation should be set up prior to the agent starting, but do we even care to test that there's an event for it in this test? It seems to be the responsibility of other tests to care about agent lifecycle events. src/tests/master/update_framework_tests.cpp Lines 454 (patched) <https://reviews.apache.org/r/70534/#comment302229> For consistency, we put the .WillOnce, .Times, etc on the next line, can you do a sweep in this file? src/tests/master/update_framework_tests.cpp Lines 456 (patched) <https://reviews.apache.org/r/70534/#comment302230> I hadn't considered the case where a framework has no roles, I suppose we can allow that.. src/tests/master/update_framework_tests.cpp Lines 469 (patched) <https://reviews.apache.org/r/70534/#comment302232> newline src/tests/master/update_framework_tests.cpp Lines 472 (patched) <https://reviews.apache.org/r/70534/#comment302231> s/.get()./->/ (please do a scan of this file) Seems like 1 offer is expected given the single rescind expectation below, might as well ensure it's 1 here? src/tests/master/update_framework_tests.cpp Lines 499 (patched) <https://reviews.apache.org/r/70534/#comment302233> Pass `masterFlags.allocation_interval` instead - Benjamin Mahler On May 21, 2019, 2:10 p.m., Andrei Sekretenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70534/ > ----------------------------------------------------------- > > (Updated May 21, 2019, 2:10 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-7258 > https://issues.apache.org/jira/browse/MESOS-7258 > > > Repository: mesos > > > Description > ------- > > Added tests for the V1 UPDATE_FRAMEWORK call. > > > Diffs > ----- > > src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f > src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 > src/tests/master/update_framework_tests.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/70534/diff/7/ > > > Testing > ------- > > > Thanks, > > Andrei Sekretenko > >
