tests: Don't assume SDL_Init will leave SDL_GetError unset #258
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SDL_GetError works like Standard C errno: if a call fails it will
(generally) set the error indicator, but if a call succeeds there is no
guarantee about whether the error indicator has been set, cleared, or
left at its previous value.
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.
In particular, with a SDL 2.27.x git snapshot, a successful SDL_Init can
leave the error indicator set to a message about inability to open an
input device node, and this does not mean it has failed.
In this commit I have not attempted to check or fix other uses of
SDL_GetError, only the ones that follow SDL_Init. This is enough to make
the test suite pass on Debian 12 alpha with
`SDL_VIDEODRIVER=dummy SDL_AUDIODRIVER=dummy SDL_RENDER_DRIVER=software`
for now, but the other SDL_GetError() calls in the test suite should
ideally also be checked.
Partial solution for: py-sdl#257
Signed-off-by: Simon McVittie <smcv@collabora.com>
smcv
commented
Feb 8, 2023
| @@ -23,8 +23,7 @@ def with_sdl_audio(): | |||
| sdl2.SDL_Quit() | |||
| sdl2.SDL_ClearError() | |||
| ret = sdl2.SDL_Init(sdl2.SDL_INIT_VIDEO | sdl2.SDL_INIT_AUDIO) | |||
| assert sdl2.SDL_GetError() == b"" | |||
| assert ret == 0 | |||
| assert ret == 0, sdl2.SDL_GetError().decode('utf-8', 'replace') | |||
There was a problem hiding this comment.
This is intended to result in showing the SDL error indicator as an error message if the call fails.
|
Ah yep, I learned about this convention unexpectedly a release or two ago. I've already fixed a few older tests that were failing due to non-fatal errors, but clearly I wasn't very thorough. Thanks for the heads-up about 2.27 as well as the patch! When things calm down a bit with work and my studies I'll make time to comprehensively find/fix this assumption throughout the test suite so I can be robust against future releases. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Description
SDL_GetError works like Standard C errno: if a call fails it will (generally) set the error indicator, but if a call succeeds there is no guarantee about whether the error indicator has been set, cleared, or left at its previous value.
The SDL documentation says this:
In particular, with a SDL 2.27.x git snapshot, a successful SDL_Init can leave the error indicator set to a message about inability to open an input device node, and this does not mean it has failed.
In this commit I have not attempted to check or fix other uses of SDL_GetError, only the ones that follow SDL_Init. This is enough to make the test suite pass on Debian 12 alpha with
SDL_VIDEODRIVER=dummy SDL_AUDIODRIVER=dummy SDL_RENDER_DRIVER=softwarefor now, but the other SDL_GetError() calls in the test suite should ideally also be checked.Partial solution for: #257
Merge Checklist
closes #<issue-number>to automatically close an issue