Incorrect use of SDL_GetError() to check whether calls failed

[smcv]

The SDL documentation says this:

The message is only applicable when an SDL function has signaled an error. You must check the return values of SDL function calls to determine when to appropriately call SDL_GetError(). You should not use the results of SDL_GetError() to decide if an error has occurred! Sometimes SDL will set an error string even when reporting success.

However, the pysdl2 test suite has a lot of this pattern, which the quoted documentation calls out as incorrect:

    ret = sdl2.SDL_Init(sdl2.SDL_INIT_VIDEO | sdl2.SDL_INIT_AUDIO)
    assert sdl2.SDL_GetError() == b""
    assert ret == 0

What doesn't work? One concrete example is that when I run the test suite on Debian 12 alpha, I get a lot of failures like this:

    def test_SDL_VideoInitQuit():
        # Test with default driver
        assert sdl2.SDL_WasInit(0) & sdl2.SDL_INIT_VIDEO != sdl2.SDL_INIT_VIDEO
        ret = sdl2.SDL_VideoInit(None)
>       assert sdl2.SDL_GetError() == b""
E       AssertionError: assert b'Unable to o.../input/event8' == b''
E         Full diff:
E         - b''
E         + b'Unable to open /dev/input/event8'

How To Reproduce

SDL_VIDEODRIVER=dummy SDL_AUDIODRIVER=dummy SDL_RENDER_DRIVER=software pytest -v

I'm using a SDL 2.27.x git snapshot from 2023-01-03, in case it matters.

Platform (if relevant):

• OS: Linux (Debian 12 alpha)
• Python Version: 3.11
• SDL2 Version: 2.27.x snapshot from 2023-01-03
• Using pysdl2-dll: No, using my OS's copy of SDL

[smcv]

258 partially solves this. A complete solution for this would be to apply similar changes to all the other SDL_GetError() calls.

[a-hurst]

Adding this to the to-do milestone for the next release! I should also make sure sdl2.ext doesn't make this assumption anywhere either.

[smcv]

Thanks for working on improving this, but this issue is unresolved, so please reopen. There are still lots of places in the tests that assert that some operation left the error indicator empty, for example:

$ git grep -B3 SDL_GetError
...
sdl2/test/joystick_test.py-    count = sdl2.SDL_NumJoysticks()
sdl2/test/joystick_test.py-    for i in range(count):
sdl2/test/joystick_test.py-        stick = sdl2.SDL_JoystickOpen(i)
sdl2/test/joystick_test.py:        assert sdl2.SDL_GetError() == b""
...
sdl2/test/keyboard_test.py-
sdl2/test/keyboard_test.py-def test_SDL_StartStopTextInput(with_sdl):
sdl2/test/keyboard_test.py-    sdl2.SDL_StopTextInput()
sdl2/test/keyboard_test.py:    assert SDL_GetError() == b""
sdl2/test/keyboard_test.py-    assert sdl2.SDL_IsTextInputActive() == SDL_FALSE
sdl2/test/keyboard_test.py-    sdl2.SDL_StartTextInput()
sdl2/test/keyboard_test.py:    assert SDL_GetError() == b""
...
sdl2/test/render_test.py-    sf = surface.SDL_CreateRGBSurface(
sdl2/test/render_test.py-        0, 100, 100, 32, 0xFF000000, 0x00FF0000, 0x0000FF00, 0x000000FF
sdl2/test/render_test.py-    )
sdl2/test/render_test.py:    assert SDL_GetError() == b""
sdl2/test/render_test.py-    pixfmt = sf.contents.format.contents
sdl2/test/render_test.py-    fill = pixels.SDL_MapRGBA(pixfmt, 0, 0, 0, 255)
sdl2/test/render_test.py-    surface.SDL_FillRect(sf, None, fill)
sdl2/test/render_test.py:    assert SDL_GetError() == b""
...
sdl2/test/rwops_test.py-def test_SDL_RWFromMem():
sdl2/test/rwops_test.py-    buf = ctypes.create_string_buffer(b"1234")
sdl2/test/rwops_test.py-    rw = sdl2.SDL_RWFromMem(buf, len(buf))
sdl2/test/rwops_test.py:    assert sdl2.SDL_GetError() == b""

[a-hurst]

Oops. I thought I did a search through the codebase for incorrect SDL_GetError usage and dealt with it all, but apparently whatever search strategy I used wasn't thorough enough!

Also, thanks a ton for your continued efforts upstreaming fixes for bugs in the test suite (and elsewhere): I'm not a software developer by trade, so there's been a lot of learning-as-I-go in taking over maintenance of a project like PySDL2 (especially in terms of unit tests). I apologize that you've had to spend so much time fixing my failing tests as a result, but I sincerely appreciate your detailed bug reports and clean, well-documented patches.

[smcv]

I should probably mention why I keep opening issues about breakage with new SDL versions, to give you some context.

In Debian, we have both SDL2 and pysdl2 available as packages. We regularly run pysdl2's test suite, and when there's a new version of SDL2 waiting to be integrated, one of the things our QA infrastructure does is to run the old pysdl2's test suite against the new SDL2. If it fails, there are two possible reasons for that:

1. the new SDL2 has a regression, and pysdl2's test suite catches that regression and correctly fails
2. pysdl2 was doing something wrong (most likely, making assumptions that aren't guaranteed to stay true), and the new SDL2 breaks its assumption, so pysdl2's test suite incorrectly fails

I'm a maintainer of our SDL2 package, and when I upload new SDL2 versions, it's relatively common for them to get stuck and not be integrated immediately, because the pysdl2 test suite started failing; so to get the new SDL2 out to users, I have to dig into pysdl2 and find out whether we're in scenario 1 or 2. If we're in scenario 1, great, a regression was successfully caught, and I need to get SDL2 fixed; but if we're in scenario 2, then I need to either fix pysdl2 or get it kicked out of the development branch of Debian, otherwise SDL2 can't progress.

Obviously I prefer to fix potentially-useful software rather than removing it, hence the merge requests.

I'm not actually entirely clear on why we have pysdl2 in Debian, because we don't seem to have any games or apps that depend on it - but its test coverage is currently broader than SDL's, so it's a good way to check that SDL hasn't obviously broken something, and if it occasionally detects a genuine regression in SDL2 then that's enough to earn its place.

[a-hurst]

@smcv Ah, that makes perfect sense. Interesting that PySDL2 got added as a package if it wasn't needed for anything... looks like someone just requested it back in 2013 and it's been around ever since!

I'm glad to hear the test suite has proved useful for catching upstream SDL bugs: I've spent a lot of time and effort cleaning up/expanding the test suite at this point, so I'm glad it's paying off for something other than just making sure my bindings are correct! Hopefully with the latest set of fixes it'll be less likely to give you false positives.