mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alexander Rukletsov" <ruklet...@gmail.com>
Subject Re: Review Request 32463: Improved testing around state.json endpoints.
Date Wed, 25 Mar 2015 17:38:19 GMT

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



src/tests/master_tests.cpp
<https://reviews.apache.org/r/32463/#comment125931>

    I noticed you do drive-by clean-up of some `process::` occurrences. Do you want to go
further and remove all of them or prefer creating a newbie JIRA for that? There are e.g. `process::Message`
and `Process::Time`.



src/tests/master_tests.cpp
<https://reviews.apache.org/r/32463/#comment125936>

    s/ensure/ensures
    
    Not a native speaker, but how about a one liner:
    `// This test ensures miscellaneous keys in state.json are present.`



src/tests/master_tests.cpp
<https://reviews.apache.org/r/32463/#comment125938>

    AFAIK, we agreed to omit a whitespace between closing angle brackets.



src/tests/master_tests.cpp
<https://reviews.apache.org/r/32463/#comment125944>

    Does it make sense to use `Clock::now().secs()` and compare doubles to avoid multiple
conversions and mimic the way it is actually done in master code?



src/tests/master_tests.cpp
<https://reviews.apache.org/r/32463/#comment125940>

    Drop `process::` prefix?



src/tests/master_tests.cpp
<https://reviews.apache.org/r/32463/#comment125941>

    This fails on my Mac.
    
    Also, since you're checking start time, you can add one more `EXPECT_GT` check caching
time before starting master.



src/tests/master_tests.cpp
<https://reviews.apache.org/r/32463/#comment125943>

    Why do you check the type here, but not above for e.g. `DATE`, `TIME`, and `USER`?



src/tests/master_tests.cpp
<https://reviews.apache.org/r/32463/#comment125961>

    How about a one-liner here and below:
    ```
    EXPECT_TRUE(state.values["slaves"].as<JSON::Array>().values.empty());
    ```



src/tests/slave_tests.cpp
<https://reviews.apache.org/r/32463/#comment125932>

    Not yours, but can you do a drive-by fix and remove `process::`?



src/tests/slave_tests.cpp
<https://reviews.apache.org/r/32463/#comment125933>

    Kill `process::` prefix?


- Alexander Rukletsov


On March 24, 2015, 11:42 p.m., Ben Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32463/
> -----------------------------------------------------------
> 
> (Updated March 24, 2015, 11:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This improves the slave test, and adds a new test for the master, to help prevent API
regressions.
> 
> 
> Diffs
> -----
> 
>   src/tests/master_tests.cpp e69348be676a80017062e3abbd15b8008a6009d7 
>   src/tests/slave_tests.cpp fd09d65bf34136c0959419b451e54105300740c4 
> 
> Diff: https://reviews.apache.org/r/32463/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>


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