Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Surface.get_locked test #1826

Merged
merged 2 commits into from May 22, 2020
Merged

Surface.get_locked test #1826

merged 2 commits into from May 22, 2020

Conversation

PeanutbutterWarrior
Copy link
Contributor

Test for #1803

@MyreMylar
Copy link
Contributor

Looks pretty solid, I suppose we could also try

  • blitting a locked surface onto another one.
  • blitting onto a locked surface and confirming it produces an error.

(you can check for assertions being raised with self.assertRaises and passing the function name and it's parameters as separate arguments, see examples here:

https://github.com/pygame/pygame/blob/master/test/blit_test.py#L135)

@PeanutbutterWarrior
Copy link
Contributor Author

Sorry, but how does that test .get_locked()? As another check that a surface is locked?

@MyreMylar
Copy link
Contributor

Sorry, but how does that test .get_locked()? As another check that a surface is locked?

Yeah, my thinking was just a double check that the flag was accurately reporting what was happening deeper in the code somewhere. I mean probably the code right now is just checking for the flag in blit before raising an assert so we would be just in a circle, but perhaps at some point in the codes life it might check for locked-ness of a surface in a different way internally in SDL or pygame and the flag we check with get_locked() could get out of sync with what is going on.

The blit code is a big old mess of code so I could definitely see it :)

Honestly, I think this kind of testing could easily fit in the unit test for lock() (when that exists) - checking that surfaces locked there both can't be blitted on and report that they are locked with get_locked() so we could just punt it over to there if you think it makes this test clearer?

@PeanutbutterWarrior
Copy link
Contributor Author

Yes, I think this would work as a test for lock(), then for get_locked() it will check against the blit check, rather than the expected value.

Copy link
Contributor

@MyreMylar MyreMylar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then it all looks good to me, I'll make a mental note for the .lock() unit test. 👍 🍾

Uses the fact that blitting to or from a locked surface raises and error to check get_locked against the actual state rather than the expected state.
@MyreMylar
Copy link
Contributor

New changes also look good! :)

Copy link
Member

@illume illume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 thanks

@illume illume merged commit 3bb0760 into pygame:master May 22, 2020
@PeanutbutterWarrior PeanutbutterWarrior deleted the add-test_get_locked branch May 24, 2020 11:03
@illume illume added the Surface pygame.Surface label May 29, 2020
@illume illume changed the title Add test_get_locked Surface.get_locked test May 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Surface pygame.Surface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants