mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Park" <mcyp...@gmail.com>
Subject Re: Review Request 37827: Added a test for converting JSON arrays to repeated protobufs.
Date Fri, 11 Sep 2015 01:25:00 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message