Added unit test for surface.get_colorkey(). Closes #1801

[khuang0312]

Some of the original code was actually wrong. For instance, checking the type of the returned value from surface.get_colorkey() to see if it is a Color object or an instance of a child class of Color.

I also added some more cases to make the test more comprehensive. For example, checking what happens when you set the color key using an RGBA value, the alpha not 255, and what surface.get_colorkey() should return.

[MyreMylar]

Looks good! The only other thing to check is the two asserts the function raises in this bit of C code here:

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

#if IS_SDLv1
if (surf->flags & SDL_OPENGL)
    return RAISE(pgExc_SDLError, "Cannot call on OPENGL Surfaces");

Which I think should bubble up to the python code when you try to use get_colorkey() on the display surface (returned from set_mode()), after display.quit() has been called, and for the second one when you try to use get_colorkey() on the display surface created with the pygame.OPENGL flag - but only under SDL1. Both of these should hopefully be testable with self.assertRaises().

The last one will only be easily testable on your computer if you install pygame 1.9.6, but there should be a few examples in the other tests of how to flag a chunk of code to only run under SDL 1 and the Continuous Integration tests should also test it now on Travis.

Thanks for contributing!

(The CI failing is not your test it's another one we recently added to test the time module)

[khuang0312]

Okay, I just added the two new unit tests.

[khuang0312]

I seem to be failing the integration tests because they don't raise pygame.error within the two new cases, even though on my machine, it raises the error.

[MyreMylar]

Hmm, it may be that the 'dummy' video driver, which is used on CI, doesn't invalidate surfaces in the same way? I'm not certain. I suppose you could test it locally by adding

os.environ['SDL_VIDEODRIVER'] = 'dummy'

Somewhere at the top of the test file temporarily and see if it breaks the assert firing locally.

If so, perhaps the best thing to do is pull out that test and put it into a separate test_function (something like 'test_get_colorkey__quit_assertion' and then use the skip decorator if the video driver is == to dummy . e.g.

pygame/test/event_test.py

Lines 555 to 559 in ccfe35c

	@unittest.skipIf(
	os.environ.get("SDL_VIDEODRIVER") == "dummy",
	'requires the SDL_VIDEODRIVER to be a non "dummy" value',
	)
	def test_set_grab__and_get_symmetric(self):

[MyreMylar]

Should probably also note that color keys are an area with lots of bugs hovering around them right now so it may be that there are genuine code problems.

[illume]

Maybe the fix in merged in with the PR #1899 will change this now. There was a problem when quit()/init() were called.

Edit: yeah, this fixes it. To test it, I merged master branch into this PR, and the tests worked: #1907

So merge in master, or rebase against master and the CI robots should be happy.

[MyreMylar]

@illume I think we could probably just restart the Appveyor CI on our end couldn't we? At least that worked to fix Travis. I just don't have the power to restart Appveyor.

[illume]

Oh yeah. We can just merge this in now... because tests pass. No need to do it in this pr.

[illume]

👍 thanks!