> 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.

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?


- Michael


-----------------------------------------------------------
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
> 
>

Reply via email to