mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 70534: Added tests for the UPDATE_FRAMEWORK call.
Date Sun, 12 May 2019 05:44:41 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message