tests: SDL_GetError() != b'' isn't an error

[basak]

In Ubuntu, we're seeing test failures of the following pattern when moving from libsdl2 2.28.1+dfsg-1 to 2.28.2+dfsg-1:

> assert sdl2.SDL_GetError() == b""
E AssertionError: assert b'Unexpected ...r element crc' == b''
E   Full diff:
E   - b''
E   + b'Unexpected controller element crc'

This looks like an issue similar to that fixed in commit 8c39f40. We should check the relevant return value, and only if it indicates failure should we attach any particular meaning to the return value of SDL_GetError().

PR Description

I'm not sure if this is entirely correct and would appreciate review and feedback! In particular it's not clear to me if this 'Unexpected controller element crc' should result in a test failure and needs fixing elsewhere, and/or if it was caused by a change other than a version bump of libsdl2. However, it seems to me that it would be correct to fix these tests anyway?

You can view the downstream test failure logs at https://autopkgtest.ubuntu.com/results/autopkgtest-mantic/mantic/amd64/p/pysdl2/20230808_001651_edcc4@/log.gz

I found that if I just patch that one out, another test fails. So I looked for all instances of this issue in sdl2/test/gamecontroller_test.py, patched them all, and then the test suite passed.

Merge Checklist

• [x] the PR has been reviewed and all comments are resolved
• [x] 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

[a-hurst]

HI @basak, thanks so much for the patch! I tried to fix all of the GetError misusage in the test suite in #265, but it looks like I missed a few.

I think most of this patch is fine as-is, but for the handful of tests that return a pad or stick object, could you please revise the changes so they still check SDL_GetError() if the pad/stick object is a null pointer (i.e. something went wrong in SDL)? For example, for the following code:

if sdl2.SDL_IsGameController(i) == SDL_TRUE:
    pad = sdl2.SDL_GameControllerOpen(i)
    assert sdl2.SDL_GetError() == b""

revise it to look like this:

if sdl2.SDL_IsGameController(i) == SDL_TRUE:
    pad = sdl2.SDL_GameControllerOpen(i)
    if not pad:
        assert sdl2.SDL_GetError() == b""

Basically, the guidance from upstream SDL2 I've gotten is that you should be checking SDL_GetError() if (and only if) you get a bad return code or null pointer.

Thanks again for contributing! Please feel free to add yourself to the AUTHORS.txt as well, or if you'd prefer I'll try and remember to do it before I make the next release.

[a-hurst]

Also, please ignore the failing Windows 32-bit tests: looks like Pillow just recently discontinued 32-bit Windows binaries so they're failing when trying to compile things from source. I'll force pip to use --prefer-binary for those runners in a separate commit.

[basak]

Thank you for your welcome and the review!

...for the handful of tests that return a pad or stick object, could you please revise the changes so they still check SDL_GetError() if the pad/stick object is a null pointer (i.e. something went wrong in SDL)?

Sure! One question though:

In the following:

if sdl2.SDL_IsGameController(i) == SDL_TRUE:
    pad = sdl2.SDL_GameControllerOpen(i)
    if not pad:
        assert sdl2.SDL_GetError() == b""

Shouldn't we just be asserting assert pad? Because if we don't have the right object, that should be an immediate test failure regardless of what SDL_GetError() says, right? If so, then I suggest following the previously introduced pattern like this:

if sdl2.SDL_IsGameController(i) == SDL_TRUE:
    pad = sdl2.SDL_GameControllerOpen(i)
    assert pad, sdl2.SDL_GetError().decode('utf-8', 'replace')

What do you think?

[a-hurst]

Ah yes that makes perfect sense, thanks for thinking that through for me! My only note is that I already have a convenience function for doing the SDL_GetError/decode thing that I use elsewhere in the test suite (_check_error_msg()), can you use that instead? e.g.

assert pad, _check_error_msg()

[basak]

OK. How does this look? Where there was already an assertion on the return value, I didn't implement a further assert. I hope that's OK?

[a-hurst]

Perfect, thanks so much!