You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Ben Mahler <be...@gmail.com> on 2015/03/25 00:42:10 UTC
Review Request 32463: Improved testing around state.json endpoints.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32463/
-----------------------------------------------------------
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
Re: Review Request 32463: Improved testing around state.json
endpoints.
Posted by Ben Mahler <be...@gmail.com>.
> On March 25, 2015, 5:38 p.m., Alexander Rukletsov wrote:
> > src/tests/master_tests.cpp, line 2816
> > <https://reviews.apache.org/r/32463/diff/1/?file=904857#file904857line2816>
> >
> > Why do you check the type here, but not above for e.g. `DATE`, `TIME`, and `USER`?
Ah nice catch, `"id"` needs it but the ones below don't need it!
For DATE, TIME, and USER, we can check equality (note that the `std::string build::DATE` implicitly becomes a `JSON::String`, which is `==` comparable to a `JSON::Value`). However, for `"id"`, we want to confirm that it's a non-empty `JSON::String`. If you check for `"" != value` then this holds for all non-empty strings, but it also holds for JSON::Number, JSON::Object, etc. So I check the type explicitly, make sense?
> On March 25, 2015, 5:38 p.m., Alexander Rukletsov wrote:
> > src/tests/master_tests.cpp, line 66
> > <https://reviews.apache.org/r/32463/diff/1/?file=904857#file904857line66>
> >
> > 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`.
Thanks for noting this, I originally added this to avoid having to add using clauses for a lot of things from process::http. However, we generally are avoiding using namespaces like this so I've reverted back to fine-grained using clauses, and a namespace alias at the top to deal with the `proces::http` problem.
> On March 25, 2015, 5:38 p.m., Alexander Rukletsov wrote:
> > src/tests/master_tests.cpp, lines 2847-2849
> > <https://reviews.apache.org/r/32463/diff/1/?file=904857#file904857line2847>
> >
> > How about a one-liner here and below:
> > ```
> > EXPECT_TRUE(state.values["slaves"].as<JSON::Array>().values.empty());
> > ```
Thanks!
> On March 25, 2015, 5:38 p.m., Alexander Rukletsov wrote:
> > src/tests/slave_tests.cpp, line 792
> > <https://reviews.apache.org/r/32463/diff/1/?file=904858#file904858line792>
> >
> > Not yours, but can you do a drive-by fix and remove `process::`?
I'd like to avoid the noise in the diff since its in an unrelated test :)
> On March 25, 2015, 5:38 p.m., Alexander Rukletsov wrote:
> > src/tests/master_tests.cpp, line 2814
> > <https://reviews.apache.org/r/32463/diff/1/?file=904857#file904857line2814>
> >
> > 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.
It turns out this fails because `Time::create` compensates for the advanced state the clock by adding `clock::advanced`. That has already been added from `Clock::now` in the endpoint and so the times are not equal.
> On March 25, 2015, 5:38 p.m., Alexander Rukletsov wrote:
> > src/tests/master_tests.cpp, lines 2807-2809
> > <https://reviews.apache.org/r/32463/diff/1/?file=904857#file904857line2807>
> >
> > 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?
Hm.. but you have to pause the clock, yes?
I've gone with your suggestion since its cleaner, paused the Clock at the top of the test to ensure Clock::now matches the master's time.
> On March 25, 2015, 5:38 p.m., Alexander Rukletsov wrote:
> > src/tests/slave_tests.cpp, line 937
> > <https://reviews.apache.org/r/32463/diff/1/?file=904858#file904858line937>
> >
> > Kill `process::` prefix?
Will leave the unrelated tests for now. :)
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32463/#review77738
-----------------------------------------------------------
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
>
>
Re: Review Request 32463: Improved testing around state.json
endpoints.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 32463: Improved testing around state.json
endpoints.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32463/#review77933
-----------------------------------------------------------
Ship it!
lgtm modulo alex's comments.
- Vinod Kone
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
>
>