> On Sept. 10, 2015, 9:54 a.m., Michael Park wrote: > > 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp, lines 191-192 > > <https://reviews.apache.org/r/37827/diff/6/?file=1066089#file1066089line191> > > > > The above test already performs the roundtrip of Protobuf -> JSON -> > > Protobuf. Is it beneficial to add an additional conversion to JSON here? > > What does it further test? > > Alexander Rukletsov wrote: > The rationale here is that equality check for protobufs is not well > defined (we do it overselves), hence an additional check via converted `JSON` > objects looked to me like a reasonable addition. > > Michael Park wrote: > Hm, I see. So I think you're saying that if the protobuf happened to > parse incorrectly but the test passed due to a bug in `operator==`, > converting it back to `JSON` for the final test could expose that bug. Is > that correct? > > If it is, it doesn't seem like this is the right place to test for that. > It would make more sense to me to add a separate test for `operator==` for > `SimpleMessage`. > In general, I think everything being used in a test aside from the thing > being tested should have already been tested for correctness. > Since otherwise we would have to test the dependent functions being used > in any given test in addition to the thing that's actually being tested. > > What do you think?
I think you're right. Though it's more work, we should do it properly. Will update the RR chain in a while. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37827/#review98371 ----------------------------------------------------------- On Sept. 10, 2015, 6:36 p.m., Alexander Rukletsov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37827/ > ----------------------------------------------------------- > > (Updated Sept. 10, 2015, 6:36 p.m.) > > > Review request for mesos, haosdent huang, Joseph Wu, and Michael Park. > > > Bugs: MESOS-3312 > https://issues.apache.org/jira/browse/MESOS-3312 > > > Repository: mesos > > > Description > ------- > > See summary. > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp > c56d6a3098293eb3659b3066f10e875927ec3ac3 > 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h > cfc2803e42284f641879fb24bce1282215c8ea52 > 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc > a1d4084661345f9367c75f9db61279f032b93e69 > 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto > bbd36d39e9588eb8eea6d739451ad3bab029ca08 > > Diff: https://reviews.apache.org/r/37827/diff/ > > > Testing > ------- > > make check (Mac OS 10.10.4) > > **NOTE**: Filed > [MESOS-3323](https://issues.apache.org/jira/browse/MESOS-3323) to clean up > protobuf generation. > > > Thanks, > > Alexander Rukletsov > >
