Re: Review Request 41806: Cleaned up assertions in test cases around JSON HTTP responses.

2016-01-13 Thread Neil Conway 


> On Jan. 12, 2016, 10:56 p.m., Adam B wrote:
> > src/tests/executor_http_api_tests.cpp, line 394
> > 
> >
> > 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.

Seems like it doesn't make a noticeable performance difference. I ran `time 
./src/mesos-tests --gtest_filter="MasterTest.SlavesEndpointWithoutSlaves" 
--gtest_repeat=100`. With the patch applied, I got the following elapsed times:

```
5.269
4.663
4.803
4.672
```

(The first time probably suffered from cold cache/warmup effects.) Without the 
patch applied, I get:

```
4.867
4.989
4.728
4.756
```


- Neil


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


On Jan. 12, 2016, 8: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, 8: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
> 
>

Re: Review Request 41806: Cleaned up assertions in test cases around JSON HTTP responses.

2016-01-13 Thread Mesos ReviewBot 

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


Patch looks great!

Reviews applied: [41806]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Jan. 13, 2016, 7:19 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41806/
> ---
> 
> (Updated Jan. 13, 2016, 7:19 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 eb47b5319b16601b8600c4d53035b8337382070c 
>   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
> 
>

Re: Review Request 41806: Cleaned up assertions in test cases around JSON HTTP responses.

2016-01-12 Thread Mesos ReviewBot 

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


Patch looks great!

Reviews applied: [41806]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Jan. 12, 2016, 8: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, 8: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
> 
>

Re: Review Request 41806: Cleaned up assertions in test cases around JSON HTTP responses.

2016-01-12 Thread Neil Conway 

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

(Updated Jan. 12, 2016, 8:47 p.m.)


Review request for mesos and Adam B.


Changes
---

Rebase.


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 (updated)
-

  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

Re: Review Request 41806: Cleaned up assertions in test cases around JSON HTTP responses.

2016-01-12 Thread Adam B 

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


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)


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)


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



src/tests/master_tests.cpp (line 285)


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)


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

Re: Review Request 41806: Cleaned up assertions in test cases around JSON HTTP responses.

2015-12-30 Thread Mesos ReviewBot 

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


Patch looks great!

Reviews applied: [41806]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 30, 2015, 9:53 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41806/
> ---
> 
> (Updated Dec. 30, 2015, 9:53 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 
> 8d86df5fd54bcfe998eb7dc4302509e7a00e9d84 
>   src/tests/fault_tolerance_tests.cpp 
> ba657d0e1d8515cffd1b37925bf91a84b2feaef1 
>   src/tests/master_tests.cpp 865fa4a71f4bae2a218cd2c4e10873222d1ea3c4 
>   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 328c853a338145256dadfc9364b9c2a0e6c356cb 
>   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
> 
>

Review Request 41806: Cleaned up assertions in test cases around JSON HTTP responses.

2015-12-30 Thread Neil Conway 

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

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 
8d86df5fd54bcfe998eb7dc4302509e7a00e9d84 
  src/tests/fault_tolerance_tests.cpp ba657d0e1d8515cffd1b37925bf91a84b2feaef1 
  src/tests/master_tests.cpp 865fa4a71f4bae2a218cd2c4e10873222d1ea3c4 
  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 328c853a338145256dadfc9364b9c2a0e6c356cb 
  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