Fix CropBox with indices; add new assertions for unit tests

[Enderdead]

In order to cover the current crop box issue, I have added new pcl_tests functions. I'm not sure if you will agree with my adding so I'm waiting for your review about it.
The fix on CropBox class is pretty obvious just replace some index by (*indices_)[index]

Fixes #2922.

[taketwo]

Hi, we can add new test facilities, no problem. However, I would propose to implement them similarly to other assertions (e.g. equality of XYZ points). That is, have two parts:

1. Function that does actual test and returns ::testing::AssertionResult.
2. Convenience macro that puts together the function and ASSERT_PRED_FORMAT2 to provide a simpler interface to the user.

The function you have already, we only need to rename it to something like VectorContainsAll (note no "expect" because this function can be used both in "expect" and "assert" checks) and move to internal namespace.

The macro should be trivial to implement, just check the existing ones. Note that we need a pair of "expect" and "assert" macros so that the user can choose the one that is more appropriate for the task at hand.

[Enderdead]

Sorry I didn't see these macros on the bottom.
I'm not really proud of the current log on internal::VectorContainsAll.
In my initial commit, I wrote this :

std::ostringstream vec_rep;
std::copy(v.cbegin(), v.cend()-1, std::ostream_iterator<V>(vec_rep, ", "));
vec_rep<<v.back();

std::ostringstream elements_rep;
std::copy(elements.cbegin(), elements.cend()-1, std::ostream_iterator<V>(elements_rep, ", "));
elements_rep << elements.back();

 return ::testing::AssertionFailure ()
       << "Vector       : "<< expr1<< std::endl
       << "contains     : " << vec_rep.str() << std::endl
       << "From         : " << expr2<< std::endl
       << "must contain : " << elements_rep.str() << std::endl;

This program will display log like this :

/pcl/test/filters/test_clipper.cpp:356: Failure
Vector       : *removed_indices
contains     : 2, 3, 4, 6
From         : std::vector<int>({3, 5, 6, 8})
must contain : 3, 5, 6, 8

As you can see here, you have a duplicate target vector and if you get large vectors, it will be very hard to figure out the log. Do you have any ideas to improve that?

[taketwo]

I don't think we can do much about this. Perhaps do not print vector contents if its size is larger than a certain threshold. As for the duplication, it only happens if you supply "from" vector in-place. When you know that you are dealing with a large vector, create a temporary variable (e.g. expected_result) to hold it and then pass to the test macro.

[Enderdead]

Another way is to use the gmock lib (https://github.com/google/googletest/blob/master/googlemock/docs/cheat_sheet.md#container-matchers) . It provide many new predicates like UnorderedElementsAre.

[taketwo]

To be honest, I am reluctant to add a new dependency just to get a few predicates. If we really need them, I'd rather copy their implementation (gmock is a BSD library, so should be ok). @SergioRAgostinho what's your stance?

[Enderdead]

I think you are right. While only test_clipper.cpp use vector asserts, it's pretty useless to add another dependency on the whole project. Do I have to squash my new commit with the previous one?

[taketwo]

Do I have to squash my new commit with the previous one?

While we are iterating in the review process just push more and more commits. Once we a fully done we can rewrite history before merging.

[taketwo]

Great, looks good to me. Can you please rewrite history to have one commit adding new unit testing macros and second one fixing the bug and improving unit tests? Then we are good to go.

[Enderdead]

It's done, tell me if I misunderstood your instructions.