----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70534/#review215206 -----------------------------------------------------------
Thanks for the tests! Having a test file just for a specific scheduler::Call doesn't fit our existing tests well. It seems reasonable to have its own file, but we could badly use some organization. This seems to be a "master test" but it does seem nice to cleanly have its own file. How about a tests/master/ folder under which we move the master tests, as well as this test? src/tests/update_framework_tests.cpp Lines 88-94 (patched) <https://reviews.apache.org/r/70534/#comment301791> These types of functions lead to low readability. When reading tests, I can't intuit what this function does and I need to read the code to know that it puts in a special role. In general, if we can't come up with a function that has some clear meaning, we will inline the code. This means that while we will have duplication, the test will be more readable (the reader doesn't need to jump up to this function and read what it does to understand the test). src/tests/update_framework_tests.cpp Lines 96-102 (patched) <https://reviews.apache.org/r/70534/#comment301794> We generally try to avoid such heavy context test fixtures if possible, since they reduce readability. The reader of any of the tests below needs a lot of context about what the fixture has set up to understand how the test works. If possible it's better to compose pieces together to achieve the test. For example, I imagine the whitelist is only relevant to a few of the tests, so no need to have it for all of them. The agent reservations, I'm not quite sure how they're relevant, there's no indication of why we need them here so I would have to read all the tests to find out. The scheduler, well, we can just use it in each test, no need to have a fixture for it. src/tests/update_framework_tests.cpp Lines 98 (patched) <https://reviews.apache.org/r/70534/#comment301793> What does the presence of agent reservations matter to testing UPDATE_FRAMEWORK? src/tests/update_framework_tests.cpp Lines 177 (patched) <https://reviews.apache.org/r/70534/#comment301792> On the other hand, this function seems rather intuitive. I could figure out what it does from the call site. However.. I would get a bit confused from the fact that it returns a single FrameworkInfo, since the master can have several. How about returning a vector of FrameworkInfo here and the caller ASSERTs that its size is 1. src/tests/update_framework_tests.cpp Lines 390-421 (patched) <https://reviews.apache.org/r/70534/#comment301795> Is there no mock for this yet? How was the event streaming API tested without it? How about pulling this out in front of this patch into its own header so that we can review it on its own? It's independent and adds to the complexity of this review. src/tests/update_framework_tests.cpp Lines 477 (patched) <https://reviews.apache.org/r/70534/#comment301796> Do you have a bad indentation setting in your code editor? There seem to be a lot of 4 space indents in this chain of patches - Benjamin Mahler On May 7, 2019, 12:13 p.m., Andrei Sekretenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70534/ > ----------------------------------------------------------- > > (Updated May 7, 2019, 12:13 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 UPDATE_FRAMEWORK call. > > > Diffs > ----- > > src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f > src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 > src/tests/update_framework_tests.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/70534/diff/4/ > > > Testing > ------- > > > Thanks, > > Andrei Sekretenko > >
