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 53511: Parameterized existing decoder tests on the type of decoder.
Date Wed, 09 Nov 2016 00:57:30 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53511/#review155388
-----------------------------------------------------------


Fix it, then Ship it!




Very clean update to the test! I left a comment about whether we can do this similar parameterization
on the response side, perhaps you'll want to leave a TODO for this if you don't want to take
it on now?


3rdparty/libprocess/src/tests/decoder_tests.cpp (lines 43 - 44)
<https://reviews.apache.org/r/53511/#comment225273>

    Seems a little odd to have DecoderTest as a request specific typed test, and then DecoderTest
is also still the name for the response tests. Wouldn't this break if we made the same cleanup
for the response decoder tests?
    
    How about s/Decoder/RequestDecoder/ for these names?
    
    Looks like you want to add a TODO to do this similar parameterization for the response
tests? Or is there a reason we can't?



3rdparty/libprocess/src/tests/decoder_tests.cpp (lines 77 - 83)
<https://reviews.apache.org/r/53511/#comment225284>

    Let's invoke the function immediately so that it can only be called once:
    
    ```
    Future<string> body = [&request]() {
      ...
    }();
    ```
    
    Or if you want the function, how about avoiding the capture since this function has no
need to be tied to a specific request?
    
    ```
    auto getBody = [](Request* request) {
      ...
    };
    
    AWAIT_EXPECT_EQ(string(), getBody(request));
    ```
    
    Although in this case, calling getBody twice would be problematic in the case of a pipe
body, so the first option seems better to me, unless we needed to call this for different
requests.


- Benjamin Mahler


On Nov. 5, 2016, 1:32 a.m., Anand Mazumdar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53511/
> -----------------------------------------------------------
> 
> (Updated Nov. 5, 2016, 1:32 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6466
>     https://issues.apache.org/jira/browse/MESOS-6466
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This allows us to not duplicate tests for the streaming request
> decoder.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/src/tests/decoder_tests.cpp 4535614312373b0515025f09f9f8495f9ce353a3

> 
> Diff: https://reviews.apache.org/r/53511/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>


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