Add some mixer Sound tests

[ghost]

I remembered a bug I fixed (#1431) a while ago that wasn't caught in unittests, so I made a really simple change to fix that.

[ghost]

While I was there, I also tried to do the get_length test above it. It kind of strange to essentially do the same thing pygame does and compare it... but I don't see how else I could do it.
I thought about timing how long the sound plays, but I fear it would be too imprecise, especially on the CI tools.

[MyreMylar]

OK, for the get_length() test I see a couple of things that probably won't be tested yet:

• This part of the code:

    if (format == AUDIO_S8 || format == AUDIO_U8)
        mixerbytes = 1;
#if IS_SDLv2
    else if (format == AUDIO_F32 || format == AUDIO_F32LSB || format == AUDIO_F32MSB){
        mixerbytes = 4;
    }
#endif
    else
        mixerbytes = 2;

Suggests that there are three possible paths through the code on SDL 2, and two on on SDL 1, depening on the audio data started with. Probably the easiest way to test this is to find three audio file in each of the different formats and run each of them through the test. There is more detail on the SDL Audio spec here:

https://wiki.libsdl.org/SDL_AudioSpec

which covers what those audio format constants represent.

• The second thing is this macro that is called:

pygame/src_c/mixer.h

Lines 7 to 9 in ff86036

	#define MIXER_INIT_CHECK() \
	if(!SDL_WasInit(SDL_INIT_AUDIO)) \
	return RAISE(pgExc_SDLError, "mixer not initialized")

When testing a similar macro in the surface module there was a format for the unit tests that worked reasonably well:

pygame.mixer.init()
try:
    # main testing of function/method here
finally:
    pygame.mixer.quit()
    with self.assertRaises(Whatever the assert is in here):
        # call the function once more expecting failure

The mixer tests may currently be set up in a slightly different way, I know there are setUp() & tearDown() type functions in some of the test modules.

For the num_channels() test, the function in C looks pretty simple, but it also has the MIXER_INIT_CHECK() macro in it to test.

I thought I was being a bit overzealous getting people to test this in the surface module, but then we actually found a bug in the SDL2 version's quit() function, so I guess you never know.

[ghost]

Ok. I will try to look at this in the next days. Thanks 😄

[ghost]

I started thinking about the get_length issue.
The code actually uses Mix_QuerySpec, that returns the format used by the opened audio device. As soon as the soud file is loaded, it's converted interally to the output format. So the code you mentionned is for different mixer settings, not for different files formats.

MIX_QuerySpec

Get the actual audio format in use by the opened audio device. This may or may not match the parameters you passed to Mix_OpenAudio.

Mix_LoadWAV_RW

It must know the output characteristics so it can convert the sample for playback, it does this conversion at load time.

So at this point... It's not about testing with different files, but with different mixer formats.

[MyreMylar]

So at this point... It's not about testing with different files, but with different mixer formats.

Ah right, well that makes it easier I guess you can just init() and quit() the mixer module three different times with different settings. Perhaps a good idea to make it into three tests something like test_get_length__size_32(), test_get_length_size_16() and test_get_length_size_8() - then you can easily skip the 32bit float one on SDL1.

[ghost]

I have a question about the test for MIX_INIT_CHECK()
It is called by almost all the sound functions, so it would be a little strange to test for it in get_num_channels only.

Do you want to change all the tests that use it to see if they don't work when mixer is not initialized? This would mean calling mixer.init() and mixer.quit() for every test instead of only once in setUpClass and tearDownClass. I see that it was done this way to save on the initialization time :

def setUpClass(cls):
        # Initializing the mixer is slow, so minimize the times it is called.
        mixer.init()

but perhaps it's not so much of a problem anymore...

[illume]

I think it can take 1 second on some platforms.

Perhaps all of them can be tested together in one method to raise? This way it would only init/quit once.

mixer.quit()
...
# test all mixer functions raise when quit

[MyreMylar]

I have a question about the test for MIX_INIT_CHECK()
It is called by almost all the sound functions, so it would be a little strange to test for it in get_num_channels only.

Do you want to change all the tests that use it to see if they don't work when mixer is not initialized? This would mean calling mixer.init() and mixer.quit() for every test instead of only once in setUpClass and tearDownClass. I see that it was done this way to save on the initialization time :

def setUpClass(cls):
        # Initializing the mixer is slow, so minimize the times it is called.
        mixer.init()

but perhaps it's not so much of a problem anymore...

I suppose the best way is to test it out - if switching every test over to init() then quit() increases the time it takes to run all the tests on a CI build by 30 seconds then it's probably not worth it (there is a compounding effect on Appveyor because it runs builds in series), we could just test the macro on it's own in one test and hope that nobody has accidentally deleted it from any of the functions it is supposed to be on, but if it increases the time to run a CI build by say, only 1 second, then it really doesn't matter.

My preference, absent speed concerns, is to have it on every test for consistency with the surface module and general safety when refactoring and to create implicit guidelines/expectations for people writing new functions in the future.

[ghost]

ok thanks! I may get the time to try some stuff in the next days. I converted the pr to a draft and I will let you know if I do something useful. 😄

[ghost]

• I changed the get_length test to run with all available mixer sizes.

• I also added tests for MIXER_INIT_CHECK() when needed. The tests for most of the other channel and mixer functions that use it are not even done yet, so I won't get into that for now. The time difference seems really small on most platforms... I guess it's not that bad after all.

• I only did it when it's useful. The first tests don't need it because of test_sound__before_init().
  I keep the setUp that does the init if needed, and I keep the tearDownClass in case the last test didn't call quit(). I only remove the setUpClass that isn't needed in this case and do a try: finally: to call mixer.quit() and test for the pygame exception.

I only wanted to add a test for Sound.get_num_channels 😄 So, if it's good enough, I'm fine if you want to merge the PR. If I ever decide to do some more tests, I will open a new one.
Thanks

Oh, and thank you for taking the time to look at my pr and making suggestions to make it better instead of rejecting it or simply changing it yourself. All of this is a really big and fun learning experience for me and it's really appreciated. 😃

[MyreMylar]

Looks good to me, code and time wise, and all the CI tests pass! 👍 🎉

Thank you for the contribution.