You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Gastón Kleiman <ga...@mesosphere.io> on 2017/08/09 00:14:54 UTC
Review Request 61511: Improved the readability of some
assertions/expectations.
-----------------------------------------------------------
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
Re: Review Request 61511: Improved the readability of some
assertions/expectations.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Aug. 9, 2017, 5:21 p.m., Benjamin Bannier wrote:
> > src/tests/api_tests.cpp
> > Line 1 (original), 1 (patched)
> > <https://reviews.apache.org/r/61511/diff/2/?file=1793693#file1793693line1>
> >
> > 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
> 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/
>
>
> Testing
> -------
>
> `sudo bin/mesos-tests.sh`
>
>
> Thanks,
>
> Gastón Kleiman
>
>
Re: Review Request 61511: Improved the readability of some
assertions/expectations.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
> On Aug. 9, 2017, 3:21 p.m., Benjamin Bannier wrote:
> > src/tests/api_tests.cpp
> > Line 1 (original), 1 (patched)
> > <https://reviews.apache.org/r/61511/diff/2/?file=1793693#file1793693line1>
> >
> > 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 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: https://reviews.apache.org/r/61511/diff/3/
>
>
> Testing
> -------
>
> `sudo bin/mesos-tests.sh`
>
>
> Thanks,
>
> Gastón Kleiman
>
>
Re: Review Request 61511: Improved the readability of some
assertions/expectations.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> 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/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/
>
>
> Testing
> -------
>
> `sudo bin/mesos-tests.sh`
>
>
> Thanks,
>
> Gastón Kleiman
>
>
Re: Review Request 61511: Improved the readability of some
assertions/expectations.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
> On Aug. 9, 2017, 3:21 p.m., Benjamin Bannier wrote:
> > src/tests/api_tests.cpp
> > Line 1 (original), 1 (patched)
> > <https://reviews.apache.org/r/61511/diff/2/?file=1793693#file1793693line1>
> >
> > 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 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/
>
>
> Testing
> -------
>
> `sudo bin/mesos-tests.sh`
>
>
> Thanks,
>
> Gastón Kleiman
>
>
Re: Review Request 61511: Improved the readability of some
assertions/expectations.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/61511/#comment258398>
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_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/
>
>
> Testing
> -------
>
> `sudo bin/mesos-tests.sh`
>
>
> Thanks,
>
> Gastón Kleiman
>
>
Re: Review Request 61511: Improved the readability of some
assertions/expectations.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
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 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: https://reviews.apache.org/r/61511/diff/3/
>
>
> Testing
> -------
>
> `sudo bin/mesos-tests.sh`
>
>
> Thanks,
>
> Gastón Kleiman
>
>
Re: Review Request 61511: Improved the readability of some
assertions/expectations.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
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 moaaaar 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: https://reviews.apache.org/r/61511/diff/3/
Changes: https://reviews.apache.org/r/61511/diff/2-3/
Testing
-------
`sudo bin/mesos-tests.sh`
Thanks,
Gastón Kleiman
Re: Review Request 61511: Improved the readability of some
assertions/expectations.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
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