mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Vinod Kone <vinodk...@gmail.com>
Subject Re: Review Request 56213: Added check tests for command executor.
Date Fri, 10 Feb 2017 02:42:08 GMT

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




src/tests/check_tests.cpp (lines 62 - 63)
<https://reviews.apache.org/r/56213/#comment236889>

    i don't see tests for these in this review. if they are added in the latter part of the
chain, please don't mention them here. or add a TODO in front of them.



src/tests/check_tests.cpp (line 68)
<https://reviews.apache.org/r/56213/#comment236886>

    Remove `call` prefixes for these functions.



src/tests/check_tests.cpp (line 73)
<https://reviews.apache.org/r/56213/#comment236905>

    Any particular reason for using v1 scheduler API? The general recommendation has been
to use v0 scheduler API unless you are testing v1 features. The main reason being most users
are still using v0 scheduler API.



src/tests/check_tests.cpp (line 138)
<https://reviews.apache.org/r/56213/#comment236887>

    no need for this.



src/tests/check_tests.cpp (line 141)
<https://reviews.apache.org/r/56213/#comment236888>

    s/reconciledTask/reconcile/



src/tests/check_tests.cpp (line 171)
<https://reviews.apache.org/r/56213/#comment236890>

    Instead of these long test names how about you create a fixture for CommandExecutor check
tests for better namespacing?
    
    class CommandExecutorCheckTest : public CheckTest {};
    
    TEST_F(CommandExecutorCheckTest, CommandCheckDeliveredAndReconciled)



src/tests/check_tests.cpp (line 177)
<https://reviews.apache.org/r/56213/#comment236893>

    s/agent/slave/ 
    
    here and everywhere else.
    
    we are not doing this change yet.



src/tests/check_tests.cpp (line 237)
<https://reviews.apache.org/r/56213/#comment236894>

    s/checkCommand/command/
    
    here and below.



src/tests/check_tests.cpp (line 241)
<https://reviews.apache.org/r/56213/#comment236895>

    s/exitStatusVar/variable/
    
    here and below.



src/tests/check_tests.cpp (line 249)
<https://reviews.apache.org/r/56213/#comment236892>

    why auto here? it's not clear to me what the type is from just looking at this statement.
    
    please fix it here and everywhere else.



src/tests/check_tests.cpp (line 391)
<https://reviews.apache.org/r/56213/#comment236898>

    s/checkCommandString/command/



src/tests/check_tests.cpp (lines 505 - 515)
<https://reviews.apache.org/r/56213/#comment236900>

    this description needs to be updated to reflect the sleep?



src/tests/check_tests.cpp (line 517)
<https://reviews.apache.org/r/56213/#comment236899>

    s/checkCommandString/command/



src/tests/check_tests.cpp (lines 673 - 677)
<https://reviews.apache.org/r/56213/#comment236902>

    also check for health check status?


- Vinod Kone


On Feb. 8, 2017, 4:56 p.m., Alexander Rukletsov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56213/
> -----------------------------------------------------------
> 
> (Updated Feb. 8, 2017, 4:56 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-6906
>     https://issues.apache.org/jira/browse/MESOS-6906
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   src/tests/check_tests.cpp b5a5d8e80c80b480992a3c8160ee7d0e4443111c 
>   src/tests/mesos.hpp b450a04dfbf3bbeeb6b605fb78097dca390cbdbe 
> 
> Diff: https://reviews.apache.org/r/56213/diff/
> 
> 
> Testing
> -------
> 
> See https://reviews.apache.org/r/56218/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>


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