mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ben Mahler" <benjamin.mah...@gmail.com>
Subject Re: Review Request 41943: Added test case for stout protobuf parse containing JSON null.
Date Tue, 12 Jan 2016 01:21:23 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41943/#review113883
-----------------------------------------------------------

Ship it!



3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp (line 344)
<https://reviews.apache.org/r/41943/#comment174658>

    How about ParseJSONNull?



3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp (line 349)
<https://reviews.apache.org/r/41943/#comment174660>

    Why "message1" and "message2" for the values of 'str'? How about we set these to "value"
to keep it simple?
    
    Also, it looks like we should probably compare equality via SerializeAsString, rather
than checking only one field?



3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp (line 359)
<https://reviews.apache.org/r/41943/#comment174662>

    You can use the '`->`' operator rather than '`.get().`' Ditto elsewhere.



3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp (line 380)
<https://reviews.apache.org/r/41943/#comment174661>

    Seems that you don't need to set the optional str here, can we omit it?



3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp (lines 386 - 387)
<https://reviews.apache.org/r/41943/#comment174659>

    This can be an EXPECT and no need to store the 'parse' variable:
    
    ```
    ASSERT_ERROR(protobuf::parse<tests::Nested>(json.get()););
    ```


- Ben Mahler


On Jan. 11, 2016, 11:40 p.m., Gilbert Song wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41943/
> -----------------------------------------------------------
> 
> (Updated Jan. 11, 2016, 11:40 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Jie Yu, Joris Van Remoortere,
Joseph Wu, and Timothy Chen.
> 
> 
> Bugs: MESOS-4294
>     https://issues.apache.org/jira/browse/MESOS-4294
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added test case for stout protobuf parse containing JSON null.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp bf2a2b8a9f67c6a4cf66b156b9c14fae015a8af0

>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 08793c9d21d4b9b7dd3081cfa35afa47a7e0d28a

>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 797b3b000f3c3fea42cabc3b40baf7235eab6b1e

>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 4c11e23f875b75bcb9d5c65ee66d55b6f72e546d

> 
> Diff: https://reviews.apache.org/r/41943/diff/
> 
> 
> Testing
> -------
> 
> make check (tested on both ubuntu14.04 and OSX10.10)
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>


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