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

Added math.Vector2 subclass test for issue #552 #560

Merged
merged 2 commits into from Oct 16, 2018

Conversation

@cmtrapp02
Copy link
Contributor

@cmtrapp02 cmtrapp02 commented Oct 15, 2018

Added new function that tests a subclassed math.Vector2 for issue #552.
This is my first contribution on Github, so any feedback if necessary would be greatly appreciated!

Added new function that tests a subclassed math.Vector2 for issue #552.
This is my first contribution on Github, so any feedback if necessary would be greatly appreciated!
@illume
Copy link
Member

@illume illume commented Oct 16, 2018

Thank you!

def testVector2Subclass(self):
class Vector(pygame.math.Vector2):
pass
v = Vector(2.0,2.0)

This comment has been minimized.

@illume

illume Oct 16, 2018
Member

It's usual to put a space after arguments. For example: v = Vector(2.0, 2.0)

This comment has been minimized.

@cmtrapp02

cmtrapp02 Oct 16, 2018
Author Contributor

Okay thanks, I'll be sure to do that next time.

This comment has been minimized.

@illume

illume Oct 16, 2018
Member

The pygame code isn't the cleanest yet, or using modern conventions everywhere. But eventually we will move to using pylint to help catch these types of issues.

@@ -1577,6 +1577,13 @@ def test_pickle(self):
self.assertEqual(pickle.loads(pickle.dumps(v2)), v2)
self.assertEqual(pickle.loads(pickle.dumps(v3)), v3)

def testVector2Subclass(self):

This comment has been minimized.

@illume

illume Oct 16, 2018
Member

Can you please add a decorator to show that this is an expected failure?
Please see the documentation here on how to do that: https://docs.python.org/3/library/unittest.html#unittest.expectedFailure

This way the tests will pass on TravisCI and Appveyor.

This comment has been minimized.

@cmtrapp02

cmtrapp02 Oct 16, 2018
Author Contributor

Yup, I'll do that as soon as I can. Thanks for the help

@illume illume mentioned this pull request Oct 16, 2018
4 tasks done
@llenck
Copy link

@llenck llenck commented Oct 16, 2018

Wouldn't it be desirable to test this in a child process? The way it is now the test doesn't really fail, but instead the process just terminates with only the message "Segmentation fault (core dumped)"

Edit: It doesn't look all that nice, but at least on my system this works (relies on OS returning nonzero for segmentation faulting processes but every one I've used so far does that):

(at the top of the file)
import subprocess

[...]

    def testVector2Subclass(self):
        self.assertEqual(subprocess.call(
            ["python", __file__, "--test-for-segv"]), 0)

class Vector(pygame.math.Vector2):
    pass

if len(sys.argv) >= 2:
    if sys.argv[1] == "--test-for-segv":
        v = Vector(2.0, 2.0)
        v *= 2
        exit(0)

I only recently did my first contribution here, so I'm not really good at writing good looking / maintainable code and I'd appreciate someone else rewriting this as i think it looks pretty horrible :/

@illume
Copy link
Member

@illume illume commented Oct 16, 2018

Hello @Nunu-Willump . Thanks for that.

Yeah, testing code which uses C extensions like pygame makes it a bit harder to test things than normal python code which crashes nicely. The test runner has an option run each test in a separate process. Since there is already a fix lined up for this issue, I think it's fine in this instance to avoid adding the subprocess code in as you've done. You can use subprocesses in the test runner with python test/__init__.py --usesubprocess

-Added the expectedFailure decorator over testVector2Subclass()
-added space inbetween the arguements of v = Vector and self.assertEqual for code beautification
@illume
Copy link
Member

@illume illume commented Oct 16, 2018

Wonderful. Thanks :)

@illume
Copy link
Member

@illume illume commented Oct 16, 2018

Ah yes. The CI (Appveyor and Travis) are failing because it crashes python. I'll merge this in now anyway, and merge the other one right away.

@illume illume changed the base branch from master to subtypemath Oct 16, 2018
@illume illume merged commit d011583 into pygame:subtypemath Oct 16, 2018
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants