> On Feb. 4, 2015, 7:02 p.m., Alexander Rukletsov wrote:
> > Thanks for the patch, Alexander! I have a high-level question. In such
> > checks:
> > ```
> > Try<JSON::Value> expected = JSON::parse("[]");
> > ASSERT_SOME(expected);
> >
> > Result<JSON::Value> _tasks = framework.find<JSON::Value>("tasks");
> > EXPECT_SOME_EQ(expected.get(), _tasks);
> > ```
> > do we want instead (or additionally) to check that the task array is empty?
> > If possible, I would prefer to operate with high-level abstractions here
> > and leave JSON representation details for JSON tests.
Originally the implementation was as you suggested, but Niklas mentioned it was
more readably if you just set an expected object and then compare the result to
it. I not only agree with this, but is also what all other tests on the file
do, so it is also consistent.
> On Feb. 4, 2015, 7:02 p.m., Alexander Rukletsov wrote:
> > src/tests/master_tests.cpp, lines 1593-1596
> > <https://reviews.apache.org/r/30612/diff/2/?file=847633#file847633line1593>
> >
> > Isn't it more stable to check that `frameworks` key is present and the
> > associated array is empty?
See my comment above.
> On Feb. 4, 2015, 7:02 p.m., Alexander Rukletsov wrote:
> > src/tests/master_tests.cpp, line 1819
> > <https://reviews.apache.org/r/30612/diff/2/?file=847633#file847633line1819>
> >
> > Shouldn't we check the array is empty?
See my first comment. It just compares two json objects and expect them to be
equivalent.
> On Feb. 4, 2015, 7:02 p.m., Alexander Rukletsov wrote:
> > src/tests/master_tests.cpp, lines 1985-1989
> > <https://reviews.apache.org/r/30612/diff/2/?file=847633#file847633line1985>
> >
> > Isn't it covered in a previous test?
The tests are different. The returned objects are different whether you use
`/master/frameworks` or `master/frameworks/{frameworkId}` or
`master/frameworks/{frameworkId}/tasks` or
`master/frameworks/{frameworkId}/tasks/{taskId}`. However, since the set up of
the cluster is the same, you compare to the same values.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30612/#review70982
-----------------------------------------------------------
On Feb. 4, 2015, 2:34 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30612/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2015, 2:34 p.m.)
>
>
> Review request for mesos, Ben Mahler, Niklas Nielsen, and Till Toenshoff.
>
>
> Bugs: MESOS-2157
> https://issues.apache.org/jira/browse/MESOS-2157
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds endpoints for paths /master/frameworks/{framework}/tasks/{task}.
>
> Works with [29883](https://reviews.apache.org/r/29883).
>
> Builds on the abandoned patch 14286.
>
>
> Diffs
> -----
>
> src/master/http.cpp 3981b18
> src/master/master.hpp cd37ee9
> src/master/master.cpp d04b2c4
> src/tests/master_tests.cpp 678d27f
>
> Diff: https://reviews.apache.org/r/30612/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
