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

Update unit tests draw.ellipse, add unit tests draw.(aa)line(s) #510

Merged
merged 7 commits into from Aug 30, 2018
Merged

Update unit tests draw.ellipse, add unit tests draw.(aa)line(s) #510

merged 7 commits into from Aug 30, 2018

Conversation

@amosbastian
Copy link
Contributor

@amosbastian amosbastian commented Aug 27, 2018

This pull request contains the following:

Ellipse

I updated the draw.ellipse unit test so that it now also includes a test that checks if at least two sides of an ellipse, that has a width and height that is 1 pixel smaller than the surface, is touching the surface's border.

It also cleans up the code slightly, including a better way of retrieving the border's values and adds reusability in the form of additional functions.

See: #233

Lines

It also adds unit tests for draw.aaline, draw.line, draw.aalines and draw.lines.

For draw.aaline and draw.line it now draws a line of differing colours on all types of surfaces and checks if the colour stays the same at position (0, 0). There is also a test that checks if the line contains any gaps, as I noticed aaline(s) sometimes does (or is not drawn at all when close to a surface's border).

The same is done for draw.aalines and draw.lines, except these tests draw a line around the border of the given surface then check if all pixels are of the given colour, or if they contain gaps.

See: #395

amosbastian added 4 commits Aug 27, 2018
The unit test now includes a test that checks if at least two
sides of an ellipse, that has a width and height that is 1 pixel
smaller than the surface, is touching the surface's border.

It also cleans up the code slightly, including a better way of
retrieving the border's values and adds reusability.

See: #233
Adds unit tests for draw.aaline, draw.line, draw.aalines and
draw.lines.

For draw.aaline and draw.line it now draws a line of differing
colours on all types of surfaces and checks if the colour
stays the same at position (0, 0).

The same is done for draw.aalines and draw.lines, except they
draw a line around the border of the given surface and check
if all pixels are of the given colour.

See: #395
from pygame.tests import test_utils
#################################### IMPORTS ###################################

if __name__ == '__main__':
Copy link
Member

@illume illume Aug 28, 2018

This is_pygame_pkg stuff should be removed.

The imports should be something like:

import unittest

import pygame
from pygame import draw
from pygame.locals import SRCALPHA
from pygame.tests import test_utils

test/draw_test.py Show resolved Hide resolved
@e1000
Copy link

@e1000 e1000 commented Aug 28, 2018

Personnally, I have some issues about line_is_color ,line_has_gaps and lines_have_gaps :

  1. all repeat the if line_type == "aalines": , hence it seems to be repeated code
  2. it mixes doing something and testing something ; so imho, that's bit confusing, it looks bit as you were testing your own test functions ;)
  3. line_has_gaps and lines_have_gaps : with a quick look I can not understand what the gaps are? And why does the equalities at the end mean that there is a gap (or are gaps) ?

So , for me, these helper functions contain a bit too much logic / factorisation ?

# if true, a line will be draw between the first and last points. The
# boolean blend argument set to true will blend the shades with
# existing shades instead of overwriting them. This function accepts
# floating point values for the end points.
Copy link

@e1000 e1000 Aug 28, 2018

@illume : should we keep or should we delete copies of 2008 docstrings ?

Copy link
Member

@illume illume Aug 28, 2018

Yes, removing these ones seems like a good idea :)

@illume
Copy link
Member

@illume illume commented Aug 28, 2018

@e1000 some good suggestions. Thank you.

  1. line_is_color and friends could be improved by taking a draw function as the argument instead of a string. Maybe call the argument draw_line and then pass in draw.line or draw.aaline.

  2. Then when using line_is_color, the draw_line can be just another parameter in the one test function(test_line_color). This would reduce the number of tests you have too. Also the implementation of line_is_color could be moved inside that one test(because now there are not two). So it would be easier for the person reading it to see what it is doing. Because then line_is_color and test_line_color would fit in one screen.

These two changes would reduce the amount of repeated code (DRY) https://en.wikipedia.org/wiki/Don%27t_repeat_yourself And I think it would bring related test code closer together in the file, making it easier to understand.

  1. Yeah, a different explanation of what gaps are would improve this. It would also help readers of the code to put a link to the issue in the docstring. So they could see the image of the issue themselves.

@illume
Copy link
Member

@illume illume commented Aug 28, 2018

On Travis CI tests I see this error come up:

======================================================================
ERROR: test_line (pygame.tests.draw_test.DrawModuleTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/virtualenv/python2.7.14/lib/python2.7/site-packages/pygame-1.9.5.dev0-py2.7-linux-x86_64.egg/pygame/tests/draw_test.py", line 312, in setUp
    self.surf = pygame.Surface(self.surf_size, pygame.SRCALPHA)
ValueError: no standard masks exist for given bitdepth with alpha

So maybe we need to specify the bit depth for when the tests run like this.

This error is raised when the depth is 8, but it works for depth 16, and depth 32.

>>> pygame.Surface((10,10), pygame.SRCALPHA, 8)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: no standard masks exist for given bitdepth with alpha

Merges some of the unit tests into one unit test and moves
some of the functions closer to the relevant tests for better
readability. Also improves the way the surfaces are created for
testing as it now includes the depth.

See: #512
@amosbastian
Copy link
Contributor Author

@amosbastian amosbastian commented Aug 28, 2018

Thanks for the feedback @e1000 @illume, I just refactored everything and updated the lines_set_up() function so it creates surfaces of an even width/height and odd width/height, with differing depths (16 and 32). If there is anything else left to be improved then please let me know.


draw_line(surface, color, (0, 0), (width - 1, 0))

colors = [surface.get_at((width, 0)) for width in range(width)]
Copy link

@e1000 e1000 Aug 28, 2018

Hello, thank you for modifications ! Some more remarks :

  1. it is not so nice to give same name for loop variable width and the outer width ; better use a single letter for loop variable in list-comprehension :
colors = [surface.get_at((x, 0)) for x in range(width)]
  1. In the cases below, you can write in_border.count(True) == 4 as all(in_border) which seems clearer to me.

  2. Also, I still don't understand : for me reading the code, line_has_gaps( checks that there is no gap since all are the same color ... (same remark for below lines_have_gaps ) ; could you please explain what the "gaps" are , or am I missing something ?

Copy link
Contributor Author

@amosbastian amosbastian Aug 29, 2018

  1. Will update that.
  2. Will update that.
  3. Since it wasn't clear before I already created #512 showing what I meant and reference it in the docstring.

draw_lines(surface, color, True, points)

borders = get_border_values(surface, width, height)
return [color in border for border in borders]
Copy link

@e1000 e1000 Aug 29, 2018

Here also little question : color in border tests that the color appears at least one time on the border ; if I use any (he is a good friend of all), it is equivalent to :

return  [any(c == color for c in border) for border in borders]

# but shouldn't it be ?
return  [all(c == color for c in border) for border in borders]

Copy link
Contributor Author

@amosbastian amosbastian Aug 29, 2018

I see what you mean, I will also update that.

draw_lines(surface, color, True, points)

borders = get_border_values(surface, width, height)
return [len(border) == border.count(color) for border in borders]
Copy link

@e1000 e1000 Aug 29, 2018

I suggest to replace len(border) == border.count(color) with the pattern

all(c == color for c in border)

Copy link
Contributor Author

@amosbastian amosbastian Aug 29, 2018

Will also update this, thanks.

@illume
Copy link
Member

@illume illume commented Aug 30, 2018

Wonderful! 🎈 🎉

@illume illume merged commit 6ee51cd into pygame:master Aug 30, 2018
2 checks passed
@illume illume mentioned this pull request Oct 16, 2018
4 tasks
@notpygame notpygame added the draw label Jun 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants