Surface.get_bitsize and Surface.get_bytesize tests.

[lkito]

Added get_bytesize unit tests as per issue #1799
Hi, this is my first time contributing to pygame so forgive newb mistakes I make.

I wrote get_bitsize tests and added some get_bytesite tests, too. Thing is, I'm not sure I understand something in the doc of get_bitsize:
"Returns the number of bits used to represent each pixel. This value may not exactly fill the number of bytes used per pixel. For example a 15 bit Surface still requires a full 2 bytes."

Is this right? Because when I create a Surface with depth of 15, get_bitsize returns 15, not 16, as the doc suggests.

Also, if invalid depth passing checks are out of place in get_bitsize, I'll get rid of them, or maybe I should put them elsewhere?

[MyreMylar]

Is this right? Because when I create a Surface with depth of 15, get_bitsize returns 15, not 16, as the >doc suggests.

I think the doc there is just warning that you won't save any bytes per pixel by using a 15bit depth surface over a 16bit depth surface as there is no such thing as a 7th of a byte in this context.

Test looks pretty good to me. 👍

The only thing it looks like isn't being tested is the same thing that we are having some trouble with over in this PR: #1881

Namely this assert:

pygame/src_c/surface.c

Lines 2667 to 2668 in de3654a

	if (!surf)
	return RAISE(pgExc_SDLError, "display Surface quit");

... raised - I believe - when display.quit() has been called and afterwards you try to call get_bitsize() on the display surface. However, it seems like the appveyor CI didn't work with that check for get_colorkey() so if you add it you may want to try the same process I outlined in the other pull request.

Thanks for contributing!

[lkito]

I tried adding
'with self.assertRaises(pygame.error):
surface = pygame.display.set_mode()
pygame.display.quit()
surface.get_bitsize()'

The weird thing is, it doesn't raise an error. After adding
os.environ['SDL_VIDEODRIVER'] = 'dummy'
as you suggested in the other issue, it just seg faults on 'surface.get_bitsize()'.
I'm really confused

[lkito]

I looked into it a bit and
pygame.display.get_init()
returns false after quitting, but the
SDL_Surface *surf = pgSurface_AsSurface(self);
which is line just before the assertion line you linked(in surf_get_bitsize function) doesn't return null. As I understand, this might be SDL2 problem, as pg_display_autoquit() in display.c is different based on the sdl version, but I'm not sure, as I don't have a deep understanding of the code.

[MyreMylar]

Sounds to me like unit testing may have uncovered a genuine bug, which is half of the point of it after all! 👍

I'll have a look into it.

[lkito]

Are you serious? How am I supposed to cope with not writing unit tests for my projects now?!

Well, anyway. That bug's probably way too complicated for my level of knowledge, so am I done here? If so, I'll find the next issue to solve.

Also, why don't you guys enforce squashing multiple commits into one for pull requests(when multiple don't make sense)?

[MyreMylar]

Are you serious? How am I supposed to cope with not writing unit tests for my projects now?!

Well, anyway. That bug's probably way too complicated for my level of knowledge, so am I done here? If so, I'll find the next issue to solve.

Yeah, I think this can probably just sit here until the underlying problem is found and fixed and we can merge this in.

Also, why don't you guys enforce squashing multiple commits into one for pull requests(when multiple don't make sense)?

Is that a thing other open source projects are doing? I heard it was a contentious topic with some preferring to see every dirty commit secret. I think the truth is that there are no rules but those people make a good case for end everyone agrees to use.

On the topic of the actual issue I can reproduce with this on windows:

import pygame

pygame.display.init()
screen = pygame.display.set_mode((800, 600))
pygame.display.quit()
print(screen.get_bitsize())

That block raises the correct assert in pygame 1.9.6 but will often segfault (or sometimes return odd values) on pygame 2.0.0.dev10.

[MyreMylar]

Looks like you could add that assert test back in now, the fix has been merged into master and seems to work.

[lkito]

That's great. I'll probably do that in a few hours, if not, tomorrow for sure.

[MyreMylar]

Looks like this test may be causing another test to fail on the SDL 1 build. I think this is not a specific problem with this test but actually a general problem with the surface tests.

Some tests are being careful to display.init() at the start and display.quit() at the end but others are just calling init() and leaving it initialised for future tests. This means that if tests are currently wrongly relying on display being initialised then another test calling quit() earlier in the order will cause them to fail.

So, I think it is going to be a good idea to make sure all tests that call display.init() have called quit() by the end, and that all tests that call display.quit()/otherwise rely on a init'd display call display.init() at the start.

Here is a decent example:

pygame/test/surface_test.py

Lines 738 to 761 in 9e8fedc

	def test_image_convert_bug_131(self):
	# Bitbucket bug #131: Unable to Surface.convert(32) some 1-bit images.
	# https://bitbucket.org/pygame/pygame/issue/131/unable-to-surfaceconvert-32-some-1-bit
	pygame.display.init()
	try:
	pygame.display.set_mode((640, 480))
	im = pygame.image.load(example_path(os.path.join("data", "city.png")))
	im2 = pygame.image.load(example_path(os.path.join("data", "brick.png")))
	self.assertEqual(im.get_palette(), ((0, 0, 0, 255), (255, 255, 255, 255)))
	self.assertEqual(im2.get_palette(), ((0, 0, 0, 255), (0, 0, 0, 255)))
	self.assertEqual(repr(im.convert(32)), "<Surface(24x24x32 SW)>")
	self.assertEqual(repr(im2.convert(32)), "<Surface(469x137x32 SW)>")
	# Ensure a palette format to palette format works.
	im3 = im.convert(8)
	self.assertEqual(repr(im3), "<Surface(24x24x8 SW)>")
	self.assertEqual(im3.get_palette(), im.get_palette())
	finally:
	pygame.display.quit()

[MyreMylar]

CI for this test should be fixed by this PR:
#1914

[MyreMylar]

Hooray CI is passing. LGTM! 👍

[illume]

Thanks @lkito. Are you ready for it to be merged in?

[illume]

👍 🎉

[lkito]

Yes, this one is ready and once #1900 passes tests, it will be ready too.