[Discussion]: Unifying tests for SegmentationExtractor?

[weiglszonja]

While working on #188, and #187 I realised it might be useful to have tests for methods defined in SegmentationExtractor that can be reused when testing other segmentation extractors.

The idea for instance, if we wanted to test set_times which is defined in SegmentationExtractor, we wouldn't necessarily have to implement a test for set_times in every segmentation extractors that we are creating unittests for in the future.

I was thinking about something like this:

class SegmentationExtractorTestCase(TestCase):

    def test_set_times(self, segmentation_extractor: SegmentationExtractor):
        """Test that set_times sets the times in the expected way."""
        num_frames = segmentation_extractor.get_num_frames()
        sampling_frequency = segmentation_extractor.get_sampling_frequency()

        # Check that times have not been set yet
        assert segmentation_extractor._times is None

        # Set times with an array that has the same length as the number of frames
        times_to_set = np.round(np.arange(num_frames) / sampling_frequency, 6)
        segmentation_extractor.set_times(times_to_set)

        assert_array_equal(segmentation_extractor._times, times_to_set)

        _assert_iterable_complete(
            iterable=segmentation_extractor._times,
            dtypes=np.ndarray,
            element_dtypes=np.float64,
            shape=(num_frames,))

        # Set times with an array that is too short
        times_to_set = np.round(np.arange(num_frames - 1) / sampling_frequency, 6)
        with self.assertRaisesWith(
                exc_type=AssertionError,
                exc_msg="'times' should have the same length of the number of frames!",
        ):
            segmentation_extractor.set_times(times_to_set)

Which then can be used as TestCase for the other segmentation extractors and tests reused?

class TestDummySegmentationExtractor(SegmentationExtractorTestCase):

    def test_set_times():
        segmentation_extractor = generate_dummy_segmentation_extractor()
        self.test_times(segmentation_extractor)

@CodyCBakerPhD @h-mayorquin
Let me know what you think about it, maybe dummy idea .

[CodyCBakerPhD]

I'm all for code reduction, and it looks like it could do that in our testing suite. Better organization of tests is also always good, but having the tests to begin with is more important. This is also always something we can come back to but wouldn't give it highest priority

[bendichter]

@weiglszonja if you are finding yourself wanting to write the same test for multiple objects, I think it's a good idea, particularly if those objects have the same parent. If you name the method to start with test_ then you don't even need to call it explicitly in child classes- the TestCase parent will automatically run all methods that start with test_ as tests.

[weiglszonja]

thanks for the input, I'll keep this in my backlog to revisit later.

[h-mayorquin]

This makes sense to me, check:

roiextractors/tests/test_io.py

Lines 89 to 108 in 66899bf

	def test_imaging_extractors_canonical_shape(self, extractor_class, extractor_kwargs):
	"""Test that get_video and get_frame methods for their shapes and types under different indexing scenarios"""
	extractor = extractor_class(**extractor_kwargs)
	image_size = extractor.get_image_size()
	num_channels = extractor.get_num_channels()
	# Test canonical shape for get video
	video = extractor.get_video()
	canonical_video_shape = [extractor.get_num_frames(), image_size[0], image_size[1]]
	if num_channels > 1:
	canonical_video_shape.append(num_channels)
	assert video.shape == tuple(canonical_video_shape)
	# Test spikeinterface-like behavior
	one_element_video_shape = extractor.get_video(start_frame=0, end_frame=1, channel=0).shape
	expected_shape = (1, image_size[0], image_size[1])
	assert one_element_video_shape == expected_shape
	# Test frames behavior
	assert_get_frames_return_shape(imaging_extractor=extractor)

And assert_get_frames_return_shape(imaging_extractor=extractor) specifically for something similar that we have enable for the imaging extractor pipeline.