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

Add draw_ellipse unit test #501

Merged
merged 5 commits into from Aug 23, 2018
Merged

Add draw_ellipse unit test #501

merged 5 commits into from Aug 23, 2018

Conversation

@amosbastian
Copy link
Contributor

@amosbastian amosbastian commented Aug 22, 2018

The unit test creates a surface of a certain width and height, then creates an ellipse with the same width and height on the surface. It then creates four lists containing the values of the top, left, right and bottom borders respectively. Finally it checks if each of these lists contains the colour of the ellipse, and if they don't then the test fails.

#233

amosbastian added 3 commits Aug 21, 2018
Creates a surface containing an ellipse with equal width and
height. It then checks for each border if it contains the
colour of the ellipse, which it should.

Resolves unit test task of #233
@illume
Copy link
Member

@illume illume commented Aug 23, 2018

Very good! Thank you.

There is little improvement you could make to avoid the display.set_mode, that I've made a note for. Could you please fit that one?

self.fail()
sizes = [(4, 4), (5, 4), (4, 5), (5, 5)]
color = (1, 13, 24, 255)

This comment has been minimized.

@illume

illume Aug 23, 2018
Member

Can we avoid the display.set_mode call? Because this opens a new window each time. Instead I think we can just use a surface?

for width, height in sizes:
    surface = pygame.Surface((width, height))
@amosbastian
Copy link
Contributor Author

@amosbastian amosbastian commented Aug 23, 2018

I removed the display.set_mode and added another loop so it checks ellipses with a border width of 1, but also filled ellipses. Now both draw_ellipse and draw_fillellipse in draw.c are covered by this unit test.

@illume
Copy link
Member

@illume illume commented Aug 23, 2018

Nice one :) Thanks.

Aha! I see what the problem is. If you click on the travis ci ones that fail you can see the error.

The print statement needs to be removed. The tests shouldn't have print in them.
The fstrings it uses is in python 3.6+ only.

@illume illume merged commit 318519d into pygame:master Aug 23, 2018
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@illume
Copy link
Member

@illume illume commented Aug 23, 2018

Oh. I forgot to mention...

People usually put a link to the issue in the description of the PR. So that people looking at the PR can more easily click on the issue.

@amosbastian amosbastian changed the title Add draw_ellipse unit test #233 Add draw_ellipse unit test Aug 23, 2018
@illume illume mentioned this pull request Oct 16, 2018
4 tasks done
@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

3 participants