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

tests: Complete the method lists #91

Merged
merged 4 commits into from
Dec 17, 2018
Merged

tests: Complete the method lists #91

merged 4 commits into from
Dec 17, 2018

Conversation

nbraud
Copy link
Collaborator

@nbraud nbraud commented Dec 14, 2018

  • Add methods that do not return a Vector2
  • Use Vector2.convert directly, rather than a lambda, so it has a human-meaningful name.

No test overrides it, and using a lambda makes the benchmark unable to find the
function's name.
While we are at it, sort the lists in alphabetical order.
tests/utils.py Outdated
@@ -36,7 +36,7 @@ def angle_isclose(x, y, epsilon = 6.5e-5):

# List of operations that (Vector2) -> Vector2
UNARY_OPS = [
lambda v: type(v).convert(v),
Copy link
Member

@AstraLuma AstraLuma Dec 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this cause tests to fail?

class V(Vector2): pass

assert isinstance(Vector2.convert((1,2), Vector2)
assert isinstance(V.convert((1,2), V)

Oh, I see, it's because it's passing in a Vector2 in the test, so .convert() just short-circuits and passes the instance right back out.

This is still probably wrong.

Copy link
Collaborator Author

@nbraud nbraud Dec 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still probably wrong.

How so? As the commit message details, tests never redefine Vector2.convert in a subclass, so it's equivalent, and we get Vector2.convert as the function name instead of <lambda somehorriblestuff>, in tests and benchmark output.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A subclass doesn't have to override it for it to return a subclass.

If .convert() is called on a subclass, it'll return that subclass if construction was involved. eg, MyVector.convert((1,2)) will return an instance of MyVector with no overrides necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is true, but it's also not tested by anything in our testsuite (as far as I can tell), nor is it related to any of the tests that use UNARY_OPS.

Case in point, calling that lambda on something that isn't a (subclass of) Vector2, like a tuple, would expand to tuple.convert((1, 2)) and fail:

>>> l = lambda v: type(v).convert(v)
>>> l((1, 0))
AttributeError: type object 'tuple' has no attribute 'convert'

>>> l = Vector2.convert
>>> l((1, 0))
Vector2(1.0, 0.0)

As such, I don't think there is a way for a test that uses UNARY_OPS (whether with the lambda or with Vector2.convert) to be testing that behaviour, and there definitely is no test in the testsuite that does (because it would have broken with the lambda).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, fine. We should probably have other tests testing it, though.

mypy cannot currently cope with the attributes of properties, incl. fget:

  python/mypy#220
@nbraud
Copy link
Collaborator Author

nbraud commented Dec 15, 2018

PS: The CI failure was caused by a mypy issue, so I added a type: ignore annotation as a workaround.

@AstraLuma AstraLuma merged commit 450c3f8 into ppb:master Dec 17, 2018
@nbraud nbraud deleted the ops branch December 17, 2018 17:26
nbraud added a commit to nbraud/ppb-vector that referenced this pull request Dec 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants