mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Adam B" <a...@mesosphere.io>
Subject Re: Review Request 41806: Cleaned up assertions in test cases around JSON HTTP responses.
Date Tue, 12 Jan 2016 22:56:14 GMT

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


Thanks for cleaning this up. I've got a question about the overhead of the extra AWAITs, and
a few nits. Resolve those and we'll commit it.


src/tests/executor_http_api_tests.cpp (line 394)
<https://reviews.apache.org/r/41806/#comment174874>

    Do you think there's any test overhead in doing another AWAIT for a `response` that has
already been awaited? I'd think it would be a near instantaneous noop, but if not, the EXPECT_SOME_EQ
could be faster.
    Maybe you could time a few test runs before/after this patch to make sure we're not noticeably
slowing things down.



src/tests/executor_http_api_tests.cpp (lines 775 - 776)
<https://reviews.apache.org/r/41806/#comment174877>

    If it wraps to another line, then each parameter should get its own line.
    First parameter can stay on the original line and align other params with it, if that
doesn't cause too much "jaggedness".
    
    Or you could create a variable for `stringify(contentType)` so that it all fits on one
line, since it's used above on line 771 too.



src/tests/master_tests.cpp (line 282)
<https://reviews.apache.org/r/41806/#comment174879>

    Why not `using process::http::Response;`?



src/tests/master_tests.cpp (line 285)
<https://reviews.apache.org/r/41806/#comment174878>

    Why aren't we `using process::http::OK;` or at least `using process::http`? Is there some
other OK conflicting with it? Doesn't look like it.



src/tests/repair_tests.cpp (lines 84 - 87)
<https://reviews.apache.org/r/41806/#comment174882>

    Not yours, but could you fix the indentation please?


- Adam B


On Jan. 12, 2016, 12:47 p.m., Neil Conway wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41806/
> -----------------------------------------------------------
> 
> (Updated Jan. 12, 2016, 12:47 p.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Used `AWAIT_EXPECT_RESPONSE_HEADER_EQ()` to check the "Content-Type" of
> the response, rather than accessing the "headers" field directly, and
> used the symbol `APPLICATION_JSON` rather than a string literal. Also
> added "Content-Type" checks to a few places that had neglected to make
> them, and cleaned up some whitespace style.
> 
> 
> Diffs
> -----
> 
>   src/tests/executor_http_api_tests.cpp 952aacb720715ad26f91fa08bc386b242b7e007b 
>   src/tests/fault_tolerance_tests.cpp fd1c3c90101eed9ef9352511e0c72a463cca965c 
>   src/tests/master_tests.cpp 223b9d20a3a8a8194a3a6a605ec2394c37ab5957 
>   src/tests/metrics_tests.cpp f081fb9b68f25c6d6005f195c34253fba8abc146 
>   src/tests/monitor_tests.cpp a848d14ebb6cab79c06bcf55bd39f044b41a006e 
>   src/tests/repair_tests.cpp 63ec889c4954c2c60d3466952551aa25b3284ddf 
>   src/tests/scheduler_driver_tests.cpp 1365d21ccad87923b862fb4942f1fd51630a62b7 
>   src/tests/scheduler_http_api_tests.cpp 4d23a5a8368e0ed126469fa4a90a889b339ad004 
>   src/tests/slave_tests.cpp e4fb490a1d877547fe883c22dbc47bb4969ecef6 
>   src/tests/status_update_manager_tests.cpp bd34b97a3559a5fea9a7a253a89e0ac3029f4a33

>   src/tests/utils.cpp 877139e97249761658dce3b1058cdc2e2a52367b 
> 
> Diff: https://reviews.apache.org/r/41806/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>


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