mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anand Mazumdar <an...@apache.org>
Subject Re: Review Request 57883: Added new tests for executor secret generation.
Date Fri, 24 Mar 2017 18:24:18 GMT

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



There might be some comments I would have missed that apply to the second test too. When modifying
the second test, please ensure that you fix the second test too.


src/tests/slave_tests.cpp
Lines 5116 (patched)
<https://reviews.apache.org/r/57883/#comment242766>

    How about:
    
    ```
    This test verifies that TASK_FAILED updates are sent correctly for all the tasks in a
task group when secret generation fails.
    ```



src/tests/slave_tests.cpp
Lines 5125 (patched)
<https://reviews.apache.org/r/57883/#comment242767>

    Move it to after L5138 closer to where it's used. Also, might need to change to `Owned`
as per my last review comment.



src/tests/slave_tests.cpp
Lines 5127-5128 (patched)
<https://reviews.apache.org/r/57883/#comment242771>

    Can you use the v1 classes/helpers that we recently added? I am already working on doing
a sweep for moving the others tests in this file to use them similar to what we did for `default_executor_tests.cpp`.
Hopefully, we can land them together. This would reduce the `evolve` calls needed in this
test and make it more readable.
    
    e.g., `v1::Resources`, `v1::ExecutorInfo` `v1::createTask` etc.



src/tests/slave_tests.cpp
Lines 5167 (patched)
<https://reviews.apache.org/r/57883/#comment242768>

    // Ignore subsequent offers.



src/tests/slave_tests.cpp
Lines 5189 (patched)
<https://reviews.apache.org/r/57883/#comment242774>

    s/EXPECT_NE/ASSERT_NE
    
    It would crash on the next line otherwise when you access offers[0].



src/tests/slave_tests.cpp
Lines 5191-5228 (patched)
<https://reviews.apache.org/r/57883/#comment242772>

    Move this after L5241.



src/tests/slave_tests.cpp
Lines 5208 (patched)
<https://reviews.apache.org/r/57883/#comment242777>

    Add a comment here for posterity that the tasks fail due to the secret generation failing?



src/tests/slave_tests.cpp
Lines 5215 (patched)
<https://reviews.apache.org/r/57883/#comment242784>

    Kill this newline?



src/tests/slave_tests.cpp
Lines 5219 (patched)
<https://reviews.apache.org/r/57883/#comment242769>

    Why do you need this?



src/tests/slave_tests.cpp
Lines 5220 (patched)
<https://reviews.apache.org/r/57883/#comment242770>

    New line before.
    
    Also, can we wait on the `Future` for failure? It's not immediately clear to me if by
the time `removeFramework` is invoked `failure()` would be invoked once. The test might be
flaky otherwise.



src/tests/slave_tests.cpp
Lines 5267 (patched)
<https://reviews.apache.org/r/57883/#comment242782>

    4 space indent.



src/tests/slave_tests.cpp
Lines 5269 (patched)
<https://reviews.apache.org/r/57883/#comment242783>

    s/EXPECT_EQ/ASSERT_EQ
    
    Do you want the expectation on L5272 to run otherwise?



src/tests/slave_tests.cpp
Lines 5277-5301 (patched)
<https://reviews.apache.org/r/57883/#comment242781>

    Kill this? You are not expecting more status updates for the same task.



src/tests/slave_tests.cpp
Lines 5310-5311 (patched)
<https://reviews.apache.org/r/57883/#comment242786>

    Reword as per the comments above?



src/tests/slave_tests.cpp
Lines 5319 (patched)
<https://reviews.apache.org/r/57883/#comment242788>

    Move this.



src/tests/slave_tests.cpp
Lines 5321-5322 (patched)
<https://reviews.apache.org/r/57883/#comment242789>

    Use v1 helpers as per earlier comment.



src/tests/slave_tests.cpp
Lines 5383 (patched)
<https://reviews.apache.org/r/57883/#comment242797>

    ASSERT_NE



src/tests/slave_tests.cpp
Lines 5385-5401 (patched)
<https://reviews.apache.org/r/57883/#comment242790>

    Move this before sending the `ACCEPT` call as per my earlier comment.



src/tests/slave_tests.cpp
Lines 5403 (patched)
<https://reviews.apache.org/r/57883/#comment242793>

    Also add a comment on why the task failed updates are generated?



src/tests/slave_tests.cpp
Lines 5406-5409 (patched)
<https://reviews.apache.org/r/57883/#comment242792>

    Newline before.
    
    ```cpp
    authenticationToken.mutable_reference()->set_name(...);
    authenticationToken.mutable_reference()->set_key(...);
    ```



src/tests/slave_tests.cpp
Lines 5416 (patched)
<https://reviews.apache.org/r/57883/#comment242794>

    Kill this newline



src/tests/slave_tests.cpp
Lines 5421 (patched)
<https://reviews.apache.org/r/57883/#comment242795>

    newline
    
    Also, might want to wait on this `Future` similar to my comment in the earlier test.



src/tests/slave_tests.cpp
Lines 5470 (patched)
<https://reviews.apache.org/r/57883/#comment242799>

    s/EXPECT_EQ/ASSERT_EQ.



src/tests/slave_tests.cpp
Lines 5475 (patched)
<https://reviews.apache.org/r/57883/#comment242798>

    Newline after a multi line statement.



src/tests/slave_tests.cpp
Lines 5480-5504 (patched)
<https://reviews.apache.org/r/57883/#comment242796>

    Kill these


- Anand Mazumdar


On March 23, 2017, 8:35 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57883/
> -----------------------------------------------------------
> 
> (Updated March 23, 2017, 8:35 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6999
>     https://issues.apache.org/jira/browse/MESOS-6999
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch adds new tests,
> `SlaveTest.RunTaskGroupFailedSecretGeneration` and
> `SlaveTest.RunTaskGroupInvalidExecutorSecret`, to
> verify the agent's behavior when generation of the
> executor secret fails.
> 
> 
> Diffs
> -----
> 
>   src/tests/slave_tests.cpp c31c670b667240c4876d415aa5cf90cb34273e8a 
> 
> 
> Diff: https://reviews.apache.org/r/57883/diff/2/
> 
> 
> Testing
> -------
> 
> `make check`
> `bin/mesos-tests.sh --gtest_filter="*SlaveTest.RunTaskGroupFailedSecretGeneration*:SlaveTest.RunTaskGroupInvalidExecutorSecret*"
--gtest_repeat=-1 --gtest_break_on_failure` was used to verify that the new tests are not
flaky.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>


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