----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32163/#review80854 -----------------------------------------------------------
Ship it! Looks good to me! Very minor nits, if you can fix them, happy for you to merge this without another round. Thanks for adding tests too! 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp <https://reviews.apache.org/r/32163/#comment130965> nit: `foreach(i)` 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp <https://reviews.apache.org/r/32163/#comment130967> what happens if one of `this.key` is `null` and `other.key` is missing (and vice versa)? in other words: do we treat a `null` differently from a key's absence? (I don't have an opinion on the matter, just suggesting we should document behavior) 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp <https://reviews.apache.org/r/32163/#comment130992> while I appreciate being thorough, strictly speaking this should not be here: you're actually unittesting `JSON::parse()` with this `CHECK` two downsides: 1. your test code is more verbose (and less readable); 2. you may have failures here that are entirely unrelated to `Contains` (causing folks to go scratching their head) Also, I would add some variety to your tests: ``` {"an_array_of_doubles": [1.0, -22.33, 99.876]} {"an_array_of_arrays": [[2, 3], [99, -1]]} {"an_array_of_objects":[{"foo": 1, "bar": 2, "values": [5, 8, -1]} ``` 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp <https://reviews.apache.org/r/32163/#comment130993> nit: you may want to call the key something different (`"should_be_null"` or whatever) or the error messages are going to be real confusing :) - Marco Massenzio On April 20, 2015, 4:28 p.m., Alexander Rojas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/32163/ > ----------------------------------------------------------- > > (Updated April 20, 2015, 4:28 p.m.) > > > Review request for mesos, Benjamin Hindman, Bernd Mathiske, Joerg Schad, > Marco Massenzio, Niklas Nielsen, and Till Toenshoff. > > > Bugs: MESOS-2510 > https://issues.apache.org/jira/browse/MESOS-2510 > > > Repository: mesos > > > Description > ------- > > Adds a function which allows to perform comparison tests on subsets of json > blobs. i.e. > > ```cpp > JSON::Value expected = JSON::parse( > "{" > " \"key\" : true" > "}").get(); > > // Returned json: > // { > // "uptime" : 45234.123, > // "key" : true > // } > JSON::Value actual = bar(); > > // I'm only interested on the "key" entry and ignore the rest. > EXPECT_TRUE(contains(actual, expected)); > ``` > > Increasing readability for tests that include json. > > For more information on the reason of why this patch is needed, please check > the JIRA entry. > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp > 334c898906018be6e663f53815abbe047806b95c > 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp > f60d1bbe60f2e2b6460c06bba98e8b85ebb6a3f9 > > Diff: https://reviews.apache.org/r/32163/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Alexander Rojas > >
