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 60847: Add test cases for /slaves, /slave/containers, /frameworks endpoints.
Date Tue, 18 Jul 2017 04:52:34 GMT

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




src/tests/master_tests.cpp
Lines 2593 (patched)
<https://reviews.apache.org/r/60847/#comment255983>

    s/slave_id/slaveId/
    
    here and elsewhere



src/tests/master_tests.cpp
Line 5856 (original), 5979 (patched)
<https://reviews.apache.org/r/60847/#comment255984>

    s/termintated/terminated/



src/tests/master_tests.cpp
Lines 6047-6048 (patched)
<https://reviews.apache.org/r/60847/#comment256019>

    Let's leave a comment here explaining why we need two different expected values.



src/tests/master_tests.cpp
Lines 6048-6076 (patched)
<https://reviews.apache.org/r/60847/#comment256024>

    I think that we can find a more elegant way to handle scenarios like this. Consider the
following:
    
    ```
    Try<JSON::Value> frameworkJson1 = JSON::parse(
        "{"
            "\"id\":\"" + frameworkId1->value() + "\","
            "\"name\":\"default\""
        "}");
    
    Try<JSON::Value> frameworkJson2 = JSON::parse(
        "{"
            "\"id\":\"" + frameworkId2->value() + "\","
            "\"name\":\"default\""
        "}");
    
    ASSERT_SOME(frameworkJson1);
    ASSERT_SOME(frameworkJson2);
    
    // Since frameworks are stored in a hashmap, there is no strict guarantee of
    // their ordering when listed. For this reason, we test both possibilities.
    if (array->values[0].contains(frameworkJson1.get())) {
      ASSERT_TRUE(array->values[1].contains(frameworkJson2.get()));
    } else {
      ASSERT_TRUE(array->values[0].contains(frameworkJson2.get()));
      ASSERT_TRUE(array->values[1].contains(frameworkJson1.get()));
    }
    ```
    
    What do you think? We could do something like this here and elsewhere.



src/tests/master_tests.cpp
Lines 6075-6076 (patched)
<https://reviews.apache.org/r/60847/#comment255985>

    ```
    EXPECT_TRUE(
        value->contains(expected1.get()) ||
        value->contains(expected2.get()));
    ```



src/tests/master_tests.cpp
Lines 6114 (patched)
<https://reviews.apache.org/r/60847/#comment255986>

    s/master has known that the framework is terminated/master has marked the framework as
completed/



src/tests/master_tests.cpp
Lines 6120-6121 (patched)
<https://reviews.apache.org/r/60847/#comment256025>

    How about:
    
    "Complete the first framework. As a result, it will appear in the response's 'completed_frameworks'
field."



src/tests/master_tests.cpp
Lines 6125 (patched)
<https://reviews.apache.org/r/60847/#comment256026>

    You can remove this comment.



src/tests/master_tests.cpp
Lines 6128 (patched)
<https://reviews.apache.org/r/60847/#comment256027>

    You can remove this comment.



src/tests/slave_tests.cpp
Lines 2403-2404 (patched)
<https://reviews.apache.org/r/60847/#comment256032>

    Newline here.



src/tests/slave_tests.cpp
Line 2419 (original), 2459 (patched)
<https://reviews.apache.org/r/60847/#comment256033>

    s/status/statistics/



src/tests/slave_tests.cpp
Lines 2470-2473 (patched)
<https://reviews.apache.org/r/60847/#comment256035>

    I think you mean to say that this is called twice in the first query.
    
    Also, perhaps this comment could mention that we extract the container ID for use later
on. How about:
    
    "Will be called twice during the first request. We extract the assigned container IDs
for use when requesting information on a single container."



src/tests/slave_tests.cpp
Line 2421 (original), 2480-2481 (patched)
<https://reviews.apache.org/r/60847/#comment256034>

    Newline here.



src/tests/slave_tests.cpp
Lines 2514 (patched)
<https://reviews.apache.org/r/60847/#comment256036>

    Both of these `WillOnce` calls are for the first query, right?
    
    How about:
    
    "Will be called twice during the first request."



src/tests/slave_tests.cpp
Lines 2519-2520 (patched)
<https://reviews.apache.org/r/60847/#comment256037>

    How about:
    
    "Request information about all containers."


- Greg Mann


On July 14, 2017, 1:43 a.m., Quinn Leng wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60847/
> -----------------------------------------------------------
> 
> (Updated July 14, 2017, 1:43 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Alexander Rojas, Greg Mann, and Vinod Kone.
> 
> 
> Bugs: MESOS-7630
>     https://issues.apache.org/jira/browse/MESOS-7630
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added query parameter test cases for '/slaves' and '/frameworks' on
> the master, and '/slave/containers' endpoint on the slave.
> 
> 
> Diffs
> -----
> 
>   src/tests/master_tests.cpp 6e6461c2e13c3eb055aa3c2d8ad8e3ac54a1d197 
>   src/tests/slave_tests.cpp 035db18db3a64a9e358c1c54cc18a4bdeb85d8bf 
> 
> 
> Diff: https://reviews.apache.org/r/60847/diff/2/
> 
> 
> Testing
> -------
> 
> Passed 'make check -j48'
> Passed 'GTEST_FILTER="MasterTest.FrameworksEndpointQueryFramework" make check -j48'
> Passed 'GLOG_v=1 ./bin/mesos-tests.sh --gtest_filter="MasterTest.FrameworksEndpointQueryFramework"
--gtest_repeat=1000 --gtest_break_on_failure'
> Passed 'GTEST_FILTER="MasterTest.SlavesEndpointQuerySlave" make check -j48'
> Passed 'GLOG_v=1 ./bin/mesos-tests.sh --gtest_filter="MasterTest.SlavesEndpointQuerySlave"
--gtest_repeat=1000 --gtest_break_on_failure'
> 
> 
> Thanks,
> 
> Quinn Leng
> 
>


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