Re: Review Request 61511: Improved the readability of some assertions/expectations.

2017-08-11 Thread Greg Mann 

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


Ship it!




Ship It!

- Greg Mann


On Aug. 10, 2017, 9:10 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61511/
> ---
> 
> (Updated Aug. 10, 2017, 9:10 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prefer checking whether a container is empty instead of checking its
> size.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 75f7a585ec75f1d8b78453dd203bb78822ee7bc9 
>   src/tests/check_tests.cpp 0810851cf1811f2b4f511f5ca49fe372fd0bac82 
>   src/tests/container_logger_tests.cpp 
> 2ebff809636fbf68c9e3fb87b98f168338f80d6d 
>   src/tests/containerizer/appc_spec_tests.cpp 
> 9bc82531dbb5f10b3d17f26609867c909d86816d 
>   src/tests/containerizer/cgroups_isolator_tests.cpp 
> 67242119f2d10f6f3010c374ea58138e40e4a33e 
>   src/tests/containerizer/cgroups_tests.cpp 
> 506fc7f00dc1ac48476334de930b720b85a691dc 
>   src/tests/containerizer/cni_isolator_tests.cpp 
> 60c85adab11af06be474661544957ca20b7de8c3 
>   src/tests/containerizer/cpu_isolator_tests.cpp 
> 0e86318bb7e261ac00be19f6405557f29a2e92af 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> fa91bcf8d4fdcda44fb5607d99b86a6323096244 
>   src/tests/containerizer/docker_volume_isolator_tests.cpp 
> 866af61f8669163ff3ddd10ed7ecb655568f8250 
>   src/tests/containerizer/environment_secret_isolator_tests.cpp 
> b034ceec7d1bf92db8a1d344835ad48ea2d24952 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> 742143a0f093e415fe98235bbd25342fd65e0483 
>   src/tests/containerizer/linux_capabilities_isolator_tests.cpp 
> 6d95d607b81358953a4afcec03b60e87e7192edd 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> f9cab2fd123e48a5b4387fb2b609d70c7bf535cd 
>   src/tests/containerizer/memory_isolator_tests.cpp 
> b7b7acd37f69832e9ba79c9fe7ed0bc473b53e02 
>   src/tests/containerizer/memory_pressure_tests.cpp 
> 8a43c4fada8bdcedc6e1e24833d0d6c94cf927d2 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 1fc56c4a8ca95bb47af968ad72dfef69455e7d46 
>   src/tests/containerizer/port_mapping_tests.cpp 
> 84b39b15880c7b5d9b7967f1e686baa59f52a015 
>   src/tests/containerizer/posix_rlimits_isolator_tests.cpp 
> bf25049515e8b8d0c085be24c4de22b3ade261b2 
>   src/tests/containerizer/routing_tests.cpp 
> d05b3b11dc5fcd54d956990f252509865168e6b6 
>   src/tests/containerizer/runtime_isolator_tests.cpp 
> fbca31a4da1c83574cce7414fe5e03b1f86591cb 
>   src/tests/containerizer/xfs_quota_tests.cpp 
> c220a6ba5b274892bee195b26a5069d0750b4cb2 
>   src/tests/default_executor_tests.cpp 
> b9776314a8781963b92ba9ac297654f61a443bc8 
>   src/tests/dynamic_weights_tests.cpp 
> 3c86325a0749112606683bffdf305661170493e5 
>   src/tests/exception_tests.cpp b5b657c2d1b31b59ab1554bf854ef1a69b1517ed 
>   src/tests/fault_tolerance_tests.cpp 
> 5b8213531f5688e94063937af19e674508f0dd8c 
>   src/tests/fetcher_cache_tests.cpp 6d212cd460322ad6e97f4cf6ef537323275b7da0 
>   src/tests/group_tests.cpp f2e37cc7184dd328466b554a16da3173cfbca4a7 
>   src/tests/health_check_tests.cpp e824d1b2c2b17423394da0b089ae8940450378b7 
>   src/tests/hook_tests.cpp 04edbf809b7289a7d5336e4f00f23dfb55d3dbf8 
>   src/tests/http_fault_tolerance_tests.cpp 
> 8fcd56d86dcbdd181864756187beb4ff2ac1ff2a 
>   src/tests/master_authorization_tests.cpp 
> 8d54472d8a31956e6e8bb5fe7ffbf47dc793c0bc 
>   src/tests/master_maintenance_tests.cpp 
> e7a80ff4199927df8bf0fb54458d357ae444260d 
>   src/tests/master_quota_tests.cpp 9d52f767c112abd55ab3f49d172eb6e3caea511b 
>   src/tests/master_slave_reconciliation_tests.cpp 
> 9c31eeae1a1b67af142a01e6c548b509ba06740c 
>   src/tests/master_tests.cpp e6ed02e07a9860ca3a56bba9c502a09444c5b26e 
>   src/tests/master_validation_tests.cpp 
> 813fb25007973f3499b94dcd0e9d2184ba08634c 
>   src/tests/oversubscription_tests.cpp 
> 54535a35e33dde0db3b547b9e31b4545d2900b67 
>   src/tests/partition_tests.cpp 62a84f797201ccd18b71490949e3130d2b9c3668 
>   src/tests/persistent_volume_tests.cpp 
> 7b0f436aa270f64cde93adc58833eef97c73dc26 
>   src/tests/reconciliation_tests.cpp f1635fd627d9a7944cb9d4b0117fcded7b6693e1 
>   src/tests/registrar_tests.cpp e2c38d32bc5882b95bbcf1da2e849e4d5ec6a9af 
>   src/tests/registrar_zookeeper_tests.cpp 
> cc082d320b4b150717728ec48edd0b26169cecd7 
>   src/tests/resource_offers_tests.cpp 
> e1fcab4b8fbb625876cf246505db2d05ac5d5710 
>   src/tests/scheduler_driver_tests.cpp 
> 47ad1dc9a4e5e0c5444fcc3a90d4645c3b6013ca 
>   src/tests/scheduler_tests.cpp 5c42dc00f177a61d6ee595c26255492aa07aaad9 
>   src/tests/slave_authorization_tests.cpp 
> 4e55148af77e4aae

Re: Review Request 61511: Improved the readability of some assertions/expectations.

2017-08-10 Thread Gastón Kleiman 


> On Aug. 9, 2017, 3:21 p.m., Benjamin Bannier wrote:
> > src/tests/api_tests.cpp
> > Line 1 (original), 1 (patched)
> > 
> >
> > Searching with
> > 
> > $ git grep -E '(EXPECT|ASSERT)_(EQ|NE).*(0u,\s.*size())'
> > 
> > I still get ~50 hits. Since you are at it, would you mind adjusting 
> > also?
> 
> Gastón Kleiman wrote:
> I had done a similar search, and as far as I can see, the other matches 
> are either false possitives or in stout/libprocess:
> 
> ```
> src/tests/api_tests.cpp:ASSERT_EQ(0u, 
> getState.get_tasks().tasks_size());
> src/tests/api_tests.cpp:ASSERT_EQ(0u, 
> getState.get_executors().executors_size());
> src/tests/api_tests.cpp:ASSERT_EQ(0u, 
> getState.get_tasks().completed_tasks_size());
> src/tests/api_tests.cpp:ASSERT_EQ(0u, 
> getState.get_tasks().tasks_size());
> src/tests/api_tests.cpp:  EXPECT_EQ(0u, 
> getState.get_frameworks().frameworks_size());
> src/tests/api_tests.cpp:  EXPECT_EQ(0u, 
> getState.get_agents().agents_size());
> src/tests/api_tests.cpp:  EXPECT_EQ(0u, 
> getState.get_tasks().tasks_size());
> src/tests/api_tests.cpp:  EXPECT_EQ(0u, 
> getState.get_executors().executors_size());
> src/tests/api_tests.cpp:ASSERT_EQ(0u, 
> v1Response->get_agents().agents_size());
> src/tests/api_tests.cpp:ASSERT_EQ(0u, 
> v1Response->get_agents().recovered_agents_size());
> src/tests/api_tests.cpp:  EXPECT_EQ(0u, 
> getState.get_tasks().tasks_size());
> src/tests/api_tests.cpp:  EXPECT_EQ(0u, 
> getState.get_executors().executors_size());
> src/tests/api_tests.cpp:  EXPECT_EQ(0u, 
> getState.get_frameworks().frameworks_size());
> src/tests/api_tests.cpp:  EXPECT_EQ(0u, 
> getState.get_agents().agents_size());
> src/tests/api_tests.cpp:  EXPECT_EQ(0u, 
> getState.get_tasks().tasks_size());
> src/tests/api_tests.cpp:  EXPECT_EQ(0u, 
> getState.get_executors().executors_size());
> src/tests/api_tests.cpp:ASSERT_EQ(0u, 
> getState.get_frameworks().frameworks_size());
> src/tests/api_tests.cpp:ASSERT_EQ(0u, 
> getState.get_tasks().launched_tasks_size());
> src/tests/api_tests.cpp:ASSERT_EQ(0u, 
> getState.get_executors().executors_size());
> src/tests/api_tests.cpp:ASSERT_EQ(0u, 
> getState.get_frameworks().completed_frameworks_size());
> src/tests/api_tests.cpp:ASSERT_EQ(0u, 
> getState.get_tasks().completed_tasks_size());
> src/tests/api_tests.cpp:ASSERT_EQ(0u, 
> getState.get_executors().completed_executors_size());
> src/tests/api_tests.cpp:ASSERT_EQ(0u, 
> getState.get_frameworks().frameworks_size());
> src/tests/api_tests.cpp:ASSERT_EQ(0u, 
> getState.get_tasks().launched_tasks_size());
> src/tests/api_tests.cpp:ASSERT_EQ(0u, 
> getState.get_executors().executors_size());
> ```
> 
> I don't think we can use `empty()` here.
> 
> And in the following match `manifest` is not a real container:
> 
> ```
> src/tests/containerizer/docker_spec_tests.cpp:  EXPECT_EQ(0u, 
> manifest->size());
> ```
> 
> I'll drop this issue and add two more patches for stout and libprocess to 
> the chain.
> 
> Benjamin Bannier wrote:
> In principle we are still able to explicitly check emptiness here, e.g., 
> we can replace 
> 
> ASSERT_EQ(0u, getState.get_tasks().tasks_size());
> 
> with 
> 
> ASSERT_TRUE(getState.get_tasks().tasks().empty());
> 
> There is some trade-off between local and global consistency in most of 
> the cases in e.g., `api_tests.cpp`. When checking proto fields there, we 
> often mix checking emptiness and checks for a particular size. I would go 
> with a checks capturing our intention as much as possible, i.e., prefer 
> checking for emptiness over local typographic consistency.

I fixed this, I also made the `u` in `0u` optional running: `git grep -E 
'(EXPECT|ASSERT)_(EQ|NE).*(0u,\s.*size())'`

This returned a lot of matches that I had initially missed. I fixed 'em all =).


- Gastón


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


On Aug. 10, 2017, 9:10 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61511/
> ---
> 
> (Updated Aug. 10, 2017, 9:10 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prefer checking whether a container is empty instead of checking its
> size.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 75f7a585ec75f1d8b78453dd203bb7882

Re: Review Request 61511: Improved the readability of some assertions/expectations.

2017-08-10 Thread Gastón Kleiman 

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

(Updated Aug. 10, 2017, 9:10 p.m.)


Review request for mesos, Benjamin Bannier, Jie Yu, and Vinod Kone.


Changes
---

Used `git grep -E '(EXPECT|ASSERT)_(EQ|NE).*(0u?,\s.*size())'` to find even 
mor occurences.


Repository: mesos


Description
---

Prefer checking whether a container is empty instead of checking its
size.


Diffs (updated)
-

  src/tests/api_tests.cpp 75f7a585ec75f1d8b78453dd203bb78822ee7bc9 
  src/tests/check_tests.cpp 0810851cf1811f2b4f511f5ca49fe372fd0bac82 
  src/tests/container_logger_tests.cpp 2ebff809636fbf68c9e3fb87b98f168338f80d6d 
  src/tests/containerizer/appc_spec_tests.cpp 
9bc82531dbb5f10b3d17f26609867c909d86816d 
  src/tests/containerizer/cgroups_isolator_tests.cpp 
67242119f2d10f6f3010c374ea58138e40e4a33e 
  src/tests/containerizer/cgroups_tests.cpp 
506fc7f00dc1ac48476334de930b720b85a691dc 
  src/tests/containerizer/cni_isolator_tests.cpp 
60c85adab11af06be474661544957ca20b7de8c3 
  src/tests/containerizer/cpu_isolator_tests.cpp 
0e86318bb7e261ac00be19f6405557f29a2e92af 
  src/tests/containerizer/docker_containerizer_tests.cpp 
fa91bcf8d4fdcda44fb5607d99b86a6323096244 
  src/tests/containerizer/docker_volume_isolator_tests.cpp 
866af61f8669163ff3ddd10ed7ecb655568f8250 
  src/tests/containerizer/environment_secret_isolator_tests.cpp 
b034ceec7d1bf92db8a1d344835ad48ea2d24952 
  src/tests/containerizer/io_switchboard_tests.cpp 
742143a0f093e415fe98235bbd25342fd65e0483 
  src/tests/containerizer/linux_capabilities_isolator_tests.cpp 
6d95d607b81358953a4afcec03b60e87e7192edd 
  src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
f9cab2fd123e48a5b4387fb2b609d70c7bf535cd 
  src/tests/containerizer/memory_isolator_tests.cpp 
b7b7acd37f69832e9ba79c9fe7ed0bc473b53e02 
  src/tests/containerizer/memory_pressure_tests.cpp 
8a43c4fada8bdcedc6e1e24833d0d6c94cf927d2 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
1fc56c4a8ca95bb47af968ad72dfef69455e7d46 
  src/tests/containerizer/port_mapping_tests.cpp 
84b39b15880c7b5d9b7967f1e686baa59f52a015 
  src/tests/containerizer/posix_rlimits_isolator_tests.cpp 
bf25049515e8b8d0c085be24c4de22b3ade261b2 
  src/tests/containerizer/routing_tests.cpp 
d05b3b11dc5fcd54d956990f252509865168e6b6 
  src/tests/containerizer/runtime_isolator_tests.cpp 
fbca31a4da1c83574cce7414fe5e03b1f86591cb 
  src/tests/containerizer/xfs_quota_tests.cpp 
c220a6ba5b274892bee195b26a5069d0750b4cb2 
  src/tests/default_executor_tests.cpp b9776314a8781963b92ba9ac297654f61a443bc8 
  src/tests/dynamic_weights_tests.cpp 3c86325a0749112606683bffdf305661170493e5 
  src/tests/exception_tests.cpp b5b657c2d1b31b59ab1554bf854ef1a69b1517ed 
  src/tests/fault_tolerance_tests.cpp 5b8213531f5688e94063937af19e674508f0dd8c 
  src/tests/fetcher_cache_tests.cpp 6d212cd460322ad6e97f4cf6ef537323275b7da0 
  src/tests/group_tests.cpp f2e37cc7184dd328466b554a16da3173cfbca4a7 
  src/tests/health_check_tests.cpp e824d1b2c2b17423394da0b089ae8940450378b7 
  src/tests/hook_tests.cpp 04edbf809b7289a7d5336e4f00f23dfb55d3dbf8 
  src/tests/http_fault_tolerance_tests.cpp 
8fcd56d86dcbdd181864756187beb4ff2ac1ff2a 
  src/tests/master_authorization_tests.cpp 
8d54472d8a31956e6e8bb5fe7ffbf47dc793c0bc 
  src/tests/master_maintenance_tests.cpp 
e7a80ff4199927df8bf0fb54458d357ae444260d 
  src/tests/master_quota_tests.cpp 9d52f767c112abd55ab3f49d172eb6e3caea511b 
  src/tests/master_slave_reconciliation_tests.cpp 
9c31eeae1a1b67af142a01e6c548b509ba06740c 
  src/tests/master_tests.cpp e6ed02e07a9860ca3a56bba9c502a09444c5b26e 
  src/tests/master_validation_tests.cpp 
813fb25007973f3499b94dcd0e9d2184ba08634c 
  src/tests/oversubscription_tests.cpp 54535a35e33dde0db3b547b9e31b4545d2900b67 
  src/tests/partition_tests.cpp 62a84f797201ccd18b71490949e3130d2b9c3668 
  src/tests/persistent_volume_tests.cpp 
7b0f436aa270f64cde93adc58833eef97c73dc26 
  src/tests/reconciliation_tests.cpp f1635fd627d9a7944cb9d4b0117fcded7b6693e1 
  src/tests/registrar_tests.cpp e2c38d32bc5882b95bbcf1da2e849e4d5ec6a9af 
  src/tests/registrar_zookeeper_tests.cpp 
cc082d320b4b150717728ec48edd0b26169cecd7 
  src/tests/resource_offers_tests.cpp e1fcab4b8fbb625876cf246505db2d05ac5d5710 
  src/tests/scheduler_driver_tests.cpp 47ad1dc9a4e5e0c5444fcc3a90d4645c3b6013ca 
  src/tests/scheduler_tests.cpp 5c42dc00f177a61d6ee595c26255492aa07aaad9 
  src/tests/slave_authorization_tests.cpp 
4e55148af77e4aae5829a056b0599954277758bb 
  src/tests/slave_recovery_tests.cpp 9ba6f6005c2edce67e1f63005518ab71ac981423 
  src/tests/slave_tests.cpp 896591527f20ae3697fcc98f66a3cbb97f2ec33c 
  src/tests/state_tests.cpp 7434473795d5122061eca9f7e62e87ae84af3133 
  src/tests/status_update_manager_tests.cpp 
710723c2a6ff7b72fce5d1d9ac69bf351e37a2ff 
  src/tests/upgrade_tests.cpp 7c900b17e5c895ea3a842f0f5b05b3a2876e61dd 


Diff: h

Re: Review Request 61511: Improved the readability of some assertions/expectations.

2017-08-10 Thread Benjamin Bannier 


> On Aug. 9, 2017, 5:21 p.m., Benjamin Bannier wrote:
> > Thanks for the cleanup Gaston!
> > 
> > Until looking at your patch I didn't realize how widespread the mistake of 
> > possibly using junk elements from possibly empty containers was, e.g.,
> > 
> >  EXPECT_NE(0u, offers.size()); // or equivalent: 
> > EXPECT_FALSE(offers.empty())
> >  
> >  // Do something with offers[0] which for e.g., 
> >  // `vector` doesn't do bounds checks.
> > 
> > The correct pattern here would be to do a hard assert for a non-empty 
> > collection so we don't run into undefined behavior should we against our 
> > expectations receive an empty collection. Could you please file a ticket 
> > for that?

I filed https://issues.apache.org/jira/browse/MESOS-7877.


- Benjamin


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


On Aug. 9, 2017, 3:01 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61511/
> ---
> 
> (Updated Aug. 9, 2017, 3:01 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prefer checking whether a container is empty instead of checking its
> size.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 75f7a585ec75f1d8b78453dd203bb78822ee7bc9 
>   src/tests/container_logger_tests.cpp 
> 2ebff809636fbf68c9e3fb87b98f168338f80d6d 
>   src/tests/containerizer/cgroups_isolator_tests.cpp 
> 67242119f2d10f6f3010c374ea58138e40e4a33e 
>   src/tests/containerizer/cgroups_tests.cpp 
> 506fc7f00dc1ac48476334de930b720b85a691dc 
>   src/tests/containerizer/cpu_isolator_tests.cpp 
> 0e86318bb7e261ac00be19f6405557f29a2e92af 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> c7984592aec2d4e7d1eb7ce077e742c4edc03bb9 
>   src/tests/containerizer/docker_volume_isolator_tests.cpp 
> 866af61f8669163ff3ddd10ed7ecb655568f8250 
>   src/tests/containerizer/environment_secret_isolator_tests.cpp 
> b034ceec7d1bf92db8a1d344835ad48ea2d24952 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> 742143a0f093e415fe98235bbd25342fd65e0483 
>   src/tests/containerizer/linux_capabilities_isolator_tests.cpp 
> 6d95d607b81358953a4afcec03b60e87e7192edd 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> f9cab2fd123e48a5b4387fb2b609d70c7bf535cd 
>   src/tests/containerizer/memory_isolator_tests.cpp 
> b7b7acd37f69832e9ba79c9fe7ed0bc473b53e02 
>   src/tests/containerizer/memory_pressure_tests.cpp 
> 8a43c4fada8bdcedc6e1e24833d0d6c94cf927d2 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 1fc56c4a8ca95bb47af968ad72dfef69455e7d46 
>   src/tests/containerizer/port_mapping_tests.cpp 
> 84b39b15880c7b5d9b7967f1e686baa59f52a015 
>   src/tests/containerizer/posix_rlimits_isolator_tests.cpp 
> bf25049515e8b8d0c085be24c4de22b3ade261b2 
>   src/tests/containerizer/routing_tests.cpp 
> d05b3b11dc5fcd54d956990f252509865168e6b6 
>   src/tests/dynamic_weights_tests.cpp 
> 3c86325a0749112606683bffdf305661170493e5 
>   src/tests/exception_tests.cpp b5b657c2d1b31b59ab1554bf854ef1a69b1517ed 
>   src/tests/fault_tolerance_tests.cpp 
> 5b8213531f5688e94063937af19e674508f0dd8c 
>   src/tests/fetcher_cache_tests.cpp 6d212cd460322ad6e97f4cf6ef537323275b7da0 
>   src/tests/group_tests.cpp f2e37cc7184dd328466b554a16da3173cfbca4a7 
>   src/tests/health_check_tests.cpp e824d1b2c2b17423394da0b089ae8940450378b7 
>   src/tests/hook_tests.cpp 04edbf809b7289a7d5336e4f00f23dfb55d3dbf8 
>   src/tests/master_authorization_tests.cpp 
> 8d54472d8a31956e6e8bb5fe7ffbf47dc793c0bc 
>   src/tests/master_maintenance_tests.cpp 
> e7a80ff4199927df8bf0fb54458d357ae444260d 
>   src/tests/master_slave_reconciliation_tests.cpp 
> 9c31eeae1a1b67af142a01e6c548b509ba06740c 
>   src/tests/master_tests.cpp e6ed02e07a9860ca3a56bba9c502a09444c5b26e 
>   src/tests/master_validation_tests.cpp 
> 813fb25007973f3499b94dcd0e9d2184ba08634c 
>   src/tests/oversubscription_tests.cpp 
> 54535a35e33dde0db3b547b9e31b4545d2900b67 
>   src/tests/partition_tests.cpp 62a84f797201ccd18b71490949e3130d2b9c3668 
>   src/tests/persistent_volume_tests.cpp 
> 7b0f436aa270f64cde93adc58833eef97c73dc26 
>   src/tests/reconciliation_tests.cpp f1635fd627d9a7944cb9d4b0117fcded7b6693e1 
>   src/tests/registrar_zookeeper_tests.cpp 
> cc082d320b4b150717728ec48edd0b26169cecd7 
>   src/tests/resource_offers_tests.cpp 
> e1fcab4b8fbb625876cf246505db2d05ac5d5710 
>   src/tests/scheduler_driver_tests.cpp 
> 47ad1dc9a4e5e0c5444fcc3a90d4645c3b6013ca 
>   src/tests/slave_authorization_tests.cpp 
> 4e55148af77e4aae5829a056b0599954277758bb 
>   src/tests/slave_recovery_tests.cpp 9ba6f6005c2edce67e1f63005518ab71ac981423 
>   src/t

Re: Review Request 61511: Improved the readability of some assertions/expectations.

2017-08-10 Thread Benjamin Bannier 


> On Aug. 9, 2017, 5:21 p.m., Benjamin Bannier wrote:
> > src/tests/api_tests.cpp
> > Line 1 (original), 1 (patched)
> > 
> >
> > Searching with
> > 
> > $ git grep -E '(EXPECT|ASSERT)_(EQ|NE).*(0u,\s.*size())'
> > 
> > I still get ~50 hits. Since you are at it, would you mind adjusting 
> > also?
> 
> Gastón Kleiman wrote:
> I had done a similar search, and as far as I can see, the other matches 
> are either false possitives or in stout/libprocess:
> 
> ```
> src/tests/api_tests.cpp:ASSERT_EQ(0u, 
> getState.get_tasks().tasks_size());
> src/tests/api_tests.cpp:ASSERT_EQ(0u, 
> getState.get_executors().executors_size());
> src/tests/api_tests.cpp:ASSERT_EQ(0u, 
> getState.get_tasks().completed_tasks_size());
> src/tests/api_tests.cpp:ASSERT_EQ(0u, 
> getState.get_tasks().tasks_size());
> src/tests/api_tests.cpp:  EXPECT_EQ(0u, 
> getState.get_frameworks().frameworks_size());
> src/tests/api_tests.cpp:  EXPECT_EQ(0u, 
> getState.get_agents().agents_size());
> src/tests/api_tests.cpp:  EXPECT_EQ(0u, 
> getState.get_tasks().tasks_size());
> src/tests/api_tests.cpp:  EXPECT_EQ(0u, 
> getState.get_executors().executors_size());
> src/tests/api_tests.cpp:ASSERT_EQ(0u, 
> v1Response->get_agents().agents_size());
> src/tests/api_tests.cpp:ASSERT_EQ(0u, 
> v1Response->get_agents().recovered_agents_size());
> src/tests/api_tests.cpp:  EXPECT_EQ(0u, 
> getState.get_tasks().tasks_size());
> src/tests/api_tests.cpp:  EXPECT_EQ(0u, 
> getState.get_executors().executors_size());
> src/tests/api_tests.cpp:  EXPECT_EQ(0u, 
> getState.get_frameworks().frameworks_size());
> src/tests/api_tests.cpp:  EXPECT_EQ(0u, 
> getState.get_agents().agents_size());
> src/tests/api_tests.cpp:  EXPECT_EQ(0u, 
> getState.get_tasks().tasks_size());
> src/tests/api_tests.cpp:  EXPECT_EQ(0u, 
> getState.get_executors().executors_size());
> src/tests/api_tests.cpp:ASSERT_EQ(0u, 
> getState.get_frameworks().frameworks_size());
> src/tests/api_tests.cpp:ASSERT_EQ(0u, 
> getState.get_tasks().launched_tasks_size());
> src/tests/api_tests.cpp:ASSERT_EQ(0u, 
> getState.get_executors().executors_size());
> src/tests/api_tests.cpp:ASSERT_EQ(0u, 
> getState.get_frameworks().completed_frameworks_size());
> src/tests/api_tests.cpp:ASSERT_EQ(0u, 
> getState.get_tasks().completed_tasks_size());
> src/tests/api_tests.cpp:ASSERT_EQ(0u, 
> getState.get_executors().completed_executors_size());
> src/tests/api_tests.cpp:ASSERT_EQ(0u, 
> getState.get_frameworks().frameworks_size());
> src/tests/api_tests.cpp:ASSERT_EQ(0u, 
> getState.get_tasks().launched_tasks_size());
> src/tests/api_tests.cpp:ASSERT_EQ(0u, 
> getState.get_executors().executors_size());
> ```
> 
> I don't think we can use `empty()` here.
> 
> And in the following match `manifest` is not a real container:
> 
> ```
> src/tests/containerizer/docker_spec_tests.cpp:  EXPECT_EQ(0u, 
> manifest->size());
> ```
> 
> I'll drop this issue and add two more patches for stout and libprocess to 
> the chain.

In principle we are still able to explicitly check emptiness here, e.g., we can 
replace 

ASSERT_EQ(0u, getState.get_tasks().tasks_size());

with 

ASSERT_TRUE(getState.get_tasks().tasks().empty());

There is some trade-off between local and global consistency in most of the 
cases in e.g., `api_tests.cpp`. When checking proto fields there, we often mix 
checking emptiness and checks for a particular size. I would go with a checks 
capturing our intention as much as possible, i.e., prefer checking for 
emptiness over local typographic consistency.


- Benjamin


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


On Aug. 9, 2017, 3:01 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61511/
> ---
> 
> (Updated Aug. 9, 2017, 3:01 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prefer checking whether a container is empty instead of checking its
> size.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 75f7a585ec75f1d8b78453dd203bb78822ee7bc9 
>   src/tests/container_logger_tests.cpp 
> 2ebff809636fbf68c9e3fb87b98f168338f80d6d 
>   src/tests/containerizer/cgroups_isolator_tests.cpp 
> 67242119f2d10f6f3010c374ea58138e40e4a33e 
>   src/tests/containerizer/cgroups_tests.cpp 
> 506fc7f00dc1ac48476334de930b720b85a691dc 
>   sr

Re: Review Request 61511: Improved the readability of some assertions/expectations.

2017-08-09 Thread Gastón Kleiman 


> On Aug. 9, 2017, 3:21 p.m., Benjamin Bannier wrote:
> > src/tests/api_tests.cpp
> > Line 1 (original), 1 (patched)
> > 
> >
> > Searching with
> > 
> > $ git grep -E '(EXPECT|ASSERT)_(EQ|NE).*(0u,\s.*size())'
> > 
> > I still get ~50 hits. Since you are at it, would you mind adjusting 
> > also?

I had done a similar search, and as far as I can see, the other matches are 
either false possitives or in stout/libprocess:

```
src/tests/api_tests.cpp:ASSERT_EQ(0u, getState.get_tasks().tasks_size());
src/tests/api_tests.cpp:ASSERT_EQ(0u, 
getState.get_executors().executors_size());
src/tests/api_tests.cpp:ASSERT_EQ(0u, 
getState.get_tasks().completed_tasks_size());
src/tests/api_tests.cpp:ASSERT_EQ(0u, getState.get_tasks().tasks_size());
src/tests/api_tests.cpp:  EXPECT_EQ(0u, 
getState.get_frameworks().frameworks_size());
src/tests/api_tests.cpp:  EXPECT_EQ(0u, getState.get_agents().agents_size());
src/tests/api_tests.cpp:  EXPECT_EQ(0u, getState.get_tasks().tasks_size());
src/tests/api_tests.cpp:  EXPECT_EQ(0u, 
getState.get_executors().executors_size());
src/tests/api_tests.cpp:ASSERT_EQ(0u, 
v1Response->get_agents().agents_size());
src/tests/api_tests.cpp:ASSERT_EQ(0u, 
v1Response->get_agents().recovered_agents_size());
src/tests/api_tests.cpp:  EXPECT_EQ(0u, getState.get_tasks().tasks_size());
src/tests/api_tests.cpp:  EXPECT_EQ(0u, 
getState.get_executors().executors_size());
src/tests/api_tests.cpp:  EXPECT_EQ(0u, 
getState.get_frameworks().frameworks_size());
src/tests/api_tests.cpp:  EXPECT_EQ(0u, getState.get_agents().agents_size());
src/tests/api_tests.cpp:  EXPECT_EQ(0u, getState.get_tasks().tasks_size());
src/tests/api_tests.cpp:  EXPECT_EQ(0u, 
getState.get_executors().executors_size());
src/tests/api_tests.cpp:ASSERT_EQ(0u, 
getState.get_frameworks().frameworks_size());
src/tests/api_tests.cpp:ASSERT_EQ(0u, 
getState.get_tasks().launched_tasks_size());
src/tests/api_tests.cpp:ASSERT_EQ(0u, 
getState.get_executors().executors_size());
src/tests/api_tests.cpp:ASSERT_EQ(0u, 
getState.get_frameworks().completed_frameworks_size());
src/tests/api_tests.cpp:ASSERT_EQ(0u, 
getState.get_tasks().completed_tasks_size());
src/tests/api_tests.cpp:ASSERT_EQ(0u, 
getState.get_executors().completed_executors_size());
src/tests/api_tests.cpp:ASSERT_EQ(0u, 
getState.get_frameworks().frameworks_size());
src/tests/api_tests.cpp:ASSERT_EQ(0u, 
getState.get_tasks().launched_tasks_size());
src/tests/api_tests.cpp:ASSERT_EQ(0u, 
getState.get_executors().executors_size());
```

I don't think we can use `empty()` here.

And in the following match `manifest` is not a real container:

```
src/tests/containerizer/docker_spec_tests.cpp:  EXPECT_EQ(0u, manifest->size());
```

I'll drop this issue and add two more patches for stout and libprocess to the 
chain.


- Gastón


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


On Aug. 9, 2017, 1:01 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61511/
> ---
> 
> (Updated Aug. 9, 2017, 1:01 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prefer checking whether a container is empty instead of checking its
> size.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 75f7a585ec75f1d8b78453dd203bb78822ee7bc9 
>   src/tests/container_logger_tests.cpp 
> 2ebff809636fbf68c9e3fb87b98f168338f80d6d 
>   src/tests/containerizer/cgroups_isolator_tests.cpp 
> 67242119f2d10f6f3010c374ea58138e40e4a33e 
>   src/tests/containerizer/cgroups_tests.cpp 
> 506fc7f00dc1ac48476334de930b720b85a691dc 
>   src/tests/containerizer/cpu_isolator_tests.cpp 
> 0e86318bb7e261ac00be19f6405557f29a2e92af 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> c7984592aec2d4e7d1eb7ce077e742c4edc03bb9 
>   src/tests/containerizer/docker_volume_isolator_tests.cpp 
> 866af61f8669163ff3ddd10ed7ecb655568f8250 
>   src/tests/containerizer/environment_secret_isolator_tests.cpp 
> b034ceec7d1bf92db8a1d344835ad48ea2d24952 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> 742143a0f093e415fe98235bbd25342fd65e0483 
>   src/tests/containerizer/linux_capabilities_isolator_tests.cpp 
> 6d95d607b81358953a4afcec03b60e87e7192edd 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> f9cab2fd123e48a5b4387fb2b609d70c7bf535cd 
>   src/tests/containerizer/memory_isolator_tests.cpp 
> b7b7acd37f69832e9ba79c9fe7ed0bc473b53e02 
>   src/tests/containerizer/memory_pressure_tests.cpp 
> 8a43c

Re: Review Request 61511: Improved the readability of some assertions/expectations.

2017-08-09 Thread Benjamin Bannier 

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


Fix it, then Ship it!




Thanks for the cleanup Gaston!

Until looking at your patch I didn't realize how widespread the mistake of 
possibly using junk elements from possibly empty containers was, e.g.,

 EXPECT_NE(0u, offers.size()); // or equivalent: 
EXPECT_FALSE(offers.empty())
 
 // Do something with offers[0] which for e.g., 
 // `vector` doesn't do bounds checks.

The correct pattern here would be to do a hard assert for a non-empty 
collection so we don't run into undefined behavior should we against our 
expectations receive an empty collection. Could you please file a ticket for 
that?


src/tests/api_tests.cpp
Line 1 (original), 1 (patched)


Searching with

$ git grep -E '(EXPECT|ASSERT)_(EQ|NE).*(0u,\s.*size())'

I still get ~50 hits. Since you are at it, would you mind adjusting also?


- Benjamin Bannier


On Aug. 9, 2017, 3:01 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61511/
> ---
> 
> (Updated Aug. 9, 2017, 3:01 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prefer checking whether a container is empty instead of checking its
> size.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 75f7a585ec75f1d8b78453dd203bb78822ee7bc9 
>   src/tests/container_logger_tests.cpp 
> 2ebff809636fbf68c9e3fb87b98f168338f80d6d 
>   src/tests/containerizer/cgroups_isolator_tests.cpp 
> 67242119f2d10f6f3010c374ea58138e40e4a33e 
>   src/tests/containerizer/cgroups_tests.cpp 
> 506fc7f00dc1ac48476334de930b720b85a691dc 
>   src/tests/containerizer/cpu_isolator_tests.cpp 
> 0e86318bb7e261ac00be19f6405557f29a2e92af 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> c7984592aec2d4e7d1eb7ce077e742c4edc03bb9 
>   src/tests/containerizer/docker_volume_isolator_tests.cpp 
> 866af61f8669163ff3ddd10ed7ecb655568f8250 
>   src/tests/containerizer/environment_secret_isolator_tests.cpp 
> b034ceec7d1bf92db8a1d344835ad48ea2d24952 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> 742143a0f093e415fe98235bbd25342fd65e0483 
>   src/tests/containerizer/linux_capabilities_isolator_tests.cpp 
> 6d95d607b81358953a4afcec03b60e87e7192edd 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> f9cab2fd123e48a5b4387fb2b609d70c7bf535cd 
>   src/tests/containerizer/memory_isolator_tests.cpp 
> b7b7acd37f69832e9ba79c9fe7ed0bc473b53e02 
>   src/tests/containerizer/memory_pressure_tests.cpp 
> 8a43c4fada8bdcedc6e1e24833d0d6c94cf927d2 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 1fc56c4a8ca95bb47af968ad72dfef69455e7d46 
>   src/tests/containerizer/port_mapping_tests.cpp 
> 84b39b15880c7b5d9b7967f1e686baa59f52a015 
>   src/tests/containerizer/posix_rlimits_isolator_tests.cpp 
> bf25049515e8b8d0c085be24c4de22b3ade261b2 
>   src/tests/containerizer/routing_tests.cpp 
> d05b3b11dc5fcd54d956990f252509865168e6b6 
>   src/tests/dynamic_weights_tests.cpp 
> 3c86325a0749112606683bffdf305661170493e5 
>   src/tests/exception_tests.cpp b5b657c2d1b31b59ab1554bf854ef1a69b1517ed 
>   src/tests/fault_tolerance_tests.cpp 
> 5b8213531f5688e94063937af19e674508f0dd8c 
>   src/tests/fetcher_cache_tests.cpp 6d212cd460322ad6e97f4cf6ef537323275b7da0 
>   src/tests/group_tests.cpp f2e37cc7184dd328466b554a16da3173cfbca4a7 
>   src/tests/health_check_tests.cpp e824d1b2c2b17423394da0b089ae8940450378b7 
>   src/tests/hook_tests.cpp 04edbf809b7289a7d5336e4f00f23dfb55d3dbf8 
>   src/tests/master_authorization_tests.cpp 
> 8d54472d8a31956e6e8bb5fe7ffbf47dc793c0bc 
>   src/tests/master_maintenance_tests.cpp 
> e7a80ff4199927df8bf0fb54458d357ae444260d 
>   src/tests/master_slave_reconciliation_tests.cpp 
> 9c31eeae1a1b67af142a01e6c548b509ba06740c 
>   src/tests/master_tests.cpp e6ed02e07a9860ca3a56bba9c502a09444c5b26e 
>   src/tests/master_validation_tests.cpp 
> 813fb25007973f3499b94dcd0e9d2184ba08634c 
>   src/tests/oversubscription_tests.cpp 
> 54535a35e33dde0db3b547b9e31b4545d2900b67 
>   src/tests/partition_tests.cpp 62a84f797201ccd18b71490949e3130d2b9c3668 
>   src/tests/persistent_volume_tests.cpp 
> 7b0f436aa270f64cde93adc58833eef97c73dc26 
>   src/tests/reconciliation_tests.cpp f1635fd627d9a7944cb9d4b0117fcded7b6693e1 
>   src/tests/registrar_zookeeper_tests.cpp 
> cc082d320b4b150717728ec48edd0b26169cecd7 
>   src/tests/resource_offers_tests.cpp 
> e1fcab4b8fbb625876cf246505db2d05ac5d5710 
>   src/tests/scheduler_driver_tests.cpp 
> 47ad1dc9a4e5e0c5444fcc3a90d4645c3b6013ca 
>   src/tests/slave_authorization

Re: Review Request 61511: Improved the readability of some assertions/expectations.

2017-08-08 Thread Gastón Kleiman 

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

(Updated Aug. 9, 2017, 1:01 a.m.)


Review request for mesos, Benjamin Bannier, Jie Yu, and Vinod Kone.


Changes
---

Fixed an error.


Repository: mesos


Description
---

Prefer checking whether a container is empty instead of checking its
size.


Diffs (updated)
-

  src/tests/api_tests.cpp 75f7a585ec75f1d8b78453dd203bb78822ee7bc9 
  src/tests/container_logger_tests.cpp 2ebff809636fbf68c9e3fb87b98f168338f80d6d 
  src/tests/containerizer/cgroups_isolator_tests.cpp 
67242119f2d10f6f3010c374ea58138e40e4a33e 
  src/tests/containerizer/cgroups_tests.cpp 
506fc7f00dc1ac48476334de930b720b85a691dc 
  src/tests/containerizer/cpu_isolator_tests.cpp 
0e86318bb7e261ac00be19f6405557f29a2e92af 
  src/tests/containerizer/docker_containerizer_tests.cpp 
c7984592aec2d4e7d1eb7ce077e742c4edc03bb9 
  src/tests/containerizer/docker_volume_isolator_tests.cpp 
866af61f8669163ff3ddd10ed7ecb655568f8250 
  src/tests/containerizer/environment_secret_isolator_tests.cpp 
b034ceec7d1bf92db8a1d344835ad48ea2d24952 
  src/tests/containerizer/io_switchboard_tests.cpp 
742143a0f093e415fe98235bbd25342fd65e0483 
  src/tests/containerizer/linux_capabilities_isolator_tests.cpp 
6d95d607b81358953a4afcec03b60e87e7192edd 
  src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
f9cab2fd123e48a5b4387fb2b609d70c7bf535cd 
  src/tests/containerizer/memory_isolator_tests.cpp 
b7b7acd37f69832e9ba79c9fe7ed0bc473b53e02 
  src/tests/containerizer/memory_pressure_tests.cpp 
8a43c4fada8bdcedc6e1e24833d0d6c94cf927d2 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
1fc56c4a8ca95bb47af968ad72dfef69455e7d46 
  src/tests/containerizer/port_mapping_tests.cpp 
84b39b15880c7b5d9b7967f1e686baa59f52a015 
  src/tests/containerizer/posix_rlimits_isolator_tests.cpp 
bf25049515e8b8d0c085be24c4de22b3ade261b2 
  src/tests/containerizer/routing_tests.cpp 
d05b3b11dc5fcd54d956990f252509865168e6b6 
  src/tests/dynamic_weights_tests.cpp 3c86325a0749112606683bffdf305661170493e5 
  src/tests/exception_tests.cpp b5b657c2d1b31b59ab1554bf854ef1a69b1517ed 
  src/tests/fault_tolerance_tests.cpp 5b8213531f5688e94063937af19e674508f0dd8c 
  src/tests/fetcher_cache_tests.cpp 6d212cd460322ad6e97f4cf6ef537323275b7da0 
  src/tests/group_tests.cpp f2e37cc7184dd328466b554a16da3173cfbca4a7 
  src/tests/health_check_tests.cpp e824d1b2c2b17423394da0b089ae8940450378b7 
  src/tests/hook_tests.cpp 04edbf809b7289a7d5336e4f00f23dfb55d3dbf8 
  src/tests/master_authorization_tests.cpp 
8d54472d8a31956e6e8bb5fe7ffbf47dc793c0bc 
  src/tests/master_maintenance_tests.cpp 
e7a80ff4199927df8bf0fb54458d357ae444260d 
  src/tests/master_slave_reconciliation_tests.cpp 
9c31eeae1a1b67af142a01e6c548b509ba06740c 
  src/tests/master_tests.cpp e6ed02e07a9860ca3a56bba9c502a09444c5b26e 
  src/tests/master_validation_tests.cpp 
813fb25007973f3499b94dcd0e9d2184ba08634c 
  src/tests/oversubscription_tests.cpp 54535a35e33dde0db3b547b9e31b4545d2900b67 
  src/tests/partition_tests.cpp 62a84f797201ccd18b71490949e3130d2b9c3668 
  src/tests/persistent_volume_tests.cpp 
7b0f436aa270f64cde93adc58833eef97c73dc26 
  src/tests/reconciliation_tests.cpp f1635fd627d9a7944cb9d4b0117fcded7b6693e1 
  src/tests/registrar_zookeeper_tests.cpp 
cc082d320b4b150717728ec48edd0b26169cecd7 
  src/tests/resource_offers_tests.cpp e1fcab4b8fbb625876cf246505db2d05ac5d5710 
  src/tests/scheduler_driver_tests.cpp 47ad1dc9a4e5e0c5444fcc3a90d4645c3b6013ca 
  src/tests/slave_authorization_tests.cpp 
4e55148af77e4aae5829a056b0599954277758bb 
  src/tests/slave_recovery_tests.cpp 9ba6f6005c2edce67e1f63005518ab71ac981423 
  src/tests/slave_tests.cpp 1d9d142ed9e801b79535a2c28f5a94ffbf1bf160 
  src/tests/status_update_manager_tests.cpp 
710723c2a6ff7b72fce5d1d9ac69bf351e37a2ff 
  src/tests/upgrade_tests.cpp 7c900b17e5c895ea3a842f0f5b05b3a2876e61dd 


Diff: https://reviews.apache.org/r/61511/diff/2/

Changes: https://reviews.apache.org/r/61511/diff/1-2/


Testing
---

`sudo bin/mesos-tests.sh`


Thanks,

Gastón Kleiman

Review Request 61511: Improved the readability of some assertions/expectations.

2017-08-08 Thread Gastón Kleiman 

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

Review request for mesos, Benjamin Bannier, Jie Yu, and Vinod Kone.


Repository: mesos


Description
---

Prefer checking whether a container is empty instead of checking its
size.


Diffs
-

  src/tests/api_tests.cpp 75f7a585ec75f1d8b78453dd203bb78822ee7bc9 
  src/tests/container_logger_tests.cpp 2ebff809636fbf68c9e3fb87b98f168338f80d6d 
  src/tests/containerizer/cgroups_isolator_tests.cpp 
67242119f2d10f6f3010c374ea58138e40e4a33e 
  src/tests/containerizer/cgroups_tests.cpp 
506fc7f00dc1ac48476334de930b720b85a691dc 
  src/tests/containerizer/cpu_isolator_tests.cpp 
0e86318bb7e261ac00be19f6405557f29a2e92af 
  src/tests/containerizer/docker_containerizer_tests.cpp 
c7984592aec2d4e7d1eb7ce077e742c4edc03bb9 
  src/tests/containerizer/docker_volume_isolator_tests.cpp 
866af61f8669163ff3ddd10ed7ecb655568f8250 
  src/tests/containerizer/environment_secret_isolator_tests.cpp 
b034ceec7d1bf92db8a1d344835ad48ea2d24952 
  src/tests/containerizer/io_switchboard_tests.cpp 
742143a0f093e415fe98235bbd25342fd65e0483 
  src/tests/containerizer/linux_capabilities_isolator_tests.cpp 
6d95d607b81358953a4afcec03b60e87e7192edd 
  src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
f9cab2fd123e48a5b4387fb2b609d70c7bf535cd 
  src/tests/containerizer/memory_isolator_tests.cpp 
b7b7acd37f69832e9ba79c9fe7ed0bc473b53e02 
  src/tests/containerizer/memory_pressure_tests.cpp 
8a43c4fada8bdcedc6e1e24833d0d6c94cf927d2 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
1fc56c4a8ca95bb47af968ad72dfef69455e7d46 
  src/tests/containerizer/port_mapping_tests.cpp 
84b39b15880c7b5d9b7967f1e686baa59f52a015 
  src/tests/containerizer/posix_rlimits_isolator_tests.cpp 
bf25049515e8b8d0c085be24c4de22b3ade261b2 
  src/tests/containerizer/routing_tests.cpp 
d05b3b11dc5fcd54d956990f252509865168e6b6 
  src/tests/dynamic_weights_tests.cpp 3c86325a0749112606683bffdf305661170493e5 
  src/tests/exception_tests.cpp b5b657c2d1b31b59ab1554bf854ef1a69b1517ed 
  src/tests/fault_tolerance_tests.cpp 5b8213531f5688e94063937af19e674508f0dd8c 
  src/tests/fetcher_cache_tests.cpp 6d212cd460322ad6e97f4cf6ef537323275b7da0 
  src/tests/group_tests.cpp f2e37cc7184dd328466b554a16da3173cfbca4a7 
  src/tests/health_check_tests.cpp e824d1b2c2b17423394da0b089ae8940450378b7 
  src/tests/hook_tests.cpp 04edbf809b7289a7d5336e4f00f23dfb55d3dbf8 
  src/tests/master_authorization_tests.cpp 
8d54472d8a31956e6e8bb5fe7ffbf47dc793c0bc 
  src/tests/master_maintenance_tests.cpp 
e7a80ff4199927df8bf0fb54458d357ae444260d 
  src/tests/master_slave_reconciliation_tests.cpp 
9c31eeae1a1b67af142a01e6c548b509ba06740c 
  src/tests/master_tests.cpp e6ed02e07a9860ca3a56bba9c502a09444c5b26e 
  src/tests/master_validation_tests.cpp 
813fb25007973f3499b94dcd0e9d2184ba08634c 
  src/tests/oversubscription_tests.cpp 54535a35e33dde0db3b547b9e31b4545d2900b67 
  src/tests/partition_tests.cpp 62a84f797201ccd18b71490949e3130d2b9c3668 
  src/tests/persistent_volume_tests.cpp 
7b0f436aa270f64cde93adc58833eef97c73dc26 
  src/tests/reconciliation_tests.cpp f1635fd627d9a7944cb9d4b0117fcded7b6693e1 
  src/tests/registrar_zookeeper_tests.cpp 
cc082d320b4b150717728ec48edd0b26169cecd7 
  src/tests/resource_offers_tests.cpp e1fcab4b8fbb625876cf246505db2d05ac5d5710 
  src/tests/scheduler_driver_tests.cpp 47ad1dc9a4e5e0c5444fcc3a90d4645c3b6013ca 
  src/tests/slave_authorization_tests.cpp 
4e55148af77e4aae5829a056b0599954277758bb 
  src/tests/slave_recovery_tests.cpp 9ba6f6005c2edce67e1f63005518ab71ac981423 
  src/tests/slave_tests.cpp 1d9d142ed9e801b79535a2c28f5a94ffbf1bf160 
  src/tests/status_update_manager_tests.cpp 
710723c2a6ff7b72fce5d1d9ac69bf351e37a2ff 
  src/tests/upgrade_tests.cpp 7c900b17e5c895ea3a842f0f5b05b3a2876e61dd 


Diff: https://reviews.apache.org/r/61511/diff/1/


Testing
---

`sudo bin/mesos-tests.sh`


Thanks,

Gastón Kleiman