Remove more misuse of SDL_GetError in tests

[a-hurst]

PR Description

Closes #257 (for real this time, I think). This fixes a bunch of assorted unit tests to only check for SDL_GetError output when a function's return value indicates a failure, since SDL_SetError is often used internally by SDL2 to set non-fatal warning messages (leading to tests breaking unexpectedly with new SDL releases).

Merge Checklist

• the PR has been reviewed and all comments are resolved
• all CI checks pass
• (if applicable): the PR description includes the phrase closes #<issue-number> to automatically close an issue
• (if applicable): bug fixes, new features, or API changes are documented in news.rst

[smcv]

This generally looks good, thanks! I haven't verified that it covers all SDL_GetError() calls, but it's definitely an improvement.

As a general comment, you've added a bunch of SDL_ClearError() calls that are harmless but probably unnecessary. If you are using the recommended pattern, like this pseudocode:

ret = SDL_Something()
if ret indicates failure:
    error "{SDL_GetError()}"

then there's no need to call SDL_ClearError() before SDL_Something(). The only time you really need SDL_ClearError() is when a function doesn't tell you whether it succeeded or not, like this pseudocode:

SDL_ClearError()
SDL_SomeFunctionReturningVoid()
error = SDL_GetError()
if error:
    warning "perhaps SDL_SomeFunctionReturningVoid() failed, or perhaps this is harmless, I can't really tell: {error}"

to minimize the chance that the error indicator is still set for some unrelated reason.

[a-hurst]

@smcv Thanks for the review! Definitely feel more confident I got it right this time with a second set of eyes on it.

Re: the added SDL_ClearError calls, my reasoning for those is that for tests with loops (e.g. testing SDL_JoystickGetVendor for each connected joystick) I wanted to be sure that there wouldn't be a situation where item i succeeds but sets a non-fatal error, and then item i+1 fails but doesn't set an error, resulting in a misleading message. Is that generally something I shouldn't have to worry about with how SDL2 handles errors?

[smcv]

Re: the added SDL_ClearError calls, my reasoning for those is that for tests with loops (e.g. testing SDL_JoystickGetVendor for each connected joystick) I wanted to be sure that there wouldn't be a situation where item i succeeds but sets a non-fatal error, and then item i+1 fails but doesn't set an error, resulting in a misleading message. Is that generally something I shouldn't have to worry about with how SDL2 handles errors?

You're the maintainer, and if you prefer to do it like this, do it like this. As I said, it's harmless, even if unnecessary.

I believe the intention is that if a SDL function fails (not just "returns an empty result" if that's a thing that is possible, but actually fails), then it is guaranteed to set the error indicator, so that if it didn't, that would be considered to be a SDL bug.

I'm actually more familiar with GLib and CPython than SDL, and in those C APIs, it is certainly intended to be an API guarantee that: if you call a function in a way that is not already considered to be a programming error (undefined behaviour), and it fails, then the error indicator will be set.

Similarly, in the ISO/POSIX C library interface, most functions have it as an API guarantee that if you call the function and it fails, then errno will be set to the reason for failure. Like SDL, the converse is not true: if the function succeeds, then the state of errno is unspecified (might have been changed, might be 0, might be some unrelated value left over from a previous function call). SDL's error indicator is modelled on errno, but with strings instead of integer codes.

[a-hurst]

I believe the intention is that if a SDL function fails (not just "returns an empty result" if that's a thing that is possible, but actually fails), then it is guaranteed to set the error indicator, so that if it didn't, that would be considered to be a SDL bug.

That makes sense, thanks for the explanation! I'll leave the extra ClearErrors in if for now if they're harmless, but now I know that if I ever hit an error return value and it doesn't set a message it's something to report upstream to SDL proper.