Test name length


(Dženan Zukić) #1

Recently, length of test names have been getting out of hand. This is quite inconvenient for for viewing in the console, as all the lines are aligned based on the longest one. In practice this causes output line wrapping even for console window 160 characters wide.

Descriptive names are good, but a name does not have to be a whole sentence. This is the case for the worst “offenders”. Can we agree on shortening these names?

Here is a list of the longest test names. The longest one is 124, and the last on this excerpt 71 characters long.

ShapedImageNeighborhoodRange.ShapedImageNeighborhoodRange_iterator_retrieves_same_pixel_values_as_ConstNeighborhoodIterator
ImageNeighborhoodOffsets.GenerateHyperrectangularImageNeighborhoodOffsets_returns_one_offset_for_default_radius
ImageNeighborhoodOffsets.GenerateHyperrectangularImageNeighborhoodOffsets_for_smallest_horizontal_neigborhood
ImageNeighborhoodOffsets.GenerateHyperrectangularImageNeighborhoodOffsets_for_smallest_vertical_neigborhood
ShapedImageNeighborhoodRange.ShapedImageNeighborhoodRange_iterator_reference_acts_like_a_real_reference
ShapedImageNeighborhoodRange.NeigborhoodRange_iterators_can_be_used_as_arguments_std_vector_constructor
ShapedImageNeighborhoodRange.Neigborhood_iterators_can_be_used_as_parameters_of_std_inner_product
ShapedImageNeighborhoodRange.Neigborhood_iterators_can_be_used_as_parameters_of_std_nth_element
ShapedImageNeighborhoodRange.Neigborhood_iterators_can_be_used_as_arguments_of_std_reverse_copy
ImageNeighborhoodOffsets.GenerateImageNeighborhoodOffsets_returns_empty_vector_for_empty_shape
ConnectedImageNeighborhoodShape.The_middle_offset_is_all_zero_when_center_pixel_is_included
ConnectedImageNeighborhoodShape.GenerateImageNeighborhoodOffsets_returns_expected_offsets
ShapedImageNeighborhoodRange.Neigborhood_iterators_can_be_used_as_parameters_of_std_sort
ShapedImageNeighborhoodRange.Distance_between_iterators_can_be_obtained_by_subtraction
ShapedImageNeighborhoodRange.NeigborhoodRange_can_be_used_with_range_based_for_loop
ConnectedImageNeighborhoodShape.Offsets_are_unique_and_colexicographically_ordered
ShapedImageNeighborhoodRange.ShapedImageNeighborhoodRange_supports_VectorImage
ResampleImageFilter.Filter_preserves_min_and_max_int64_pixel_values_by_default
ShapedImageNeighborhoodRange.Equivalent_begin_or_end_iterators_compare_equal
itkGaussianSmoothingOnUpdateDisplacementFieldTransformParametersAdaptorTest
itkBSplineSmoothingOnUpdateDisplacementFieldTransformParametersAdaptorTest
ConnectedImageNeighborhoodShape.GetNumberOfOffsets_returns_expected_value
ShapedImageNeighborhoodRange.NeigborhoodRange_can_be_used_in_std_for_each
ShapedImageNeighborhoodRange.Neigborhood_iterators_support_random_access
itkDiscreteGradientMagnitudeGaussianImageFunctionTestUseImageSpacingOff
ResampleImageFilter.Filter_preserves_any_double_pixel_value_by_default

(Jon Haitz Legarreta Gorroño) #2

These look like my way of naming :sweat_smile:

Our SW Guide does not establish a convention for this as of now, but may be it should draw certain general lines.

Although it is a matter of tastes, I do agree that too long names do not help much, although adding a test is already better than not having it. That being said, I would discourage the use of punctuation marks, and I would use CamelCase, as customary in ITK classes, to name the tests.


(Bradley Lowekamp) #3

The tests names with the period in the names are google tests. When you define a google test you do it like this:

TEST(TestCaseName, TestName) {
  ... test body ...
}

And then CMake names the tests TestCaseName.TestName. It should be possible to group the google tests by the TestCaseName which envokes all the TestNames tests.


(Bradley Lowekamp) #4

For this case the name of the class is BSplineSmoothingOnUpdateDisplacementFieldTransformParametersAdaptor. This follows the basic conventions of itkNameOfClasssTest#.

The google tests may follow the convention:
TEST(NameOfClass, some_descriptive_name_of_the_test)


(Dženan Zukić) #5

GTests being TestCaseName.TestName is fine. But TestCaseName.TestCaseName_TestName provides no benefit, and reduces readability. Not to mention increase length of the name considerably. some_descriptive_name_of_the_test could be a little more condensed/shorter for the tests on this list.

Let’s consider these 3 test names:

ImageNeighborhoodOffsets.GenerateHyperrectangularImageNeighborhoodOffsets_returns_one_offset_for_default_radius
ImageNeighborhoodOffsets.GenerateHyperrectangularImageNeighborhoodOffsets_for_smallest_horizontal_neigborhood
ImageNeighborhoodOffsets.GenerateHyperrectangularImageNeighborhoodOffsets_for_smallest_vertical_neigborhood

with little loss of meaning these can be renamed into:

ImageNeighborhoodOffsets.return_one_offset_for_default_radius
ImageNeighborhoodOffsets.smallest_horizontal_neigborhood
ImageNeighborhoodOffsets.smallest_vertical_neigborhood

approximately reducing length of names from 110 to 62 characters. Similar treatment can be applied to other tests with super-long names.


(Bradley Lowekamp) #6

Those are good suggestions.

Regarding CTest having a TestCaseName calling the sub test_names, the CMake documentation has the following to say:

Both commands […] register tests, and will create a separate CTest test for each Google Test test case… Note that this is in some cases less efficient, as common set-up and tear-down logic cannot be shared by multiple test cases executing in the same instance. However, it provides more fine-grained pass/fail information to CTest, which is usually considered as more beneficial. By default, the CTest test name is the same as the Google Test name (i.e. suite.testcase); see also TEST_PREFIX and TEST_SUFFIX.

CMake’s terminology differs from that in the GTest Primer which I used. However, the above recommendations is to have CTest not run the GTests in any group/batch fashion. For example ./ITKCommonGTestDriver --gtest_filter=SmartPointer\.\*

There are some reasons to want to look into running GTests in groups.

  • the long names generated,
  • there can be a large number of the individual tests
  • more time is spent creating the processes and many tests burden CTest or CDash

I don’t think that we should change calling the individual tests because it goes against the CMake recommendation and would require additional CMake effort. However, these are just issues to be made aware of.


(Niels Dekker) #7

First of all I’m glad we agree that descriptive names are A Good Thing. :smiley: While I agree that a test name should not be longer than necessary, I think it should still be long enough to indicate clearly which specific requirement is tested. (And I think it’s usually preferable for a test function to test one specific requirement.) So I would certainly not like to have a “hard” (enforced) upper limit on the length of a test name. However, I can imagine having a suggested or preferred maximum length (which might still be exceeded, when there is a specific reason for that particular test).

A related issue: Unfortunately, it appears that GTest does not support underscores in test names. As I see now at the Googletest FAQ:

for simplicity, we just ask the users to avoid _ in TestCaseName and TestName

Although underscores in test function names just seem to work well, Googletest just does not want to support them officially. :cry: I find that very disappointing, as I find underscores helpful to write clearer test names… Anyway, I’ll submit a patch to remove those underscores from GTest test names (replacing them by CamelCase), so that you can judge for yourself…

Update: I just submitted the patch: BUG: Removed underscores from GTest test names


(Dženan Zukić) #8

Please also see if you can generally shorten them. And do avoid repeating e.g. ImageNeighborhoodOffsets does not need to be right of the “dot” if it is already left of the dot.


(Dženan Zukić) #9

What matters for the friendly alignment of CTest output is the length of the longest test name. It would be convenient if we could reduce that.


(Niels Dekker) #10

Excuse me, @dzenanz, but that would actually cause a significant loss of meaning. These three tests are not about neighborhood offsets in general. They are specifically testing the GenerateHyperrectangularImageNeighborhoodOffsets function from itkImageNeighborhoodOffsets.h.

Fortunately these test names have already been shortened in the meantime, by two merged patches:

  1. BUG: Removed underscores from GTest test names
  2. STYLE: Replaced “Hyperrect…” by “Rect…” in class + function names

Kind regards, Niels