mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 61511: Improved the readability of some assertions/expectations.
Date Thu, 10 Aug 2017 09:04:45 GMT


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


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