mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Mann <g...@mesosphere.io>
Subject Re: Review Request 69064: Added unit tests for Master HTTP endpoints.
Date Tue, 18 Dec 2018 08:32:02 GMT

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




src/tests/master_load_tests.cpp
Lines 96-102 (patched)
<https://reviews.apache.org/r/69064/#comment296395>

    Let's be consistent with the underscore convention for data members in this file. This
class uses a trailing underscore for data member names, while the blocking authorizer does
not.



src/tests/master_load_tests.cpp
Lines 152 (patched)
<https://reviews.apache.org/r/69064/#comment296408>

    We could do
    
    ```
    promises.front().associate(futures.front());
    ```



src/tests/master_load_tests.cpp
Lines 224 (patched)
<https://reviews.apache.org/r/69064/#comment296381>

    If you end up keeping the trailing underscores on the data members of this class, then:
s/_authorizer/authorizer/



src/tests/master_load_tests.cpp
Lines 293-297 (patched)
<https://reviews.apache.org/r/69064/#comment296397>

    Should we add something like
    
    ASSERT_TRUE(Clock::now() - whileLoopStartTime < Seconds(15));
    
    to make sure that we don't end up getting stuck spinning here due to some bug in the future?



src/tests/master_load_tests.cpp
Lines 310 (patched)
<https://reviews.apache.org/r/69064/#comment296398>

    s/deferals/deferrals/



src/tests/master_load_tests.cpp
Lines 357 (patched)
<https://reviews.apache.org/r/69064/#comment296399>

    Why not use `LOG()` here?



src/tests/master_load_tests.cpp
Lines 400-401 (patched)
<https://reviews.apache.org/r/69064/#comment296400>

    Could you make this patch a dependant of the metric patch and assert that the metric value
has been incremented here?
    
    Ditto for the other tests in this file.



src/tests/master_load_tests.cpp
Lines 414 (patched)
<https://reviews.apache.org/r/69064/#comment296401>

    s/roles/frameworks/ ??
    
    Here and below.



src/tests/master_load_tests.cpp
Lines 461 (patched)
<https://reviews.apache.org/r/69064/#comment296402>

    Remove trailing space after `EXPECT_TRUE`.



src/tests/master_load_tests.cpp
Lines 480 (patched)
<https://reviews.apache.org/r/69064/#comment296403>

    I think you could remove this comment.



src/tests/master_load_tests.cpp
Lines 526 (patched)
<https://reviews.apache.org/r/69064/#comment296406>

    Remove space after `EXPECT_TRUE`.



src/tests/master_load_tests.cpp
Lines 555 (patched)
<https://reviews.apache.org/r/69064/#comment296407>

    Can you use some other header to test this case, so that we don't need to disable the
test? You could even set an arbitrary header which is not interpreted by libprocess.


- Greg Mann


On Dec. 12, 2018, 8:53 p.m., Benno Evers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69064/
> -----------------------------------------------------------
> 
> (Updated Dec. 12, 2018, 8:53 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This commit adds a set of unit test to verify that
> basic Master HTTP endpoints still work correctly
> under the presence of request caching.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 35500516e18ff251357b9e8d6bba894c44a9253b 
>   src/tests/master_load_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69064/diff/4/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Benno Evers
> 
>


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