math.Vector.scale_to_length docs and add test for display.set_palette()

[hanzohasashi33]

Changed the docs for pygame math.Vector.2 and math.vector.3 to report that a ValueError is raised instead of ZeroDivisionError when attempting to scale a zero length vector.
fixes #1957

[MyreMylar]

Looks good to me 👍

Thank you for the contribution!

[MightyJosip]

Most recent change also address: #1754. You should fix the error you are getting right now

ERROR: test_set_palette (pygame.tests.display_test.DisplayModuleTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Python36-x64\lib\site-packages\pygame\tests\display_test.py", line 406, in test_set_palette
    self.assertIsNone(pygame.display.set_palette(palette))
pygame.error: Display mode is not colormapped

and also probably add some more palettes (right now you only check for the empty list)

changes since review

[robertpfeiffer]

bit depth is ignored by set_mode on SDL2, and modern hardware doesn't do 8-bit color-mapped display modes more. This test should just be marked to be skipped on SDL2.

[hanzohasashi33]

@robertpfeiffer , So shall I just close the pr and mark it to be skipped on SDL2?

[robertpfeiffer]

@robertpfeiffer , So shall I just close the pr and mark it to be skipped on SDL2?

Don't close the PR, but mark to skip the palette test on SDL2.

[hanzohasashi33]

@robertpfeiffer , So shall I just close the pr and mark it to be skipped on SDL2?

Don't close the PR, but mark to skip the palette test on SDL2.

@robertpfeiffer May I know how to do the above ?

[MyreMylar]

Here is an example of a test that uses a decorator to skip the test when running on SDL2:

pygame/test/display_test.py

Lines 152 to 166 in 48571a6

	@unittest.skipIf(SDL2, "SDL2 issues")
	def test_get_surface(self):
	"""Ensures get_surface gets the current display surface."""
	lengths = (1, 5, 100)
	for expected_size in ((w, h) for w in lengths for h in lengths):
	for expected_depth in (8, 16, 24, 32):
	expected_surface = display.set_mode(expected_size, 0, expected_depth)
	surface = pygame.display.get_surface()
	self.assertEqual(surface, expected_surface)
	self.assertIsInstance(surface, pygame.Surface)
	self.assertEqual(surface.get_size(), expected_size)
	self.assertEqual(surface.get_bitsize(), expected_depth)

You can change the string in the decorator to describe why this test is being skipped e.g. "set_palette() not supported in SDL2"

[hanzohasashi33]

Here is an example of a test that uses a decorator to skip the test when running on SDL2:

pygame/test/display_test.py

Lines 152 to 166 in 48571a6

	@unittest.skipIf(SDL2, "SDL2 issues")
	def test_get_surface(self):
	"""Ensures get_surface gets the current display surface."""
	lengths = (1, 5, 100)
	for expected_size in ((w, h) for w in lengths for h in lengths):
	for expected_depth in (8, 16, 24, 32):
	expected_surface = display.set_mode(expected_size, 0, expected_depth)
	surface = pygame.display.get_surface()
	self.assertEqual(surface, expected_surface)
	self.assertIsInstance(surface, pygame.Surface)
	self.assertEqual(surface.get_size(), expected_size)
	self.assertEqual(surface.get_bitsize(), expected_depth)

You can change the string in the decorator to describe why this test is being skipped e.g. "set_palette() not supported in SDL2"

Thanks for the help!

[MyreMylar]

OK, first up thank you for contributing! Looking at the unit test part now and the C code to be tested here:

pygame/src_c/display.c

Lines 2174 to 2239 in 48571a6

	#else /* IS_SDLv1 */
	static PyObject *
	pg_set_palette(PyObject *self, PyObject *args)
	{
	SDL_Surface *surf;
	SDL_Palette *pal;
	SDL_Color *colors;
	PyObject *list, *item = NULL;
	int i, len;
	int r, g, b;
	VIDEO_INIT_CHECK();
	if (!PyArg_ParseTuple(args, "|O", &list))
	return NULL;
	surf = SDL_GetVideoSurface();
	if (!surf)
	return RAISE(pgExc_SDLError, "No display mode is set");
	pal = surf->format->palette;
	if (surf->format->BytesPerPixel != 1 || !pal)
	return RAISE(pgExc_SDLError, "Display mode is not colormapped");
	if (!list) {
	colors = pal->colors;
	len = pal->ncolors;
	SDL_SetPalette(surf, SDL_PHYSPAL, colors, 0, len);
	Py_RETURN_NONE;
	}
	if (!PySequence_Check(list))
	return RAISE(PyExc_ValueError, "Argument must be a sequence type");
	len = MIN(pal->ncolors, PySequence_Length(list));
	colors = (SDL_Color *)malloc(len * sizeof(SDL_Color));
	if (!colors)
	return NULL;
	for (i = 0; i < len; i++) {
	item = PySequence_GetItem(list, i);
	if (!PySequence_Check(item) || PySequence_Length(item) != 3) {
	Py_DECREF(item);
	free((char *)colors);
	return RAISE(PyExc_TypeError,
	"takes a sequence of sequence of RGB");
	}
	if (!pg_IntFromObjIndex(item, 0, &r) ||
	!pg_IntFromObjIndex(item, 1, &g) ||
	!pg_IntFromObjIndex(item, 2, &b)) {
	Py_DECREF(item);
	free((char *)colors);
	return RAISE(PyExc_TypeError,
	"RGB sequence must contain numeric values");
	}
	colors[i].r = (unsigned char)r;
	colors[i].g = (unsigned char)g;
	colors[i].b = (unsigned char)b;
	Py_DECREF(item);
	}
	SDL_SetPalette(surf, SDL_PHYSPAL, colors, 0, len);
	free((char *)colors);
	Py_RETURN_NONE;
	}

it looks like it raises a lot of asserts when it's unhappy with various things, ideally our unit test will check for those things as well as the expected behaviour when everything is going right.

This is stuff like - what happens when the input is not a sequence of RGB triplets (say it is RGBA int quadruplets, a triplet of floats or just a single string or something)? What happens when the display mode is not 8 bit? What if the display module has been quit? Looking at all the stuff in the C code that starts with RAISES should give you a good idea of the sorts of things to check but I expect you can puzzle out how to put wrong stuff into a function call.

To check the asserts are being generated, and that they are the right asserts, you can use the with self.assertRaises(): pattern. for example:

pygame/test/rect_test.py

Lines 162 to 163 in 3bb0760

	with self.assertRaises(AttributeError):
	del r.x

I would also say that in general it is better to create separate pull requests for separate issues as it helps keep things tidy and makes the process of reviewing less confusing. Not a problem for this pull request, but something to bear in mind in the future.

Cheers!

[MyreMylar]

Tested it on SDL1 on windows and the test passes locally too, looks decent on a look over.

Thanks for the contribution! 🎉 👍