[ https://issues.apache.org/jira/browse/MESOS-7877?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16165042#comment-16165042 ]
Greg Mann commented on MESOS-7877: ---------------------------------- {code} commit 1f4d7ef27e0e4936c1ea15d4e56d778e35a92507 Author: Gastón Kleiman gas...@mesosphere.io Date: Wed Sep 13 09:21:20 2017 -0700 Added new overloads for the `createExecutorInfo` test helper method. These new overloads make it possible to specify framework ID, executor resources, and executor ID as a protobuf message rather than a string. Review: https://reviews.apache.org/r/62197/ {code} {code} commit 2a6f6b7aedf05b23ae0fe04364159c87f6c5cea8 Author: Gastón Kleiman gas...@mesosphere.io Date: Wed Sep 13 09:21:25 2017 -0700 Changed `EXPECT` to `ASSERT` when relying on the assertion afterwards. A common pattern in our tests is to check that at least one offer is received using: 'EXPECT_FALSE(offers->offers().empty())' The test then accesses the first element of the array returned by `offers->offers()` to extract information such as the agent ID. This patch makes the tests that follow this pattern use `ASSERT_FALSE` instead of `EXPECT_FALSE` to avoid invalid memory accesses when the array is empty. Review: https://reviews.apache.org/r/62042/ {code} > Audit test code for undefined behavior in accessing container elements > ---------------------------------------------------------------------- > > Key: MESOS-7877 > URL: https://issues.apache.org/jira/browse/MESOS-7877 > Project: Mesos > Issue Type: Bug > Components: test > Reporter: Benjamin Bannier > Assignee: Gastón Kleiman > Priority: Minor > Labels: mesosphere, newbie, tech-debt, test > > We do not always make sure we never access elements from empty containers, > e.g., we use patterns like the following > {code} > Future<vector<Offer>> offers; > // Satisfy offers. > EXPECT_FALSE(offers.empty()); > const auto& offer = (*offers)[0]; > {code} > While the intention here is to diagnose an empty {{offers}}, the code still > exhibits undefined behavior in the element access if {{offers}} was indeed > empty (compilers might aggressively exploit undefined behavior to e.g., > remove "impossible" code). Instead one should prevent accessing any elements > of an empty container, e.g., > {code} > ASSERT_FALSE(offers.empty()); // Prevent execution of rest of test body. > {code} > We should audit and fix existing test code for such incorrect checks and > variations involving e.g., {{EXPECT_NE}}. -- This message was sent by Atlassian JIRA (v6.4.14#64029)